Search before asking
- [x] I searched in the issues and found nothing similar.
Describe the bug
When using Jackson along with the Android record support module, processing of record types fails when R8 (Android's code minifier) is used.
The real cause behind this problem lies not within Jackson, but is a consequence of the fact that R8 will rename all occurences of "java.lang.Record" to "com.android.tools.r8.RecordTag" when minification is enabled (for some reason, it does not happen when it is off - in that case everything works fine).
However, Jackson's ClassUtil.isRecordType detects whether a class is or is not a record by comparing the class name (which R8 replaces), thus erroneously deeming an Android record to be a Java record, which it is, in fact, not. Subsequently, an exception is thrown in JDK14Util when it tries to access Record-related methods on java.lang.Class, which are not present on Android.
I came up with a workaround of using "java.lang.Record".equals(parent.getName()) && !"com.android.tools.r8.RecordTag".equals(parent.getName()), which will always evaluate to false on Android. But given that Jackson by itself isn't Android-focused, I'm not sure if that is a proper solution. Perhaps calling class.isRecord() via reflection (and returning false if the method does not exist) would be better?
Version Information
2.19.0
Reproduction
- Create a
recordin an Android project and compile it with R8/Proguard enabled. - Attempt to deserialize the record using Jackson.
- An exception
Failed to access Methods needed to supportjava.lang.Record: (java.lang.NoSuchMethodException) getRecordComponents []will be thrown.
Expected behavior
Jackson should not consider records on Android to be actual Java records, even if R8 manipulates the class name strings.
Additional context
No response
Comment From: pjfanning
Can't you contact Android R8 team to see if there is a way to turn off minification of Records? jackson-databind is a Java lib. We don't want to end up with lots of Android specific workarounds. We have zero Android test coverage and I don't see any plan for us to create Android specific testing.
Comment From: pjfanning
It may be feasible to adjust ClassUtil.isRecord to make it return false for com.android.tools.r8.RecordTag but the real issue is that we don't have Android enabled test coverage to make sure that it works as expected and that it continues to work as expected in future.
Comment From: pjfanning
All that I know of is this wiki page - https://github.com/FasterXML/jackson-docs/wiki/JacksonOnAndroid
If anyone can suggest improvements to that wiki, it would be appreciated.
Comment From: HelloOO7
Can't you contact Android R8 team to see if there is a way to turn off minification of Records
Possibly, but barring the fact that it would probably be resolved no sooner than in a matter of months, the Record processing in R8/D8 is a rather hardcoded matter and there are probably already a lot of cases that rely on the current behavior (which is, as far as Proguard goes, "correct").
It may be feasible to adjust ClassUtil.isRecord to make it return false for com.android.tools.r8.RecordTag but the real issue is that we don't have Android enabled test coverage
That's fair. I agree that having Android-specific code in jackson-databind is not a great solution. What is the rationale behind checking for records using the class name? Would there be a way to use Java's isRecord instead? As far as I know, that would fix this problem and still be transparent implementation-wise.
Comment From: pjfanning
I think isRecord is written the current way so that the code can compiled with Java 8 and 11 (which don't have the Record class). I wonder if Jackson-Databind should try to support Multi-Release jar format so that we can have Java 17+ specific code? https://www.baeldung.com/java-multi-release-jar
Comment From: HelloOO7
That could be alleviated by calling the method via Reflection. If the method does not exist, then records are simply not supported (so it can return false), and if it does, then "isRecord" returns whether the class is an actual Java record. IIRC this is the way JDK14Util works.
That is not to say it's the end of all problems, as starting with Android U, the isRecord method is present in Android as well (see https://issuetracker.google.com/issues/293569157). At the moment, it should not break anything, as it does not return true for R8'd records, but not sure yet what will happen once these methods are implemented properly. I would not consider this to be an issue at the present time though.
Comment From: pjfanning
tools.jackson.core:jackson-databind 3.0.0-rc5 only supports Java 17+ - so it already uses the Class#isRecord method.
Comment From: HelloOO7
Realistically, we're not going to be targeting Android apps to Android 14 for the next couple of years, so I'm not very optimistic about being able to use Jackson 3.x on Android to be honest.
Any chance this could be done in 2.x? I could work on a PR for that if needed, provided that the Reflection approach would be acceptable.
Comment From: pjfanning
I'm working on a Jackson 2 PR.
Comment From: pjfanning
@HelloOO7 do you think #5195 would solve your issue?
Comment From: HelloOO7
Not entirely - it would only solve it on Android 14 and up, which have the isRecord method.
You mention that isRecord was added in JDK 16 - is that really so? It was part of the original JEP, so I would expect it to be present in JDK 14 as a preview feature as well. OpenJDK sources would confirm that: https://github.com/openjdk/jdk14/blob/abc56193174dd6a11a8453bd5036fc8618ecca61/src/java.base/share/classes/java/lang/Class.java#L3632
If it were possible to use isRecord universally (without the fallback, i. e. always returning false if isRecord is not available), then it would solve this issue.
Comment From: pjfanning
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Class.java#L3380-L3382 says Java16 - maybe Java14 had it only enabled if you enabled preview. See the annotations in the Java 14 code.
I don't really mind throwing Java 14 and 15 users under the bus. So I will remove the fallback.
Comment From: HelloOO7
maybe Java14 had it only enabled if you enabled preview
Indeed, but that should not be an issue, as preview-compiled classes will not run under JVMs without enabled preview features.
If you'd like, I can test your branch on my Android setup.
Comment From: pjfanning
@HelloOO7 feel free to try out my branch.
Comment From: HelloOO7
Can confirm that it fixes the issue, at least on Android 9 and 13 🎉
Comment From: cowtowncoder
I think isRecord is written the current way so that the code can compiled with Java 8 and 11 (which don't have the Record class). I wonder if Jackson-Databind should try to support Multi-Release jar format so that we can have Java 17+ specific code? https://www.baeldung.com/java-multi-release-jar
I really do not want to go Multi-Release route with tooling, so -1 for that.
Comment From: cowtowncoder
Quick note: there is jackson-module-android-record, added in 2.16:
https://github.com/FasterXML/jackson-modules-base/tree/2.x/android-record
would that help?
Comment From: HelloOO7
Yes, that module is what I'm using for record support on Android. Some time ago I PRd some code to it that used some new 2.18 features to automatically determine the default creator for them. However, that code would not run if Jackson deemed the class to be a Java record, which is what happens in the case of R8'd classes. The changes proposed in #5195 fix this, as they make Jackson not recognize Android records as Java records and the module then processes them properly.
Comment From: cowtowncoder
I am not sure I actually understand #5195 fix, now that I think about it -- is the idea to NOT recognize java.lang.Record for Android? But based that on existence (or lack thereof) of Class.isRecord()?
If so, I think I'd actually prefer adding MapperFeature for something like DETECT_RECORD_TYPE, instead of relying on inclusion (or not) of accessors in JDK.
Comment From: pjfanning
I am not sure I actually understand #5195 fix, now that I think about it -- is the idea to NOT recognize
java.lang.Recordfor Android? But based that on existence (or lack thereof) ofClass.isRecord()?If so, I think I'd actually prefer adding
MapperFeaturefor something likeDETECT_RECORD_TYPE, instead of relying on inclusion (or not) of accessors in JDK.
The idea is to rely on isRecord(Class<?> cls) instead of rolling our own custom check. This is what we already do in Jackson 3. Android has implemented isRecord(Class<?> cls) and it has whatever implementation suits Android. Jackson doesn't need to 2nd guess what that Android implementation is.
Comment From: HelloOO7
The proposed approach of using isRecord() will always return false for Android <14, which does not support Java records, and some Android-y implementation-specific result on newer versions. In contrast, the current approach would return true even for desugared records, as R8 changes all references (and therefore Jackson's check) to the java.lang.Record class name to com.android.tools.r8.RecordTag when building the executable. I'd argue that using isRecord() is more correct in that sense, moreover, it solves the issues that arise when Jackson considers desugared records to be Java records (which is what this issue is about).
Comment From: cowtowncoder
The idea is to rely on isRecord(Class cls) instead of rolling our own custom check. This is what we already do in Jackson 3. Android has implemented isRecord(Class cls) and it has whatever implementation suits Android. Jackson doesn't need to 2nd guess what that Android implementation is.
Based on @HelloOO7 's explanation, I don't think Android Record handling works quite the same way beyond Class.isRecord(cls) detection, does it? Specifically:
The changes proposed in https://github.com/FasterXML/jackson-databind/pull/5195 fix this, as they make Jackson not recognize Android records as Java records and the module then processes them properly.
so it's inverse of what code suggest it is doing? Or there is some other inconsistency here.
Does Android versions exists where either:
- No "proper" Java Record support exists, need jackson-module-android-record
- Actual Java Record support
in which case things are super confusing, yet make more sense wrt PR.
Comment From: pjfanning
I forgot about jackson-module-android-record - @HelloOO7 would it be feasible for you to try that?
https://github.com/FasterXML/jackson-modules-base/tree/2.x/android-record
Comment From: HelloOO7
No "proper" Java Record support exists, need jackson-module-android-record
Yes - this applies to all Android apps with minimum SDK <A14, which will always "desugar" Java records to so-called Android records, which are subclasses of com.android.tools.r8.RecordTag and use special annotations instead of getRecordComponents etc (as older Android VMs do not support these).
Actual Java Record support
Since Android 14, the Java APIs related to records now exist in Android and from what I know, apps that use it as the minimum SDK should use native Java records. However, no such apps exist in practice (except AOSP - but I doubt it uses records), so I'm not confident enough to say they will work OOTB. In theory, though, they should.
I forgot about jackson-module-android-record - @HelloOO7 would it be feasible for you to try that?
The tests I have conducted so far all relied on this module (as they used desugared records) - both serialization and de/serialization passed all checks my app needs, although that's hardly satisfactory coverage:)
Comment From: cowtowncoder
Hmmmh. Ok. As much as I dislike having to use Reflection to poke into JDK internal methods -- which may as well start failing on later JDK -- perhaps it is necessary.
But it also feels like something that could have non-trivial chance of breaking Something Else, so target branch of 2.x makes sense to me.
Comment From: cowtowncoder
I will merge #5195 fix as soon as Sonatype Central snapshot dep problem -- whatever it is -- gets resolved. CI is now all failing, unrelated to PR itself.