Skip to content

Conversation

@ludovicsteinbach
Copy link
Contributor

@ludovicsteinbach ludovicsteinbach commented Mar 4, 2025

Sister PR on openapi-client-template https://github.com/ansys/openapi-client-template/pull/235

Relax validation on deserialization of data for enums. Enums raises sufficiently helpful exceptions when provided with an invalid value.

@github-actions github-actions bot added the enhancement New features or code improvements label Mar 4, 2025
@ludovicsteinbach ludovicsteinbach requested a review from da1910 March 4, 2025 10:51
@codecov
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.43%. Comparing base (9dbe0d3) to head (ba15b85).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   94.43%   94.43%   -0.01%     
==========================================
  Files           7        7              
  Lines         791      790       -1     
==========================================
- Hits          747      746       -1     
  Misses         44       44              

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

Copy link
Collaborator

@da1910 da1910 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, if the API returns a valid result for an enum that's expecting a string then this should be fine

@Andy-Grigg
Copy link
Contributor

I have looked into potential backwards-compatibility issues with releasing this fix as a minor or patch release. TLDR, no issues.

The change itself I would argue is a bug fix: we are replacing an unhelpful empty assertion error with a more useful ValueError. It's a breaking change, but it's breaking incorrect functionality.

As far as it relates to breaking existing releases of PyGranta packages, I do not believe there are any cases where this is possible. Reasoning below:

  • There are two cases of non-string enums currently in all generated bindings. SystemNetHttpStatusCode and GrantaMISearchRecordPropertyFakeAttributeNumbers. GrantaMISearchRecordPropertyFakeAttributeNumbers is documented as being type: string and format: int32, but contains objects of strings and ints. It's not referenced by any other response type or any API, and so should probably be removed from the definition.
  • SystemNetHttpStatusCode, the cause of this issue, has been included in the generated bindings since at least 2024 R1. However, until the current dev release (2025 R2), it has been included in exception responses only.
  • OpenAPI-Common does not deserialize non-2xx responses, the handle_response function here
    def handle_response(response: requests.Response) -> requests.Response:
    raises an exception, and the exception class also does not deserialize the response.
  • As a result, the code being changed was never run pre-2025 R2.
  • There is a chance that there are other bindings outside of BAS and ServerAPI that exist, but in that case I would fall back to the position of this being a bug fix.

Making both the changes here and the changes to the Enum definition result in successful use of the bulk endpoint, with a warning that we're falling through to an undeserialized object as expected.

If I make only the changes in this PR, I get the helpful message ValueError: 200 is not a valid SystemNetHttpStatusCode. This is an error message that someone could see if they upgraded openapi-common to a version after this merge and maybe patched the code to deserialize the exception or generated their own bindings using an old version of the template.

If I make only the changes to the template, I get the unhelpful assertion

@ludovicsteinbach ludovicsteinbach changed the title Feat: support int enums Fix: relax validation on enum values deserialization Mar 21, 2025
@ludovicsteinbach ludovicsteinbach changed the title Fix: relax validation on enum values deserialization Relax validation on enum values deserialization Mar 21, 2025
@ludovicsteinbach
Copy link
Contributor Author

I unfortunately have to close this PR and recreate it with a different branch name to get it to correctly create the changelog as a bug fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New features or code improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants