search

jakartaee / jsonb-api

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

@JsonbProperty enables serialization of private fields in Johnzon, but not in Yasson #271

Closed mkarg closed 3 years ago

mkarg commented 3 years ago

I have noticed that putting @JsonbProperty on a private field having neither getter nor setter, Johnzon will write a JSON attribute when serializing, while Yasson will not. Please clarify which one is wrong, and unambigulously describe in the spec how it MUST work in all compliant products. Thanks.

mkarg commented 3 years ago

@romain-grecourt @m0mus Please discuss and decide. Thanks. :-)

rmannibucau commented 3 years ago

This is fully defined by property visibility AFAIK and not the default behavior in the johnzon version passing jsonb tck IIRC.

m0mus commented 3 years ago

With default settings Java visibility modifiers have priority, so Yasson behavior is correct.

mkarg commented 3 years ago

Thank you, Romain and Dmitry, for your quick opinions! I checked the spec and agree that Yasson's outcome is right, so Johnzon's is wrong.

Please find the attached self-contained reproducible, which you can run with mvn exec:java -PJonzon or -PYasson. It uses the same default configuration always, and proofs that:

According to your answers above I think it is undisputed that both products should return the same outcome instead, and that it should be {"publicproperty":4} simply.

So do you agree this is a bug in Johnzon or does Johnzon need an explicit configuration to behave compliant to the specification (which IMHO should be the default behavior actually)?

@rmannibucau As a result, I will file an issue with Johnzon, if you agree that this is a bug in Johnzon (otherwise it makes no sense to file it).

rmannibucau commented 3 years ago

@mkarg reference is https://issues.apache.org/jira/browse/JOHNZON-100 and is an anticipation of next spec release so you can open a ticket but best fix you will get is a toggle for that feature I assume.

mkarg commented 3 years ago

Your answer is confusing me a bit. You said "best fix you will get is a toggle".

mkarg commented 3 years ago

@rmannibucau I filed https://issues.apache.org/jira/browse/JOHNZON-341 for the further discussion, thanks!

As this issue's original question meanwhile was answered, I am hereby closing this issue. Thank you, @rmannibucau and @m0mus, for resolving this issue such quickly! :-)

rmannibucau commented 3 years ago

@mkarg passing tck enable to exclude some tests if irrelevant (several were already) but this is also why i mentionned a toggle since spec tend to enable this case in next releases. To be vendor independent you have to respect strictly the spec - your sample does not. Vendors are always enabled to do more than the spec (look all spec impl ;)). The toggle being on/off by default - or even existing - is a project choice at the end while spec cases are enabled.

struberg commented 3 years ago

I think JSONB-1.0 spec does not define that private fields are not allowed for @ JsonProperty! At least that's the way I do read it:

4.1.2 javax.json.bind.annotation.JsonbProperty JsonbProperty annotation may be specified on field, getter or setter method.

There is no single word about visibility. Also used to be none in the original JavaDocs. Not sure if any wording got changed later on, but don't think so. Of course there was also no TCK test nor explicit word that it MUST be supported.

What is the argument that private fields must not be annotated? The problem we tried to solve with doing it that way in Johnzon is that there is a use case where you have internal fields which are not accessible via setter/getter. E.g. when you have a json with a single timestamp field but getter/setter for date and clock separate. Or the other way around. How to solve this easily otherwise? Only other option I can quickly think of is doing a tinkered Adapter or PropertyVisibilityStrategy. But for me that sounds like quite some overhead. If a user adds an annotation to a private field it might be rather unexpected that it does not get picked up - and not even issue a warning about it.

m0mus commented 3 years ago

@struberg You can place @JsonvProperty on any field. By default it should not be processed because default fields access strategy works only with public properties and with properties having public getters/setters. I was always saying on conferences that JSONB respects Java visibility rules. See here: https://jakarta.ee/specifications/jsonb/2.0/jakarta-jsonb-spec-2.0.html#scope-and-field-access-strategy

mkarg commented 3 years ago

@mkarg To be vendor independent you have to respect strictly the spec - your sample does not.

In what way is my example not strictly respecting the spec, actually...?

rmannibucau commented 3 years ago

@mkarg behavior of jsonbproperty annotation on a private field with default visibility is not a jsonb feature, drop it and your code becomes portable in v1/v2, keep it and wait for v3 decision which seems to be requested regularly to align on other specs by supporting private fields too.