-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cpp-ue4] bug-fix issue 10205: response json parsing logging a error to unreal console when there is no actual parsing problem #10221
[cpp-ue4] bug-fix issue 10205: response json parsing logging a error to unreal console when there is no actual parsing problem #10221
Conversation
A rebase to development HEAD and subsequent rebuild and generation shows the Docs and Samples CI issue might be with development HEAD rather than this request. evidence after doing a rebase and generation locally:
likely culprit: #10193 |
@leith-bartrich the build failure issue due to outdated doc/samples has been fixed in the master. Please rebase or merge latest master into your branch to resolve the issue. |
FYI. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword contains keywords to close an issue automatically when the PR is merged. |
…eOnly' or 'required' into account and was logging a error to the console when there was no actual problem.
…appearance. Added baseName to comments for clarity of reading.
b6eb499
to
8c6392f
Compare
Rebased, rebuilt, regenerated and pushed. The docs and samples passed fine this time. I'm unclear as to if the ci/circleci failure is meaningful. It seems to be the parallel test that failed? trying out your suggested auto issue link: Also, @wing328 , Do you want me to squash commits in this kind of pull requests? Or leave them broken out? I already squashed some silly ones that were just me fixing white-space mistakes. But what about intermediate changes like moving comments around and such? |
modules/openapi-generator/src/main/resources/cpp-ue4/model-source.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/cpp-ue4/model-source.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/cpp-ue4/model-source.mustache
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/cpp-ue4/model-source.mustache
Outdated
Show resolved
Hide resolved
…plated version of TryGetJsonValue does actually handle the absense of the field correctly for TOptional.
…ter mirror the loop in WriteJson
8c6392f
to
e694431
Compare
Requested changes made, generated samples and docs, and pushed. One outstanding request remains based on what may be a misunderstanding on syntax highlighting. Testing locally on my codebase fixes the errant behavior in a satisfactory manner. Of note: the petstore sample doesn't seem to show any change here because it doesn't seem to use any writeOnly fields in its components. Which is likely why this case never came up. |
modules/openapi-generator/src/main/resources/cpp-ue4/model-source.mustache
Outdated
Show resolved
Hide resolved
Made requested change for conciseness. Tested against local codebase. Regenerated samples and docs. Pushed. |
Looks good for me. Thanks for the contribution! |
This is my proposed fix to #10205
Fixes #10205
In summary, rather than walk all vars, it walks required vars and non-required (optional) vars. And within the required vars, it differentiates between writeOnly and non-writeOnly vars. It allows optional or writeOnly vars to fail to parse (because they're absent) and not trigger a big red error log indication of a parsing failure. To be clear: The generated code should not yell in red about an error, when a server is actually following the schema. The balance here, is that it tries to parse the vars anyway, but doesn't yell unless the schema truly indicates their absence is critical.
There are many deficiencies here. e.g. A TryGetJsonValue call doesn't differentiate between a parse failure versus the absence of a value, in its return. Which forces a lowest common denominator solution here. e.g. It's not necessarily providing much runtime feedback where it could. However, I think both of those deficiencies are actually a design change, rather than a bug fix. They could be handled in a different issue and and discussed there.
Here, I'm only seeking to keep the generated code from logging errors for missing vars, that probably should be missing.
I moved the comments to keep the code looking nicer, since my original bug submission.
This change hasn't been tested against a large array of schemas. But it has been tested against the simple schema provided in the bug, against a dirt simple Django backend.
The code generated from the samples seems sane to me. But I'm not as familiar with that schema. Of note: the pet.yml model is really the only one that marks certain fields required for a response. However, this is why I think it should instead be logging more useful information to Verbose and VeryVerbose on a per field basis, rather than declare a ParsingFailure on the whole response object.
tagging: @Kahncode
PR checklist
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.
master
(5.3.0),6.0.x