-
-
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
[Java] internal config option to use jakarta or javax package #14310
Conversation
378fe94
to
a2f8e66
Compare
@@ -576,7 +576,8 @@ public void processOpts() { | |||
// The flag below should be set for all Java libraries, but the templates need to be ported | |||
// one by one for each library. | |||
supportsAdditionalPropertiesWithComposedSchema = true; | |||
|
|||
setUseJakartaEe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the plan to expose the option to users via the cli? SpringCodegen has the useSpringBoot3 indirection, which triggers the use of Jakarta EE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for now this would be internal config option. later I want to use this config for changes like this one - #14276
rest-template
to be migrated to spring 6 which means jakarta
should be used, but on the other hand rest-template
shares common model.mustache
with other libraries which still should use javax
package.
that's the reason why this config option is internal - developer of library's template is aware whether javax
or jakarta
package to be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it. But the default should then be false setUseJakartaEe(false)
, otherwise this PR would activate jakarta EE for all JavaClientCodegen implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh. got you question) expand this part of code and you will notice that this is only for jersey 3 which already use jakarta
package. by default this option is set to false
(i.e. we are using javax
package)
@borsch Thanks for applying this to all Java based Codegen implementation. LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the mustaches probably could have been left as-is if they can't support Jakarta anyway, but it doesn't hurt to have the configuration
@@ -576,7 +576,8 @@ public void processOpts() { | |||
// The flag below should be set for all Java libraries, but the templates need to be ported | |||
// one by one for each library. | |||
supportsAdditionalPropertiesWithComposedSchema = true; | |||
|
|||
setUseJakartaEe(true); | |||
applyJavaxPackage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... if you did setUseJakartaEe()
before the super
call, then you wouldn't also need to call applyJavaxPackage()
. But then you'd need to duplicate the if statement logic... maybe just some documentation?
// When using `setJakertaEe()` you should also call `applyJavaxPackage()` to capture the property value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@welshm I've slightly changed approach, please review one more time
a2f8e66
to
94038f3
Compare
@wing328 could you please merge this one? This will allow easier update to Spring 6 for some libraries |
@@ -6455,7 +6455,7 @@ public boolean convertPropertyToBoolean(String propertyKey) { | |||
return result; | |||
} | |||
|
|||
public void writePropertyBack(String propertyKey, boolean value) { | |||
public void writePropertyBack(String propertyKey, Object value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @OpenAPITools/generator-core-team
@@ -27,9 +27,9 @@ import java.io.IOException; | |||
import java.io.InputStream; | |||
|
|||
import java.net.URI; | |||
import javax.net.ssl.SSLContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javax.net.ssl
package ist unaffected by Jakarta EE.
This change should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for spotting. I've create PR for fix this #14341
Internal option to control whether
javax
orjakarta
package should be used. This option will allow much easier transition to never version of dependencies withjakarta
support.Example - #14276 rest-template to be migrated to
jakarta
while other libraries might still usejavax
packages in common templatesPR 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
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)