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

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

Open
struberg opened this issue Mar 28, 2022 · 5 comments

Comments

@struberg
Copy link

struberg commented Mar 28, 2022

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
Copy link

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
Copy link
Author

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
Copy link

@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
Copy link
Author

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
Copy link

rmannibucau commented Apr 11, 2022

@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 #328 explains that inheritance + serialization/deserialization can't be handled with current SPI

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

2 participants