Skip to content

JsonTypeInfo.Id.MINIMAL_CLASS generates invalid type on sub-package #4983

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

Open
blacelle opened this issue Feb 23, 2025 · 13 comments
Open

JsonTypeInfo.Id.MINIMAL_CLASS generates invalid type on sub-package #4983

blacelle opened this issue Feb 23, 2025 · 13 comments

Comments

@blacelle
Copy link

blacelle commented Feb 23, 2025

Jackson: 2.18.2

I consider an interface with @JsonTypeInfo(use = JsonTypeInfo.Id.MINIMAL_CLASS, property = "type").

It works correctly with implementations in thr same package, but it fails with implementations in a child package.

Can be reproduced with:

package eu.solven.adhoc.jackson;

import com.fasterxml.jackson.annotation.JsonTypeInfo;

@JsonTypeInfo(use = JsonTypeInfo.Id.MINIMAL_CLASS, property = "type")
public interface IParent {

}

OK:

package eu.solven.adhoc.jackson;

import java.util.Map;

import com.fasterxml.jackson.databind.ObjectMapper;

public class Children implements IParent {
	String someField;

	public String getSomeField() {
		return someField;
	}

	public void setSomeField(String someField) {
		this.someField = someField;
	}

	public static void main(String[] args) {
		ObjectMapper om = new ObjectMapper();

		Map<?, ?> asMap = om.convertValue(new Children(), Map.class);
		System.out.println(asMap);

		IParent asParent = om.convertValue(asMap, IParent.class);

	}
}

KO:

package eu.solven.adhoc.jackson.sub;

import java.util.Map;

import com.fasterxml.jackson.databind.ObjectMapper;

import eu.solven.adhoc.jackson.IParent;

public class ChildrenSubPackage implements IParent {
	String someField;

	public String getSomeField() {
		return someField;
	}

	public void setSomeField(String someField) {
		this.someField = someField;
	}

	public static void main(String[] args) {
		ObjectMapper om = new ObjectMapper();

		Map<?, ?> asMap = om.convertValue(new ChildrenSubPackage(), Map.class);
		System.out.println(asMap);

		IParent asParent = om.convertValue(asMap, IParent.class);

	}
}

Could not resolve type id 'eu.solven.adhoc.jackson.ChildrenSubPackage' as a subtype of eu.solven.adhoc.jackson.IParent: no such class found at [Source: UNKNOWN; byte offset: #UNKNOWN]

Indeed, the type is invalid in {type=.ChildrenSubPackage, someField=null} as it should be .sub.ChildrenSubPackage.


Also, the javadoc seems invalid:

For example, for supertype "com.foobar.Base", and concrete type "com.foo.Impl", only ".Impl" would be included; and for "com.foo.impl.Impl2" only ".impl.Impl2" would be included.

I suppose we want to read:

For example, for supertype "com.foo.Base", and concrete type "com.foo.Impl", only ".Impl" would be included; and for "com.foo.impl.Impl2" only ".impl.Impl2" would be included.

@cowtowncoder
Copy link
Member

Since handling of annotations (processing) is not here but in jackson-databind, will transfer.

@cowtowncoder cowtowncoder transferred this issue from FasterXML/jackson-annotations Feb 25, 2025
@cowtowncoder
Copy link
Member

Minor note: will fix the Javadoc.

@cowtowncoder
Copy link
Member

I suspect this is working in unexpected way, not because child package handling didn't work, but because of the way base type is determined during serialization. This is an unfortunate consequence of of handling of "root values" (Java objects directly passed to ObjectMapper -- as opposed to ones reference indirectly by root value), and asymmetry to deserialization.

So, although your intent is to consider IParent as the base type, during serialization it isn't (it is during deserialization, since it's directly passed) -- for root values, type is just that type (there is no way to determine this due to way annotations are flattened for processing).
So "base" is simply determined to be ChildrenSubPackage and not IParent.
Unfortunately for deserialization correct type, IParent is chosen. Hence discrepancy.

You can see that if using a non-polymorphic wrapper and having value be IParent -valued property.

To fix this, a mechanism would need to be added to try to retain and use actual Java class on which @JsonTypeInfo is defined.

There might be an issue for this (old one), but no implementation.

@cowtowncoder
Copy link
Member

Ah. The issue that I think you hit is #1358.

