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

JsonSubType.Type should accept array of names #171

Closed
swayamraina opened this issue May 29, 2020 · 15 comments
Closed

JsonSubType.Type should accept array of names #171

swayamraina opened this issue May 29, 2020 · 15 comments
Milestone

Comments

@swayamraina
Copy link

swayamraina commented May 29, 2020

While adding values to JsonSubTypes we have to add each element manually even if multiple keys map to same object.

example:

@JsonSubTypes({
        @JsonSubTypes.Type(name = "not_equals", value = NotEqualsExpression.class),
        @JsonSubTypes.Type(name = "ne", value = NotEqualsExpression.class),
        @JsonSubTypes.Type(name = "equals", value = EqualsExpression.class)
})
public abstract class Expression { ... }

Current implementation maps one class with one key whereas there are multiple keys in json which can map to same data-structure.

public @interface JsonSubTypes {
    JsonSubTypes.Type[] value();

    public @interface Type {
        Class<?> value();
        String name() default "";
    }
}

So as per the new feature multiple keys can be mapped to single data-structure without adding multiple entries.

public @interface JsonSubTypes {
    JsonSubTypes.Type[] value();

    public @interface Type {
        Class<?> value();
        String[] name() default "";
    }
}

Thoughts?
If this is approved, I am also willing to contribute to this feature.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 29, 2020

One immediate concern I have is that type change for value would be major breaking change, immediately breaking all existing use since it is binary incompatible (although possibly source-compatible, I forget if single-element is allowed without brackets).
So I don't think change as proposed would be allowed for 2.x.

Otherwise, either adding this in 3.0 (... but which will take long time to get released...), or adding separate optional names, that could be doable.
If changes to jackson-databind (JacksonAnnotationIntrospector.findSubtypes()) can be made easily, without changing method signature (which I think is possible), I guess that sounds reasonable?

So it is important to make sure code that uses annotations can be easily changed; changing
annotation is simple.
One good thing is that value has default value (of ""), so users do not have to specify that.

@swayamraina
Copy link
Author

As far as I know, "abc" is equivalent to [ "abc" ]
So existing builds should not have an issue.

annotation:

@Retention(RetentionPolicy.RUNTIME)
public @interface Internal {
    String[] name() default "";
}

usage:

    @Internal(name="abc") private String a;
    @Internal(name={"abc", "xyz"}) private String b;

    public static void main(String[] args) throws Exception {
        Field f1 = SignalGroup.class.getDeclaredField("a");
        Field f2 = SignalGroup.class.getDeclaredField("b");

        System.out.println(f1.getAnnotation(Internal.class).name().length);
        System.out.println(f2.getAnnotation(Internal.class).name().length);
    }

The output is,

1
2

@swayamraina swayamraina changed the title Feature Request: JsonSubType.Type should accept array of values Feature Request: JsonSubType.Type should accept array of names May 30, 2020
@cowtowncoder
Copy link
Member

@swayamraina Have you actually looked at generated byte-code, or tried cross-compiling one definition (declaring as String with code assuming String[]; vice versa)? What you are showing above is source-compatibility, that compiler can generate array from String value.
This means that if one was guaranteed to compile code against new definition things would work. This is true.

But what I am worried about is that existing code, compiled to use definition that expects String would not have same signature and would either give link error, or, since annotation handling seems to differ a little bit from regular code handling, silently not define name.

@swayamraina
Copy link
Author

So if you are talking about some other lib using these annotations to build something on top. Then yes this would throw a compile time error for them when they upgrade to this version.

@cowtowncoder
Copy link
Member

No, not compile time, runtime; and typically via transitive dependencies so it is not necessarily user code that fails during startup (or possibly later), when loading classes.
Biggest problem are cases where user code upgrades to newer version, but library/-ies user code depends on has been compiled against older version. Ideally there are tests that would trigger failures, making problem at least easier to detect before system gets to production; but better yet, such change would be avoided.

So, two ways here: (1) show that resulting byte code is not different between different declarations (that is, annotation class signature is identical between String/String[]), or (2) create new names property with String[] type (defaulting to []).

But beyond that, PR would be needed to make use of new functionality: this may be relatively easy to in jackson-databind for JacksonAnnotationIntrospector (and a unit test to show it working). I think it should go against master branch (for 3.0), or 2.12.

@swayamraina
Copy link
Author

As an incremental thing, we can go with the second approach you talked about.
We'll introduce a new field that will consume an array in this minor release (2.12) and will remove the old one with the release of 3.0

@swayamraina
Copy link
Author

swayamraina commented Jun 11, 2020

I have a raised a PR : #172
Will submit another PR in jackson-databind

@cowtowncoder
Copy link
Member

@swayamraina Ok it all looks good, and I am ready to merge changes. Just one last thing: before the first contribution (but only then), I'd need CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
Once I receive it, I can merge changes. CLA itself is good for all future contributions too, it's just one-time thing we need.

