jakartaee / jsonb-api

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

Create a JsonbConfig from an existing Jsonb instance #209

Open kaqqao opened 4 years ago

kaqqao commented 4 years ago

It is occasionally necessary to clone an existing Jsonb instance, and optionally override a specific setting. Would it be possible to add a method like Jsonb#getConfig that returns JsonbConfig?

Usage example

JsonbConfig customConfig = originalJsonb.getConfig()
            .withPropertyNamingStrategy(new CustomNamingStrategy());
Jsonb customJsonb = JsonbBuilder.create(customConfig);

(I replaced the example. The original was drawing attention away from the central point.)

rmannibucau commented 4 years ago

I would note a few points:

  1. An unwrap(Class) method makes sense on jsonb api to me
  2. However a contextual jsonb instance does not exist and is impl dependent so the impl can also define a contextual jsonbconfig instance so no need of this issue
  3. Sounds a bad idea to change a jsonbconfig for the naming only without owning the rest since it can lead to inconsistencies + mp should enforce the consistency IMHO

So at the end 1 is a good option/feature and 2/3 means the root cause is a bug in MP IMHO so we can also just not do anything at jsonb level if mp is cleaned up

kaqqao commented 4 years ago
  1. Agreed
  2. It doesn't have to exist. We'd just like to have a way to respect it if does. It's true that there could be an environmental config instance, but

    a) There isn't, and changing every MP impl out there isn't a reasonable expectation either

    b) The proposed feature would be useful regardless, as it is in no way tied to our use-case.

  3. We'd only wrap the existing naming strategy and fall back to it unless a more explicit GraphQL-specific naming is provided. How is respecting the user's Jsonb configuration less consistent than not respecting it? I'm confused by the argument. And again, the proposed feature is in no way tied to this specific use-case. Both Jackson and Gson have such a feature and none of them have MP or GraphQL use-cases in mind. The context for just an illustrative example.
aguibert commented 4 years ago

I think the overall usecase is reasonable to support. We should document that originalJsonb.getConfig() returns a COPY of the JsonbConfig though, so that it's clear the configuration for originalJsonb won't be impacted by the subsequent configuration.

Note: Implementations would only performa a full clone, because we would not be able to automatically clone Adapters/Serializers/Deserializers

rmannibucau commented 4 years ago
  1. @Inject JsonbConfig config; dont look from jsonb then - at least for mp platform this is what makes the most sense otherwise it means you dont want to expose and let the use do what you ask (at least this is how we do today). Since there are only yasson and johnzon impl, it is trivial to make it real in the cdi integration btw. A contextual cdi instance of jsonb is a big required debt and being able to read the config from CDI to override/enrich it makes a lot of sense. Ensuring a default config is regustered is not shocking too. I would push for a clean CDI integration rather than anything else on this.
  2. Issue is you can bind naming and runtime with jsonbconfig (like adapters, serializers or other parts) so changing one part can break it all.

Side note: isnt it weird to not respect jsonbproperty which is part of the platform in mp-graphql?

kaqqao commented 4 years ago

@rmannibucau It does respect @JsonbProperty. This whole thing is exactly about fully respecting all Jsonb settings. The user just has an option of renaming the fields in the GraphQL schema (effectively by changing the naming strategy under the hood). And this is something the users themselves have to do, nothing happens by itself. So any potential inconsistencies are no different than what you can normally get into when using Jsonb.

rmannibucau commented 4 years ago

If so, you dont have any need of this ticket or it is vendor specific, right?