JSON-B should process any field/method with JSON-B annotations regardless of visibility

[jsonbrobot]

@JsonbProperty annotation if put on not public field/getter/setter must automatically make it visible for JSON-B engine.

[jsonbrobot]

• Issue Imported From: https://github.com/javaee/jsonb-spec/issues/61
• Original Issue Raised By:@m0mus
• Original Issue Assigned To: Unassigned

[aguibert]

Given the field/method visibility restrictions added in Java 9, I do not think it would be a good idea to encourage users to expose non-accessible fields/methods which would require reflection hacks by a JSON-B implementation.

Closing this issue, but CC @m0mus in case you would like to argue further in favor of this.

[m0mus]

It's still possible to access private fields in JDK > 8. I still think that this is a valuable feature. Sometimes it makes sense initialize an immutable object from JSON. This feature can help here. Reopening.

[rmannibucau]

My worry about this feature is to start getting conflicts with visibility SPI and therefore not being able to modify the visibility from the SPI which should always win - to enable it to be dynamic by deployment/config - and when you make it win then respecting the static model (vs dynamic) is a pain for end users. Wdyt?

[m0mus]

Visibility SPI is not supported by the spec (yet?). So, there is no issue for now. I actually don't see an issue here at all. As we started discussing here https://github.com/eclipse-ee4j/jsonb-api/issues/88 we will extend JsonbConfig and allow to configure all customizations provided by annotations now. It should include visibility tuning functionality too. We will also allow reading configuration from external sources using MP Config. Please correct me if I'm wrong, but I think that MP Config supports adding config sources using SPI.

[rmannibucau]

@m0mus it is: https://github.com/eclipse-ee4j/jsonb-api/blob/master/api/src/main/java/javax/json/bind/config/PropertyVisibilityStrategy.java

[aguibert]

It's still possible to access private fields in JDK > 8.

It is only possible in JDK > 8 because --illegal-access=warn is enabled by default. Illegal access will not always be enabled by default. Furthermore it will be removed entirely in some future release of Java:

    --illegal-access=<value>
                      permit or deny access to members of types in named modules
                      by code in unnamed modules.
                      <value> is one of "deny", "permit", "warn", or "debug"
                      This option will be removed in a future release.

APIs and libraries should be moving away from illegal access behavior, not toward it.

[rmannibucau]

@aguibert no, you can configure it in your module-info too....this would break all libraries otherwise ;). See open module or opens

[aguibert]

true... all of my experience with Java modules has been about compatibility and making things work as-is, but once java modules are more properly adopted then people will have the "opens" option in their module-info.

Regarding Romain's concerns about collision with the visibility SPI, I think we can make this change in a way that is non-breaking, and update the spec/javadoc accordingly to define what happens in the case of conceptual conflicts in property visibility.

I propose the following visibility resolution priority:

1. If a custom visibility strategy is enabled via the SPI, it is used to determine property visibility
2. [proposed addition] If the default visibility strategy is used, if a field/method is annotated with @JsonbProperty it is visible regardless of the visibility of the Java element (e.g. public/private/etc...)
3. If the default visibility strategy is used, a property field/method is visible if it is marked public

[rmannibucau]

3 likely need to be adjusted to today's behavior (from memory it is public or protected for fields). Your modification of 2 works but violates java bean standard so not sure it is a good thing. Can we at least adjust it this way: the visibility of the java property must be public (protected?) but if the field is private and has a @Jsonb* then it is used. It would at least be aligned with most of the specs. In other words: overall visibility is still respected but modifiers can be set on the private part of the property.

Enabling the following use cases sounds like a counter productive solution:

public class Foo {
    @JsonbProperty("bar")
    private String foo;
}


public class Foo {
    private String foo;

    @JsonbProperty("bar")
    private String getFoo() { return foo; }
}

public class Foo {
    @JsonbProperty("bar")
    private String getFoo() { return foo; }
}


public class Foo {
    @JsonbProperty("bar")
    private String foo;

    private String getFoo() { return foo; }
}

wdyt?

I would also highlight that if we solve the metamodel issue then this issue is a detail and does not need anything - in pseudo code and inspired from CDI metamodel:

public class MyMetaModelExtension implements JsonbModelLoader {
    @Override // you can set the modifier to public in all cases adjusting your need in term of model for each class you own or *not* which is not possible with current proposal
    public Annotated[Field|Method|Constructor|Type] getModel(final Class|Field|Method|Constructor element) { .... }
}

[aguibert]

2 works but violates java bean standard so not sure it is a good thing. Can we at least adjust it this way: the visibility of the java property must be public (protected?) but if the field is private and has a @jsonb* then it is used

If I'm reading this correctly, your proposed change is to consider ANY JSON-B annotation as making a property visible-by-default rather than just the @JsonbProperty annotation? I'd be fine with that adjustment.

Enabling the following use cases sounds like a counter productive solution:

All of those examples look fine to me, and seems to be what Dmitry is proposing we do here. Can you elaborate on why you think those code examples are counter productive?

[rmannibucau]

Almost, it also require the property to be public somehow and we would read jsonb annotation on private fields if getter/setter is public/protected (already taken into account). Fully private properties stay ignored as expected to not have half of properties serialized and not the other half ignored which is a pain. We use convention over config and this would break this good pattern.

[aguibert]

Updating this issue to cover JSON-B annotations in general so it also encompasses @JsonbCreator as requested by this Yasson issue; https://github.com/eclipse-ee4j/yasson/issues/326

[rdehuyss]

Please also consider deserializing final fields.

It allows us to think about the code in an immutable way once the object is correctly constructed by JSON-B.

Both Jackson and Gson allow to deserialize final fields.

[rmannibucau]

@rdehuyss is @JsonbCreator just missing to deserialize null for you, cause JSONB already supports that?

[rdehuyss]

Well, I cannot use @JsonbCreator as I develop a library (https://github.com/jobrunr/jobrunr) that supports Gson, Jackson and (almost there) also Json-B. I prefer not to have 3 types of annotation on my model so all serializing is done either out-of-the-box or using custom serializers/deserializers/adapters.

I only have compile time dependencies on the JSON-B api and it is up to the consumer to select one of these JSON libs.

[rmannibucau]

@rdehuyss then not sure what you miss since a custom deserializer or custom adapter (or even adding annotation since annotations are ignored if missing in the classloader) solutions works.

Back to this issue Im concerned about an inconsistent programming model if we do it. Basically jsonb is mainly implicit and this issue breaks it meaning

private String name;

Is no more equivalent to

@JsonbProperty private String name;

So issue is either to consider private fields by default or not IMHO but not to have activator annotations. This is a breaking change so I'd see it as a new visibility mode activable on classes and/or model package. Think it is saner and keeps some consistency which is important IMHO.

[rdehuyss]

Wait, let me give a test case to show what I mean:

package org.jobrunr.jobs.mappers;

import org.eclipse.yasson.FieldAccessStrategy;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import javax.json.bind.Jsonb;
import javax.json.bind.JsonbBuilder;
import javax.json.bind.JsonbConfig;

public class JsonbTest {

    @Test
    public void test() {
        final Jsonb jsonb = JsonbBuilder.create(new JsonbConfig().withPropertyVisibilityStrategy(new FieldAccessStrategy()));

        // succeeds
        final ObjectWithoutFinalFields withoutFinalInput = new ObjectWithoutFinalFields("a string");
        final ObjectWithoutFinalFields withoutFinalActual = jsonb.fromJson(jsonb.toJson(withoutFinalInput), ObjectWithoutFinalFields.class);
        Assertions.assertEquals(withoutFinalInput.getString(), withoutFinalActual.getString());

        // fails
        final ObjectWithFinalFields withFinalInput = new ObjectWithFinalFields("a string");
        final ObjectWithFinalFields withFinalActual = jsonb.fromJson(jsonb.toJson(withFinalInput), ObjectWithFinalFields.class);
        Assertions.assertEquals(withFinalInput.getString(), withFinalActual.getString());
    }

    public static class ObjectWithoutFinalFields {

        private String string;

        protected ObjectWithoutFinalFields() {
            //needed for deserialization
        }

        public ObjectWithoutFinalFields(String string) {
            this.string = string;
        }

        public String getString() {
            return string;
        }
    }

    public static class ObjectWithFinalFields {

        private final String string;

        protected ObjectWithFinalFields() {
            //needed for deserialization
            this(null);
        }

        public ObjectWithFinalFields(String string) {
            this.string = string;
        }

        public String getString() {
            return string;
        }
    }

}

Since I'm working on a multi-threaded library, the final help me assure that I don't make silly mistakes :-) ... however, when I want to support Jsonb (Yasson), I need to remove all the final Modifiers. I prefer to keep those.

The line causing the issue (in Yassin) is https://github.com/eclipse-ee4j/yasson/blob/master/src/main/java/org/eclipse/yasson/internal/model/PropertyValuePropagation.java#L103

Does this make sense?

[rdehuyss]

The same is happening in Johnzon: https://github.com/apache/johnzon/blob/master/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/access/FieldAccessMode.java#L88

When writing, includeFinalFields is false and they are thus not deserialized.

Could there be a strategy just like the PropertyVisibilityStrategy?

[rmannibucau]

@rdebusscher well, as mentionned, it is solved by @JsonbCreator and if you don't want it you can add a custom deserializer/adapter to handle it as you mentionned. Final fields will never be taken for deserialization since we can't set it portably but constructor or factory methods are the only ones you can use. Typically for johnzon this is not the line you mentionned but this method https://github.com/apache/johnzon/blob/master/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonbAccessMode.java#L185.

side note about johnzon: johnzon has an interesting mode, if you drop your default constructor (which makes the jsonb model invalid since without an explicit @JsonbCreator it defaults to the no-arg constructor so it shouldn't be needed at all, in particular with final fields), then you can use @ConstructorProperties to deserialize the model. It enables to be a bit more portable.

Now back to the need, it sounds to me that your need is related to the ability to decorate the model programmatically - we have others issues about it - which sounds to me more elegant than having an ad-hoc SPI for that particular feature. Otherwise - in case you can decorate explicitly your model - you don't need it at all since your constructor is public.

Side note: i'll just repeat cause rereading one of your previous comment i'm not sure it is obvious, you can put jackson+gson+jsonb annotations on your model and keep these dependencies in scope provided to let the consumer pick only one, it works well thanks to the way java loads annotations.

[robmv]

There are situations where mapping frameworks should legitimately access private fields. Take note that there are other libraries handling the problem, for example Hibernate JPA implementation, read https://in.relation.to/2017/04/11/accessing-private-state-of-java-9-modules/

I don't know what route they took, but I really prefer the open module to the reflective using module. If someone needs to map private fields, they probably will not have a problem deciding to open the module to the JSON-B implementation they are using.

[rmannibucau]

Think the issue is more about including a @JsonbProperty private String foo by default or not. JPMS just requires the user to open or not the related reflection but it is up in the module (even if JSON-B implies reflection usage it is not strictly require but I assume we can consider it is needed by default even if we can need to explicit it and some impl bypass the relfection by build time generation). The issue is that this is a breaking change and kind of violate java rules. The issue issue is that it misuses @JsonbProperty which is intended to rename (remap) a name and not be used to include a field. The inclusion is done with PropertyVisibilityStrategy SPI so already in the box. Only question for be can be to potentially add some flag to enable to ALL_SCOPE or ALL_MARKED impl but i'm not sure it is that needed - I actually find as much drawbacks to these impl than advantages and since the impl is trivial to do when needed I'm not sure it is a gain to have it OOTB.

[asbachb]

Actually I agree with @rdehuyss when you want to have your object immutable your forced to do more or less a manual mapping via a custom adapter or @JsonbCreator annotation which raises the question why should I use a mapping framework at all when I need to do the stuff manually.

This kind of solution is also quite fragile when you think about renaming fields.

[rmannibucau]

@asbachb well there are multiple points:

1. visibility respect -> I see no valid reason to check more than what is supported in the spec. I agree checking all fields would maybe have been better - and to be honest I didn't push to limit to protected field at all ;) - but this is unrelated to immutability at all.
2. @JsonbCreator should relax its usage to not require a value for all attributes -> I clearly agree and this is what the property johnzon.failOnMissingCreatorValues does in Johnzon implementation enabling value objects or record to work smoothly in all cases (here the discussion point was that if a field is required, it is trivial to do a requireNonNull in the constructor so the JSON-B value enforcement is overkill and not even consistent with the fact the constructor will require more validations in most cases)
3. Renaming: does not change much since IDE/Editors (thinking to Idea and vscode) know how to detect that these days so for me only point 2 is a blocker as of today and we can relax it but it is clearly another issue and not this one.