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

Fixup of initial jt-264-basics merge PR #282 #286

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

laurentschoelens
Copy link
Collaborator

Fixing build issues of PR #282 : build is now successfull on jdk8/11/17 with maven profile -P all

Renaming all modules to JAXB Tools :: ...
Fixing build dependencies of samples / tests of jaxb-maven-plugin on jaxb2-basics

Also fixes highsource/jaxb2-basics#130
Also fixes build issue (reflection access disallowed for URI class) on jdk17 (creating new internal class)

@@ -267,6 +270,74 @@ protected Object copyInternal(ObjectLocator locator, Cloneable object) {
}
}
}
protected Object copyInternalJdk11(ObjectLocator locator, Cloneable object) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method has a lot of copy+paste from the other copyInternal. Perhaps we can just add an argument to adjust behavior to the original method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to change existing behaviour for jdk8 / jdk11 which makes it works well.
The exception is only thrown with jdk17 since jdk16 has locked more aggressively the internal jdk classes.
This can be the default impl with jakarta-jaxb4 since baseline will be jdk11 (jaxb4 requires jdk11)

Copy link
Collaborator

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

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

This looks good! Please add a unit test for the failure case (ie.. class w/ Cloneable interface, but no clone() method) and we'll be good to go!

@laurentschoelens
Copy link
Collaborator Author

This looks good! Please add a unit test for the failure case (ie.. class w/ Cloneable interface, but no clone() method) and we'll be good to go!

Done, see org.jvnet.jaxb2_commons.lang.tests.DefaultCopyStrategyTest

Copy link
Collaborator

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

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

LGTM

@mattrpav mattrpav self-assigned this Aug 4, 2023
@mattrpav mattrpav added this to the 2.0.4 milestone Aug 4, 2023
@mattrpav mattrpav merged commit 949454e into highsource:master Aug 4, 2023
@laurentschoelens laurentschoelens deleted the jt-264-basics-fixup branch September 12, 2023 07:13
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.

InaccessibleObjectException
2 participants