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

Fix #3906 by forcing Record constructors to be visible regardless of visibility overrides #3944

Closed
wants to merge 1 commit into from

Conversation

cowtowncoder
Copy link
Member

Fixes #3906 as per title.

@cowtowncoder cowtowncoder added 2.15 Record Issue related to JDK17 java.lang.Record support labels May 21, 2023
@cowtowncoder
Copy link
Member Author

@yihtserns WDYT?

@yihtserns
Copy link
Contributor

As noted by this comment, private canonical constructor (i.e. for private record XXX(...) { }) is visible for deserialization before #3724 (tested & confirmed using 2.14.2).

Do we want to change NON_PRIVATE to ANY to preserve that behaviour?

@yihtserns
Copy link
Contributor

(Sorry for going MIA with regards to #3906 - besides day job, I was under the impression that it wasn't urgent so I've been researching for a different way to fix that)

@cowtowncoder
Copy link
Member Author

@yihtserns Hmmh. Good question. I don't know to be sure; perhaps?

But I also noticed that there's one regression here, for RecordImplicitCreatorsTest: doing this will now expose implicit factory methods (valueOf()).

And NP for no follow up: I agree that this is not the most important thing to tackle. And with the issues there are maybe another approach would be useful.

@CatarinaRibas
Copy link

Hi,

Can you please fix this problem? I think this is the reason for some troubles that we have been having.

@cowtowncoder
Copy link
Member Author

Unfortunately this breaks some tests. So someone with time should figure out what the problem is, and whether this fix is actually safe to apply or not. I can't (and shouldn't) fix in PR that breaks existing tests.

@CatarinaRibas perhaps you have time to dig into this to help?

Discussion on #3906 side may be a good place to continue discussions on people involved; I created PR but am not most knowledgeable here (nor have time right now)

@cowtowncoder cowtowncoder force-pushed the tatu/2.15/3906-record-ctor-visibility branch from 6b441ed to 018372e Compare June 13, 2023 23:42
@yuzawa-san
Copy link

@cowtowncoder looks the failures are related to this. the expected result for that is causing a single test (based around that specific disabled AUTO_DETECT_CREATORS case) to fail.

i got the tests to pass when i changed the condition in the new code to

ClassUtil.isRecordType(baseType) && isEnabled(MapperFeature.AUTO_DETECT_CREATORS)

@cowtowncoder
Copy link
Member Author

Superceded by #4057 (targeting 2.16, too risky for patch release)

@cowtowncoder cowtowncoder mentioned this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Record Issue related to JDK17 java.lang.Record support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants