`JsonSubType.Type` should accept array of names

[swayamraina]

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]

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]

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

[cowtowncoder]

@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]

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]

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]

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]

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

[cowtowncoder]

@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]

will submit this in a couple of days!

[cowtowncoder]

Merged, will be in 2.12.0.