`@JsonNaming` does not work with Records

[GregoireW]

Hello,

When I try to use a @JsonNaming annotation 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:

    @Test
    void tryJsonNamingOnRecord() throws Exception{
        ObjectMapper mapper=new ObjectMapper();

        @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
                record Test(String myId, String myValue){}

        var src=new Test("id", "value");
        String json=mapper.writeValueAsString(src);
        assertThat(json).contains("\"my_id\":\"id\"", "\"my_value\":\"value\"");
        var after=mapper.readValue(json, Test.class);
        assertThat(after).isEqualTo(src);
    }

The json String is generated correctly, but when unmarshalling, I got an exception

com.fasterxml.jackson.databind.JsonMappingException: Can not set final java.lang.String field test.Tests$1Test.myValue to java.lang.String
	at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:274)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty._throwAsIOE(SettableBeanProperty.java:623)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty._throwAsIOE(SettableBeanProperty.java:611)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty._throwAsIOE(SettableBeanProperty.java:634)
	at com.fasterxml.jackson.databind.deser.impl.FieldProperty.set(FieldProperty.java:193)
	at com.fasterxml.jackson.databind.deser.impl.PropertyValue$Regular.assign(PropertyValue.java:62)
	at com.fasterxml.jackson.databind.deser.impl.PropertyBasedCreator.build(PropertyBasedCreator.java:211)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:520)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1390)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:362)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:195)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:322)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4591)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3546)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3514)
	at test.Tests.tryJsonNamingOnRecord(Tests.java:100)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	...
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
...
Caused by: java.lang.IllegalAccessException: Can not set final java.lang.String field test.Tests$1Test.myValue to java.lang.String
	at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwFinalFieldIllegalAccessException(UnsafeFieldAccessorImpl.java:76)
	at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwFinalFieldIllegalAccessException(UnsafeFieldAccessorImpl.java:80)
	at java.base/jdk.internal.reflect.UnsafeQualifiedObjectFieldAccessorImpl.set(UnsafeQualifiedObjectFieldAccessorImpl.java:79)
	at java.base/java.lang.reflect.Field.set(Field.java:793)
	at com.fasterxml.jackson.databind.deser.impl.FieldProperty.set(FieldProperty.java:190)
	... 76 more

[cowtowncoder]

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.
Reason being that you cannot deserialize non-static inner classes: they have that gnarly invisible "this" that needs to be passed: at least in case of regular POJOs and I don't think Records are different here.

PropertyNamingStrategy itself is supported.

So you'd need to declare Record as regular inner class (they are always static, at least) or as stand-alone type.

[GregoireW]

Putting a record as stand alone type in is own file and setting @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) fail with the same exception as before.

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

The implicitly declared fields corresponding to the record components of a record class are final and moreover are not modifiable via reflection (doing so will throw IllegalAccessException). These restrictions embody an immutable by default policy that is widely applicable for data-carrier classes.

On the record JEP for JDK16 (Release version) the quote become:

The fields derived from the record components are final. This restriction embodies an immutable by default policy that is widely applicable for data-carrier classes.

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);
    }

[cowtowncoder]

@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)
But what is possible -- and compatible with your observations -- is that due to different issue (*), the names of underlying fields and names of constructor parameters might not get linked during initial introspection, leading to only former getting renamed according to naming strategy: this would essentially leave two sets of unrelated properties instead of one unified set.

(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).
And what might be possible to do would be to explicitly prevent use of fields for Records.

... 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.
So multiple bad options.

(*) 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 @JsonProperty

[GregoireW]

I only scratch the surface on how it works, so yes I more guess on what I saw than really drill down the code.

So multiple bad options.

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.

[cowtowncoder]

@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.
But now I am beginning to think it might be one big thing to do for 2.13 (along with increasing baseline to Java 8).

So that's a long of saying "thank you very much for doing what you did already". :)
I can take it from here as necessary. Additional help obviously very welcome, but not expected.

[GregoireW]

btw, I did a test with toolchains to check what was possible to do.

If you setup a ~/.m2/toolchains.xml file like (well update the JDK location to fit your setup):

<?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 section of the pom.xml by:

    <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 jdk14 profile, then running maven with a jdk 8, will compile source with this jdk, compile test 3 times (jdk8, jdk14 (preview mode) and jdk15 (preview mode) ), and run all test 3 times, with jdk8, 14 and 15 (well jdk14 test are disabled when running on jdk8).

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.
It would theoricaly possible to filter test to run only some test with jdk14 or 15, but option are limited on compilation part and surefire part.

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.

[cowtowncoder]

@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 jackson-integration-tests (external testing package that can add cross-dependencies across Jackson components) and some individual repos.

[cowtowncoder]

For what it is worth, I was able to replicate the failure by forcing removal of Field accessors for Records (in POJOPropertiesCollector); this fails naming-strategy case.
Hopign to address the issue for 2.13.

[jedvardsson]

Would the preferred way to fix this be to rewrite the property names when deriving the bean description?
I guess I could add a beandeserializermodifier to fix this temporarily, right?

Edit:

package com.examaple.jackson;

import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.DeserializationConfig;
import com.fasterxml.jackson.databind.deser.SettableBeanProperty;
import com.fasterxml.jackson.databind.deser.ValueInstantiator;
import com.fasterxml.jackson.databind.deser.ValueInstantiators;
import com.fasterxml.jackson.databind.deser.std.StdValueInstantiator;
import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
import com.fasterxml.jackson.databind.module.SimpleModule;

import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collectors;

public class RecordNamingStrategyPatchModule extends SimpleModule {

    @Override
    public void setupModule(SetupContext context) {
        context.addValueInstantiators(new ValueInstantiatorsModifier());
        super.setupModule(context);
    }

    /**
     * <a href="https://github.com/FasterXML/jackson-databind/issues/2992">Properties naming strategy do not work with Record #2992</a>
     */
    private static class ValueInstantiatorsModifier extends ValueInstantiators.Base {
        @Override
        public ValueInstantiator findValueInstantiator(
                DeserializationConfig config, BeanDescription beanDesc, ValueInstantiator defaultInstantiator
        ) {
            if (!beanDesc.getBeanClass().isRecord() || !(defaultInstantiator instanceof StdValueInstantiator) || !defaultInstantiator.canCreateFromObjectWith()) {
                return defaultInstantiator;
            }
            // StdValueInstantiator can be modified in place
            SettableBeanProperty[] fromObjectArguments = defaultInstantiator.getFromObjectArguments(config);
            Map<String, BeanPropertyDefinition> map = beanDesc.findProperties().stream().collect(Collectors.toMap(p -> p.getInternalName(), Function.identity()));
            for (int i = 0; i < fromObjectArguments.length; i++) {
                SettableBeanProperty arg = fromObjectArguments[i];
                BeanPropertyDefinition prop = map.get(arg.getName());
                if (prop != null) {
                    fromObjectArguments[i] = arg.withName(prop.getFullName());
                }
            }
            return defaultInstantiator;
        }
    }
}

package com.examaple.jackson;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

class RecordNamingStrategyPatchModuleTest {

    private static final JsonMapper STANDARD_MAPPER = JsonMapper.builder()
            .addModule(new ParameterNamesModule())
            .propertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE)
            .build();

    private static final JsonMapper PATCHED_MAPPER = JsonMapper.builder()
            .addModule(new ParameterNamesModule())
            .addModule(new RecordNamingStrategyPatchModule())
            .propertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE)
            .build();

    public record SampleItem(int itemId) {
    }

    @Test
    void testNamingStrategyDeserializeRecordBug() throws JsonProcessingException {
        assertThrows(JsonMappingException.class, () ->
                STANDARD_MAPPER.readValue("""
                        {"item_id": 5}
                        """, SampleItem.class), "https://github.com/FasterXML/jackson-databind/issues/2992 has been fixed!");

        assertEquals(new SampleItem(5),
                PATCHED_MAPPER.readValue("""
                        {"item_id": 5}
                        """, SampleItem.class));
    }
}

[cowtowncoder]

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.