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 config-override for @JsonTypeInfo.Value #3959

Closed

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim marked this pull request as ready for review June 2, 2023 15:03
@JooHyukKim JooHyukKim changed the title [DRAFT] Add config-override for @JsonTypeInfo.Value Add config-override for @JsonTypeInfo.Value Jun 3, 2023
@Override
public JsonTypeInfo.Value getDefaultPolymorphicTypeHandling(Class<?> baseType)
{
ConfigOverride superOverride = _configOverrides.findOverride(baseType.getSuperclass());
Copy link
Member

Choose a reason for hiding this comment

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

Ok this is unfortunately problematic in couple of ways:

  1. It uses less specific (parent) information if both exist (should prefer specific one)
  2. Assumes 2 levels of inheritance (could have more)
  3. For interfaces getSuperClass() returns null I think? I don't think this would work when base type is interface

It is also different from all other config overrides which only consider exact baseType passed. I think that'd be correct thing to do here -- except I suspect there was a problem wrt serialization side: on deserialization one usually specifies proper base type (to get to polymorphic subtype). But on serialization, unfortunately, the way things are "baseType" is probably actual implementation type.
So using just that might not work.

If so I am not sure if this can actually be made to work, without fixing more fundamental problem: Jackson really should find proper base type on serialization too and not rely on annotation inheritance (which is why things work at all wrt @JsonTypeInfo).

Copy link
Member Author

@JooHyukKim JooHyukKim Jun 19, 2023

Choose a reason for hiding this comment

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

But on serialization, unfortunately, the way things are "baseType" is probably actual implementation type. So using just that might not work.

Ah, okay. So this was what bothered me, but couldn't figure out why 😅. I should double check, if I remember correctly, the getSuperClass() checking was needed on the serialization side.

Copy link
Member Author

Choose a reason for hiding this comment

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

problem: Jackson really should find proper base type on serialization too and not rely on annotation inheritance

@cowtowncoder Hmm unless there is a work going on about this, someday probably I will make time tacle this (or together if any). May I bring this up after your vacation (congrats😆)

@cowtowncoder
Copy link
Member

Ok so I am bit worried about that issue I mentioned wrt locating ConfigOverride to use: I think it should not try to traverse inheritance hierarchy due to complexity. If you only look at specific baseType would that work? (I am guessing not, due to serialization side not passing true base type, but I might be wrong)

@JooHyukKim
Copy link
Member Author

.... traverse inheritance hierarchy due to complexity. If you only look at specific baseType would that work?

At this point, I doubt.

Ok so I am bit worried about that issue I mentioned wrt locating ConfigOverride to use: I think it should not try to traverse inheritance hierarchy due to complexity.

Agreed, it seems like a dangerous path down 🙃... I will try to find cleaner way --plus after refering to other config-override mechanisms, it "might" be possible to make use of AnnotatedClass, just like MapperConfig.getDefaultPropertyInclusions(Class<?>, AnnotatedClass); to introspect in Jackson-style 👍🏻👍🏻

@JooHyukKim
Copy link
Member Author

Temporarily drafted

Will turn into draft until un-natural introspection at #3959 (comment) is resolved.

@JooHyukKim JooHyukKim marked this pull request as draft June 19, 2023 03:59
@JooHyukKim
Copy link
Member Author

Closing this as part of clean up, until solution is found 🙏🏼

@JooHyukKim JooHyukKim closed this Jul 16, 2023
@JooHyukKim JooHyukKim reopened this Aug 22, 2023
@JooHyukKim JooHyukKim marked this pull request as ready for review August 22, 2023 13:22
@@ -875,6 +875,23 @@ public Boolean getDefaultMergeable(Class<?> baseType) {
return _configOverrides.getDefaultMergeable();
}

// since 2.16
@Override
public JsonTypeInfo.Value getDefaultPolymorphicTypeHandling(JavaType baseType)
Copy link
Member Author

@JooHyukKim JooHyukKim Aug 22, 2023

Choose a reason for hiding this comment

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

The changed part since review #3959 (comment). Here we traverse using ClassUtil.findSuperTypes() that supports both concrete types and interfaces.

EDIT : Traversal is to complement serialization side not locating the "exact" baseType.

WDYT? /cc @cowtowncoder

@JooHyukKim
Copy link
Member Author

Closing for now. Because of low user demand AND 2.16-rc is released.

@JooHyukKim JooHyukKim closed this Oct 22, 2023
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

Successfully merging this pull request may close these issues.

2 participants