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.

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

Add option to generate a fully sealed model in the JavaSpring generator #20503

Merged
merged 15 commits into from
Feb 19, 2025

Conversation

alex-nt
Copy link
Contributor

@alex-nt alex-nt commented Jan 19, 2025

Add option to generate a fully sealed model in the JavaSpring generator.
Related with #15586

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 || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (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.x.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.

@alex-nt alex-nt closed this Jan 19, 2025
@alex-nt alex-nt changed the title Add option to generated sealed interfaces for oneOf in the JavaSpring generator Add option to generated sealed model in the JavaSpring generator Jan 19, 2025
@alex-nt alex-nt changed the title Add option to generated sealed model in the JavaSpring generator Add option to generate a fully sealed model in the JavaSpring generator Jan 19, 2025
@alex-nt alex-nt reopened this Jan 19, 2025
Copy link
Contributor

@martin-mfg martin-mfg left a comment

Choose a reason for hiding this comment

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

Hi @alex-nt, thanks for the PR!

It generally looks good, but there are some special cases which need to be handled. When using your new feature on the input spec modules/openapi-generator/src/test/resources/3_0/oneOf_array.yaml, this is the generated code:

public sealed interface MyExampleGet200Response permits List<@Valid OneOf1> {
  • HTML encoding needs to be disabled
  • generics are not supported in the permits clause, afaik
  • List shouldn't be in the permits list (same applies to other classes not generated by us)
  • annotations are not supported in the permits clause, afaik

Could you fix these problems please?

It might also make sense to test with some other "oneOf" files from modules/openapi-generator/src/test/resources/3_0 to find further cases that need special handling.

@alex-nt
Copy link
Contributor Author

alex-nt commented Jan 27, 2025

@martin-mfg Thanks a lot, I'll look into all the oneOf samples, I only tested with the weirdest one and our own file 😅, that was quite naive from me.

@alex-nt
Copy link
Contributor Author

alex-nt commented Jan 27, 2025

  • generics are not supported in the permits clause, afaik
  • List shouldn't be in the permits list (same applies to other classes not generated by us)
  • annotations are not supported in the permits clause, afaik

@martin-mfg how should I handle these? Just skip them?

flake.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to include these changes? If yes that's fine. Just want to check, since you didn't include this in your initial PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran nix flake update as I wanted to make sure that it has the latest JDK11 build (got burned in the past in a minor jdk11 release). I can revert if that is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, leave it as it is. :) No need to revert.

@martin-mfg
Copy link
Contributor

how should I handle these? Just skip them?

Yes please. (Unless you have a better idea?)

@alex-nt
Copy link
Contributor Author

alex-nt commented Jan 31, 2025

@martin-mfg I tested with most of the oneOf samples and it seems ok, I will double check tomorrow with fresh eyes what I did with some test project, hopefully it is ok.

Regarding these 2:

generics are not supported in the permits clause, afaik

Now the collections are skipped, but can there be other generics items there? In the spec it does allow generics (without the parametrized type).

annotations are not supported in the permits clause, afaik

I can't figure out how an annotation would get in there, any test file I should use to test this scenario?

@alex-nt
Copy link
Contributor Author

alex-nt commented Feb 1, 2025

@martin-mfg so, I think it now works in all scenarios were code generation works for oneOf samples. Unfortunately I found 2 cases where irregardless of the useSealed flag it fails to compile (same thing happens with the latest released version). Seem to be previously existing bugs.

  1. oneOfDiscriminator.yaml
  • FruitAnyOfDisc.java
    @JsonSubTypes.Type(value = AppleAnyOfDisc.class, name = "AppleAnyOfDisc"),
    @JsonSubTypes.Type(value = BananaAnyOfDisc.class, name = "BananaAnyOfDisc")
    AppleAnyOfDisc and BananaAnyOfDisc are not generated, so it fails to compile.
  1. oneOf_reuseRef.yaml
    Banana.java and Apple.java do not implement the method from Fruit.java.
public String getFruitType();

Copy link
Contributor

@martin-mfg martin-mfg left a comment

Choose a reason for hiding this comment

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

@alex-nt The new changes mostly look good to me. In addition to the small comments below, there is one more problem: the generated code doesn't compile if the sealed keyword appears anywhere, because the generated pom.xml sets the Java version to 1.8, which is too old for the sealed keyword.

The problems from my earlier comment are all resolved now. 👍 To still answer your questions:

Now the collections are skipped, but can there be other generics items there?

No, I don't think so.

I can't figure out how an annotation would get in there

Since collections are skipped now, annotations should not be a problem anymore.

@alex-nt
Copy link
Contributor Author

alex-nt commented Feb 1, 2025

@martin-mfg did the cleanup, I hope it is ok (retested with some examples, seems good).

@@ -59,6 +59,8 @@ public class CodegenModel implements IJsonSchemaValidationProperties {
public Set<String> oneOf = new TreeSet<>();
public Set<String> allOf = new TreeSet<>();

public List<String> permits = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

can you please add a 1-liner explain what permits is?

Copy link
Member

Choose a reason for hiding this comment

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

is that "similar to" children (List<CodegenModel>) ?

Copy link
Contributor Author

@alex-nt alex-nt Feb 11, 2025

Choose a reason for hiding this comment

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

Similar but different. children contains the full list of children, even indirect children. permits contains only the direct descendants (first level).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 do you see this differently? Any suggestion on how I should change it? :D

@wing328 wing328 merged commit cd7cdb1 into OpenAPITools:master Feb 19, 2025
15 checks passed
@wing328 wing328 added this to the 7.12.0 milestone Feb 27, 2025
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.

3 participants