-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add Android Record Module #227
Conversation
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.
Could you use the animal sniffer maven plugin to ensure that we support a specific Android SDK version? It would be useful to set the android version to the lowest one that works - so that we can document the minimum supported Android SDK version.
See https://github.com/FasterXML/jackson-core/blob/2.16/pom.xml - jackson-core tests that we support Android SDK version 26.
https://www.mojohaus.org/animal-sniffer/animal-sniffer-maven-plugin/
Done. I put the version settings in the main POM file. Is that correct? |
.constructType(parameter.getParameterizedType()), null, null, null, null, i, null, null); | ||
} | ||
|
||
((StdValueInstantiator) defaultInstantiator).configureFromObjectSettings(null, null, null, null, |
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.
This looks dangerous... having mostly null
s, and overwriting internal state.
Ideally I'd rather create a new instance (even if of type StdValueInstantiator
) instead of forcibly trying to modify passed-in instantiator
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.
I've improved the Property construction code. Please take a look.
Regarding modifying the instantiator, I followed this example. Should I use a different pattern?
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.
Will do. I think that usage pattern is probably fine -- will add a comment if I remember reasons to use different approach.
...oid-record/src/main/java/com/fasterxml/jackson/module/androidrecord/AndroidRecordModule.java
Outdated
Show resolved
Hide resolved
...oid-record/src/main/java/com/fasterxml/jackson/module/androidrecord/AndroidRecordModule.java
Outdated
Show resolved
Hide resolved
With the goal of maximizing consistency with built-in record support, I copied and "desugared" some unit tests from https://github.com/FasterXML/jackson-databind/tree/2.16/src/test-jdk17/java/com/fasterxml/jackson/databind/records. A few of the test cases are failing, and I marked them with a "Failing" comment and a "notest" name prefix. I'm hoping for guidance about whether and how I should fix them. Fixed handling of getters Added support for injected values Added use of constructor parameter names Skip module if class already has a withArgsCreator
With the goal of maximizing consistency with built-in record support, I copied and "desugared" some unit tests from jackson-databind. A few of the test cases are failing, and I marked them with a "Failing" comment and a "notest" name prefix. I'm hoping for guidance about whether and how I should fix them. |
That sounds good. I don't think you necessarily need to copy all tests, although I guess number of tests is not huge in grand scheme of things. It's just that there's then extra maintenance needed. |
Ok I will try to go over code again; I like how things are developing and my only concern is wrt 2.16.0 release whether we can make this ready to go in. But in the meantime one thing that I will need to merge is CLA (assuming you haven't sent one earlier @eranl ?). https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and the usual way is to print, fill & sign, scan/photo, email to Looking forward to merging this & once again thank you for contributing it @eranl ! |
What should I do about the failing tests? Fix them (I'd need guidance on that)? Delete them? Move them to a |
Is this ok? |
public void setupModule(SetupContext context) { | ||
super.setupModule(context); | ||
context.addValueInstantiators(AndroidRecordModule::findValueInstantiator); | ||
context.setClassIntrospector(new AndroidRecordClassIntrospector()); |
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.
Hmmmmh. This is somewhat problematic as it does not compose; ideally Modules wouldn't have to override it, as only one module can really change it (others would override it).
I'll need to think about this a bit: I can see why this is necessary here, and how it otherwise is quite elegant approach. Just sub-classing part is challenging wrt (lack of) composeability.
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.
Does that mean that my statement in the doc that "this module is a no-op when no Android-desugared records are being (de)serialized" is not accurate anymore? That is problematic.
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.
Only if some other module tries to override ClassIntrospector
-- it is not problem if this is the only module to do so.
So unlike, say, AnnotationIntrospector
s that can be chained, there's no existing mechanism to combine ClassIntrospector
s
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.
How common is it for modules to do that? Should I document this limitation?
Also, can/should I implement a ClassIntrospector
chaining feature?
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.
@eranl It might be best to add something in README to mention this.
I don't remember any other module overriding it, so this is theoretical problem, not actual one.
(at least wrt modules under FasterXML)
On chaining... I thought about that, but in general I'm not sure how commonly this is needed etc.
Due to timing it needs to wait until 2.17 (that is, post 2.16.0 and cannot go in a patch) so there's no hurry.
So what I am thinking is that for 2.17 we could consider various approaches to remove this problem; and not have to make last minute changes that may turn out problematic.
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.
@eranl It might be best to add something in README to mention this. I don't remember any other module overriding it, so this is theoretical problem, not actual one. (at least wrt modules under FasterXML)
Done. see #230.
On chaining... I thought about that, but in general I'm not sure how commonly this is needed etc. Due to timing it needs to wait until 2.17 (that is, post 2.16.0 and cannot go in a patch) so there's no hurry. So what I am thinking is that for 2.17 we could consider various approaches to remove this problem; and not have to make last minute changes that may turn out problematic.
Sure, I meant in a future version. Please let me know when is a good time, and what's your preferred approach.
...oid-record/src/main/java/com/fasterxml/jackson/module/androidrecord/AndroidRecordModule.java
Show resolved
Hide resolved
...oid-record/src/main/java/com/fasterxml/jackson/module/androidrecord/AndroidRecordModule.java
Show resolved
Hide resolved
@eranl I think that due to timing constraints -- I really need to get this merged ASAP, if it is to make it in 2.16.0! -- I am ok with the implementation as is; we can tackle some changes in 2.17. But one thing that must occur before I can merge this is the CLA (see my earlier comment). As to failing tests; yes, moving them under |
Moved failing tests to 'failing' package. Added test for differing generic parameter types. Pruned BaseTest and BaseMapTest. Added comment about '-parameters' compiler option.
Sent.
Done.
Done. |
<inherited>true</inherited> | ||
<configuration> | ||
<compilerArgs> | ||
<!-- Module uses constructor parameter names (and types) to identify the canonical constructor --> |
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.
Ah. Because unlike Records there's no specified metadata to fix ordering, so names need 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.
Right. I've been wondering if I should document this requirement, but based on my local testing, it seems that the android build system always stores parameter names for desugared record canonical constructors in class files, at least with SDK 34.
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.
Added note to doc in #230.
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.
Thank you @eranl --merged!
@eranl My apologies: I don't think we have |
Sent. |
Add a module that allows deserialization into records using the canonical constructor on Android, where java records are supported through desugaring, and Jackson's built-in support for records doesn't work, since the desugared classes have a non-standard super class, and record component-related reflection methods are missing.
See Android Developers Blog article
Note: the canonical record constructor is found through matching of parameter types with field types. Therefore, this module doesn't allow a deserialized desugared record class to have a custom
constructor with a signature that's any permutation of the canonical one's.
See FasterXML/jackson-databind#4166
This is the simplest implementation I could make. Please let me know if I should handle more cases.