jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
79 stars 39 forks source link

enhance PropertyVisibilityStrategy and visibility strategy to cope with class hierarchies #325

Open struberg opened 2 years ago

struberg commented 2 years ago

JSONB currently has some weakness when it comes to class hierarchies like class A extends B.

In 3.7.1 of the JSONB-1.0 spec it states that For a serialization operation, if a matching public getter method exists, the method is called to obtain the value of the property. If a matching getter method with private, protected, or defaulted to package-only access exists, then this field is ignored. If no matching getter method exists and the field is public, then the value is obtained directly from the field.

This can get changed using a @JsonbVisibility

4.6 Custom visibility To customize scope and field access strategy as specified in section 3.7.1, it is possible to specify javax.json.bind.annotation.JsonbVisibility annotation or to override default behavior globally calling JsonbConfig::withPropertyVisibilityStrategy method with given custom property visibility strategy.

There are 2 problems: 1.) 3.7.1 does not state what happens in the case of the public field being in class B long B#myVal and a getter exists in class A extends B public long A#getMyVal().

2.) PropertyVisibilityStrategy only has an interface public boolean isVisible(final Field field). But Field does not contain the information that the visibility of the field myVal needs to get checked for class A. Thus it would not even be possible to implement the rules of 3.7.1 via a portable PropertyVisibilityStrategy if we assume that the getter in the subclass is taken into account.


Another (related) question: what effective visibility do we get if we have the following situation and try to serialise an instance of A?

@JsonbVisibility(VisibleAllFields.class)
public class B {
    public long myVal;
    public long getMyVal() { return myVal*2;}
    ...
}

@JsonbVisibility(VisibleAllMethods.class)
public class A extends B {
    public int otherVal;
    public long getMyVal() { return myVal*4;}
    public int getOtherVal() { return otherVal+5;}
    ....
}

What do we get in the end? does VisibleAllFields apply to the parts from class B? Or does VisibleAllMethods apply to the whole class hierarchy? Afaict this is not yet clarified, isn't it?

rmannibucau commented 2 years ago

Hi Mark,

think you spotted an ambiguity of the spec but, since I think we also don't want a @JsonbVisibilityOverride kind of annotation to change the parent one and it is trivial in the child visibility impl to reuse the parent one, I think we should just use the child one and ignore the parent one until the parent instance is the root instance passed to jsonb.

struberg commented 2 years ago

Yes, but for this to work properly we probably need to enhance the current API to something like public boolean isVisible(final Type rootType, final Field field) (not quite sure whether Type or Class btw).

rmannibucau commented 2 years ago

@struberg not really since you can already do a ChildVisibility. It is true it requires some duplication in the rare cases you don't want the same in child and parent but I consider it is rare enough to not be a big deal to have to do it. So the visibility class gives the context there.

struberg commented 2 years ago

Sorry cannot follow. I assume that visibility strategies are reused and no new implementation is needed for every class.. And java.lang.reflect.Field#getDeclaringClass() simply doesn't know about the class inheritances a field is used in. So - given the current API - for B#myVal you'll never get the information that in class A extends B a getter for it does exist in class A.

rmannibucau commented 2 years ago

@struberg agree on that and opened a generic issue about it but point is that for general case it is okish and for these rare cases you can contextualize it with a dedicated impl per class as a workaround.

edit: for ref https://github.com/eclipse-ee4j/jsonb-api/issues/328 explains that inheritance + serialization/deserialization can't be handled with current SPI