@JsonNaming does not work with Records
#2992
Comments
GregoireW
added
the
to-evaluate
|
I cannot quite reproduce this with the current 2.12.1-SNAPSHOT, but I do get different error, if Record is declared as a method-local class like in your example: this is not usage that is or can be supported.
So you'd need to declare Record as regular inner class (they are always static, at least) or as stand-alone type. |
|
Putting a record as stand alone type in is own file and setting From what I read in the code, Jackson first create an object from the constructor, filling parameters with properties it can match by name (setting null if it can not), then apply the naming strategy which may override some attributes values (the default naming strategy which is based on property name means there is nothing to do in the second step in case of a record). Record are immutable, the second phase will always fail if there is at least one attributes which name do not match the name in the json. Your test is success because there is a "bug" in jdk14 (record are not so immutable after all in JDK14). With JDK15 your test fail ( I use JDK15, and I was able to have a green if I use JDK14 and red if I use JDK15. Turns out there is a quote in the JEP record second preview, JDK15
On the record JEP for JDK16 (Release version) the quote become:
Even if the "IllegalAccessException" is not mention, I think it means it will be kept that way (There is nothing about relaxing this in the 'refinement' section). On a positive note, the record in method local is fine, this test is success for both JDK14 and JDK15. @Test
void tryLocalMethodRecord() throws Exception{
ObjectMapper mapper=new ObjectMapper();
record Test(String myId, String myValue){}
var src=new Test("id", "value");
String json=mapper.writeValueAsString(src);
assertThat(json).contains("\"myId\":\"id\"", "\"myValue\":\"value\"");
var after=mapper.readValue(json, Test.class);
assertThat(after).isEqualTo(src);
} |
|
@GregoireW I don't think your explanation of how you think naming strategy works is quite accurate: naming strategy is used as part of constructing deserializer and modifies "true" names used and expected -- it is not used dynamically during deserialization itself (that would be rather inefficient) (sidenote: What is interesting, on method local Records, is that I do get actual failure: this is why I suggested it as likely root cause (it is something reported for POJOs every now and then -- but it does make sense Records differ a bit wrt static-ness) ) Anyway. If above is true, this is unfortunately much much more difficult problem to solve, and probably something I cannot fix for 2.12.x at all (due to high risk of refactoring). But first things first... good things is that as per your comments, while I cannot reproduce this with JDK 14 (latest I have installed), I can then get 15 installed (even if it's not LTS at least it'd be ok for reproduction). ... although while removing Fields might be the right thing to do, for longer term, it would also immediately "break" existing users, use cases, for Records with JDK 14 and Preview option. (*) See f.ex #2974 -- not specific to Records, but to creator method handling for POJOs as well -- due to creator parameters not being recognized unless explicit annotated with |
|
I only scratch the surface on how it works, so yes I more guess on what I saw than really drill down the code.
I was guessing so... Unfortunately this one need a drill down to multiple aspect of this library, I will not have time to check what can be done. |
|
@GregoireW thank you for reporting the issue and digging pretty deep actually -- constructor-handling part of property introspection is a mess. Apologies for lecturing on internal workings, your educated guess on behavior is pretty good considering symptoms. But more importantly I had completely missed the possibility that just because use cases seemed to work, they might have worked for the wrong reasons: that is, use of fields was intended to be completely avoided with 2.12.0. It sounds like this has not happened. This is not completely unexpected, for what that is worth, and the specific part of functionality (discrepancy between initial POJOPropertiesCollector detection and later Basic/BeanDeserializerFactory gathering of Creator methods) has been known to be in need of rewrite for a while. 2.12 did some rewriting of latter part but I am bit afraid of full rewrite and was wondering if it could be only attempted for 3.0. So that's a long of saying "thank you very much for doing what you did already". :) |
|
btw, I did a test with toolchains to check what was possible to do. If you setup a <?xml version="1.0" encoding="UTF-8"?>
<toolchains xmlns="http://maven.apache.org/TOOLCHAINS/1.1.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/TOOLCHAINS/1.1.0 http://maven.apache.org/xsd/toolchains-1.1.0.xsd">
<toolchain>
<type>jdk</type>
<provides>
<version>1.8</version>
<vendor>sun</vendor>
</provides>
<configuration>
<jdkHome>/opt/java/jdk1.8.0_202</jdkHome>
</configuration>
</toolchain>
<toolchain>
<type>jdk</type>
<provides>
<version>14</version>
<vendor>openjdk</vendor>
</provides>
<configuration>
<jdkHome>/opt/java/jdk-14</jdkHome>
</configuration>
</toolchain>
<toolchain>
<type>jdk</type>
<provides>
<version>15</version>
<vendor>openjdk</vendor>
</provides>
<configuration>
<jdkHome>/opt/java/jdk-15</jdkHome>
</configuration>
</toolchain>
</toolchains>you can replace the <plugins>
<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>0.8.6</version>
<executions>
<execution>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>verify</phase>
<goals>
<goal>report</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<executions>
<execution>
<id>add-test-source</id>
<phase>generate-test-sources</phase>
<goals>
<goal>add-test-source</goal>
</goals>
<configuration>
<sources>
<source>src/test-jdk14/java</source>
</sources>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<executions>
<execution>
<id>default-testCompile</id>
<goals>
<goal>testCompile</goal>
</goals>
<configuration>
<optimize>true</optimize>
<jdkToolchain>
<version>1.8</version>
</jdkToolchain>
<compileSourceRoots>
<compileSourceRoot>${project.basedir}/src/test/java</compileSourceRoot>
</compileSourceRoots>
</configuration>
</execution>
<execution>
<id>jdk-14</id>
<goals>
<goal>testCompile</goal>
</goals>
<configuration>
<optimize>true</optimize>
<jdkToolchain>
<version>14</version>
</jdkToolchain>
<compileSourceRoots>
<compileSourceRoot>${project.basedir}/src/test/java</compileSourceRoot>
<compileSourceRoot>${project.basedir}/src/test-jdk14/java</compileSourceRoot>
</compileSourceRoots>
<outputDirectory>${project.build.directory}/test-classes-jdk14</outputDirectory>
<testExcludes>
<testExclude>**/Java9ListsTest.java</testExclude>
</testExcludes>
<source>14</source>
<release>14</release>
<compilerArgs>
<arg>-parameters</arg>
<arg>--enable-preview</arg>
</compilerArgs>
</configuration>
</execution>
<execution>
<id>jdk-15</id>
<goals>
<goal>testCompile</goal>
</goals>
<configuration>
<optimize>true</optimize>
<jdkToolchain>
<version>15</version>
</jdkToolchain>
<compileSourceRoots>
<compileSourceRoot>${project.basedir}/src/test/java</compileSourceRoot>
<compileSourceRoot>${project.basedir}/src/test-jdk14/java</compileSourceRoot>
</compileSourceRoots>
<outputDirectory>${project.build.directory}/test-classes-jdk15</outputDirectory>
<testExcludes>
<testExclude>**/Java9ListsTest.java</testExclude>
</testExcludes>
<source>15</source>
<release>15</release>
<compilerArgs>
<arg>-parameters</arg>
<arg>--enable-preview</arg>
</compilerArgs>
</configuration>
</execution>
</executions>
<configuration>
<!-- 17-Sep-2017, tatu: With 3.0 need to ensure parameter names compiled in -->
<optimize>true</optimize>
<compilerArgs>
<arg>-parameters</arg>
</compilerArgs>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-resources-plugin</artifactId>
<version>3.2.0</version>
<executions>
<execution>
<id>resources-jdk14</id>
<goals>
<goal>testResources</goal>
</goals>
<configuration>
<outputDirectory>${project.build.directory}/test-classes-jdk14</outputDirectory>
</configuration>
</execution>
<execution>
<id>resources-jdk15</id>
<goals>
<goal>testResources</goal>
</goals>
<configuration>
<outputDirectory>${project.build.directory}/test-classes-jdk15</outputDirectory>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<version>3.0.0-M5</version>
<artifactId>maven-surefire-plugin</artifactId>
<executions>
<execution>
<id>default-test</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<jdkToolchain>
<version>1.8</version>
</jdkToolchain>
<excludes>
<exclude>com/fasterxml/jackson/failing/*.java</exclude>
<exclude>**/RecordBasicsTest.class</exclude>
<exclude>**/RecordCreatorsTest.class</exclude>
<exclude>**/Java9ListsTest.class</exclude>
<exclude>**/RecordWithJsonSetter2974Test.class</exclude>
</excludes>
</configuration>
</execution>
<execution>
<id>test-jdk14</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<jdkToolchain>
<version>14</version>
</jdkToolchain>
<testClassesDirectory>${project.build.directory}/test-classes-jdk14</testClassesDirectory>
<excludes>
<exclude>com/fasterxml/jackson/failing/*.java</exclude>
<exclude>**/RecordWithJsonSetter2974Test.class</exclude>
</excludes>
<argLine>@{argLine} --enable-preview -XX:+ShowCodeDetailsInExceptionMessages</argLine>
</configuration>
</execution>
<execution>
<id>test-jdk15</id>
<goals>
<goal>test</goal>
</goals>
<configuration>
<jdkToolchain>
<version>15</version>
</jdkToolchain>
<testClassesDirectory>${project.build.directory}/test-classes-jdk15</testClassesDirectory>
<excludes>
<exclude>com/fasterxml/jackson/failing/*.java</exclude>
<exclude>**/RecordWithJsonSetter2974Test.class</exclude>
</excludes>
<argLine>@{argLine} --enable-preview -XX:+ShowCodeDetailsInExceptionMessages</argLine>
</configuration>
</execution>
</executions>
<configuration>
<classpathDependencyExcludes>
<exclude>javax.measure:jsr-275</exclude>
</classpathDependencyExcludes>
<!-- 26-Nov-2019, tatu: moar parallelism! Per-class basis, safe, efficient enough
... although not 100% sure this makes much difference TBH
-->
<threadCount>4</threadCount>
<parallel>classes</parallel>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<configuration>
<links combine.children="append">
<link>http://fasterxml.github.com/jackson-annotations/javadoc/3.0</link>
<link>http://fasterxml.github.com/jackson-core/javadoc/3.0</link>
</links>
</configuration>
</plugin>
<plugin> <!-- default settings are fine, just need to enable here -->
<!-- Inherited from oss-base. Generate PackageVersion.java.-->
<groupId>com.google.code.maven-replacer-plugin</groupId>
<artifactId>replacer</artifactId>
<executions>
<execution>
<id>generate-package-version-java</id>
<phase>prepare-package</phase>
<goals>
<goal>replace</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.moditect</groupId>
<artifactId>moditect-maven-plugin</artifactId>
</plugin>
<!-- 03-Nov-2020, tatu: Add LICENSE from main level -->
<plugin>
<groupId>de.jjohannes</groupId>
<artifactId>gradle-module-metadata-maven-plugin</artifactId>
</plugin>
</plugins>you can remove the This approach is simpler to test all needed jdk in one time, but need the toolchains setup. The not so good aspect is we need to compile 3 times all the tests as jdk14 compiled with preview cannot be run by jdk15. Jacoco plugin has to be bound to verify step to prevent run before all test, but I'm not sure how code coverage will be with the multiple test run. Anyway, this approach may not be the correct approach (what will be the jdk to test in march when jdk16 is released or in september with jdk17) , but as I did some test with this I share it if you want to test too. |
|
@GregoireW thank you for sharing this! It is probably not something I'd consider at this point for databind repo (if it triples build time, requires use of later JDK), but sounds like something that could work well for projects like |
|
For what it is worth, I was able to replicate the failure by forcing removal of |
cowtowncoder
added
2.13
and removed
to-evaluate
|
Would the preferred way to fix this be to rewrite the property names when deriving the bean description? Edit: |
|
Yes, I want to fix the problem by correctly linking constructor parameter -- something I am planning to achieve for 2.13. As to fix... ability to modify the array is not part of API so I would not really recommend it as something that is likely to keep working (that is, this implementation detail could change, or might behave differently for some edge case). But if it works now it might work in future. |
|
Great. I will only use my fix will until 2.13. Here is a version that does not modify in place: |
|
@jedvardsson excellent -- thank you very much for sharing this solution! |
|
Does this workaroud work on every case?
|
@MRamonLeon, am not sure how your JsonMapper is setup, but are you using the |
|
A poor man's workaround for this issue is to just manually annotate the fields with the wrong naming pattern with |
|
Any idea when this will be fixed? Considering moving to record for all DTOs but we use kebab case as a standard and having to annotate every field removes a big part of the advantage of reduced verbosity that record types provide. Right now this is a blocker to adopting record. |
|
@cgpassante you CSN just add the workaround I posted in a comment No need for annotations. Use the test in comment to track the bug. |
|
@cgpassante Unfortunately there is no concrete idea: problem is that it would be fixable as part of big rewrite of property introspection. But I have not found enough contiguous time to spend on tackling this (I only work on Jackson during my spare time): it could be that I might be able to do that within next month if something opened up. And yet with all the other things going on, it may be that it won't happen this year. What I do not want to try is a one-off hack to work around just this problem since that tends to complicate eventual rewrite. I agree that this problem area -- proper support of Java Records -- is VERY important, and I try my best to find time to resolve it properly. |
|
Dr. Jackson -- if you have a candidate strategy in mind, you are welcome to preflight it at the amber-dev@openjdk.org list, to validate that you're not subtly stepping on the Record contract. |
|
@briangoetz Thank you for suggestion. I think at this point the issues are relatively well-known (basically Record support is a tweak on earlier code that does not consider there being module-system imposed access limits, nor Fields being off-limits). Once those are cleared if there are more subtle problems we can and will reach out as necessary. |
|
@cgpassante how does your non-record setup (i.e. the DTO classes) looks like currently? Bean class with getters & setters? |
They are standard Bean classes. The issue is really our use of PropertyNamingStrategies.KEBAB_CASE. It does not translate attribute names in records. So we must use @JsonProperty("tracking-id") String trackingId, to explicitly name each record field. It removes the major benefit of records over beans...brevity. |
|
any updates on this? |
|
Greetings! I love jedvardsson's solution. It would be great though to support this out of the box :) |
|
@M4urici0GM if there were updates, there would be notes here. Hence, no. @nazarii-klymok the idea is to make all annotations work, but unfortunately there is no simple/easy fix: a complete overhaul of property introspection is needed. I haven't had time to dedicate for this due to may daytime job. I do hope to get there for 2.15 but it is difficult to predict if that happens or not. |
|
Hey @cowtowncoder, first of all, thank you so much for maintaining this project (in your spare time no less)! The community really stands on the shoulders of a giant here. Having been in that same position myself before, I relate to how you may be feel; at least I often felt overwhelmed and eventually lost the motivation to further work on the project (hence, a big boo to whoever gave that thumbs down to your comment above 😠). On the issue itself, I don't mean to further add to your workload, so just in case: is there by any chance a description or design doc of what that complete overhaul would look like? Or asked differently, is there a way for folks in the community to help? Oftentimes there are people who may be willing to help out, but aren't able to do so (or don't dare) because there's not good enough of a description of the problem, its context and potential solution constraints which already are present in the mind of the core maintainers. I probably won't have the bandwidth myself (as much as I'd like to do it, as said, I feel the community owes you so much here), but perhaps having a more in-depth description could help finding others willing to step up. Anyways, just a thought, and thanks again for this amazing project! |
|
Hi @gunnarmorling! I REALLY appreciate your suggestion here. I will have to think of how I could do something along those lines. I have added some vague outlines in various issues, but nothing concrete enough to let others start from somewhere other than square 1. But maybe I can do some preparation work to dig out details that I'd need myself. I will add whatever I know, tho, as a separate comment in case that might help someone interested in maybe tackling the issue, or at least learn more about what it might take. |
|
Ok, so the issue is roughly as follows:
What should happen, instead, it that process should start with Creator discovery (step (3)) followed by accessor discovery. This would allow reliable linking of all accessors/properties as well as make it possible to prevent discovery of underlying fields Records have. I will need to trace through code again to remind myself of code flow: I think it:
But now I am bit confused wrt how Anyway, not sure above helps... I will need to go dig deeper. I will create a Discussion for this tho. |
|
Quick update: Enabled Discussions and created #3719 |
cowtowncoder
changed the title
@JsonNaming does not work with Records
Hello,
When I try to use a
@JsonNamingannotation on a record, I cannot unmarshall json to an object because a mapping exception occurs.I use jackson 2.12.0 with JDK 15.
A Test example can be something like:
The json String is generated correctly, but when unmarshalling, I got an exception
The text was updated successfully, but these errors were encountered: