Fix @returns.json support for casting response using provided type

CHANGELOG.rst

@@ -10,12 +10,22 @@ Unreleased
 ==========
 Added
 -----
-- Add a new base class, ``uplink.retry.RetryBackoff``, which can be extended to implement custom backoff strategies. An instance of a ``RetryBackoff`` subclass can be provided through the ``backoff`` argument of the ``@retry`` decorator. (`#238`_)
+- Add a new base class, ``uplink.retry.RetryBackoff``, which can be extended to
+  implement custom backoff strategies. An instance of a ``RetryBackoff`` subclass
+  can be provided through the ``backoff`` argument of the ``@retry`` decorator.
+  (`#238`_)
 
 Changed
 -------
 - Bump minimum version of ``six`` to ``1.13.0``. (`#246`_)
 
+Fixed
+-----
+- Fix ``@returns.json`` to cast JSON response (or field referenced by the ``key``
+  argument) when the ``type`` argument is callable. This effectively reverts a
+  change in the decorator's behavior that was introduced in v0.9.3. (`#215`_)
+
+
 0.9.5_ - 2022-01-04
 ====================
 Added
@@ -415,6 +425,7 @@ Added
 .. _#204: https://github.com/prkumar/uplink/pull/204
 .. _#207: https://github.com/prkumar/uplink/pull/207
 .. _#209: https://github.com/prkumar/uplink/pull/209
+.. _#215: https://github.com/prkumar/uplink/issues/215
 .. _#217: https://github.com/prkumar/uplink/issues/217
 .. _#221: https://github.com/prkumar/uplink/issues/221
 .. _#237: https://github.com/prkumar/uplink/discussions/237

tests/integration/test_returns.py

@@ -49,15 +49,47 @@ def get_repo(self, user, repo):
     def get_repos(self, user):
         pass
 
+    @uplink.returns.from_json(key=("data", 0, "size"))
+    @uplink.get("/users/{user}/repos")
+    def get_first_repo_size(self, user):
+        pass
+
+    @uplink.returns.from_json(key=("data", 0, "stars"), type=int)
+    @uplink.get("/users/{user}/repos")
+    def get_first_repo_stars(self, user):
+        pass
+
     @uplink.json
     @uplink.post("/users/{user}/repos", args={"repo": uplink.Body(Repo)})
     def create_repo(self, user, repo):
         pass
 
+    @uplink.returns(object)
+    @uplink.get("/users")
+    def list_users(self):
+        pass
+
 
 # Tests
 
 
+def test_returns_response_when_type_has_no_converter(
+    mock_client, mock_response
+):
+    # Setup
+    mock_response.with_json({"id": 123, "name": "prkumar"})
+    mock_client.with_response(mock_response)
+    github = GitHub(
+        base_url=BASE_URL, client=mock_client, converters=user_reader
+    )
+
+    # Run
+    response = github.list_users()
+
+    # Verify
+    assert response == mock_response
+
+
 def test_returns_with_type(mock_client, mock_response):
     # Setup
     mock_response.with_json({"id": 123, "name": "prkumar"})
@@ -114,6 +146,48 @@ def test_returns_json_with_list(mock_client, mock_response):
     ] == repo
 
 
+def test_returns_json_by_key(mock_client, mock_response):
+    # Setup
+    mock_response.with_json(
+        {
+            "data": [
+                {"owner": "prkumar", "name": "uplink", "size": 300},
+                {"owner": "prkumar", "name": "uplink-protobuf", "size": 400},
+            ],
+            "errors": [],
+        }
+    )
+    mock_client.with_response(mock_response)
+    github = GitHub(base_url=BASE_URL, client=mock_client)
+
+    # Run
+    size = github.get_first_repo_size("prkumar")
+
+    # Verify
+    assert size == 300
+
+
+def test_returns_json_with_key_and_type(mock_client, mock_response):
+    # Setup
+    mock_response.with_json(
+        {
+            "data": [
+                {"owner": "prkumar", "name": "uplink", "stars": "300"},
+                {"owner": "prkumar", "name": "uplink-protobuf", "stars": "400"},
+            ],
+            "errors": [],
+        }
+    )
+    mock_client.with_response(mock_response)
+    github = GitHub(base_url=BASE_URL, client=mock_client)
+
+    # Run
+    stars = github.get_first_repo_stars("prkumar")
+
+    # Verify
+    assert stars == 300
+
+
 def test_post_json(mock_client):
     # Setup
     github = GitHub(

tests/unit/test_returns.py

@@ -19,9 +19,7 @@ def test_returns_with_multiple_decorators(request_builder, mocker):
     first_type = returns.ReturnType.with_decorator(None, decorator1)
     second_type = (
         request_builder.return_type
-    ) = returns.ReturnType.with_decorator(
-        first_type, decorator2
-    )
+    ) = returns.ReturnType.with_decorator(first_type, decorator2)
 
     # Verify that the return type doesn't change after being handled by first decorator
     decorator1.modify_request(request_builder)
@@ -59,14 +57,73 @@ def test_returns_json(request_builder, mocker):
         mock_response.json()
     )
 
-    # Verify: Doesn't apply to unsupported types
+    # Verify: Returns JSON when type cannot be converted
     request_builder.get_converter.return_value = None
-    returns_json = returns.json(str, ())
+    returns_json = returns.json(None, ())
     request_builder.return_type = returns.ReturnType.with_decorator(
         None, returns_json
     )
     returns_json.modify_request(request_builder)
-    assert not callable(request_builder.return_type)
+    assert callable(request_builder.return_type)
+    assert request_builder.return_type(mock_response) == mock_response.json()
+
+
+def test_returns_json_builtin_type(request_builder, mocker):
+    mock_response = mocker.Mock()
+    mock_response.json.return_value = {"key": "1"}
+    request_builder.get_converter.return_value = None
+    returns_json = returns.json(type=int, key="key")
+    request_builder.return_type = returns.ReturnType.with_decorator(
+        None, returns_json
+    )
+    returns_json.modify_request(request_builder)
+    print(request_builder.return_type)
+    assert callable(request_builder.return_type)
+    assert request_builder.return_type(mock_response) == 1
+
+
+class TestReturnsJsonCast(object):
+    default_value = {"key": "1"}
+
+    @staticmethod
+    def prepare_test(request_builder, mocker, value=default_value, **kwargs):
+        mock_response = mocker.Mock()
+        mock_response.json.return_value = value
+        request_builder.get_converter.return_value = None
+        returns_json = returns.json(**kwargs)
+        request_builder.return_type = returns.ReturnType.with_decorator(
+            None, returns_json
+        )
+        returns_json.modify_request(request_builder)
+        return mock_response
+
+    def test_without_type(self, request_builder, mocker):
+        mock_response = self.prepare_test(request_builder, mocker, key="key")
+        assert request_builder.return_type(mock_response) == "1"
+
+    def test_with_callable_type(self, request_builder, mocker):
+        mock_response = self.prepare_test(
+            request_builder,
+            mocker,
+            type=lambda _: "test",
+        )
+        assert request_builder.return_type(mock_response) == "test"
+
+    def test_with_builtin_type(self, request_builder, mocker):
+        mock_response = self.prepare_test(request_builder, mocker, type=str)
+        assert request_builder.return_type(mock_response) == str(
+            self.default_value
+        )
+
+    def test_with_builtin_type_and_key(self, request_builder, mocker):
+        mock_response = self.prepare_test(
+            request_builder, mocker, key="key", type=int
+        )
+        assert request_builder.return_type(mock_response) == 1
+
+    def test_with_not_callable_cast(self, request_builder, mocker):
+        mock_response = self.prepare_test(request_builder, mocker, type=1)
+        assert request_builder.return_type(mock_response) == self.default_value
 
 
 def test_returns_JsonStrategy(mocker):
@@ -77,5 +134,3 @@ def test_returns_JsonStrategy(mocker):
 
     converter = returns.JsonStrategy(lambda y: y + "!", "hello")
     assert converter(response) == "world!"
-
-    assert returns.JsonStrategy(1).unwrap() == 1

uplink/returns.py

@@ -44,9 +44,6 @@ class _ReturnsBase(decorators.MethodAnnotation):
     def return_type(self):  # pragma: no cover
         raise NotImplementedError
 
