Skip to content

Do not use cached properties for additionalProperties#7955

Merged
spacether merged 2 commits intoOpenAPITools:masterfrom
spacether:fixesAddPropsBug
Nov 16, 2020
Merged

Do not use cached properties for additionalProperties#7955
spacether merged 2 commits intoOpenAPITools:masterfrom
spacether:fixesAddPropsBug

Conversation

@spacether
Copy link
Contributor

@spacether spacether commented Nov 16, 2020

Do not use cached properties for additionalProperties
This should fix the bug where additionalProperties tests are intermittently failing

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.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Core Team Members
@wing328 (2015/07) ❤️
@jimschubert (2016/05) ❤️
@cbornet (2016/05)
@ackintosh (2018/02) ❤️
@jmini (2018/04) ❤️
@etherealjoy (2019/06)
@spacether (2020/05)

@spacether spacether added this to the 5.0.0 milestone Nov 16, 2020
@spacether spacether requested a review from wing328 November 16, 2020 17:20
@spacether spacether merged commit 057647c into OpenAPITools:master Nov 16, 2020
@spacether spacether deleted the fixesAddPropsBug branch November 16, 2020 19:27
@wing328
Copy link
Member

wing328 commented Nov 17, 2020

@macjohnny FYI. This change has an impact to the TS generators and looks to be minor as only the order in the samples has changed.

@macjohnny
Copy link
Member

@wing328 I think the changes in the TS samples are due to this PR: #7894
it was not up-to-date with the master, so some samples needed to be re-generated. everything is fine.

@wing328
Copy link
Member

wing328 commented Nov 17, 2020

@macjohnny ah ok. Thanks for the clarification.

@reganheath
Copy link

reganheath commented Nov 24, 2020

I believe I am seeing intermittent failures in tests due to this issue.

I have a mirror of the openapi-tester, which has a branch, which has changes which diverge from "master" in some generators. It is otherwise identical to master as of commit b5ce7ce ("comment out angular v6, v7 tests") with the exclusion of commit 3bf8ca7 ("[python] Renames python generators (#7965)") - as I don't have time to deal with any issues this might cause to my use of python.

My CI runs "docker build -t TAG ." and this intermittently fails with assertions on lines

  • DefaultCodegenTest.java:2409
  • DefaultCodegenTest.java:2452
  • DefaultCodegenTest.java:2495

Or, in other words, lines like this..

assertEquals(mapWithAddPropsTrue.getAdditionalProperties(), anyTypeSchema);

Due to mapWithAddPropsTrue.getAdditionalProperties() being NULL

The failures happen when the schema in Q reaches the later stages without being resolved from the reference ($ref: #/components/schemas/AdditionalPropertiesTrue), to the thing it references (a map).

@wing328
Copy link
Member

wing328 commented Nov 24, 2020

@reganheath are you able to repeat the issue with the latest master?

@reganheath
Copy link

@wing328 No. I have not (yet) reproduced in our mirror of master (using the same CI process).

It has only happened for our branch, which has an additional generator plus changes to two more.

What is truly frustrating is that it didn't happen in the branch I used to merge the latest master changes into our branch, it only happened once it was merged.

Also, adding debug logging to a branch where it was occurring, caused it to also stop occurring, or occur less frequently such that it hasn't happened since.

Frustrating!

@wing328
Copy link
Member

wing328 commented Nov 24, 2020

@reganheath the issue should have been resolved in the latest master. Please pull it into your branch and let us know if you can still repeat the issue.

@reganheath
Copy link

reganheath commented Nov 24, 2020

@wing328 Sorry, I cannot do that at this time. Is there a specific commit you can point me at, which should have corrected this issue?

Edit: It's commit 5e02a5b isn't it?
Edit2: Yep, that one fixes it. For some reason, when I run the build/tests locally generateAliasAsModel is false, but in CI it's true. Go figure.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants