Skip to content
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

Deserialization using @JsonCreator with renamed property failing (since 2.18) #4810

Closed
1 task done
jmesny opened this issue Nov 25, 2024 · 16 comments
Closed
1 task done
Labels

Comments

@jmesny
Copy link

jmesny commented Nov 25, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

Hello,

Since v2.18, some of our data classes failed to be used for deserialization.
Those classes are making use of @JsonProperty et @JsonCreator annotations, and rely on the ParameterNamesModule.

Version Information

2.18.1

Reproduction

Example reproducing the issue:

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;
import com.google.auto.value.AutoValue;
import org.junit.jupiter.api.Test;

import static com.google.common.collect.Sets.newHashSet;
import static org.assertj.core.api.Assertions.assertThat;

public final class MappingIssueTest {

    @AutoValue
    public static abstract class DataClassWithPropRenaming {
        @JsonProperty("bar")
        public abstract String getFoo();

        @JsonCreator
        public static DataClassWithPropRenaming create(String bar) {
            return new AutoValue_MappingIssueTest_DataClassWithPropRenaming(bar);
        }
    }

    @Test
    void shouldSupportPropertyRenaming() throws Exception {
        var mapper = JsonMapper.builder()
                .addModule(new ParameterNamesModule())
                .build();

        var serializationResult = mapper.valueToTree(DataClassWithPropRenaming.create("42"));

        assertThat(newHashSet(serializationResult.fieldNames()))
                .containsExactlyInAnyOrder("bar");

        var deserializationResult = mapper.treeToValue(serializationResult, DataClassWithPropRenaming.class);

        assertThat(deserializationResult)
                .isEqualTo(DataClassWithPropRenaming.create("42"));
    }
}

Fails with the following error:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `MappingIssueTest$DataClassWithPropRenaming` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: UNKNOWN; byte offset: #UNKNOWN]

	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1754)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1379)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1512)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:348)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
	at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:4893)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3036)
	at com.fasterxml.jackson.databind.ObjectMapper.treeToValue(ObjectMapper.java:3500)

Expected behavior

We would expect that the @JsonCreator method parameter name be used to map the field having the same name in the JsonNode.

Additional context

Adding a @JsonProperty("bar") on the creator parameter solves the issue.

@jmesny jmesny added the to-evaluate Issue that has been received but not yet evaluated label Nov 25, 2024
@pjfanning
Copy link
Member

Please try 2.18.2. This issue sounds similar to issues that were fixed in that release.

@K-Fl
Copy link

K-Fl commented Nov 26, 2024

There is no 2.18.2?

@JooHyukKim
Copy link
Member

Try building latest 2.18 branch. That would be 2.18.2?

@jmesny
Copy link
Author

jmesny commented Nov 26, 2024

I tried with the 2.18 and 2.19 branches, the issue is still here.

@yihtserns
Copy link
Contributor

yihtserns commented Nov 26, 2024

Observations (tested without AutoValue, though):

  1. A workaround is setting @JsonCreator.mode = PROPERTIES.
  2. Automagically works when @JsonCreator is removed.

@jmesny
Copy link
Author

jmesny commented Nov 26, 2024

I provided a simplified example. In real life, we don't really have any single-argument factory methods that could use this DELEGATING mode.

@yihtserns
Copy link
Contributor

Eh sorry I meant PROPERTIES not DELEGATING, updated that incorrect comment. 😅

@cowtowncoder
Copy link
Member

I would strongly recommend against using plain @JsonCreator for 1-argument constructor/factory method: use of mode = Mode.PROPERTIES. Other than that, it seems like a flaw in heuristics: if name bar is recognized this should be considered Properties-style Creator.

@cowtowncoder cowtowncoder changed the title Deserialization issue of a renamed property since v2.18 Deserialization using @JsonCreator with renamed property failing (since 2.18) Nov 27, 2024
@cowtowncoder cowtowncoder added 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 27, 2024
@cowtowncoder
Copy link
Member

Ok, I can reproduce this, and the problem is that heuristics for mode detection only have access to properties before renaming, accessible only by implicit name. In 2.17 handling was done at a later point where access was post-renaming.

I'll need to see if there is any way to be able to access explicit renaming.

@jmesny
Copy link
Author

jmesny commented Nov 27, 2024

@cowtowncoder I gave a try to your patch, but the issue remains unchanged... 😕

This simpler example below, like the one you committed (but without the Implicit* stuffs), outputs the same error:

static class DataClass4810 {
        private String x;

        private DataClass4810(String x) {
            this.x = x;
        }

        @JsonProperty("bar")
        public String getFoo() {
            return x;
        }

        @JsonCreator
        public static DataClass4810 create(String bar) {
            return new DataClass4810(bar);
        }
    }

    @Test
    void shouldSupportPropertyRenaming4810() throws Exception {
        ObjectMapper mapper = JsonMapper.builder()
                .addModule(new ParameterNamesModule())
                .build();

        JsonNode serializationResult = mapper.valueToTree(DataClass4810.create("42"));

        assertEquals("{\"bar\":\"42\"}", serializationResult.toString());

        DataClass4810 deserializationResult = mapper.treeToValue(serializationResult, DataClass4810.class);

        assertEquals("42", deserializationResult.getFoo());
    }

What do you think?

@yihtserns
Copy link
Contributor

I gave a try to your patch, but the issue remains unchanged...

Should work - I tested with 2.18.2-SNAPSHOT (generated Wed Nov 13 03:09:24 UTC 2024) from https://oss.sonatype.org/content/repositories/snapshots.

Give it another try?

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 27, 2024

The fix went in 2.18 branch last night so only the latest 2.18.2-SNAPSHOT should have it.

@jmesny So how does your example differ from test I added? Test was failing before, passing after fix. It is possible there may be remaining edge case(s) but I'd need a reproduction.
The only difference I see is use of ImplicitNameIntrospector because jackson-databind cannot depend on parameter-names module; but that should work identically (and is used by many other tests).

So I'd need to be able to reproduce remaining problems; can't do it yet.

@jmesny
Copy link
Author

jmesny commented Nov 28, 2024

My attempt was made using a local build of the sources containing the patch.

Thanks @cowtowncoder!

@cowtowncoder
Copy link
Member

@jmesny Just to be sure tho: now that 2.18.2 is released (last night), does the problem get resolved? If not, I would like to understand why; if yes, that's great.

@jmesny
Copy link
Author

jmesny commented Nov 29, 2024

My attempt was made using a local build of the sources containing the patch.

Yes, but I forgot to pass the required -parameters compiler option... 🤦‍♂️

Sorry folks, everything works great with the 2.18.2 release!

Thanks again @cowtowncoder

@cowtowncoder
Copy link
Member

@jmesny Ah! Yes, it's all good then -- thank you for confirmation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants