-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Do not require the usage of opens in a modular app when using records. #3352
Comments
This problem might be a side-effect of some other issues wrt handling of Does this problem occur with any Record type, even ones with no annotations, deserialize using basic default |
I can confirm that Jackson 2.13.1 is unable to deserialize public records without opening the package. |
My setup: ObjectMapper mapper = new ObjectMapper(new CBORFactory());
mapper.readValue(serialized, MyRecord.class) |
Ok but here's my question: wouldn't "opens" always be required for Reflection to work both for:
and if so access must be given, does not exist by default. |
@robertvazan use AccessType=Property instead of field ;) This behavior is correct for strict encapsulation, no bug. |
Put this on your record ;)
|
@cowtowncoder using ASM to proxy records into the current module and avoid opens clauses adds too much overhead, I believe the constraints of strict encapsulation must be adhered to, there is a lot of risk in trying to avoid java locking people out of invalid operations |
Ok I am just not 100% sure I understand what exactly is allowed and what is not, by default. If introspection of public accessors (field, methods), reflective access are ok, then that'd be fine. As to Records tho, with 2.13 at least, no access should be made for fields... I wish I had more time to dig in this as there is no need by Jackson to use fields in case of records. But I didn't think they'd be accessed with latest versions. |
I had asked a slightly unrelated question on the amber mailing list a while back about records and got some clarification:
Key point: The idea is that, in a method, if you are able to see a record, you can create it Based on this I don't think there is much risk in future java versions locking people out from serialising/deserialising public records which are in packages which are explicitly exported in the
This is the crux of the matter... can someone please answer definitively? |
@GedMarc I don't want to add annotations to the records themselves, because they are part of public API, but I have tried the following on private static final ObjectMapper mapper = new ObjectMapper(new CBORFactory())
.setVisibility(PropertyAccessor.FIELD, Visibility.NONE)
.setVisibility(PropertyAccessor.GETTER, Visibility.PUBLIC_ONLY)
.setVisibility(PropertyAccessor.SETTER, Visibility.PUBLIC_ONLY)
.setVisibility(PropertyAccessor.CREATOR, Visibility.PUBLIC_ONLY); I have also tried These are public records in exported package, so reflection should work on them according to setAccessible documentation:
|
@vab2048 Noted - Yes if the package is exported and not sealed, the API is public and should be accessible across the module-loader, I notice two things here, the first is the CBOR factory, So I'm checking the order of the calling for that particular addon, if field access isn't required, checking the field access is not necessary and that might be triggering the invalid access, the other is if the property accessor is being applied in the factory @cowtowncoder did say earlier records shouldn't do field access at all, I think he's coming up with a plan around that, but the cbor factory is very important to know in debugging this, thanks! |
If there was a way to write a unit test (for maybe My main suspicion would be that code that does property discovery will attempt to force access, perhaps as a "backup" setter via field. If I did have time to proceed with Property Introspection rewrite (long-time plan), this could help here too but... as of now, my time to work on that is rather limited, unfortunately. |
@cowtowncoder Why do you think the test would be hard to write? All you need is a tiny Java 16+ project with |
Sorry, I meant to suggest that getting suitable information out on offending access seemed non-trivial. Not triggering of the issue. Well, that, and then running it from IDE -- I have had some problems with module-info settings wrt test code (esp. with Eclipse). But I'd be happy to find my concerns unfounded. :) |
@cowtowncoder Well, exception is trivial to obtain:
But I notice that Jackson fails to include the original exception in exception chain. I set breakpoint on
Member being accessed is field rather than corresponding getter method, which is also apparent from the exception message. Here I got lost. I have no idea what makes Jackson use fields or getters. I tried to add |
You cannot test module-info viability in any unit test unfortunately, right now the only real way is to create a JLink project and monitor the outputs, what may be working for you in class path mode, will not as a native runtime. The results from unit tests are not on a modular environment but still run in class path and you are getting away with a lot of incorrect implementations which only appear when turning it off. Not an easy task by any means :) |
@robertvazan Thanks, that is useful -- in earlier cases I have only seen warnings wrt access; those do not have stack traces. This does leave the issue @GedMarc mentions, but I think that as long as it need not be unit from Now as to fields, setters: since Jackson's Record support is based on earlier POJO access, it does consider Fields as potential secondary (or tertiary) way to set property values during deserialization -- that is, each logical property can have one or more modifiers. For Records we do not really need that but I suspect that reuse of earlier functionality leaves some of these access changes in place (i.e. code does not have special handling for Record type in relevant places). |
@GedMarc I am observing these exceptions in unit tests of the project that defines the records, so unit tests definitely do use modules. The only gotcha I can see is that automatic modules don't work with workspace dependency resolution (in Eclipse at least), so locally modified Jackson has to be installed in local maven repo before it can be tested. I am not sure about the MRJAR |
naw @robertvazan it doesn't quite work like that :) There are a lot of restrictions put in place on the module-info that take place outside of running on a classpath (are you running jdk11^ apps on a JDK?) - if you are using jar files, you are on a classpath - with the -m flag or not, With the phasing out of this execution mode, the restrictions are being brought in slowly into classpath mode to assist with people moving over Also take note of how the service loader changes as well between the two operational modes We have a jdk11-compat-test which generates the final artifact and performs all necessary validations on the module-info so it works on module "mode", which I am updating, I'm just going through a job change right now so it's going a little slow The other thing we need to check on is sealed packages (package-info) and sealed classes, and it's effect on the library as well, especially with records - I hope this clears a few things up, and maybe highlights a misstep in some of the assumptions? |
@GedMarc Thanks for stating all these upcoming issues (and the sealed class issue). It is interesting to me as an observer and user of Jackson to see how it will evolve. @cowtowncoder I don't have the faintest clue of how to write a unit test for Jackson but I made this repo here where you can reference the problem with an illustrative integration test. Hope it helps. |
@vab2048 thank you! Np wrt unit tests part (first things first). I hope to find time to look into this issue in near future (there are a few others on my longish todo list) as it seems possible we might have relatively easy improvements to make. But need to make sure there's a way to verify results with reproduction. |
@vab2048 On sample repo: I may have missed it, but which JDK is required for it? JDK 17? And is the command to use
or something else? With JDK 17 I get failure like so:
|
@cowtowncoder apologies - I was running the test directly in IntelliJ 2021.3.1 (using the JUnit launcher) and it was displaying fine like so: I have now since updated the build file so Hope this helps. |
Thank you! It does; test now runs, fails, and results show the stack trace. |
Interestingly enough, skipping collection of Fields for Record types, in It could even be that the annotations added in Record declaration are associated with underlying fields (I don't remember if this is the case or not). If so, Fields need to be collected first for the annotations that are then associated with getters and Constructor. So I'll have to think of a less direct route; collecting Fields but preventing their use as Mutators. |
@vab2048 disabling this should fix your issue: http://fasterxml.github.io/jackson-databind/javadoc/2.13/com/fasterxml/jackson/databind/MapperFeature.html#ALLOW_FINAL_FIELDS_AS_MUTATORS. |
@cowtowncoder Just tested this in a project with a record, using |
Ok. Closing this then, thank you @Blacklands. |
Is your feature request related to a problem? Please describe.
When using records in a Java modular app you must add opens declarations to the
module-info.java
for the package which contains the records you want to be serializable/deserializable:If you do not do this, when it comes to using an object mapper you will fail to deserialize with the error:
Describe the solution you'd like
Since it is a record I would like for there to be no need to provide 'opens' clauses in the
module-info.java
file.I do not know if this is actually possible.
The text was updated successfully, but these errors were encountered: