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

[General] Remove defunct global property withXml from generator, docs, maven & gradle plugin #18568

Merged
merged 15 commits into from
May 27, 2024

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented May 3, 2024

Users of the plugins can just delete it (b/c the setting never transpired through to the actual generator), or move it into configOptions (given the generator supports XML, you can enable it this way) should they choose so.


Closes #3839
Closes #5764
Closes #8902
Likely closes #11578

The basic issue is that withXml is documented as a global property on the main web site as well as the maven plugin. However, no matter what value is provided, Spring or Java generators won't generate XML-related annotations (only when supplying it as a configOption), rendering it effectively useless.

This PR applies the global property to the CodeGen's additionalProperties – if (and only if) it not already defined via --additionalProperties (being a <configOption> in the maven plugin respectively).

Or as a config matrix:

Before

not provided as add. prop. / <configOption> --additional-properties withXml=false --additional-properties withXml=true
not provided as global prop. ✔️
--global-property withXml=false ✔️
--global-property withXml=true ❌ ❗ ✔️

After

not provided as add. prop. / <configOption> --additional-properties withXml=false --additional-properties withXml=true
not provided as global prop. ✔️
--global-property withXml=false ✔️
--global-property withXml=true ✔️ 👌 ✔️

Also fixes the maven plugin description for withXml.

Looking at d609893, i realized that many test cases generate files that they don't test against, so that was just a small optimization (that brought down the execution time for all tests in that file by ~20 % on my machine). Maybe out of scope and can be cherry-picked somewhere else if desired, however i realize there is a lot of potential here to bring down the test suite execution time overall.

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.1.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.

@Philzen Philzen requested a review from jimschubert as a code owner May 3, 2024 23:32
@Philzen
Copy link
Contributor Author

Philzen commented May 5, 2024

... of course, another option would be to remove withXml completely from the global options – which would lead to less code instead of more here, and at the end of the day, the generator implementation determines whether it does have any effect at all.

@wing328
Copy link
Member

wing328 commented May 6, 2024

thanks for the PR.

when you've time this week, can you please PM me via Slack?

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

@wing328
Copy link
Member

wing328 commented May 21, 2024

The basic issue is that withXml is documented as a global property on the main web site as well as the maven plugin. However, no matter what value is provided, Spring or Java generators won't generate XML-related annotations (only when supplying it as a configOption), rendering it effectively useless.

my take is to remove withXml from the global option and added it as a generator option instead to the generators that support XML payload.

@Philzen
Copy link
Contributor Author

Philzen commented May 21, 2024

my take is to remove withXml from the global option and added it as a generator option instead to the generators that support XML payload.

Fully agreed – the best code is code that can be deleted, meaning less complexity. Will rebase and commit the removal on top of my existing changes.

@Philzen Philzen force-pushed the fix/withXml-topic branch 2 times, most recently from 1decced to de6033a Compare May 21, 2024 16:30
@Philzen Philzen changed the title Fix global property withXml ["Soft" Breaking Change] Remove defunct global property withXml from generator, docs, maven & gradle plugin May 21, 2024
@Philzen Philzen changed the base branch from master to 8.0.x May 21, 2024 18:16
@Philzen
Copy link
Contributor Author

Philzen commented May 21, 2024

@wing328 Ready – realising this is a breaking change, i now rebased this against the 8.0.x branch. A couple of considerations arose on my side regarding this:

  • i realised there are >100 commits in master that have not gone into 8.0.x yet. Is there going to be a rebase for that anytime soon? Wondering what the official process here is (ie. shouldn't any PRs against master be immediately also picked into 8.0.x?)
  • if this goes into 8.x, maybe i should file a PR for master first that formally deprecates any withXml option, so we have a smoother transition? Of course, in that case we should hold this PR until that is merged and 8.0.x is rebased on master to get a clean removal.

@wing328 wing328 changed the base branch from 8.0.x to master May 26, 2024 09:55
@wing328 wing328 changed the base branch from master to 8.0.x May 26, 2024 09:55
@wing328
Copy link
Member

wing328 commented May 26, 2024

@Philzen thanks again for the PR.

can you please make this change based on current master instead as it's a breaking change with fallback and we don't need to wait for 8.0.0 major release to ship this change?

Looking at src/main/resources/go/model_simple.mustache and
src/main/java/org/openapitools/codegen/languages/GoClientCodegen.java
the GoLang seems to cater for withXml=true
Philzen added 11 commits May 26, 2024 23:54
Currently there is only a single reference to this value in the whole
codebase (GoClientOptionsProvider). Maybe we should re-think how this
file is organised (i.e. provide a clearer split / mapping / understanding
what are system properties vs. global properties vs. configOptions and
where to put them).
This is a "soft" breaking change: Plugin will no longer execute if
user have this option – which is good, b/c it never worked as expected.
We may want to hint this in the 8.0 release notes.
This is a "soft" breaking change: Plugin will no longer execute if
user have this option – which is good, b/c it never worked as expected.
We may want to hint this in the 8.0 release notes, so they can add it
to the `configOptions` map if required, or simply delete it
@Philzen Philzen changed the base branch from 8.0.x to master May 26, 2024 22:07
@Philzen Philzen changed the title ["Soft" Breaking Change] Remove defunct global property withXml from generator, docs, maven & gradle plugin [General] Remove defunct global property withXml from generator, docs, maven & gradle plugin May 26, 2024
@Philzen
Copy link
Contributor Author

Philzen commented May 26, 2024

can you please make this change based on current master?

@wing328 done – ready for review & merge at your discretion :)

@Philzen
Copy link
Contributor Author

Philzen commented May 26, 2024

as it's a breaking change with fallback

@wing328 BTW – how can i add labels to my own existing PRs or issues? I've seen we have a label for that, i would have otherwise already put it on here.

@wing328 wing328 added Breaking change (with fallback) Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc. labels May 27, 2024
@wing328 wing328 added this to the 7.7.0 milestone May 27, 2024
@wing328 wing328 merged commit 9c999b6 into OpenAPITools:master May 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change (with fallback) Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc.
Projects
None yet
2 participants