Stackoverflow in toExampleValue() for python client by fbl100 · Pull Request #8054 · OpenAPITools/openapi-generator

fbl100 · 2020-11-30T21:30:29Z

Added a Set to the toExampleValueRecursive() method to detect circular references.
Added a watered down GeoJson API that, prior to this commit, would have thrown a StackOverflowException
Added a unit test (testRecursiveToExample()) and expected output confirming that the fix works.

@spacether this solution may be a little heavy handed. I tried not to introduce another parameter to the toExampleValueRecursive(), but this seemed like the cleanest way to do it.

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.

spacether · 2020-11-30T23:49:24Z

Hey there. Thanks for your PR. Yup adding a new parameter is the cleanest way to handle it.
Can you please run 'bin/utils/ensure-up-to-date' locally and commit changes? That will fix the CI error.

spacether

please regenerate the samples to eliminate the CI error

samples/client/petstore/python/docs/FakeApi.md

spacether · 2020-12-01T02:06:13Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java

+            return fullPrefix + modelName + closeChars;
+        } else if(modelName != null && !modelName.isEmpty()){
+            // if the model is non-null and non-empty, add it to the set
+            processedModels.add(modelName);


Shouldn't we only add a model when we have finished generating its sample?
What are our recursive base cases here for cycles?
How about naming them seenModels because we have not finished processing it yet.

I'll go back and take a look at this again, thanks. I didn't realize that the 'ran /bin/utils/ensure-up-to-date' was required and didn't pick up on the fact that my examples had changed that.

@spacether Got a CircleCI build error that I'm having a hard time interpreting. Do you mind taking a look and providing pointers for how to reproduce locally?

That error is unrelated. We. can ignore it.

fbl100 · 2020-12-11T19:56:58Z

@spacether We took another crack at the toExampleValueRecursive() issue.

spacether · 2020-12-14T23:12:54Z

modules/openapi-generator/src/test/resources/3_0/issue_8052_recursive_model_expected_value.txt

null is invalid in Python. Can you update this value? If the list is not explicitly nullable, then the example value should be [] (empty list). If it is nullable, then the example value can be None.

spacether

Thank you for making these changes. It is almost there!
Your tests sample includes null which is not valid python code.
Can you update the string null to be the string None?

fbl100 · 2020-12-18T14:44:04Z

Hi @spacether, We have updated the example value to be None in the recursive case.

spacether

Thank you for making that update. So looking at your spec None is not a valid value for geometries because geometries is type array and that array is not Nullable. Sorry for asking for None here earlier. Please change the empty value to be empty list for your output example. The only valid values for geometries as defined in your spec are:

[]
[one or more GeoJsonGeometry]

spacether · 2020-12-18T21:02:08Z

When /i is passed in can we turn on case insensitive in RgxGen? If you don't want to do this now we can leave it to the future.
The author of that library explained that we can use the CASE_INSENSITIVE option like this code does.

fbl100 · 2020-12-24T16:00:15Z

We changed the default value for the recursive item to:
None - if it's nullable
[] - if it's an array type
{} - if it's an object type

Added a Set<String> in toExampleValueRecursive() to keep track of which models we have generated to avoid an infinite recursion for recursive models. An example of a recursive model would be a GeoJson GeometryCollection.

fbl100 · 2020-12-28T13:56:11Z

Hello @spacether, I noticed that this PR had conflicts with the latest master. I went ahead and fixed the conflicts and pushed the latest. Let me know if you need any other changes.

spacether

Not: the Shippable CI error is unrelated
@fbl100 thank you for your PR and for making those updates. This looks great!

spacether · 2020-12-29T16:11:02Z

This PR broke master so I am reverting it. There is a java test failure that we missed

…n client (#8054)" This reverts commit fd02bc3.

…n client (#8054)" (#8293) This reverts commit fd02bc3.

fbl100 · 2020-12-29T17:42:38Z

@spacether everything passed on my side. Can you point me to the build log that failed? I'll figure it out.

spacether · 2020-12-29T17:53:26Z

Thank you so much! Here is a log
https://github.com/OpenAPITools/openapi-generator/runs/1622405497?check_suite_focus=true
By eye it looks like it may be an os newline issue.

spacether suggested changes Nov 30, 2020

View reviewed changes

spacether reviewed Dec 1, 2020

View reviewed changes

samples/client/petstore/python/docs/FakeApi.md Outdated Show resolved Hide resolved

spacether reviewed Dec 1, 2020

View reviewed changes

spacether linked an issue Dec 1, 2020 that may be closed by this pull request

[BUG] Stackoverflow in toExampleValue() #8052

Closed

6 tasks

fbl100 marked this pull request as draft December 3, 2020 17:06

fbl100 force-pushed the fix_issue_8052 branch from cacc4d6 to 47da5e5 Compare December 11, 2020 19:49

fbl100 marked this pull request as ready for review December 11, 2020 19:55

spacether reviewed Dec 14, 2020

View reviewed changes

spacether suggested changes Dec 14, 2020

View reviewed changes

fbl100 force-pushed the fix_issue_8052 branch from 47da5e5 to 0cd251d Compare December 18, 2020 14:40

spacether suggested changes Dec 18, 2020

View reviewed changes

spacether mentioned this pull request Dec 18, 2020

Are regex flags supported? curious-odd-man/RgxGen#57

Closed

fbl100 force-pushed the fix_issue_8052 branch from 0cd251d to 63ac823 Compare December 24, 2020 15:50

fbl100 force-pushed the fix_issue_8052 branch from 63ac823 to 3206b2b Compare December 28, 2020 13:53

spacether added the Client: Python label Dec 28, 2020

spacether added this to the 5.0.1 milestone Dec 28, 2020

spacether approved these changes Dec 28, 2020

View reviewed changes

spacether merged commit fd02bc3 into OpenAPITools:master Dec 28, 2020

spacether added a commit that referenced this pull request Dec 29, 2020

Revert "Fixes issue 8052: Stackoverflow in toExampleValue() for pytho…

38da252

…n client (#8054)" This reverts commit fd02bc3.

spacether mentioned this pull request Dec 29, 2020

Revert "Fixes issue 8052: Stackoverflow in toExampleValue() for python client" #8293

Merged

spacether added a commit that referenced this pull request Dec 29, 2020

Revert "Fixes issue 8052: Stackoverflow in toExampleValue() for pytho…

7d6063d

…n client (#8054)" (#8293) This reverts commit fd02bc3.

wing328 changed the title ~~Fixes issue 8052: Stackoverflow in toExampleValue() for python client~~ Stackoverflow in toExampleValue() for python client Feb 3, 2021

wing328 added the Issue: Bug label Feb 3, 2021

Uh oh!

Conversation

fbl100 commented Nov 30, 2020

PR checklist

Uh oh!

spacether commented Nov 30, 2020

Uh oh!

spacether left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacether Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbl100 Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

fbl100 Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

spacether Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

fbl100 commented Dec 11, 2020

Uh oh!

spacether Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether left a comment

Choose a reason for hiding this comment

Uh oh!

fbl100 commented Dec 18, 2020

Uh oh!

spacether left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacether commented Dec 18, 2020

Uh oh!

fbl100 commented Dec 24, 2020

Uh oh!

fbl100 commented Dec 28, 2020

Uh oh!

spacether left a comment

Choose a reason for hiding this comment

Uh oh!

spacether commented Dec 29, 2020

Uh oh!

fbl100 commented Dec 29, 2020

Uh oh!

spacether commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spacether Dec 1, 2020 •

edited

Loading

spacether Dec 14, 2020 •

edited

Loading

spacether left a comment •

edited

Loading

spacether commented Dec 29, 2020 •

edited

Loading