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

Add failing test for #4742 #4757

Merged
merged 5 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ public void resolve(DeserializationContext ctxt) throws JsonMappingException
}

// And now that we know CreatorProperty instances are also resolved can finally create the creator:
if (creatorProps != null) {
if (creatorProps != null && creatorProps.length > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is... it should not have create() method as PropertyBasedCreator, but instead make it default creator for Builder case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not follow the logic here. Why should we skip the case of 0-arguments Creator? (this would only apply to factory method, I think, with constructor we get separate "default constructor" case). It also does not seem to apply just to the Builder case (no checks to indicate that).

_propertyBasedCreator = PropertyBasedCreator.construct(ctxt, _valueInstantiator,
creatorProps, _beanProperties);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ public void configureFromObjectSettings(AnnotatedWithParams defaultCreator,
_delegateArguments = delegateArgs;
_withArgsCreator = withArgsCreator;
_constructorArguments = constructorArgs;
if (_defaultCreator == null) {
_defaultCreator = withArgsCreator;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea here. create() method should become the defaultCreator for Builder case. Idk how zero-arg create() method became withArgsCreator probably null-checking but not checking length?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also do not quite understand intended logic here: _defaultCtor is specifically expected to be 0-args Constructor; here no checking is done.
If such logic for defaulting was needed, it should be probably done by caller, not this method.

}

public void configureFromArraySettings(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package com.fasterxml.jackson.databind.deser.creators;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should go under tofix, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks!


import java.util.List;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.ObjectReader;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.json.JsonMapper;

import static org.junit.jupiter.api.Assertions.*;

public class JacksonBuilderCreatorSubtype4742Test {
public static class Animals {
@JsonProperty("animals")
public List<Animal> animals;
}

@JsonDeserialize(builder = Animal.Builder.class)
public static class Animal {
@JsonProperty("kind")
public String kind;

@JsonProperty("properties")
public AnimalProperties properties;

@Override
public String toString() {
return "Animal{kind='" + kind + '\'' + ", properties=" + properties + '}';
}

public static abstract class Builder {
@JsonProperty("kind")
public abstract Builder kind(String kind);

@JsonProperty("properties")
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXTERNAL_PROPERTY, property = "kind")
@JsonSubTypes({
@JsonSubTypes.Type(name = "bird", value = BirdProperties.class),
@JsonSubTypes.Type(name = "mammal", value = MammalProperties.class)
})
public abstract Builder properties(AnimalProperties properties);

@JsonCreator
public static BuilderImpl create() {
return new BuilderImpl();
}

public abstract Animal build();
}

public static class BuilderImpl extends Builder {
private String kind;
private AnimalProperties properties;

public BuilderImpl kind(String kind) {
this.kind = kind;
return this;
}

public BuilderImpl properties(AnimalProperties properties) {
this.properties = properties;
return this;
}

@Override
public Animal build() {
final Animal animal = new Animal();
animal.kind = kind;
animal.properties = properties;
return animal;
}
}
}

public interface AnimalProperties {
}

public static class MammalProperties implements AnimalProperties {
@JsonProperty("num_teeth")
public int teeth;

@Override
public String toString() {
return "MammalProperties{teeth=" + teeth + '}';
}
}

public static class BirdProperties implements AnimalProperties {
@JsonProperty("color")
public String color;

@Override
public String toString() {
return "BirdProperties{color='" + color + '\'' + '}';
}
}

public static void main(String[] args) throws Exception {
final JsonMapper jsonMapper = JsonMapper.builder().build();

final Animals animals = jsonMapper.readValue("{\n" +
" \"animals\": [\n" +
" {\"kind\": \"bird\", \"properties\": {\"color\": \"yellow\"}},\n" +
" {\"kind\": \"mammal\", \"properties\": {\"num_teeth\": 2}}\n" +
" ]\n" +
" }", Animals.class);

assertEquals(2, animals.animals.size());
assertInstanceOf(BirdProperties.class, animals.animals.get(0).properties);
assertInstanceOf(MammalProperties.class, animals.animals.get(1).properties);

}
}
Loading