-    def _get_return_type(self, return_type):  # pragma: no cover
-        return return_type
-
     def _make_strategy(self, converter):  # pragma: no cover
         pass
 
@@ -56,20 +53,24 @@ def _modify_request_definition(self, definition, kwargs):
             definition.return_type, self
         )
 
+    def _get_converter(self, request_builder, return_type):  # pragma: no cover
+        return request_builder.get_converter(
+            keys.CONVERT_FROM_RESPONSE_BODY, return_type.type
+        )
+
     def modify_request(self, request_builder):
         return_type = request_builder.return_type
         if not return_type.is_applicable(self):
             return
 
-        converter = request_builder.get_converter(
-            keys.CONVERT_FROM_RESPONSE_BODY,
-            self._get_return_type(return_type.type),
+        converter = self._get_converter(request_builder, return_type)
+        if converter is None:
+            return
+
+        # Found a converter that can handle the return type.
+        request_builder.return_type = return_type.with_strategy(
+            self._make_strategy(converter)
         )
-        if converter is not None:
-            # Found a converter that can handle the return type.
-            request_builder.return_type = return_type.with_strategy(
-                self._make_strategy(converter)
-            )
 
 
 class JsonStrategy(object):
@@ -90,9 +91,6 @@ def __call__(self, response):
         content = self._converter(content)
         return content
 
-    def unwrap(self):
-        return self._converter
-
 
 # noinspection PyPep8Naming
 class json(_ReturnsBase):
@@ -121,20 +119,30 @@ def get_user(self, username):
             :emphasize-lines: 2
 
             {
-                "data": { "user": "prkumar", "id": 140232 },
+                "data": { "user": "prkumar", "id": "140232" },
                 "errors": []
             }
 
         If returning the list of errors is unnecessary, we can use the
-        :py:attr:`key` argument to strictly return the inner field
-        :py:attr:`data`:
+        :py:attr:`key` argument to strictly return the nested field
+        :py:attr:`data.id`:
 
         .. code-block:: python
 
-            @returns.json(key="data")
+            @returns.json(key=("data", "id"))
+            @get("/users/{username}")
+            def get_user_id(self, username):
+                \"""Get a specific user's ID.\"""
+
+        We can also configure Uplink to convert the field before it's
+        returned by also specifying the``type`` argument:
+
+       .. code-block:: python
+
+            @returns.json(key=("data", "id"), type=int)
             @get("/users/{username}")
-            def get_user(self, username):
-                \"""Get a specific user.\"""
+            def get_user_id(self, username):
+                \"""Get a specific user's ID.\"""
 
     .. versionadded:: v0.5.0
     """
@@ -145,6 +153,13 @@ class _DummyConverter(interfaces.Converter):
         def convert(self, response):
             return response
 
+    class _CastConverter(interfaces.Converter):
+        def __init__(self, cast):
+            self._cast = cast
+
+        def convert(self, response):
+            return self._cast(response)
+
     __dummy_converter = _DummyConverter()
 
     def __init__(self, type=None, key=(), model=None, member=()):
@@ -167,14 +182,24 @@ def __init__(self, type=None, key=(), model=None, member=()):
     def return_type(self):
         return self._type
 
-    def _get_return_type(self, return_type):
-        # If return_type is None, the strategy should directly return
-        # the JSON body of the HTTP response, instead of trying to
+    def _get_converter(self, request_builder, return_type):
+        converter = super(json, self)._get_converter(
+            request_builder, return_type
+        )
+
+        if converter:
+            return converter
+
+        if callable(return_type.type):
+            return self._CastConverter(return_type.type)
+
+        # If the return_type cannot be converted, the strategy should directly
+        # return the JSON body of the HTTP response, instead of trying to
         # deserialize it into a certain type. In this case, by
         # defaulting the return type to the dummy converter, which
         # implements this pass-through behavior, we ensure that
         # _make_strategy is called.
-        return self.__dummy_converter if return_type is None else return_type
+        return self.__dummy_converter
 
     def _make_strategy(self, converter):
         return JsonStrategy(converter, self._key)