Thank you again for contributing this improvement, looking forward to merging it!

@swayamraina
Copy link
Author

will submit this in a couple of days!

@cowtowncoder cowtowncoder changed the title Feature Request: JsonSubType.Type should accept array of names JsonSubType.Type should accept array of names Jul 11, 2020
cowtowncoder added a commit that referenced this issue Jul 11, 2020
@cowtowncoder cowtowncoder added this to the 2.12.0 milestone Jul 11, 2020
@cowtowncoder
Copy link
Member

Merged, will be in 2.12.0.

@IrvingZhao
Copy link

IrvingZhao commented Jul 13, 2020

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "propA")
@JsonSubTypes({
        @JsonSubTypes.Type(name = "subA", value = SubA.class),
        @JsonSubTypes.Type(name = "subB", value = SubB.class)
})
class Base{
}

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "propB")
@JsonSubTypes({
        @JsonSubTypes.Type(name = "subA_A", value = SubA_A.class),
})
class SubA{
}

class SubA_A{}

the json content is :
var content1 = {propA:"subA",propB:"subA_A"}
var content2 = {propA:"subB"}

I have tryed

  1. objectMapper.readValue(content1,Base.class) not work
  2. objectMapper.readValue(content1,SubA.class) is work
  3. objectMapper.readValue(content2,Base.class) is work
    Is there has some method to make first to be work?

I mean
objectMapper.readValue(content1,Base.class) return object of type SubA, i wanna this return object of type SubA_A

@cowtowncoder
Copy link
Member

@IrvingZhao you may have accidentally added this in wrong place -- I don't really know how this would be related to the issue at hand.

@IrvingZhao
Copy link

@cowtowncoder

多层继承反序列化
第一层能分发到第二层,但是第二层没办法分发到第三层
第三层只能捅过第二层进行分发
有没有什么办法能让第一层分发到第二层后,通过第二层上的配置进行第三层分发

场景:
消息数据,第一层,包括:文本消息、视频消息、事件消息,通过msgType进行区分。第二次为不同的事件信息,包括按钮事件、定位事件,通过eventType区分

translate by baidu Translator

Multi level inheritance deserialization
The first layer can be distributed to the second layer, but the second layer cannot be distributed to the third layer
The third layer can only pass through the second layer for distribution
Is there any way to make the first layer distribute to the second layer, and then use the configuration on the second layer to distribute the third layer

Scene:

Message data, the first layer, including text message, video message and event message, is distinguished by msgType. The second is different event information, including button event and location event, which are distinguished by eventType

@IrvingZhao
Copy link

I have read the source of com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer
the method

public Object deserializeTypedFromObject(JsonParser p, DeserializationContext ctxt) throws IOException{
    // ...
    for (; t == JsonToken.FIELD_NAME; t = p.nextToken()) {
            final String name = p.getCurrentName();
            p.nextToken(); // to point to the value
            if (name.equals(_typePropertyName)
                    || (ignoreCase && name.equalsIgnoreCase(_typePropertyName))) { // gotcha!

               // this place can check subTypes,
              // if the subType has config of JsonSubTypes, 
             //  reset the parser and read value by subtype, then it will return that i want

                return _deserializeTypedForId(p, ctxt, tb);
            }
            if (tb == null) {
                tb = new TokenBuffer(p, ctxt);
            }
            tb.writeFieldName(name);
            tb.copyCurrentStructure(p);
        }
    // ...
}

some like this

        boolean hasType = false;
        Class<?> readType = null;
        for (; t == JsonToken.FIELD_NAME; t = p.nextToken()) {
            var name = p.currentName();
            p.nextToken();
            if (name.equals(_typePropertyName)
                    || (ignoreCase && name.equalsIgnoreCase(_typePropertyName))) {
                var id = p.getText();
                JavaType type = _idResolver.typeFromId(ctxt, id);
                if (type != null) {
                    hasType = true;
                    readType = type.getRawClass();
                    if (readType.getAnnotation(JsonSubTypes.class) == null) {
                        return _deserializeTypedForId(p, ctxt, tb);
                    }
                }
            }
            if (tb == null) {
                tb = new TokenBuffer(p, ctxt);
            }
            tb.writeFieldName(name);
            tb.copyCurrentStructure(p);
        }
        if (hasType) {
            return tb.asParser().readValueAs(readType);
        } else {
            return _deserializeTypedUsingDefaultImpl(p, ctxt, tb);
        }

@swayamraina
Copy link
Author

@IrvingZhao if you need some help regarding usage of the annotations, You can raise a question by creating a new issue.
Or create a new feature request issue if any of your use-case is not currently supported/implemented.

You can provide more details if you mean to point out some bug introduced by this new feature.

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

No branches or pull requests

3 participants