jakartaee / jsonb-api

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

transient-keyword should be ignored by jsonb #269

Closed nimo23 closed 3 years ago

nimo23 commented 3 years ago

Describe the bug Properties with transient-keyword should not be a trigger for jsonb for not (de)serializing.

To Reproduce I have this:

// should (de)serialize it, but jsonb ignores @JsonbProperty and give "transient" a higher priority, 
// thus jsonb, unfortunatley, does not (de)serialize user.
@JsonbProperty
private transient User user;

With the above code, jsonb (with its implementation yasson) does not generate json for property user. Only when removing transient-keyword, then yasson will generate the json.

Expected behavior Jsonb should ignore the java keyword transient for its (de)serializing stuff. Only @JsonbTransient should be a trigger for omitting properties. And if this is not possible, then marking a property with @JsonbProperty should have priority over transient- keyword.

rmannibucau commented 3 years ago

Hi @nimo23, think it is already covered https://github.com/eclipse-ee4j/jsonb-api/blob/cc5273d2b0ff06f778747428fb672830844337be/spec/src/main/asciidoc/jsonb.adoc:

JSON Binding implementations MUST NOT deserialize into transient, final or static fields and MUST ignore name/value pairs corresponding to such fields.

and

Implementations MUST support serialization of final fields. Transient and static fields MUST be ignored during serialization operation.

and AFAIK it is expected because these fields are generally not serializable (generally speaking).

nimo23 commented 3 years ago

@rmannibucau I think, you did not understand https://github.com/eclipse-ee4j/jsonb-api/issues/269#issue-856740943:

Transient and static fields MUST be ignored during serialization operation.

This is exactly what I don't like because transient-keyword can be used for other use cases than jsonb. If I use "transient" for the other use case, it clashes with jsonb spec..

Jsonb should behave like this:

// Jsonb MUST (de)serialize user (even with keyword "transient"), if the user marks this property with @JsonbProperty. This is currently NOT the case.
// The "transient" keyword is used for another use case than jsonb, so I cannot omit it.
// Unfortunatley, if I do not omit "transient", then jsonb does not (de)serialize this property.
@JsonbProperty 
private transient User user;

Btw, I have explicitly marked a property with jsonb annotations (@JsonbProperty), and, unfortunately, the spec currently says: "Hey, I ignore my own annotation because of the transient keyword).

The spec should behave like this:

1.

// (de)serialize because of explicitly marked with @JsonbProperty (actually, not covered by spec)
@JsonbProperty 
private transient User user;

2.

// no (de)serialize because of explicitly marked with transient (already covered by spec)
private transient User user;

3.

// no (de)serialize because of explicitly marked with @JsonbTransient (already covered by spec)
@JsonbProperty
@JsonbTransient
private User user;
rmannibucau commented 3 years ago

@nimo23 well I see two points:

  1. we must not break users so I fear it is too late to change until we add a @JsonbIncludeTransient on the class (which I think is overkill since you can drop transient)
  2. this was done for the mentionned reason: there is the case you mention where this is done for java serialization and would work for jsonb but you also have all the cases it is done because it will never work for any serialization (ClassLoader for ex) and the case you reuse models in JSONB (implicitly or explicitly like to store in a DB) and you must respect transient

So overall I don't see the default changing since it prove to be the best but we can get a toggle to change it...and luckiyl it already exists with property visibility so I guess we must just ensure it works already and clarify this API more than changing anything else.

nimo23 commented 3 years ago

and you must respect transient

To sum up: Currently, I cannot solve the conflict when using transient keyword for other use cases than json-b. There is no chance to tell json-b: "Hey, do the (de)serializing on that field even with a transient keyword, because I added explicitly @JsonbProperty."

So overall I don't see the default changing since it prove to be the best but we can get a toggle to change it...and luckiyl it already exists with property visibility so I guess we must just ensure it works already and clarify this API more than changing anything else.

Yes, I could add PropertyVisibility with field and method visibility set to true. However, then I change it globally only because json-b ignores its own @JsonbProperty in favour of "transient"..

rmannibucau commented 3 years ago

@nimo23 you mean @JsonbProperty public transient String foo;? Have to admit from a code design perspective it looks like a workaround and not a solution to me. JsonbProperty is designed to rename a property, not to include/exclude properties. Concretely public class Foo { @JsonbProperty private String foo; } will serialize any Foo instance as {}, same happens to properties filtering so I wouldn't encourage this misusage and prefer to promote the SPI usage instead.

m0mus commented 3 years ago

We believe that Java modifiers like transient and visibility options like private and public have the highest priority and it's functionality must be consistent over all upper stack frameworks. It means that if a field is transient it must behave as transient in JSONB. If you have a use case where you need to serialise or desirealize a transient property, your architecture is bad and you have to change it.

I am closing this issue as won't fix.

nimo23 commented 3 years ago

We believe that Java modifiers like transient and visibility options like private and public have the highest priority and it's functionality must be consistent over all upper stack frameworks.

Yes, this is reasonable. After thinking about, I come to the same conclusion.