FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Offer a way to access enclosing `AnnotatedClass` from `AnnotatedMember` #4141

Open SimonCockx opened 12 months ago

SimonCockx commented 12 months ago

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

The AnnotatedMember interface does not expose a method to access its enclosing AnnotatedClass.

Example use case: a custom annotation introspector

@Pojo(prefix="Prefix_")
public class Foo {
    @Field("Name")
    public int getField() {
        return 42;
    }
}

Suppose I would like to implement the AnnotationIntrospector#findNameForSerialization method to return "Prefix_Name" for the annotated method getField.

Solution 1: access annotation directly on the declaring Class<?> object

    @Override
    public PropertyName findNameForSerialization(Annotated a) {
        if (a.hasAnnotation(Field.class)) {
            String name = a.getAnnotation(Field.class).value();
            String prefix = "";
            // Try to access the prefix from the enclosing class.
            if (a instanceof AnnotatedMember) {
                AnnotatedMember m = (AnnotatedMember) a;
                Class<?> declaringClass = m.getDeclaringClass();
                if (declaringClass.isAnnotationPresent(Pojo.class)) {
                    prefix = declaringClass.getAnnotation(Pojo.class).prefix();
                }
            }
            return new PropertyName(prefix + name);
        }
        return null;
    }

Notice that, whereas I can access annotations of the Annotated input using the conventional Jackson API, I have to go through the basic Java Class API to access the annotation of the enclosing class.

This works for this particular use case. However, it does not work when inheritance is involved, e.g., for the following pojo:

@Pojo(prefix="Prefix_")
public interface Foo {
    int getField();

    public class FooImpl implements Foo {
        @Field("Name")
        @Override
        public int getField() {
            return 42;
        }
    }
}

It would be better if I would be able to somehow access the declaring AnnotatedClass object instead. This is what I try to do in the following solution.

Solution 2: access annotation on the declaring AnnotatedClass object.

    @Override
    public PropertyName findNameForSerialization(Annotated a) {
        if (a.hasAnnotation(Field.class)) {
            String name = a.getAnnotation(Field.class).value();
            String prefix = "";
            // Try to access the prefix from the enclosing class.
            if (a instanceof AnnotatedMember) {
                AnnotatedMember m = (AnnotatedMember) a;
                AnnotatedClass declaringClass = (AnnotatedClass) m.getTypeContext(); // Note: `getTypeContext` is deprecated.
                if (declaringClass.hasAnnotation(Pojo.class)) {
                    prefix = declaringClass.getAnnotation(Pojo.class).prefix();
                }
            }
            return new PropertyName(prefix + name);
        }
        return null;
    }

While this seems to work perfectly (even for the inheritance use case), I have to use a deprecated method getTypeContext, and I have to cast that result into an AnnotatedClass. I don't see a way around this though. For now, I am using this as a workaround, but ideally it would be possible to do this in a non-deprecated direct way.

Describe the solution you'd like

AnnotatedMember should expose a method called getAnnotatedDeclaringClass() that returns the enclosing AnnotatedClass.

Usage example

Taking my use case from above, it could be implemented as follows:

    @Override
    public PropertyName findNameForSerialization(Annotated a) {
        if (a.hasAnnotation(Field.class)) {
            String name = a.getAnnotation(Field.class).value();
            String prefix = "";
            // Try to access the prefix from the enclosing class.
            if (a instanceof AnnotatedMember) {
                AnnotatedMember m = (AnnotatedMember) a;
                AnnotatedClass declaringClass = m.getAnnotatedDeclaringClass(); // Example usage
                if (declaringClass.hasAnnotation(Pojo.class)) {
                    prefix = declaringClass.getAnnotation(Pojo.class).prefix();
                }
            }
            return new PropertyName(prefix + name);
        }
        return null;
    }
JooHyukKim commented 12 months ago

AnnotatedXxxx classes are deeply integrated and aren't easily customized. To assist better:

cowtowncoder commented 12 months ago

Quick note: my first thinking is -- no, I don't think adding back-link is a good idea. Further, defaulting should NOT be implemented by methods of AnnotationIntrospector but entities calling it.

I will need to think about a bit more as I do see potential benefits, but as @JooHyukKim pointed out, classes are quite tightly coupled and adding linkage may be problematic in other ways.

EDIT: thought I should clarify part of "defaulting should not be implemented" -- this refers to the idea that AnnotationIntrospector should provide information only about specific level: for example, @JsonIgnoredProperties for Class, separate from @JsonIgnoredProperties for fields/getters, or @JsonIgnore for fields/getters. And then it is BasicSerializerFactory (etc) that combines this information. That allows AnnotationIntrospector to be simple, including ones that "translate" annotations from other package like JAXB. But that obviously then requires that jackson-databind handles such aggregation. I understand that this is not the case for use case given here, for example.

SimonCockx commented 12 months ago

@JooHyukKim, @cowtowncoder Thanks for the quick reply. Details below.

Can you clarify the actual use case, beyond the implementation details provided?

Actual use case:

There is a modelling language called Rosetta, which generates Java code of the form

@RosettaDataType("MyType")
public interface MyTypeInJava {
    int getAttr();

    public class MyTypeInJavaImpl implements MyTypeInJava {
        private int attr;
        public MyTypeInJavaImpl(int attr) {
            this.attr = attr;
        }

        @Override
        @RosettaAttribute("attr")
        public int getAttr() {
            return attr;
        }
    }
}

based on a .rosetta input file of the form

type MyType:
    attr number (1..1)

We have a use case where clients need to be able to customize the serialised attribute names. Our current approach is to use a configuration file, e.g.,

{
    "MyType": {
        "attr": "MyCustomAttrName"
    },
    ... // custom names for other types
}

I would like to define a mapper that uses this configuration in such a way that mapper.writeValueAsString(new MyTypeInJavaImpl(42)) yields the following JSON:

{
    "MyCustomAttrName": 42
}

My current approach was to create an annotation processor that does the following:

  1. Given an annotated method with the RosettaAttribute annotation, lookup the RosettaDataType annotation on the enclosing class.
  2. From this RosettaDataType annotation, infer the data type name from the Rosetta model (e.g., "MyType").
  3. Lookup this name in the configuration to get a map from attribute names to custom JSON field names.
  4. From this map, lookup the custom JSON field name associated with the attribute name as indicated by the RosettaAttribute annotation (e.g., "attr"). This yields "MyCustomAttrName".

It's step 1 that I'm having trouble with. (finding the RosettaDataType annotation on the enclosing class)

Have custom de/serializers been considered?

I'm looking for a general purpose solution that will work for any generated Java class based on a Rosetta model, so this is not really possible.

Is there an similar feature out there? like @JsonSerialize or @JsonDeserialize

Since the Java code generator for Rosetta is currently independent of any serialisation framework, I don't see this happening soon.

Further, defaulting should NOT be implemented by methods of AnnotationIntrospector but entities calling it.

Could you elaborate this please? I'm not sure I get what you mean.

I will need to think about a bit more as I do see potential benefits

Keen to hear your ideas. :)

If you can think of another approach that allows me to do something similar, I'm all ears of course.

yihtserns commented 12 months ago

Why is your custom code not traversing through the class/interface hierarchy to look for the annotation? E.g.:

    @Override
    public PropertyName findNameForSerialization(Annotated a) {
        if (a.hasAnnotation(Field.class)) {
            String name = a.getAnnotation(Field.class).value();
            String prefix = "";
            if (a instanceof AnnotatedMember) {
                AnnotatedMember m = (AnnotatedMember) a;
                Class<?> declaringClass = m.getDeclaringClass();

                // TODO: Cache for performance
                List<? extends Class<?>> classes = Stream.iterate(
                                declaringClass,
                                clazz -> clazz.getSuperclass() != null,
                                clazz -> (Class) clazz.getSuperclass())
                        .toList();

                // Try find in class & superclasses
                Pojo pojoAnnotation = classes.stream()
                        .map(clazz -> clazz.getAnnotation(Pojo.class))
                        .filter(Objects::nonNull)
                        .findFirst()
                        .orElse(null);

                if (pojoAnnotation == null) {
                    // Try find in any interface
                    pojoAnnotation = classes.stream()
                            .flatMap(clazz -> Stream.of(clazz.getInterfaces()))
                            .map(interfaceClass -> interfaceClass.getAnnotation(Pojo.class))
                            .filter(Objects::nonNull)
                            .findFirst()
                            .orElse(null);
                }

                if (pojoAnnotation != null) {
                    prefix = pojoAnnotation.prefix();
                }
            }
            return new PropertyName(prefix + name);
        }
        return null;
    }
cowtowncoder commented 12 months ago

@yihtserns If using AnnotatedXxx, inheritance would be handled by Jackson so there'd be no need to traverse. So maybe it's to just show simplified example.

yihtserns commented 12 months ago

If using AnnotatedXxx, inheritance would be handled by Jackson so there'd be no need to traverse.

In the case where the same annotation is on multiple interfaces, which one wins?

cowtowncoder commented 12 months ago

@yihtserns Probably somewhat arbitrary, I'd have to check the logic. In general. "closer" one (as per class inheritance hierarchy). For classes it follows expected hierarchy.

yihtserns commented 12 months ago

My thought is that if the user has special needs/purpose, it would be better/safer for them to have control over the logic/rule for whatever they are doing, rather than relying on a 3rd party lib's behaviour. 🤔

SimonCockx commented 12 months ago

I'm fine with reusing the same inheritance mechanism that Jackson uses under the hood. I think diverging from that behaviour would only cause confusion, where users expect the same logic to apply as regular Jackson annotations.

That being said, I of course agree with @yihtserns that a manual implementation would also work. The point of this feature request is however convenience: I don't want to bother implementing some highly optimised code to traverse classes and interfaces if Jackson already implements it. :)

yihtserns commented 12 months ago

I'm fine with reusing the same inheritance mechanism that Jackson uses under the hood. I think diverging from that behaviour would only cause confusion, where users expect the same logic to apply as regular Jackson annotations.

The problem with relying on that kind of undocumented (thus unguaranteed) behaviour is, users of your code may be in trouble if Jackson decides to change the behaviour/rule. 🤷

Anyway, @cowtowncoder why was AnnotatedMember.getTypeContext() deprecated? The commit didn't explain the reason.

JooHyukKim commented 12 months ago

Anyway, @cowtowncoder why was AnnotatedMember.getTypeContext() deprecated? The commit didn't explain the reason.

Try checking out the commit git checkout 01a37b9b2b3bff82c5aa0cd8dd399918d95eaffb and see the trailing commits that lead up to the mentioned commit. Seems like part of #1381, though I am not sure if answers the question.

SimonCockx commented 11 months ago

The problem with relying on that kind of undocumented (thus unguaranteed) behaviour is, users of your code may be in trouble if Jackson decides to change the behaviour/rule.

I think you could say this about users of Jackson as well. If anybody relies on Jackson annotations combined with inheritance (which I would guess is common practice - correct me if I'm wrong), their code might be in trouble if Jackson decides to change the behaviour. If this is a concern, then IMO this is more of a Jackson docs issue, and the solution would be to add documentation about it.

Anyway, my main point is that this is a feature request for convenience. If the consensus is that Jackson's inheritance mechanism is for internal use only, then so be it; otherwise I would like to reuse it. :)

yihtserns commented 11 months ago

I think you could say this about users of Jackson as well.

I'm just sharing a word of caution since you're writing an API/lib for other people to use by relying on a behaviour that not under your control. But yeah it is not a big deal even if the behaviour changed and break your end users' usage, you can always rewrite the API/lib code anyway.

Just to be clear, I'm not objecting to this feature request, just sharing some things you may want to consider about the potential complication when it comes to annotation resolution in class hierarchy (as you can see in the two loops in my sample code - if multiple superclasses/interfaces annotated, who should win: should you decide, or should Jackson decide, or should it be random, or it is not a concern in your use case, etc) esp when dealing with interfaces (the reason why @Inherited doesn't work on interfaces in Java) because I personally hit this wall not long ago.

cowtowncoder commented 11 months ago

Ok, lots to unpack here. I will try to explain my thinking wrt annotation inheritance firsts. So here are couple of pointers on that:

  1. Resolution wrt precedence has not really changed over time much if at all; it is not well document, I agree
  2. I do not recall issues reported against either resolution logic or changes. I'll try to summarize logic as I remember it below
  3. To me, having every developer implement their own logic is extremely wasteful and more error-prone; generally they'd not handle inheritance nor support mix-in annotations
  4. To guard against regression we need tests and not just disband functionality or think in terms of "it might change". The problem are not changes themselves but unintended consequences/side-effects to behavior not covered by tests.

And here's the precedence as I remember it. Let's consider type hierarchy like:

public class Impl extends Base implements C, D { }

class Base implements A, B { }

Mix-ins are "mixed in" at level right above target class. So, we get:

cowtowncoder commented 11 months ago

And then on why AnnotatedMember.getTypeContext() was deprecated. First of all, I should have created a separate issue for its deprecation (and eventual removal from 3.0), which would have contained reasoning behind change.

But as I recall it, access to the entity is something that:

  1. Should only be needed by core pieces of jackson-databind
  2. Use by non-core components would likely be wrong, either via incorrect/unintended usage, or because caller does not have proper information/knowledge to call it.

And in particular here, it would not necessarily have meant "class in which member is declared" -- it depends on type declaration. For example, for local type variable binding like:

class Example {
    public <T extends java.io.Serializable> T value;
}

context for type for Field value would have to be Field declaration, not class.

So it is and was not intended to be containing entity for Annotated thing but context needed to resolve the type for Annotated thing. And that really should not (need to) be exposed -- type to access should be resolved by jackson-databind itself, to produce proper JavaType. And not expect calling code to handle resolution from java.lang.reflect.Type with context (which is what databind will do).

SimonCockx commented 11 months ago

@cowtowncoder Thank you for the extensive context. What is your advice? Add an additional method to AnnotatedMember so users can access the AnnotatedClass reliably without using my current hack? Provide access to a cache from Class<?> to AnnotatedClass? Something else?

cowtowncoder commented 11 months ago

@SimonCockx I don't know, to be honest. Your best bet might be cache from Class to AnnotatedClass, on short term.

I will not be adding an accessor in Jackson 2.16, for sure (partly due to timeline). Could be considered for 2.17 I suppose.

SimonCockx commented 11 months ago

@cowtowncoder Thanks. That sounds perfectly reasonable.

yihtserns commented 11 months ago

I don't get it. So the answer is stick to using the deprecated .getTypeContext()?

cowtowncoder commented 11 months ago

I don't get it. So the answer is stick to using the deprecated .getTypeContext()?

No, please do not use getTypeContext() for anything, anywhere. It should go away.

JooHyukKim commented 11 months ago

From all the comments above, have you resolved your issue, @SimonCockx? Asking because this issue seems open ended 🤔

SimonCockx commented 11 months ago

@JooHyukKim Just to be clear: this is a feature request - not an issue. I started off with working code. As described in this thread, there exist two alternative implementations for supporting annotation inheritence:

  1. The workaround that I came up with: AnnotatedClass enclosingClass = (AnnotatedClass) m.getTypeContext(). I found that this works fine at the moment, although there might be edge cases where this fails, and @cowtowncoder clearly discourages usage of getTypeContext.
  2. The manual implementation of @yihtserns.

My conclusion from this thread - correct me if anyone disagrees - is that an accessor for the enclosing AnnotatedClass will be added in a future version. At the earliest in 2.17. See @cowtowncoder:

I will not be adding an accessor in Jackson 2.16, for sure (partly due to timeline). Could be considered for 2.17 I suppose.

JooHyukKim commented 11 months ago

Just to be clear: this is a feature request - not an issue

I see, thank you for pointing out and also explanation 👍🏼👍🏼 I meant "issue" as more generic term "Github Issue", one that has a UI tab in Github. And I missed what you shared.

cowtowncoder commented 11 months ago

Just to make sure my comment was understood correctly: I did not say it is planned to be added, but rather that it is a possibility. That is, I am not ruling it out. Tagging this with 2.17 does suggest evaluation at earliest for that version.