Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Java][Spring][Issue: 18929] fix missing JsonIgnoreProperties import in oneOf interface file #18930

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Java][Spring][Issue: 18929] fix missing JsonIgnoreProperties import in oneOf interface file #18930

wants to merge 1 commit into from
+12,421 −37

Conversation

ondrej-simon
Copy link

@ondrej-simon ondrej-simon commented Jun 14, 2024 •
edited
Loading

Fixes #18929

@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08)

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/configs/*.yaml
./bin/utils/export_docs_generators.sh
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.7.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@welshm
Copy link
Contributor

welshm commented Jun 14, 2024

Can you generate the samples and include as part of this review?

./bin/generate-samples.sh ./bin/configs/*.yaml

@ondrej-simon
Copy link
Author

Hi @welshm , I ran the scripts locally before committing and it lead to no changes within the repository. I assume that was the expected outcome?

I will run them again in the evening, to make sure I did not miss anything.

@welshm
Copy link
Contributor

welshm commented Jun 14, 2024

Hi @welshm , I ran the scripts locally before committing and it lead to no changes within the repository. I assume that was the expected outcome?

I will run them again in the evening, to make sure I did not miss anything.

I will have to check if there are any samples that include Jackson. If there are, I would expect changes.

@ondrej-simon
Copy link
Author

@welshm , I only got to it today. There were some Jackson samples, anyway I added an example for the scenario I was fixing, and (re)generated the sample files. They are part of the second commit.

ondrej-simon
ondrej-simon commented Jun 15, 2024
View reviewed changes
...eof/src/main/java/org/openapitools/model/CreateParkAnimalCarerPersonResponsibleForOneOf.java Outdated
import java.util.Objects;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
Copy link
Author

Choose a reason for hiding this comment

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

This is the annotation which is missing without my fix, and which is the cause of the bug. This sample proves the code is generated correctly.

@ondrej-simon ondrej-simon force-pushed the b/18929-oneOf-within-oneOf branch 3 times, most recently from 0d8e6a1 to 02d9562 Compare June 15, 2024 10:06
welshm-ideogram
welshm-ideogram reviewed Jun 15, 2024
View reviewed changes
Copy link

@welshm-ideogram welshm-ideogram left a comment

Choose a reason for hiding this comment

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

Java change looks good - I think you need to rebase or merge master branch to remove other changes unrelated to this PR

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java
@@ -2388,7 +2388,7 @@ private boolean shouldBeImplicitHeader(CodegenParameter parameter) {
@Override
public void addImportsToOneOfInterface(List<Map<String, String>> imports) {
if (additionalProperties.containsKey(JACKSON)) {
for (String i : Arrays.asList("JsonSubTypes", "JsonTypeInfo")) {
for (String i : Arrays.asList("JsonSubTypes", "JsonTypeInfo", "JsonIgnoreProperties")) {
Copy link

Choose a reason for hiding this comment

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

@wing328 FYI as this will affect all Java codegen.

This is copying the behaviour that already exists in Java client though

Copy link
Author

Choose a reason for hiding this comment

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

I removed the overrides from JavaClient and JavaHelidon, which were duplicates of this exact implementation. I presume this is what led to the bug in the first place - someone edited only the override in specialization and forgot about the parent method.

samples/client/others/rust/hyper/oneOf/src/apis/client.rs Outdated
@@ -6,6 +6,7 @@ use super::configuration::Configuration;
pub struct APIClient {
Copy link

Choose a reason for hiding this comment

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

You might need to rebase / merge main - I wouldn't expect to see any changes for Rust

Copy link
Author

Choose a reason for hiding this comment

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

The changes for Rust (among others) are caused by me adding new sample configs - the bugfixed scenario was previously not present in any of the examples.

Rebased onto upstream master, force pushed to remote as a part of good practice.

Copy link
Author

Choose a reason for hiding this comment

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

Rebased again, resolved conflict.

@welshm
Copy link
Contributor

welshm commented Jun 16, 2024

Change LGTM - approved from wrong account 🤦

@wing328 wing328 added this to the 7.7.0 milestone Jun 19, 2024
@wing328
Copy link
Member

wing328 commented Jun 26, 2024

@ondrej-simon thanks for the PR.

can you please PM me via Slack to discuss this PR when you've time ?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

@ondrej-simon ondrej-simon force-pushed the b/18929-oneOf-within-oneOf branch 3 times, most recently from 9d51e87 to 69867ac Compare July 1, 2024 06:05
@ondrej-simon
Copy link
Author

Unfortunately some build checks have failed. Seems like I may have uncovered another bug caused by this complicated scenario, which comes to filename generation.

An interface CreateParkAnimalCarerPersonResponsibleForDtoOneOfDto is generated, but it is placed into CreateParkAnimalCarerPersonResponsibleForDtoOneOfDtoDto.java file, there is "DtoDto" in the file name, twice instead of once, hence why build does not work.

Is it possible to get some assistance with this? I do not know what to look for in the project.

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

@welshm welshm welshm approved these changes

@welshm-ideogram welshm-ideogram welshm-ideogram approved these changes

Assignees
No one assigned
Labels
Issue: Bug Server: Spring
Projects
None yet
Milestone
7.7.0
Development

Successfully merging this pull request may close these issues.

[BUG][Java][Spring] Missing JsonIgnoreProperties import when using oneOf within oneOf
4 participants
@ondrej-simon @welshm @wing328 @welshm-ideogram