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 JsonTypeInfo.Id.SIMPLE_NAME which defaults type id to Class.getSimpleName() #4061

Closed
ooraini opened this issue Jul 30, 2023 · 17 comments · Fixed by #4065
Closed

Add JsonTypeInfo.Id.SIMPLE_NAME which defaults type id to Class.getSimpleName() #4061

ooraini opened this issue Jul 30, 2023 · 17 comments · Fixed by #4065
Labels
2.16 Issues planned for 2.16

Comments

@ooraini
Copy link

ooraini commented Jul 30, 2023

Is your feature request related to a problem? Please describe.

Add JsonTypeInfo.Id.SIMPLE_NAME that would use Class::getSimpleName for type ids.

My use case is sealed hierarchies where there is single sealed super interface, it would result in cleaner type ids. My work around:

            // AnnotationIntrospector
            @Override
            public String findTypeName(AnnotatedClass ac) {
                if (Arrays.stream(ac.getAnnotated().getInterfaces()).filter(Class::isSealed).count() == 1) {
                    return ac.getAnnotated().getSimpleName();
                }

                return super.findTypeName(ac);
            }

Describe the solution you'd like

JsonTypeInfo.Id.SIMPLE_NAME

Usage example

No response

Additional context

No response

@ooraini ooraini added the to-evaluate Issue that has been received but not yet evaluated label Jul 30, 2023
@JooHyukKim
Copy link
Member

JooHyukKim commented Jul 30, 2023

@ooraini Have you checked out JsonTypeInfo.Id.MINIMAL_CLASS? Sounds like it might solve your issue 🤔

Or could you provide some usage example and what it's trying to solve? Because there might be solution for existing use case and it will be clearer what the feature will be. Hopefully without using sealed.

@ooraini
Copy link
Author

ooraini commented Jul 30, 2023

Hi @JooHyukKim, I wanted to frame the request more generally without it being too specific on sealed. MINIMAL_CLASS does include the names of enclosing classes so you get something that looks like A$B$C which is not friendly to refactoring, what I would like with SIMPLE_NAME is to get C as the type ID.

Given:

        @JsonTypeInfo(use = JsonTypeInfo.Id.NAME)
        sealed interface Super {
        }

        record A(int i) implements Super {}
        record B(String s) implements Super {}

And serializing new A(1):

{"@type":"JacksonSealedSerdeTest$AsProperty$A","i":1}

With minimal class:

{"@c":".JacksonSealedSerdeTest$AsProperty$A","i":1}

This is not specific to sealed and the general case is similar:

    @JsonTypeInfo(
      use = JsonTypeInfo.Id.SIMPLE_NAME, // NEW
      include = As.PROPERTY, 
      property = "type")
    @JsonSubTypes({
        @JsonSubTypes.Type(value = Dog.class, name = "Dog"),
        @JsonSubTypes.Type(value = Cat.class, name = "Cat")
    })
    public static class Animal {
        public String name;
    }


    public static class Dog extends Animal {
        public double barkVolume;
    }


    public static class Cat extends Animal {
        boolean likesCream;
        public int lives;
    }

SIMPLE_NAME would use Class::getSimpleName when serializing objects, and the above would work:

{
  "type": "Dog",
  "name": "Tom",
  "barkVolume": 1.5

}

Again, I want to emphasis that this is not specific to sealed, it would work really well with sealed. I mentioned sealed just as a motivation for the feature. Hope this clears things

@JooHyukKim
Copy link
Member

@ooraini I see, thank you for the explanation 👍🏻 Suggested use-case seem to make sense and general. Will there also be other values (aside from Class::getSimpleName) that users might want 🤔🤔

WDYT, @cowtowncoder?

@cowtowncoder
Copy link
Member

I am bit confused about shown use case:

    @JsonTypeInfo(
      use = JsonTypeInfo.Id.SIMPLE_NAME, // NEW
      include = As.PROPERTY, 
      property = "type")
    @JsonSubTypes({
        @JsonSubTypes.Type(value = Dog.class, name = "Dog"),
        @JsonSubTypes.Type(value = Cat.class, name = "Cat")
    })
    public static class Animal {
        public String name;
    }

since this already declares names to use, so use of JsonTypeInfo.Id seems redundant?
Is this just accidental copy-paste?

Second: when using JsonTypeInfo.Id.NAME default Type Id choice is indeed to use simple name.

So I guess I am not quite sure what would be provided here.

But if such an option makes sense the only thing I'd suggest is that instead of SIMPLE_NAME it should be either SIMPLE_CLASS or SIMPLE_CLASS_NAME; otherwise relationship to existing NAME option would be confusing (NAME uses logical name from @JsonTypeName, or, if missing, then Class.getSimpleName() as defaulting).

@JooHyukKim
Copy link
Member

JooHyukKim commented Jul 31, 2023

since this already declares names to use, so use of JsonTypeInfo.Id seems redundant?
Is this just accidental copy-paste?

More minimal reproduction would be below, Id.NAME still includes the enclosing class and the request is to omit even the enclosing class. Can you confirm @ooraini ?

public class TestPolymorphic extends BaseMapTest {

    @JsonTypeInfo(
            use = JsonTypeInfo.Id.SIMPLE_CLASS_NAME) // <<-----"NEW"
    @JsonSubTypes({
            @JsonSubTypes.Type(value = Sub4061A.class),
            @JsonSubTypes.Type(value = Sub4061B.class)
    })
    static class Super4061 { }

    static class Sub4061A extends Super4061 { }

    static class Sub4061B extends Super4061 { }

    public void test() throws Exception {
        // case (1) JsonTypeInfo.Id.NAME
        assertEquals(
                 "{\"@type\":\"TestPolymorphic$Sub4061A\"}",
                newJsonMapper().writeValueAsString(new Sub4061A()));

        // case (2) JsonTypeInfo.Id.SIMPLE_CLASS_NAME
        assertEquals(
                 "{\"name\":\"Sub4061A\"}",
                newJsonMapper().writeValueAsString(new Sub4061A()));
    }
}

But if such an option makes sense the only thing I'd suggest is that instead of SIMPLE_NAME it should be either SIMPLE_CLASS or SIMPLE_CLASS_NAME; otherwise relationship to existing NAME option would be confusing (NAME uses logical name from @JsonTypeName, or, if missing, then Class.getSimpleName() as defaulting).

+1 SIMPLE_CLASS_NAME, because it contains most characters from Class.getSimpleName().

@ooraini
Copy link
Author

ooraini commented Jul 31, 2023

Hi @cowtowncoder, Regarding the example, it would not work if you used Id.NAME, you will need to annotate Dog and Cat with @JsonTypeName("Dog") and @JsonTypeName("Cat") respectively.

Indeed, Id.NAME uses the logical name from the @JsonTypeName, but it doesn't default to Class::getSimpleName , instead, it uses Class::getName.

            // TypeNameIdResolver.java
            if (name == null) {
                // And if still not found, let's choose default?
                name = _defaultTypeId(cls); // uses Class::getName
            }

👍🏻 for SIMPLE_CLASS_NAME.

Hope this helps.

@ooraini
Copy link
Author

ooraini commented Jul 31, 2023

More minimal reproduction would be below, Id.NAME still includes the enclosing class and the request is to omit even the enclosing class. Can you confirm @ooraini ?

@JooHyukKim
👍🏻 Exactly

@cowtowncoder
Copy link
Member

Ah. Ok, I misremembered defaulting; could have sworn it uses simple class name for Id.NAME; and I think it probably should have. Unfortunately not doing so is the default behavior so it cannot be changed due to backwards-compatibility reasons.

No objections to adding Id.SIMPLE_CLASS_NAME if someone has time and interest to work on a PR. Sounds like a good addition.

I also started re-thinking whether it really ought to be SIMPLE_NAME: although Class Name is used as the basis, it is not used as direct type (unlike CLASS or MINIMAL_CLASS) but rather more like a name that just happens to be generated from unqualified class.
So I am fine with SIMPLE_NAME after all :)

@JooHyukKim
Copy link
Member

JooHyukKim commented Aug 1, 2023

Can I get assigned on this? I have encountered a couple usecases, where I would have used Id.SIMPLE_CLASS_NAME if there was one. 👍🏻 Or @ooraini would you like to work on it?

@ooraini
Copy link
Author

ooraini commented Aug 1, 2023

@JooHyukKim Feel free work on it, I haven't gotten around the code base yet 😅

@cowtowncoder
Copy link
Member

First part is probably local modification of jackson-annotations to get new enum value. Then work on types in src/main/java/com/fasterxml/jackson/databind/jsontype/impl (like src/main/java/com/fasterxml/jackson/databind/jsontype/impl/TypeNameIdResolver.java).

@JooHyukKim
Copy link
Member

Made a PR #4065, could you PTAL, @cowtowncoder? If implementation seems okay, I will file a PR in jackson-annotations module and start collecting more test cases.

@JooHyukKim
Copy link
Member

JooHyukKim commented Aug 3, 2023

Vote/Naming Idea Wanted

In the PR FasterXML/jackson-annotations#234 (comment), we are discussing what the default identifier name of JsonTypeInfo.Id would be, and current options are...

  1. @simple --new
  2. @type -- as suggested by comment
  3. ...or please help come up with other name 🙂🙂

/cc @ooraini @cowtowncoder

EDIT: also created a thread in https://groups.google.com/g/jackson-dev/c/jWK4eh1cBL8, in case someone provides for more ideas 😆

@ooraini
Copy link
Author

ooraini commented Aug 4, 2023

@JooHyukKim If this purely a preference, then my vote goes to @type.

@JooHyukKim
Copy link
Member

@ooraini Thanks! there s already two votes on @type so updated the PR FasterXML/jackson-annotations#234 (comment) accordingly.

@cowtowncoder
Copy link
Member

Yes, I like use of @type as well (I assume I am the other vote :) ).

@cowtowncoder
Copy link
Member

Just merged the annotation PR; added a comment for minor test change for #4065 -- I think this can soon be merged.

@cowtowncoder cowtowncoder changed the title Add JsonTypeInfo.Id.SIMPLE_NAME using Class::getSimpleName Add JsonTypeInfo.Id.SIMPLE_NAME which defaults type id to Class.getSimpleName() Aug 26, 2023
@cowtowncoder cowtowncoder added 2.16 Issues planned for 2.16 and removed to-evaluate Issue that has been received but not yet evaluated labels Aug 26, 2023
cowtowncoder added a commit that referenced this issue Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16
Projects
None yet
3 participants