jsonb-api icon indicating copy to clipboard operation
jsonb-api copied to clipboard

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

Open hei1233212000 opened this issue 7 years ago • 46 comments
trafficstars

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.

hei1233212000 avatar Aug 21 '18 13:08 hei1233212000

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.

aguibert avatar Mar 19 '19 21:03 aguibert

+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.

rmannibucau avatar Mar 19 '19 21:03 rmannibucau

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.

m0mus avatar Jul 23 '19 12:07 m0mus

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).

rmannibucau avatar Jul 29 '19 15:07 rmannibucau

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.

aguibert avatar Jul 29 '19 16:07 aguibert

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

rmannibucau avatar Jul 29 '19 16:07 rmannibucau

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"

aguibert avatar Jul 29 '19 19:07 aguibert

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 ;).

rmannibucau avatar Jul 29 '19 19:07 rmannibucau

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)

aguibert avatar Jul 30 '19 01:07 aguibert

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.

rmannibucau avatar Jul 30 '19 04:07 rmannibucau

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)

aguibert avatar Jul 30 '19 17:07 aguibert

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

rmannibucau avatar Jul 30 '19 18:07 rmannibucau

+1 for the Mixin configuration

hei1233212000 avatar Jul 30 '19 23:07 hei1233212000

@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.

aguibert avatar Jul 31 '19 02:07 aguibert

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

m0mus avatar Jul 31 '19 19:07 m0mus

@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.

rmannibucau avatar Jul 31 '19 21:07 rmannibucau

@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.

m0mus avatar Aug 01 '19 07:08 m0mus

@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.

rmannibucau avatar Aug 01 '19 07:08 rmannibucau

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: https://github.com/eclipse-ee4j/jsonb-api/issues/88#issuecomment-516130403

aguibert avatar Aug 02 '19 03:08 aguibert

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();
}

aguibert avatar Sep 02 '19 23:09 aguibert

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?

rmannibucau avatar Sep 03 '19 05:09 rmannibucau

  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.

aguibert avatar Sep 03 '19 14:09 aguibert

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?

rmannibucau avatar Sep 03 '19 14:09 rmannibucau

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.

aguibert avatar Sep 03 '19 14:09 aguibert

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.

rmannibucau avatar Sep 03 '19 15:09 rmannibucau

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

m0mus avatar Sep 03 '19 15:09 m0mus

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.

rmannibucau avatar Sep 03 '19 15:09 rmannibucau

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.

aguibert avatar Sep 03 '19 17:09 aguibert

  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.

rmannibucau avatar Sep 03 '19 17:09 rmannibucau

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 avatar Sep 03 '19 19:09 aguibert