In the meantime, to anchor base type on write, you might be able to use (... if you didn't use convertValue()...) ObjectWriter with forced type:

String json = mapper.writerFor(IParent.class).writeValueAsString(actualValue);

@blacelle
Copy link
Author

blacelle commented Feb 25, 2025

So "base" is simply determined to be ChildrenSubPackage and not IParent.
Unfortunately for deserialization correct type, IParent is chosen. Hence discrepancy.

It is unclear to me how this is the same issue as #1358 and the bunch of related issues, as they seem quite specific to defaultImpl. I understand the core of this a issue is the same of the core of #1358 (the way how annotations are flattened).

In the meantime, to anchor base type on write, you might be able to use (... if you didn't use convertValue()...) ObjectWriter with forced type:

My code does rely on .convertValue. I'm then considering:

objectMapper.readValue(objectMapper.writerFor(IMeasure.class).writeValueAsString(m), Map.class)

To fix this, a mechanism would need to be added to try to retain and use actual Java class on which @JsonTypeInfo is defined.

A bad workaround could also to add a field in @JsonTypeInfo, telling explicitly what should be considered as the base.


there is no way to determine this due to way annotations are flattened for processing

Do you have a code pointer around this? I doubt I would work on a PR, but I would be keen to look deeper into it.

Looking around com.fasterxml.jackson.databind.jsontype.impl.StdTypeResolverBuilder.idResolver, I see I get baseType= ChildrenSubPackage , while I understand I would like to see baseType=IParent.

I understand the flatten annotations process as mostly related with AnnotatedClass, Annotated, com.fasterxml.jackson.databind.introspect.AnnotatedClass._classAnnotations. It looks quite deep, and I suppose current issue may not important enough to too many changes around Annotated.

Hence, either public <A extends Annotation> A get(Class<A> cls); could be extended into something like public <A extends Annotation> Map.Entry<A, Class<?>> getWithAnnotated(Class<A> cls); (but i feel it would involve quite a lot of work).

Or something more specific, around :

    protected TypeResolverBuilder<?> _constructStdTypeResolverBuilder(MapperConfig<?> config,
            JsonTypeInfo.Value typeInfo, JavaType baseType) {
        return new StdTypeResolverBuilder(typeInfo);
    }

as we could analyze (again) baseType class hierarchies for annotations, to look back for the actual class providing typeInfo (let's call it annotationBaseType). And use annotationBaseType as a field into StdTypeResolverBuilder to later customize MinimalClassNameIdResolver, so that the base is not built on baseType, but on annotationBaseType.

    protected MinimalClassNameIdResolver(JavaType baseType, TypeFactory typeFactory,
            PolymorphicTypeValidator ptv)
    {
        super(baseType, typeFactory, ptv);
        String base = baseType.getRawClass().getName();
        int ix = base.lastIndexOf('.');
        if (ix < 0) { // can this ever occur?
            _basePackageName = "";
            _basePackagePrefix = ".";
        } else {
            _basePackagePrefix = base.substring(0, ix+1);
            _basePackageName = base.substring(0, ix);
        }
    }

(if (ix < 0) { // can this ever occur?: I suppose it can happen on package-less classes).

Does this sound reasonnable? (if yes, I may contribute a PR).
(Also, I do not get how it relates with defaultImpl, so given approach may not be apprppriate).

@cowtowncoder
Copy link
Member

I linked to #1358 bit too quickly, without checking. It might not be the issue I was thinking for. Problem I am thinking of is definitely not specific to defaultImpl.

But basically, it'd be good to know exact class that is annotated with @JsonTypeInfo without flattening (annotation introspection for type X will currently "flatten" all annotations from super types so it can't be known at which level annotation was originally).

And yes, changes to AnnotatedClass (and so on) might be quite intrusive. But ideally that information should be available.

But fwtw, while this issue might not on its own be important enough, the same issue is (I think) relevant for other problems... although cannot immediately recall which ones.

@blacelle
Copy link
Author

blacelle commented Feb 25, 2025

I'm assessing if I could contribute:

  • Is enriching the annotation process the (seemingly) way to go, though it appears intrusive?
  • Or is it relevant to consider computing as a second (possibly redundant) way information about an annotation? This may be a better design anyway, by not complexifying current flattening process, but computing this hierarchical information only when relevant.

The first option looks intrusive enough I could not foresee the raw impact.

The second option would turn around JacksonAnnotationIntrospector:

from:

    protected TypeResolverBuilder<?> _constructStdTypeResolverBuilder(MapperConfig<?> config,
            JsonTypeInfo.Value typeInfo, JavaType baseType) {
        return new StdTypeResolverBuilder(typeInfo);
    }

into something like:

    protected TypeResolverBuilder<?> _constructStdTypeResolverBuilder(MapperConfig<?> config,
            JsonTypeInfo.Value typeInfo, JavaType baseType) {
        JavaType annotatedType = computeAnnotatedType((Annotated) ann, baseType);
        return new StdTypeResolverBuilder(typeInfo, annotatedType);
    }

where computeAnnotatedType may or may not rely on an additional method in Annotated, for a best-effort inference of the JavaType having provided JsonTypeInfo.Value (but I suppose there could be tons of edge-cases around this (I never delved into Jackson, but I feel in handles many things ;))).

@JooHyukKim
Copy link
Member

Seems like serialization information being lost is the issue.
If I understood correctly, then we may also want to consider 'writeValueAsXxx(obj, Class)' like readValue?

@cowtowncoder
Copy link
Member

@JooHyukKim Problem is that computing of "minimal" type id uses different information during serialization and deserialization, for root values (issue not affecting non-root values):

  1. On deserialization polymorphic base type is explicitly passed (so it is available and usually accurate), but
  2. On serialization type is gotten from value being serialized, which is sub-type -- but that type is taken as base type because there is no real way to determine intended base type (except if constructing ObjectWriter with type -- but it might then not serialize full subtype properties).

And by base type I mean class that has @JsonTypeInfo annotation.

@cowtowncoder
Copy link
Member

@blacelle I am open to different ideas, but in general AnnotationIntrospector is not meant to do things more complicated than accessing annotations in flattened form. It should not traverse type/object hierarchies, for example, but provide information extracted from annotations. (fwtw, I think _constructStdTypeResolverBuilder is already bit too complicate).

Having said that, if you want to try prototyping I am happy to provide feedback.

@blacelle
Copy link
Author

@cowtowncoder I opened a PR sketching how we could rely on the annotation holder for more accurate baseType computation in idResolver.

#4988

@blacelle
Copy link
Author

(As a side-note, I'm a https://github.com/diffplug/spotless contributor. I may help if it felt relevant to wire Spotless to automatize formatting).

@cowtowncoder
Copy link
Member

Quick note: I don't think I want an auto-formatter, based on negative experiences I've had over time on level of effort to try to get things to work the way I would naturally format code and negligible value in 100% compliant formatting.
I don't consider formatting to be a huge problem with the project at this point, so investing in solving it makes no sense to me.

I mention this just so it might save time from back and forth with various proposals on formatting improvements.
(there are some areas where it might be ok; ordering of import statements come to mind).

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