Deserialize API responses for non 2XX status codes if defined #778

da1910 · 2025-04-15T13:37:55Z

Closes #757

This PR adds deserialization for exceptions if models are defined in the type map. A new property exception_model will be present on an ApiException object which will be populated with the return type if it's defined.

I considered just returning the model without throwing, but that might be a surprising outcome for users, and would break the pattern of "everything was OK because I got a response and not an exception". This is an additive change to handle_response and to the exception type, existing users of that code should continue to see the same behaviour as before.

codecov · 2025-04-15T13:51:18Z

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.28%. Comparing base (3d4264f) to head (adc720f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ansys/openapi/common/_exceptions.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   94.40%   94.28%   -0.12%     
==========================================
  Files           7        7              
  Lines         804      805       +1     
==========================================
  Hits          759      759              
- Misses         45       46       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Andy-Grigg

Mostly minor changes, naming and re-ordering. Overall, this looks good, and should help consumers of packages that depend on this handle specific exceptions more flexibly.

Andy-Grigg · 2025-04-21T12:17:41Z

src/ansys/openapi/common/_exceptions.py

    Provides the exception to raise when the remote server returns an unsuccessful response.

-    For more information about the failure, inspect ``.status_code`` and ``.reason_phrase``.
+    For more information about the failure, inspect ``.status_code``, ``.reason_phrase``, and if the


Suggested change

For more information about the failure, inspect ``.status_code``, ``.reason_phrase``, and if the

For more information about the failure, inspect ``.status_code`` and ``.reason_phrase``. If the

Andy-Grigg · 2025-04-21T12:18:06Z

src/ansys/openapi/common/_exceptions.py


-    For more information about the failure, inspect ``.status_code`` and ``.reason_phrase``.
+    For more information about the failure, inspect ``.status_code``, ``.reason_phrase``, and if the
+    server defines a custom exception model, ``.exception_model``.


Suggested change

server defines a custom exception model, ``.exception_model``.

server defines a custom exception model, ``.exception_model`` contains the deserialized response.

Andy-Grigg · 2025-04-21T12:41:35Z

src/ansys/openapi/common/_util.py

-def handle_response(response: requests.Response) -> requests.Response:
+def handle_response(
+    response: requests.Response, response_model: DeserializedType = None
+) -> requests.Response:


This function is kind of weird. It no longer needs to return anything, so really all it's doing is encapsulating the "if not 2xx then raise" logic.

Maybe we just delete the function and include the if-check in __call_api?

I thought about that, but it's not a private function, so there's a low but non-zero chance someone is relying on it.

If we think that risk is low enough then yes it can be removed and replaced with a check in the one place it's used

I think so. As you say, it's not a private function, but it's also not documented. For me this falls into a grey area, and I'm happy to take the chance that either no one's using it, or if they are, they can easily work around this change.

Andy-Grigg · 2025-04-21T12:48:47Z

src/ansys/openapi/common/_api_client.py

                _response_type = response_type_map.get(response_data.status_code, None)

            return_data = self.deserialize(response_data, _response_type)
+            deserialized_response = return_data


This is all a bit weird. We define a variable called return_data, which is a union of both the request response type and our deserialized type, and initially contains the raw request response.

We then maybe overwrite that with the deserialized response, and then write that to a different variable deserialized_response.

I realize that return_data needs to be the union of the types eventually, but it might be cleaner if that union happens closer to the end of the function.

Andy-Grigg · 2025-04-21T12:50:49Z

tests/test_api_client.py

        assert headers["Content-Type"] == "text/plain"

+    def test_get_model_returns_defined_exception(self):
+        """This test getting an object from a server which returns a defined exception object when the requested id does


Suggested change

"""This test getting an object from a server which returns a defined exception object when the requested id does

"""This test gets an object from a server which returns a defined exception object when the requested id does

Andy-Grigg · 2025-04-21T12:51:18Z

tests/test_api_client.py

+        assert exception_model.stack_trace == stack_trace
+
+    def test_get_model_throws_if_no_defined_exception_type(self):
+        """This test getting an object from a server which returns a defined exception object when the requested id does


Suggested change

"""This test getting an object from a server which returns a defined exception object when the requested id does

"""This test gets an object from a server which returns a defined exception object when the requested id does

Andy-Grigg · 2025-04-21T12:51:49Z

tests/test_api_client.py

        assert "Content-Type" in headers
        assert headers["Content-Type"] == "text/plain"

+    def test_get_model_returns_defined_exception(self):


Suggested change

def test_get_model_returns_defined_exception(self):

def test_get_model_raises_exception_with_deserialized_response(self):

Andy-Grigg · 2025-04-21T12:52:20Z

tests/test_api_client.py

+        assert exception_model.exception_code == exception_code
+        assert exception_model.stack_trace == stack_trace
+
+    def test_get_model_throws_if_no_defined_exception_type(self):


Suggested change

def test_get_model_throws_if_no_defined_exception_type(self):

def test_get_model_raises_exception_with_no_deserialized_response(self):

da1910 requested review from Andy-Grigg and EstherProsperi April 15, 2025 13:37

github-actions bot added the enhancement New features or code improvements label Apr 15, 2025

EstherProsperi approved these changes Apr 15, 2025

View reviewed changes

Andy-Grigg requested changes Apr 21, 2025

View reviewed changes

da1910 added 3 commits April 22, 2025 10:53

Deserialize exception models into error objects

98f1408

Add tests for exception deserialization

0998fa4

Review comments

da24ed9

da1910 force-pushed the feat/deserialize_exception_models branch from 132db2c to da24ed9 Compare April 22, 2025 09:54

pyansys-ci-bot and others added 2 commits April 22, 2025 09:54

chore: adding changelog file 778.added.md [dependabot-skip]

35c65d5

Add tests for the _preload_data=False case

adc720f

Andy-Grigg approved these changes Apr 22, 2025

View reviewed changes

da1910 added this pull request to the merge queue Apr 22, 2025

Merged via the queue into main with commit 2d82e92 Apr 22, 2025
31 of 32 checks passed

da1910 deleted the feat/deserialize_exception_models branch April 22, 2025 15:40

Andy-Grigg mentioned this pull request Apr 23, 2025

Failure to deserialize non-200 response results in unexpected exception #784

Open

2 tasks

	For more information about the failure, inspect ``.status_code``, ``.reason_phrase``, and if the
	For more information about the failure, inspect ``.status_code`` and ``.reason_phrase``. If the

	server defines a custom exception model, ``.exception_model``.
	server defines a custom exception model, ``.exception_model`` contains the deserialized response.

	"""This test getting an object from a server which returns a defined exception object when the requested id does
	"""This test gets an object from a server which returns a defined exception object when the requested id does

	def test_get_model_returns_defined_exception(self):
	def test_get_model_raises_exception_with_deserialized_response(self):

	def test_get_model_throws_if_no_defined_exception_type(self):
	def test_get_model_raises_exception_with_no_deserialized_response(self):

Deserialize API responses for non 2XX status codes if defined #778

Deserialize API responses for non 2XX status codes if defined #778

Uh oh!

Conversation

da1910 commented Apr 15, 2025

Uh oh!

codecov bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Andy-Grigg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Apr 15, 2025 •

edited

Loading