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

Apply JSON-B customizations to existing (i.e. 3rd party) classes #88

Open
hei1233212000 opened this issue Aug 21, 2018 · 46 comments
Open
Labels
enhancement New feature or request
Milestone

Comments

@hei1233212000
Copy link

In the application, you may need to serialize/deserialize some objects which come from 3rd party library. So that we cannot put JSON-B annotaions on it.

For resolve this issue, just like some IoC framework, support XML configuration would be valuable. So, any plan for this kind of enhancement?

Thank you.

@aguibert
Copy link
Contributor

I think applying JSON-B metadata to objects that the developer is not able to modify is a good case to have covered. However, I would be strongly opposed to any sort of XML-based configuration, especially being as this is for a JSON standard. Ideally we can come up with some sort of java-based configuration mechanism in a future version of the spec.

@rmannibucau
Copy link

+1 for a java API, in johnzon we called it accessmode and it enables to map the instantiator, read and write properties. Guess jsonb can get the same in its builder. Another use case is to bridge an api, like supporting jaxb api to produce json like jettison does.

@aguibert aguibert added the enhancement New feature or request label Jul 22, 2019
@aguibert aguibert changed the title Any plan to make it supports XML configuration? Apply JSON-B customizations to existing (i.e. 3rd party) classes Jul 22, 2019
@m0mus
Copy link
Contributor

m0mus commented Jul 23, 2019

I agree. We should provide an ability to configure JSONB in Java code. The functionality should cover all customizations/configuration provided by annotations. Also, JsonbConfig should support reading configuration from external sources using MicroProfile configuration or Config JSR which is in development now.

@rmannibucau
Copy link

Also, JsonbConfig should support reading configuration from external sources

To clarify: what I had in mind was JSON-B being able to read a JSON holding its configuration, so supporting any external tool should be as trivial as: jsonbConfig.withMappingConfiguration(myJsonConfig). myJsonConfig should be supported as being JsonObject, String or JsonbMapping (I assume we should be good enough to use ourself first).

@aguibert
Copy link
Contributor

what would be the value of holding JSON-B config in JSON format? Could you include a sample of what you had in mind?

If we need to do nesting in config, I agree JSON would be preferable. But IMO the simplest way to do config is just key/value pairs, and if config is just key/value pairs then JSON format is overkill.

@rmannibucau
Copy link

Class > field > annotations, a bit like bval does in xml.

Key pairs is fine while it is set through a single property on jsonbconfig and not read by key in another backend

@aguibert
Copy link
Contributor

aguibert commented Jul 29, 2019

Consider the following data model class:

    public static class Person {
        public String name;
        public LocalDate birthDate;
        public LocalDate deathDate;
    }

We could apply @JsonbDateFormat(JsonbDateFormat.TIME_IN_MILLIS) at the class level, or the property level.

In JSON, config might look like this:

{
  "com.example.Person" : {
    "@JsonbDateFormat" : "##time-in-millis",
    "properties" : {
      "deathDate" : { 
        "@JsonbDateFormat" : "##default"
      }
    }
  }

But we could more concisely represent this information as key/value pairs like this:

com.example.Person/JsonbDateFormat="##time-in-millis"
com.example.Person.deathDate/JsonbDateFormat="##time-in-millis"

@rmannibucau
Copy link

rmannibucau commented Jul 29, 2019

Outch, why not just keeping it as it is in java? One object per annotation, one object per class etc...

Also your concisely option breaks readability but also the one value point (keep in mind mp config will host multiple jsonb configs as today so you must use at least a prefix. Now i clearly prefer the first option which is mpconfig friendly and jsonbconfig friendly without any coupling vs second option which is coupled.

Ps: dont forget jakata will likely be closer of jakata.config than microprofile since it should superseed it at the end so withMappingConfig(model) is good. Then json or properties is not very important, just a consistency point with the spec itself but properties are part of java.se so no big deal if using the common dot notation and not a specific slash one ;).

@aguibert
Copy link
Contributor

I was chatting with my co-worker, @njr-11, on this and he raised a good argument that there shouldn't be any need to have externalized JSON-B config that can be overridden at deployment time. Really, all of the JSON-B configuration should be taken care of by the application developer who can either:
A) apply JSON-B annotations directly to the class/fields/methods (does not work for the 3rd party model classes)
B) apply JSON-B annotations at runtime programatically (must be used for 3rd party model classes)

@rmannibucau
Copy link

How does B work for not managed classes? Frameworks use jsonb to store data in a database or so but the app server is not even aware of it. It would also be good to work in SE (A) otherwise adoption will not be there IMHO.

That said I am also not super big fan of the config solution, it is not code driven. What about the model alternative, ie you can bind a class as being the model for another one, only the shape is taken into account:

 jsonbconfig.withModel(MyClass.class, MyModel.class)
//...
jsonb.toJson(out, myClassInstance) // uses MyModel metadata

This is close to jackson view for example. It does not require to invent a new config model.

@aguibert
Copy link
Contributor

Yea I think you are on the right track and we are in agreement that a code-driven solution is the best option. A config-based solution would be very error-prone and confusing for users to learn about and therefore hard to adopt.

The code snippet you proposed looks like it's on the right track, although I think the right Jackson feature to compare this to would be "Mix-ins" (see example here: https://medium.com/@shankar.ganesh.1234/jackson-mixin-a-simple-guide-to-a-powerful-feature-d984341dc9e2)

@rmannibucau
Copy link

Yep, mixed it with another framework calling views projections :s. Good catch.

@hei1233212000
Copy link
Author

+1 for the Mixin configuration

@aguibert
Copy link
Contributor

@m0mus can you elaborate on what you had in mind for this?

JsonbConfig should support reading configuration from external sources using MicroProfile configuration or Config JSR which is in development now.

@m0mus
Copy link
Contributor

m0mus commented Jul 31, 2019

Why should we introduce our own configuration solution not compatible with everything else? We should support MicroProfile Config or Jakarta Config. Both projects support different configuration sources, formats, layering and other cool features. But the most important is that it will be a part of the standard.

Something like this:

// Default configuration applies configuration from META-INF/microprofile-config.properties
Jsonb jsonb = JsonbBuilder.create();

// Does the same as above
JsonbConfig config = new JsonbConfig().read();
Jsonb jsonb = JsonbBuilder.create(config);

// Reads configuration from specific MP config instance
Config config = ConfigProvider.getConfig(); // MP Config
JsonbConfig config = new JsonbConfig().read(config);
Jsonb jsonb = JsonbBuilder.create(config);

Sample of META-INF/microprofile-config.properties

withFormatting=true
withPropertyOrderStrategy=any

@rmannibucau
Copy link

@m0mus what does it bring compared to jsonbconfig.withFormatting(config.getValue(...)).withXxx(...) etc? Nothing exception a forced (and not desired in several cases) dependency.
Also note a prefix is needed to support multiplz instances. So a withConfig(BiFunction<EnumKey, Class, Object> configReader, String prefix) is saner if you think that kind of feature bring something and will support spring env, javax.config, owner, and friends.
Last note being: mp is built on top of jsonb so they are the one owning such builder utility, not the opposite. Mp server can autoconfigure a default jsonb instance in cdi container with your proposal (using properties keys?) OOTB without nay jsonb stack breaking change.

@m0mus
Copy link
Contributor

m0mus commented Aug 1, 2019

@rmannibucau The approach with reading individual properties from external configuration and set them to JsonbConfig will still work. Having all configuration externalized brings many advantages:

  • You don't need to set all configuration properties manually when you create Jsonb instances. It's often needed to create Jsonb in multiple places with the same configuration. I know that there are many ways of achieving it. Externalized configuration is one of the easiest and usable solutions.
  • Configuration for different environments such as dev, test, prod. You may not want to use formatted output for prod, but it's useful for dev.
  • The standard way to configure your application. We are trying to unify Jakarta EE components configuration. It was in Jakarta EE goals (cannot find a link now). Something like embrace CDI and Config.

I don't know anything about MP Config implementation details. You could be right and it uses JSONB itself. In this case implementations will need to think how to solve it.

To be fair, I think that using Jakarta Config would be better because it's Jakarta. But I still don't know what's the progress and when Config JSR will be moved to Jakarta. And it may have the same JSONB dependency problem too.

@rmannibucau
Copy link

@m0mus

You don't need to set all configuration properties manually when you create Jsonb instances.

This is a big drawback for the apps I'm used to work with since it will just break the Jsonb instances. They don't share the same config and have several customizations. Even formatting=true would break a good part of apps (cause json is used by streams and a one line record is a requirement).

Configuration for different environments such as dev, test, prod.

Ack but it is already achieved trivially so not really a pro of your solution IMHO.

The standard way to configure your application.

This belongs to Microprofile (or Jakata and both standard will not use the same prefix, MP uses "mp.", jakata will likely keep "jsonb.") and they will clearly define the instances under their scope (JAX-RS @Provider, default app instance but not other instances or it would use the producer as prefix, like com.company.MyJsonbProducer.createJsonb) to ensure configuration is functional and fined grained enough to match all cases.

You could be right and it uses JSONB itself. In this case implementations will need to think how to solve it.

This is not MP-Config by itself but MP as a platform (when JSON-B is put with MP-Config typically). Individual specs must stay integrable but shouldn't be too tied otherwise the technology is already unusable in a lot of case and adoption is null from experience.

Agree on your jakarta config point but this also justifies JSON-B can't be bound to MP IMHO.

@aguibert
Copy link
Contributor

aguibert commented Aug 2, 2019

The discussion about integrating with some sort of MP/Jakarta Config spec is good, but I want to point out that it should be considered separately from this issue. I've raised #172 to track this

The use case the OP was after is applying customizations to classes they cannot control, particularly things like adding @JsonbTransient to a field/method on a 3rd-party class.

By contrast, the MP/JEE Config integration could be useful to externalize default configurations for a JsonbConfig object that can be expressed as key/value pairs, such as jsonb.formatting=true to indicate withFormatting(true) should be applied to all instances of JsonbConfig that do not explicitly state withFormatting(true|false).

MP/JEE Config should NOT be used to try and accomplish what the OP originally asked for. Such a solution would get very messy as shown in this comment: #88 (comment)

@aguibert
Copy link
Contributor

aguibert commented Sep 2, 2019

Wanted to post an update on this issue, as I think it is one of the most important issues to address in JSON-B vNext.

Proposed solution example

Consider that a developer includes a class Dog from another dependency and they do not have control over the Dog class, but they want to expose it as JSON data. Suppose the class looks like this:

public class Dog {
  public String name;
  public int age;
  public Person owner;
}

Now suppose I want to customize this class without altering the source. For example, suppose I want to:

  • Rename the name property to be dogName
  • Ignore the owner property

I think the simplest way to achieve this would be with the Jackson Mixin approach, which uses abstract classes that can be "merged" with the class being customized. For example:

public class DogCustomization {
  @JsonbProperty("dogName")
  public String name;
  @JsonbTransient
  public Person owner;
  // The "age" property is not mentioned here, so it is left as-is
}

Then, the user could configure the customization for all instances of the Dog type like this:

Jsonb jsonb = JsonbBuilder.create(new JsonbConfig()
                .withCustomization(Dog.class, DogCustomization.class));

Or, they could configure the customization at a per-property level like this:

public class Foo {
  @JsonbCustomization(DogCustomization.class)
  public Dog dog;
}

(Providing both approaches is similar to how Adapters, Serializers, and Deserializers can be configured globally or at a per-property level)

Suggested new API

Add method to the JsonbConfig class:

/**
 * Register a customization for a class. A JSON-B customization class provides a way to 
 * apply JSON-B configurations, such as @JsonbTransient or @JsonbProperty, without 
 * modifying the source code of the original class. This is typically used when the original
 * class cannot be modified, because it is included as a binary dependency.
 * @param originalClass The class to apply the customization to
 * @param customizationClass The abstract class containing the JSON-B customizations that
 * will be applied to the originalClass
 * @throws IllegalArgumentException If either of the supplied classes are null, or of customizationClass is not an abstract class
 * @see @JsonbCustomization
 */
public JsonbConfig withCustomization(Class<?> originalClass, Class<?> customizationClass);

New annotation: @JsonbCustomization

/**
 * <same description as JsonbConfig#withCustomization>
 */
@JsonbAnnotation
@Retention(RetentionPolicy.RUNTIME)
@Target({METHOD, FIELD})
public @interface JsonbCustomization {
  /**
    * The abstract class containing the JSON-B customizations that
    * will be applied to the type of the annotated field, or the return type
    * of the annotated method.
    */
  public Class<?> value();
}

@rmannibucau
Copy link

Hmm, few thoughts:

  1. Isn't it an adapter and something we already have in terms of API? If you think impl today, you can already do that.
  2. Transient support should likely be thought cause we have it with visibility API already.
  3. If you register 2 customizations for Dog then both are applied? Sounds like it should for composition reasons but think it can't handle properly serialization too so can be forbidden and need to fail?
  4. Often serialization is handled by an external serializer for external classes, guess we want to point out that using JsonValue is ok for these ones and that "customization" is for classes serialized by jsonb itself completly, maybe in the spec and not the javadoc?

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

  1. Isn't it an adapter and something we already have in terms of API? If you think impl today, you can already do that.

Valid question. While you can achieve this with an adapter, a lot of times writing a full adapter just to rename/ignore a handful of fields would be overkill. Consider the Dog example I used above. With an Adapter, it would look like this:

    public static class DogAdapter implements JsonbAdapter<Dog, Map<String, Object>> {
        @Override
        public Map<String, Object> adaptToJson(Dog obj) throws Exception {
            Map<String, Object> dogMap = new HashMap<>();
            dogMap.put("dogName", obj.name); // renamed property here
            dogMap.put("age", obj.age); // unchanged
            // ignore owner field
            return dogMap;
        }

        @Override
        public Dog adaptFromJson(Map<String, Object> obj) throws Exception {
            Dog dog = new Dog();
            dog.name = (String) obj.get("dogName"); // property name change
            dog.age = (int) obj.get("age"); // default mapping here
            // ignore owner field
            return dog;
        }
    }

This is quite a lot of code for a relatively small customization on a small class. In this example we are customizing 2 of 3 properties, but it would become even more tedious and error prone if the class had a large number of properties relative to the number of properties that wanted to be customized. For example, the class has 15 properties but I only want to customize 1 of them.

  1. Transient support should likely be thought cause we have it with visibility API already.

Again, it is possible to ignore third-party properties using a custom Visibility strategy, but it's clunky.

  1. If you register 2 customizations for Dog then both are applied? Sounds like it should for composition reasons but think it can't handle properly serialization too so can be forbidden and need to fail?

Yes, I think 2 customizations for the same class should be an error. What do we do today if you try to register 2 custom JsonbSerializer<Dog> or JsonbAdapter<Dog,Map> instances? I think we should just match that behavior.

  1. Often serialization is handled by an external serializer for external classes, guess we want to point out that using JsonValue is ok for these ones and that "customization" is for classes serialized by jsonb itself completly, maybe in the spec and not the javadoc?

Yes, ideally third party libraries would supply their own serializers. But often times there are odd cases where the third party lib would have no business creating a dependency on JSON-B API. For example in Yasson we ran into a case where generated Groovy classes needed to be ignored, and also Weld CDI proxy objects needed to be ignored.

@rmannibucau
Copy link

rmannibucau commented Sep 3, 2019

2-4. agree

  1. yes and not, if you create a dedicated Jsonb instance for the adapter you can map the Dog to a JsonObject and remove what you want/rename what you want etc, potentially using jsonpatch. So at the end it is a simple solution for patching which is already there, no?

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

I still think that the customization approach has value because it is flexible for any JSON-B annotation and gives the user what they want for this use case: the ability to annotate Classes they can't modify.

In my original example I used renaming and ignoring properties, but lets lets consider a slightly different example where we want to apply a custom adapter to a property like this:

public class DogCustomization {
  public String name;
  @JsonbAdapter(MyPersonAdapter.class)
  public Person owner;
}

I don't think you could easily represent this @JsonbAdapter(MyPersonAdapter.class) inside a DogAdapter, which is where the customization approach comes in handy.

@rmannibucau
Copy link

Well with an adapter you never hit this case or (previous Jsonb delegation) you can register it on the nested jsonb instance globally so no big deal.

So overall it can be simpler but we must not compete with our existing API IMHO , this is why a custom annotation reader was a good option for me.

@m0mus
Copy link
Contributor

m0mus commented Sep 3, 2019

I think that adapters and mix-ins can co-exist.

@rmannibucau
Copy link

Sure they can but will next feature reflection abstraction (we will not delay it very long after mixins since it is what it is built upon and allows to industrialize more user code) so maybe we can skip it? That's the main question, goal being to stay simple and not duplicate solutions for the same issue.

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

you can register it on the nested jsonb instance globally so no big deal.

Yes, but sometimes users don't want to register an adapter/serializer/deserializer globally, or they can't because they get a Jsonb instance from the container or something. I think the fact that we have a @JsonbAdapter annotation in the first place in addition to the global config option proves this use case.

Sure they can but will next feature reflection abstraction (we will not delay it very long after mixins since it is what it is built upon and allows to industrialize more user code) so maybe we can skip it?

Not sure what this "reflection abstraction" feature is. Do we have an issue open for it? Can you link to it?

That's the main question, goal being to stay simple and not duplicate solutions for the same issue.

In general I agree, but when one solution provides significant simplicity over an alternative, then it's OK. When I compare the usability of the proposed Customization approach to the existing Adapter approach, the Adapter approach seems like a hack/workaround.

@rmannibucau
Copy link

  1. We spoke of the reflection alternative in another issue, long story short it is a SPI you register on jsonbconfig and for each visited class it replaces getAnnotation on all types, default being pure reflection and "literals" - default impl of annotations - being added to @jsonb annotations. You can review cdi annotated type for an advanced version (we just need getAnnotation callback with a context, not the whole model)
  2. In this case you can register globally any adapter/serializer since ypur are not global but local with your jsonb instance and you dont have to reuse container one (note we failed to spec it means anything anyway even if im for a default instance ;))
  3. Got recently several negative feedbacks of mixins since you totally break strong typing so on the long run it is worse than alternatives for a mapper which is type based.

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

The reflection/visitor pattern you mention seems like it would be even more code overhead than the Adapter approach. For simple customizations like this I think the visitor pattern is overkill. This is why I prefer libraries like bytebuddy over ASM.

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

For the "breaks strong typing" negative feedback, this is a valid concern but:

  1. I don't think any other solutions offer strong typing. The visitor/reflection approach would still be looking for hard-coded method/field names, similar to how a custom Visibility Strategy would work for hiding certain fields/methods. If my understanding is incorrect, please show a code example so I can understand better.
  2. We would need to emphasize that customizations should only be used when the class being customized cannot be modified. Obviously using direct JSON-B annotations on the class should be preferred and is type-safe.

@rmannibucau
Copy link

Try it, it is the same as visibility pattern and it is trivial to impl most of the time. At least way simpler than doing dozens of customization for an object graph. We dont need a recursive visitor pattern at all.

Let say you want to ignore all "children" or "model" (loaded instance, not a config in some lib) attributes of external classes, you write as much mixins than models duplicating all attributes, for a lib iusing this pattern it is really overkill and long whereas an ModelAnnotationReader is 5 lines including class and method declarations and solves all cases smoothly + is naturally config friendly and dynamic friendly.

@aguibert
Copy link
Contributor

aguibert commented Sep 3, 2019

Try it, it is the same as visibility pattern and it is trivial to impl most of the time. At least way simpler than doing dozens of customization for an object graph. We dont need a recursive visitor pattern at all.

Just to make sure we are on the same page, I would appreciate it if you posted a code example of what you have in mind so we can be sure we're discussing the same thing.

Let say you want to ignore all "children" or "model" (loaded instance, not a config in some lib) attributes of external classes, you write as much mixins than models duplicating all attributes, for a lib iusing this pattern it is really overkill and long

Yea I see what you mean here. If you have a class that has 20 properties on it and you want to ignore 19 of them, you'd have to model all 19 properties just for the sake of putting @JsonbTransient on all of them. However, I think this is a minor case because if a user wants to discard/override the majority of a model, they would probably just write their own model class from scratch that wraps the real object.

For example, suppose I have:

public class ThirdPartyClass {
  public String importantProp;
  public String bogus1, bogus2, /* .... */ bogus19;
}

If I want to only keep importantProp and ignore everything else, I would just write a new model like this:

public class MyModel {
  public String importantProp;
  public MyModel(ThirdPartyClass obj) {
    this.importantProp = obj.importantProp;
  }
}

whereas an ModelAnnotationReader is 5 lines including class and method declarations and solves all cases smoothly + is naturally config friendly and dynamic friendly.

could you post a code example of what this might look like? Or provide a link to where this was previously discussed?

@rmannibucau
Copy link

@aguibert sure, something along https://github.com/apache/geronimo-openapi/blob/master/geronimo-openapi-impl/src/main/java/org/apache/geronimo/microprofile/openapi/impl/processor/AnnotatedTypeElement.java, https://github.com/apache/geronimo-openapi/blob/master/geronimo-openapi-impl/src/main/java/org/apache/geronimo/microprofile/openapi/impl/processor/AnnotatedMethodElement.java + AnnotatedFieldElement returned by a AnnotatedModelReader { AnnotatedTypeElement read( ClassContext); AnnotatedMethodElement read(MethodContext); AnnotatedFieldElement read(FieldContext); } with the context being the underlying reflect instance and maybe some other metadata (probably the default impl to be able to reuse it as in SerializationContext). Note that methods can have defaults returning null or so to fallback on the default (current) impl.

Your example is not exactly what I meant since it is exactly the good case for mixins but actually never happens (to ignore all but one property). My case was 20 classes, each of them having 1 property "X" you want to ignore. Here you need 20 mixins with potentially 20 properties redefinitions which is a lot just to ignore an internal model property. With the AnnotatedModelReader it is a blacklist impl which is very simple (you override read(FieldContext) and based on the field name return an annotated impl which adds JsonbTransient using JsonbTransient.Literal.INSTANCE or so (see https://javaee.github.io/javaee-spec/javadocs/javax/enterprise/inject/Default.Literal.html for that pattern).

Does it makes more sense this way?

@m0mus
Copy link
Contributor

m0mus commented Sep 4, 2019

@rmannibucau Reflection alternative approach you are suggesting is very powerful, but complicated. I prefer more simple user-friendly solutions. Before a user will get a full understanding how it works he will change his mind and use an alternative framework with more friendly approach to customizations. The amount of code has to be written is another drawback. I would prefer a solution requiring

  • Minimum amount of additional code.
  • This code preferably be in the class where you configure your Jsonb instance. I mean that configuration is not spread to many classes which makes it difficult to find.
  • Ability to externalize configuration.
  • Leverage existing JsonbConfig model.

I was thinking about adding a set of builders to JsonbConfig, something like:

JsonbConfig config = new JsonbConfig()
    .addClassCustomization(
        ClassCustomization.builder(MyClass.class)
            .property("originalName", "newName")  // like @JsonbProperty
            .transient("propertyName")            // like @JsonbTransient
            .creator(...)                         // custom instantiation 
            .otherCustomization()
            .build();
    )

or customization can be created somewhere else and shared between Jsonb instances this way:

JsonbConfig config = new JsonbConfig()
    .addClassCustomization(myClassCustomizationInstance);

Thoughts?

@rmannibucau
Copy link

Hi @m0mus,

looks close to my proposal - while you enable a kind of .annotation(new MyCustomAnnotation()) builder method.

I'd also add a readFrom method to be able to customize a mapping instead of creating it so:

ClassCustomization.builder(MyClass.class) // does not read jsonb annotations from the class

whereas

ClassCustomization.from(MyClass.class) // loads the model and builder can override it

or more likely to make it more friendly in the IDE and fluent:

ClassCustomization.builder(MyClass.class)
    .read()

This last option requires remove methods too:

 ClassCustomization.from(MyClass.class)
        .noTransient("ignoredByDefaultProperty")

Finally in term of API we must ensure we reuse JsonbProvider#newClassCustomization(Class)

Side question: a detail but i'm not a fan of "customization", can we move to "model" semantic? ClassModel or ClassMapping maybe?

@aguibert
Copy link
Contributor

aguibert commented Sep 4, 2019

I like the code example that @m0mus proposed, I think something along these lines would be a better way to solve the problem than what I originally suggested with the abstract class mixin approach.

@rmannibucau I also like your idea of having a way to ignore source annotations. However, I think that two separate entry points (ClassCustomization.builder(Class) and ClassCustomization.from(Class)) would be a bit confusing. Instead of two separate entry points, lets just have an additional method on the builder like this:

// This example adds to/overrides source JSON-B annotations
ClassCustomization.builder(MyClass.class)
  // ...
  .build();

// This example would discard any source annotations for the entire class
ClassCustomization.builder(MyClass.class)
  .ignoreSourceAnnotations()
  // ...
  .build();

// This example would discard any source annotations for 
// properties "foo" and "bar"
ClassCustomization.builder(MyClass.class)
  .ignoreSourceAnnotations("foo", "bar")
  // ...
  .build();

@rmannibucau
Copy link

Makes sense for me - except naming point but my next question includes it.

CDI has BeanConfigurator for that topic, do we want to align:

  1. The naming
  2. The API style? Means using .addXxx, .xxx methods and making the builder hosted on jsonbConfig - no other entry point and implicit reuse of provider.

Wdyt?

@m0mus
Copy link
Contributor

m0mus commented Sep 4, 2019

@rmannibucau I have no issues with having a method which reads customizations from a class. I agree with what @aguibert is proposing. It makes sense.

@geoand
Copy link

geoand commented Dec 2, 2019

Hi folks,

I arrived at this issue after asking @aguibert about whether or not Mixins exist for JSON-B due to some folks complaining in Quarkus about various deserialization issues (which are not the fault of JSON-B).

I initially had Jackson Mixins in mind, but I really like the proposal from @m0mus, it seems really elegant and as far as I can tell covers the use cases I had in mind (although I would like to see what a creator would look like).

@aguibert
Copy link
Contributor

aguibert commented Dec 3, 2019

thanks for bringing up that case @geoand, I think we could model it in the same way the reflection API models obtaining a constructor with the java.lang.Class.getConstructor(Class<?>... parameterTypes) method. Suppose we have a class with multiple constructors like this:

public class MyClass {
  public MyClass() {}
  public MyClass(String s) {}

  // Suppose we want to programmatically make this one the @JsonbCreator
  public MyClass(String s, Foo f, int i)
}

We could indicate which constructor we want to be the @JsonbCreator like this:

        ClassCustomization.builder(MyClass.class)
            .creator(String.class, Fool.class, int.class) // like @JsonbCreator
            .build();

Also, we need to cover the case where a static creator method is used, such as:

public class MyClass {
  // want to put @JsonbCreator here
  public static MyClass buildIt(String s) {
    // custom initialization
  }
}

This case could be represented with an overloaded method such as:

        ClassCustomization.builder(MyClass.class)
            .creator("buildIt", String.class) // like @JsonbCreator
            .build();

In summary, we could have 2 methods on ClassCustomizationBuilder:

  • creator(Class... constructorParameterTypes)
  • creator(String methodName, Class... parameterTypes)

Also, we could potentially add a 3rd method that would allow direct instantiation of a type using a Lambda:

  • creator(Supplier<T> creatorLambda)
    But this would be restricted to no-arg producers. If we wanted to have a lambda that accepts arguments, we would have to create a new data structure that would contain all of the incoming JSON data, and a lambda implementation could use whichever bits it wants to instantiate the object.
  • creator(Function<JsonbData,T> creatorLambda)

@geoand
Copy link

geoand commented Dec 3, 2019

@aguibert I like your proposal very much!

@aguibert
Copy link
Contributor

aguibert commented Dec 3, 2019

Thanks, any thoughts on the last 2 options involving the lambdas? I think creator(Supplier<T> creatorLambda) would be pretty simple, but inevitably users would ask for more capability like what's described in creator(Function<JsonbData,T> creatorLambda) and I worry about how much API surface this extra method creates

@geoand
Copy link

geoand commented Dec 3, 2019

Honestly, I would probably just start without either of those for exactly the reason you mention - I wouldn't want to expose to much of the API without actually knowing that people need such a powerful solution for creating instances.

@t1
Copy link

t1 commented Jun 3, 2020

There's a discussion about adding mixins (and more) as a general concept that can be used by other specs in the MP mailing list. Maybe we can join forces :-)

@Verdent
Copy link
Contributor

Verdent commented Jul 22, 2021

Hi,
I think this is one of the most important changes to the 2.1, so I took a look into how I think it should be done. I went through all the suggestions which were mentioned here and tried to make the feature based on those.
I would like to ask you, it you could take a look at my #279 and give me your opinion about it.

I have added a new method public final JsonbConfig withTypeDecorators(final TypeDecorator... typeDecorators) to the JsonbConfig. If you want to add some runtime type configuration, this is the entry point.

An actual customization is based on builder pattern, and these builders are building set of different classes called "Decorators" since that's what we are doing, we are decorating types, fields, parameters etc. These decoraters contain all of the information passed to the builder.

First example

Let's assume we have following class

public class Pojo {

    public String myProperty;

    public String getMyProperty() {
        return myProperty;
    }

    public void setMyProperty(String myProperty) {
        this.myProperty = myProperty;
    }

}

We cannot modify it, but we want to set custom name to "myCustomName" as we could do it easily with annotation @JsonProperty("myCustomName). We can do that easily like this

TypeDecorator.builder(Pojo.class)
        .property(PropertyDecorator.builder("myProperty")
                          .name("myCustomName")
                          .build())
        .build();

Or you can also use shortcut method to do that

TypeDecorator.builder(Pojo.class)
        .property("myProperty", builder -> builder.name("myCustomName"))
        .build();

This way you can apply every customization onto the component you are decorating with the same possibilities as with annotations.

Second example

Extend our Pojo class over the creator method

public static Pojo create(String param1, Integer param2) {
    return new Pojo(param1, param2);
}

If we could apply annotations, creating creator would look similarly to this

@JsonbCreator
public static Pojo create(@JsonbProperty("jsonName1") String param1,
                          @JsonbProperty("jsonName2") Integer param2) {
    return new Pojo(param1, param2);
}

But since we cannot do that, we can do it over the TypeDecorator

TypeDecorator.builder(Pojo.class)
        .creator(CreatorDecorator.builder("create")
                        .addParameter(String.class, "jsonName1")
                        .addParameter(Integer.class, "jsonName2")
                        .build())
        .build();

Or again with shortcut method

TypeDecorator.builder(Pojo.class)
        .creator("create",
                 builder -> builder.addParameter(String.class, "jsonName1")
                          .addParameter(Integer.class, "jsonName2"))
        .build();

It is also possible to set different customizations to the parameters since there is also separate builder to do so.

Conclusion

I think the whole feature designed like this is very user friendly and easy to use, since IDE will lead user with method completion, javadoc etc.

Please, let me know what you think about it and if there is anything to change, what would that be and what would you propose.

@Verdent Verdent modified the milestones: 2.1, 2.x Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants