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

runtime-configuration must have precedence over annotation #169

Closed
nimo23 opened this issue Jul 31, 2019 · 11 comments
Closed

runtime-configuration must have precedence over annotation #169

nimo23 opened this issue Jul 31, 2019 · 11 comments

Comments

@nimo23
Copy link

nimo23 commented Jul 31, 2019

Annotating a class with

@JsonbNillable(value = true)
@JsonbVisibility(value = PrivateFields.class)
public class Item {
..
}

and printing the json string with

try (Jsonb jsonb = JsonbBuilder.create(new JsonbConfig().withFormatting(true)
				.withNullValues(false)
				.withPropertyVisibilityStrategy(null)
				.withPropertyOrderStrategy(PropertyOrderStrategy.ANY))) {
			return jsonb.toJson(object);
}

actually prints the NULL-Values. However, it should NOT print the null values, because any runtime-configuration should have precedence over their corresponding annotations.

(Tested with jakarta.json.bind-api v1.0.1)

@m0mus
Copy link
Contributor

m0mus commented Aug 1, 2019

It looks like an implementation issue. What implementation are you using?

@nimo23
Copy link
Author

nimo23 commented Aug 1, 2019

I am using "javax.json.bind-api-1.0" with impl:

<dependency>
    <groupId>org.eclipse</groupId>
    <artifactId>yasson</artifactId>
    <version>1.0.4</version>
</dependency>

@nimo23
Copy link
Author

nimo23 commented Aug 1, 2019

Yes, thanks. Looks like an implementation issue:

Moved: eclipse-ee4j/yasson#290

@nimo23 nimo23 closed this as completed Aug 1, 2019
@nimo23
Copy link
Author

nimo23 commented Aug 5, 2019

The issue eclipse-ee4j/yasson#290 was closed because according to @aguibert:

The runtime configuration with JsonbConfig is the "global configuration" and is the broadest scope of configuration. If configuration exists as a smaller scope (e.g. annotation on a class, field, or method) then the smaller scope takes precedence.

Does this makes sense?

Annotations are hard coded within the class: this should be treated as "global configuration". Where the JsonbConfigs should be treated as "local configuration".

Imagine a json with null values (if one annotates @JsonbNillable(value = true), it is actually impossible for all clients to ignore null values even with new JsonbConfig().withNullValues(false).

Actually, it is impossible to override the annotation settings by a Jsonb-Config (which can be used multiple times with different configurations in runtime code).

Please consider to adapt the spec so that that runtime configuration can have precedence over (hard coded) annotations. I think, it makes sense.

@nimo23 nimo23 reopened this Aug 5, 2019
@rmannibucau
Copy link

Global means shared accross attributes/classes, local is the opposite so annotations are clearly local and jsonbconfig global IMHO. In all cases, config must override what is hardcoded for flexibility reasons.

@aguibert
Copy link
Contributor

aguibert commented Aug 6, 2019

Global means shared accross attributes/classes, local is the opposite so annotations are clearly local and jsonbconfig global IMHO.

Yep, agree with you here.

In all cases, config must override what is hardcoded for flexibility reasons.

I would guess the opposite here? The JsonbConfig should supply the default configurations, but if a class/method/field specifies otherwise (e.g. @JsonbNillable(true)) then it should take priority over the "global configuration". This is my interpritation of section 4.3 of the spec which states:

If annotations (JsonbNillable or JsonbProperty) on different level apply to the same field (or JavaBean property) or if there is config wide configuration and some annotation (JsonbNillable or JsonbProperty) which apply to the same field (or JavaBean property), the annotation with the smallest scope applies. For example, if there is type level JsonbNillable annotation applied to some class with field which is annotated with JsonbProperty annotation with nillable = false, then JsonbProperty annotation overrides JsonbNillable annotation.

The spec says that if there is a conflict between the "config wide configuration" and some annotation, then the annotation should win, which is the way Yasson currently behaves.

Is there a TCK test to clarify which way this behavior should be implemented? I don't have access to the JSON-B TCK, so I can't check.

@rmannibucau
Copy link

It is common to have to override the way a class is serialized for a partocular backend or to enable a dev option so annotations never override environment. Env is jsonbconfig for jsonb spec.

In other words: enable to not recompile the app to change a setting please.

@aguibert
Copy link
Contributor

I don't think that the global config-wide setting should be overriding fine-grained annotations. The global setting should be used to define what the default behavior should be if not otherwise specified by annotations on a particular class/field/method. This is the way the spec is currently written.

Inverting the behavior so that global config-wide configuration takes precedence over annotation-specific configuration would be a big breaking change for users for no real reason.

Instead, I think we should focus on these issues which will give a solution to the problem without breaking existing behavior:
#88
#168

@nimo23
Copy link
Author

nimo23 commented Aug 27, 2019

I don't think that the global config-wide setting should be overriding fine-grained annotations.The global setting should be used to define what the default behavior.

Actually, fine grained annotations are like chiseling in stone and never changable by global configurations. So there is no way to have global control over all of these annotations..If the spec really tells that what you said, then it should be changed.

@m0mus
Copy link
Contributor

m0mus commented Aug 27, 2019

Now runtime configuration is global. @aguibert is right, it can be overridden by annotations on smaller scopes like classes, fields, etc. In the new version of the spec we are planning to introduce the runtime extended configuration where you will be able to configure everything you do now with annotations. This configuration will have preference over everything.

@nimo23
Copy link
Author

nimo23 commented Aug 27, 2019

Ok, thanks. Good to know that it will be possible in future. Feel free to close this issue..

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

4 participants