Skip to content

[python] Fixes a breakage while deserializing the read-only attributes#10155

Merged
spacether merged 15 commits intoOpenAPITools:masterfrom
CiscoM31:fix/readonly_breakage
Aug 18, 2021
Merged

[python] Fixes a breakage while deserializing the read-only attributes#10155
spacether merged 15 commits intoOpenAPITools:masterfrom
CiscoM31:fix/readonly_breakage

Conversation

@vvb
Copy link
Contributor

@vvb vvb commented Aug 14, 2021

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.3.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

#9409 added _new_from_openapi_data and updated the code to invoke this method at most places except one. This PR takes care of updating this as well.
Deserialization of read-only attributes breaks without this change.

@spacether @wing328

Added a test that validates the fix.

Here is a snapshot of the same test failing when run on master branch codebase,
test_failing
test_failing_1

Here is a snapshot of the test passing when run against this PR branch,
tests_passing

@wing328
Copy link
Member

wing328 commented Aug 14, 2021

cc @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11)

@wing328
Copy link
Member

wing328 commented Aug 16, 2021

@vvb thanks for the PR. Can you please add a test using Mole in the test spec or add a new schema with readonly properties to cover the case?

    Mole:
      type: object
      required:
        - blind
        - smell
        - hearing
      properties:
        blind:
          type: boolean
          readOnly: true
        smell:
          type: string
          readOnly: false
        touch:
          type: boolean
          readOnly: true
        taste:
          type: string
          readOnly: false
        hearing:
          type: boolean
        seeingGhosts:
          type: boolean

@vvb
Copy link
Contributor Author

vvb commented Aug 17, 2021

@spacether @wing328 I have taken care of the comments. I have also added a test that checks for this issue and attached snapshots of it passing against this branch, and failing against master (which does not have the fix yet).

Please check and update if anything more is required here.

@vvb vvb requested a review from spacether August 17, 2021 20:05
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. This looks great! Can you make the suggested small tweaks to:

  • make the if statements use the dict.get check of _spec_property_naming
  • remove the exception handling when importing module sin your test
  • update your test to also check that the dog data was turned into a Dog instance

@vvb
Copy link
Contributor Author

vvb commented Aug 18, 2021

@spacether took care of the comments. here are the test results post changes,
p1

@vvb vvb requested a review from spacether August 18, 2021 17:52
@spacether spacether added this to the 5.3.0 milestone Aug 18, 2021
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Thanks for making those updates. This looks good.

@spacether
Copy link
Contributor

Ci error is csharp-netcore and is unrelated

@spacether spacether merged commit 245aec1 into OpenAPITools:master Aug 18, 2021
@vvb vvb deleted the fix/readonly_breakage branch August 19, 2021 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants