PropertyNamingStrategy translates property names of properties that should not be visible

[etay2000]

Currently the PropertyNamingStrategy#translateName method is called on all class fields and methods, regardless of the PropertyVisibilityStrategy assigned. I would assume that those 'invisible' properties would not have any reason to have their property name translated.

This led to another somewhat edge case where using a lowercase PropertyNamingStrategy leads to a property being ignored:

JsonbConfig config = new JsonbConfig().withPropertyNamingStrategy(PropertyNamingStrategy.LOWER_CASE_WITH_UNDERSCORES);
Jsonb jsonb = JsonbBuilder.create(config);
Widget widget = new Widget();
Widget2 widget2 = new Widget2();
String widgetJson = jsonb.toJson(widget); //  {"url":"getter#www.google.com"}
String widget2Json = jsonb.toJson(widget2); // {}

public class Widget  {

        private String url = "www.google.com";

        public String getUrl() {
            return "getter#" + url;
        }

    }

public class Widget2  {

        private String url = "www.google.com";

        public String getURL() {
            return "getter#" + url;
        }

    }

Because the naming strategy changes 'URL' to 'url' in Widget2, it seems to conflict with the private 'url' field, and although no exception is thrown, no value is returned either. According to the default visibility strategy, the private 'url' field should not visible, so is the fact that a 'invisible' field can cause a conflict with a visible public method a separate issue in itself?

[aguibert]

hi @etay2000 thanks for raising this issue along with a testcase! It does indeed seem like odd behavior. I'll try to look into this issue soon.

[aguibert]

@etay2000 I was able to reproduce the issue you described using Yasson 1.0.4, but when I swapped to Yasson 1.0.5-SNAPSHOT it works fine. I'll add a test for this anyway and we can aim to put out a Yasson 1.0.5 release in the next week or so.

[etay2000]

@aguibert That's good to hear, and thanks for the quick response! Just to clarify when you say it works fine, do you mean that PropertyNamingStrategy is no longer called for invisible properties or just that there no longer appears to be a conflict with the properties colliding?

[aguibert]

yes, when I say it works fine now I mean that:

String widget2Json = jsonb.toJson(widget2);

now prints: {"url":"getter#www.google.com"} as expected

The PropertyNamingStrategy was never inspecting values of the hidden (private) properties, it was just incorrectly noticing that they were there and letting those properties overwrite public properties

[etay2000]

So with the 1.0.5 SNAPSHOT, in the following example, is PropertyNamingStrategy#translateName still called on on both properties?

void jsonbTest() {
        JsonbConfig config = new JsonbConfig()
                .withPropertyNamingStrategy(new TracingNamingStrategy());
        Jsonb jsonb = JsonbBuilder.create(config);
        Widget widget = new Widget();
        String json = jsonb.toJson(widget); // {"show":"visible"}
        // prints:
        // translateName: hide
        // translateName: hide
        // translateName: show
        // translateName: show
    }

    public static class Widget {

        private String hide = "invisible";
        public String show = "visible";

    }

    public class TracingNamingStrategy extends LowerCaseStrategy {

        @Override
        public String translateName(String propertyName) {
            System.out.println("translateName: " + propertyName);
            return super.translateName(propertyName);
        }

        @Override
        protected char getSeparator() {
            return '_';
        }

    }

[aguibert]

yes, with 1.0.5.SNAPSHOT translateName is still called on both properties

[etay2000]

Is the fact that the NamingStrategy is still translating property names on properties that are never serialized a bug? Even if the translated property name is ignored because the property is not visible it still seems inefficient to do the translation in the first place.

[aguibert]

I wouldn't say it's a bug, but it is a bit inefficient yes. However, I expect the performance impact is negligible. Keep in mind that class parsing only happens on the first time a given Jsonb instance serializes/deserializes a particular class. After that, the property model is cached for reuse.

So the most important thing for JSON-B performance is that Jsonb instances are reused as much as possible (ideally saved off in a private static final field), and not recreated each time you do to/fromJson.

[etay2000]

Fair enough, perhaps not a bug but maybe an enhancement then? Without digging into the code it seems like it would be as simple as checking property visibility and only conditionally using the naming strategy on visible properties? Either way, thanks for your help.

[aguibert]

Yes it could be a minor enhancement. However, we will be working on the next version of JSON-B soon here, and one of the features being considered is to make JSON-B annotations get processed by default regardless of visibility, which would require Yasson to be looking at private fields once again.

[etay2000]

Ok cool, sounds good to me.