3rd party class customization

[Verdent]

Signed-off-by: David Kral david.k.kral@oracle.com

[rmannibucau]

SPI is not worse than before so ok on the loading. On the surface points I think a single one is better. On a design on the overall PR I confirm I think we should limit it to the introspector api and not do it a seconf time there, a third later on the introspector (required anyway to make the spec integrable by 3rd parties like mp). So think we should postpone such a direction until a minimal introspector is there and review if relevant or not (which should be IMHO).

[m0mus]

IMO, Descriptor is a better name for Decorator classes.

[Verdent]

Sure, I have no problem with changing the name to Descriptor :-)

[m0mus]

@rmannibucau If you strongly disagree with this approach I would recommend you submitting your vision as a PR so we can compare them and vote for the best approach.

[rmannibucau]

@m0mus I'm a bit out of time this month but long story short as discussed on slack:

1. you do not address https://github.com/eclipse-ee4j/jsonb-api/issues/189 with such a proposal whereas addressing it will make this feature trivial to do
2. I dislike - and it is the same kind of feedback I got when asked for a review on that proposal - the fact we fully duplicate the model we manipulate - will make it impossible for end user to find back their stuff whereas technically there is no justification at all since all can be done with annotation (including reflection less runtime)
3. we put an API before having a backend
4. it is not 3rd party libraries friendly OOTB (wither/dsl are not and require a lot of glue code for nothing IMHO, fluent setters are a bit better but do not solve previous points)

So i'd start by solving 1, put an API enabling to provide an introspector manipulating the annotations instances we already have and voilà, we are 3rd party friendly and solved 2 others issues without making the API way more complex nor duplicating our own model completely :). We can revisit a more fluent API later - as CDI 1->2 did - if needed but not sure it is and hopefully javase will help us with some common GenericLiteral impl in the jre itself.

[m0mus]

1. you do not address [Introspection API #189](https://github.com/eclipse-ee4j/jsonb-api/issues/189) with such a proposal whereas addressing it will make this feature trivial to do

You are right. We have an API for adding customizations at runtime, but we don't have an API to read them. We'll address it. Yes, we don't. We'll address it

2. I dislike - and it is the same kind of feedback I got when asked for a review on that proposal - the fact we fully duplicate the model we manipulate - will make it impossible for end user to find back their stuff whereas technically there is no justification at all since all can be done with annotation (including reflection less runtime)

I don't agree here. The goal is to have two ways of adding customizations: at compile time using annotations (it's already there) and at runtime (this is what this PR is about). I don't agree that the model (I think you are talking about the customizations model) is duplicated. The model stays the same the way it's been defined in the earlier versions of the spec. We just adding a runtime API to access it. If you are talking about well-defined API vs generic API, I am not a big fan of too generic APIs. They are too wide and allow doing things going out of scope.

3. we put an API before having a backend

We need to prove implementability of API provideing at least one compatible implementation. Yasson folks are working on it. API work is in progress and we can change it as many times as we want before the release.

4. it is not 3rd party libraries friendly OOTB (wither/dsl are not and require a lot of glue code for nothing IMHO, fluent setters are a bit better but do not solve previous points)

I am not sure I understand what you mean here. I think that this API is quite user friendly and can be used with 3-rd party objects without any problems. If you don't think so, please provide a use case.

[rmannibucau]

I don't agree here. The goal is to have two ways of adding customizations: at compile time using annotations (it's already there) and at runtime (this is what this PR is about). I don't agree that the model (I think you are talking about the customizations model) is duplicated. The model stays the same the way it's been defined in the earlier versions of the spec. We just adding a runtime API to access it. If you are talking about well-defined API vs generic API, I am not a big fan of too generic APIs. They are too wide and allow doing things going out of scope.

It is not a generic API since it is a very JSON-B specific (@Jsonb<something>), it is about two API to do the exact same thing. Customization API is 100% a way to bind markers on a model, we already have all markers so no need to duplicate them all and it is exactly what this PR does in current flavor. For any integrator it requires to handle 2 models instead of one which is useless technically and makes the model understanding harder until there is an unified view which will be the introspector which should manipulate a single model which means the annotation - at least in current form of the PR and hopefully not a 3rd model in all cases.

We need to prove implementability of API provideing at least one compatible implementation. Yasson folks are working on it. API work is in progress and we can change it as many times as we want before the release.

To be honest Johnzon supports that since a lot of time - before JSON-B - using a kind of custom introspector injection in its model parsing and it is likely what enables the most flexibility for integrators (originally it was used a lot to bind jackson model on johnzon-mapper one). With current customization form it is a lot of work - you redefine the whole model - whereas you just want to go through the model and map what is needed visiting it in 100% of case for this use case. So here again I think we miss the right level to define the API.

I am not sure I understand what you mean here. I think that this API is quite user friendly and can be used with 3-rd party objects without any problems. If you don't think so, please provide a use case.

As explained on slack and several other channel this API covers the end user application external customization case which is probably 5% of the use case of such an API. Integrators are likely the most common case and even for end applications you often just want to "map" custom annotations (stereotypes like is one use case for example). So long story short, as friendly this API can be, it requires a lot of glue code to be used and does not tackle the real challenge of customizations, this is why I think we should go back to basic and maybe move this PR as a yasson specific thing until we have the instrospector properly setup without making the spec boilerplated with a lot of interfaces and API for no real end user gain (it is really a pivot moment because JSON-B is quite acceptable in current form but I hope it does not become another JPA which is way too fat to stay mainstream and a default choice to be very concrete).

[m0mus]

@rmannibucau

For any integrator it requires to handle 2 models instead of one which is useless technically and makes the model understanding harder until there is an unified view which will be the introspector which should manipulate a single model which means the annotation - at least in current form of the PR and hopefully not a 3rd model in all cases.

Who is integrator? Is it a person/team who works on a compatible implementation? Or it's a user integrating some 3-rd party code into his application?

[rmannibucau]

@m0mus integrator somebody doing the integration between 2 layers (3rd party, custom application schema/design/contract API etc...). It is never a JSON-B provider in what I mentionned. Maybe in simpler terms: a JSON-B API consumer. Hope it clarifies it.

[m0mus]

integrator somebody doing the integration between 2 layers (3rd party, custom application schema/design/contract API etc...). It is never a JSON-B provider in what I mentionned. Maybe in simpler terms: a JSON-B API consumer. Hope it clarifies it.

Sure, thanks for your explanation. As a user, I would like to have a clear and nice API allowing me customizing my mappings at runtime. It's because you may not have access to 3-rd party sources to apply annotations. It's a well known limitation of JSON-B API and it would be nice to add it to the spec. Do you agree?

[rmannibucau]

@m0mus sure and as an user I want to be able to apply annotations on these 3rd party classes I can't modify the sources/bytecode. Adding a visitor in the introspector is the way most mappers do it and it is way more natural than redefining the model explicitly like that. It is also closer to a compile time friendly solution than a redeclaration of the model in another class. Here the decorator are just a fluent definition API which is verbose, error prone and misleading IMHO compared to a clean introspector. Once you can register an introspector in the properties the runtime and the user can retrieve the actual runtime introspector and just read what it needs there. Nothing ambiguous, more use cases covered and less duplication IMHO.

[m0mus]

sure and as an user I want to be able to apply annotations on these 3rd party classes I can't modify the sources/bytecode. Adding a visitor in the introspector is the way most mappers do it and it is way more natural than redefining the model explicitly like that. It is also closer to a compile time friendly solution than a redeclaration of the model in another class.

It's great that we agree that this API is needed. I see that we don't agree on the technical aspect. Let me provide my point of view here.

I don't think that visitor pattern is user friendly. It's too generic and doesn't fit into existing JSON-B API design. I would like to see intuitive and self-contained API with named methods so I can use IDE integration where I press ., see all these methods in the list and select one which resonates with what I want to do. I am sure that experienced developers may like visitors because of not limited abilities, but it's definitely not intuitive and too complicated for an average user. It's like using a sledgehammer to crack a nut. :)

Also, as I understand your definition of customization model is based on annotations. Another words, annotations define customizations model. It's not 100% correct. You can do some things at runtime only, there are no annotations for it. The example is I-JSON support and binary data strategy. I see customizations model as a separate entity and annotation and runtime API are different methods of reading/modifying it. Annotations are used at compile time only. They don't belong to runtime. I see the idea of using/simulating annotations at runtime to build a customizations model wrong.

[rmannibucau]

I don't think that visitor pattern is user friendly. It's too generic and doesn't fit into existing JSON-B API design.

Current proposal neither, it breaks the convention over configuration aspect of the spec and is very very verbose compared to the visitor impl.

I can use IDE integration where I press ., see all these methods in the list and select one which resonates with what I want to do

Fulfilled with a visitor as much than this proposal which does not accomplish it for the classes you want to configure anyway (this is one big error area and pitfall of such an API).

but it's definitely not intuitive and too complicated for an average user.

Once again there are two points there:

1. it solves multiple issues elegantly and consisely (whereas this issue does not)
2. this proposal creates noise as well on the configuration which is misleading for not average dev + it is the pattern used by all mappers out there so it shouldn't be that bad (check jackson to cite one used a little bit ;)).

It's like using a sledgehammer to crack a nut. :)

Let's take an example, alias the properties by using only their first letter (name -> n). Visitor will be like implementing a single method doing return name.substring(0, 1) whereas you will redefine more or less 50 classes with 10 properties each with this decorator API? Indeed you will use a programmatic automotion but then you lost all the user friendlyness you spoke about so at the end this proposal is not user friendly in practise (to say it differently "it looks like a good idea but not in practise"). Also note it does not prevent to have a more fluent DSL proposal module in the first times and get there later but first I want a strong and concise core this proposal does not target nor reaches.

You can do some things at runtime only, there are no annotations for it. The example is I-JSON support and binary data strategy.

Let's I-JSON aside since it is nothing as of today and it is a global thing anyway which belongs to JsonbConfig only but for binary data we must create an annotation since it is a local configuration at the end (this property) and shouldn't require a JsonbConfig global customization so at the end anything related to an attribute must be an annotation - and I agree we missed the binary one but it is a "bug" we ust fix IMHO.

I see the idea of using/simulating annotations at runtime to build a customizations model wrong.

It is not using annotations at runtime more than what annotations are. It is used to build the primary class model as metadata they are. Now if we have a nice and smooth visitor then the default implementation can just be to read annotations and maybe the API can be refined a bit to find something satisfying everyone but what I'm sure is current proposal does not solve most common customization needs as written.

[Verdent]

@rmannibucau I was thinking how to include your desired solution over the visitor and also to use this already created API, since I really want to avoid using literals. I think we came up with some pretty decent solution :-) Here is possible interface of the visitor pattern:

/**
 * Class model visitor adds lower level customization options for the specific type or its components.
 */
public interface ClassCustomizationVisitor {

    /**
     * Whether this model creator supports class obtained via parameter.
     *
     * @param type maybe supported type
     * @return whether type is supported by this creator
     */
    boolean supported(Class<?> type);

    /**
     * Set type specific customization.
     * <br>
     * Obtained typeCustomization parameter contains all the customization from
     * annotations applied to the class.
     * <br>
     * Any required customizations can be simply done by converting typeCustomization to the builder.
     * Builder obtained from the existing customization will contain all the previously set customizations.
     * <pre>{@code
     * //To obtain builder from the typeCustomization, can be used following piece of code
     * typeCustomization.toBuilder()
     *
     * //Or alternatively
     * TypeCustomization.builder(typeCustomization)
     * }</pre>
     * When everything required is set, build a new type customization.
     * <br>
     * It is required for the returned type customization to be bound to the same type as
     * the one obtained by the parameter. JSON-API implementation needs to verify this requirement
     * and throw {@link jakarta.json.bind.JsonbException} if it is not met.
     *
     * @param type processed type
     * @param typeCustomization processed type customization
     * @return possibly updated type customization
     */
    default TypeCustomization onType(Class<?> type, TypeCustomization typeCustomization) {
        return typeCustomization;
    }

    /**
     * Set property specific customization.
     * <br>
     * Obtained propertyCustomization parameter contains all the customization from
     * annotations applied on the field and its accessor methods.
     * <br>
     * Any required customizations can be done simply by converting propertyCustomization to the builder.
     * Builder obtained from the existing customization will contain all the previously set customizations.
     * <pre>{@code
     * //To obtain builder from the propertyCustomization, can be used following piece of code
     * propertyCustomization.toBuilder()
     *
     * //Or alternatively
     * PropertyCustomization.builder(propertyCustomization)
     * }</pre>
     * When everything required is set, build a new property customization.
     * <br>
     * It is required for the returned property customization to be bound to the same property name as
     * the one obtained by the parameter. JSON-API implementation needs to verify this requirement
     * and throw {@link jakarta.json.bind.JsonbException} if it is not met.
     *
     * @param typeProperty processed property
     * @param propertyCustomization processed property customization
     * @return possibly updated field customization
     */
    default PropertyCustomization onProperty(TypeProperty typeProperty, PropertyCustomization propertyCustomization) {
        return propertyCustomization;
    }

}

/**
 * Bean property information wrapping object.
 */
public interface TypeProperty {

    /**
     * Parent property type.
     * 
     * @return class of the parent type
     */
    Class<?> getParentType();

    /**
     * Class property field.
     * 
     * @return field instance, otherwise empty
     */
    Optional<Field> getPropertyField();

    /**
     * Field accessor getter method.
     * 
     * @return getter method, otherwise empty
     */
    Optional<Method> getGetterMethod();

    /**
     * Field accessor setter method.
     *
     * @return setter method, otherwise empty
     */
    Optional<Method> getSetterMethod();

}

This could be the solution for your enhanced usecases of the class/property customization.

Several examples

ModelCreator registration

new JsonbConfig().withConfigModelCreator(instance)

Changing the name for all of the class fields

new ClassCustomizationVisitor(){

        @Override
        public boolean supported(Class<?> type) {
            return Pojo.class.equals(type);
        }

        @Override
        public PropertyCustomization onProperty(TypeProperty typeProperty, PropertyCustomization propertyCustomization) {
            PropertyCustomizationBuilder builder = propertyCustomization.toBuilder();
            typeProperty.getPropertyField()
                    .ifPresent(field -> builder.name(field.getName().substring(1)));
            return builder.build();
        }

    };

Ignore nillable annotation set on the class

new ClassCustomizationVisitor(){

        @Override
        public boolean supported(Class<?> type) {
            return Pojo.class.equals(type);
        }

        @Override
        public TypeCustomization onType(Class<?> type, TypeCustomization typeCustomization) {
            return typeCustomization.toBuilder()
                    .ignoreNillable()
                    .build();
        }

    };

Create generic model creator to handle custom annotation on class

new ClassCustomizationVisitor(){

        @Override
        public boolean supported(Class<?> type) {
            return true;
        }

        @Override
        public TypeCustomization onType(Class<?> type, TypeCustomization typeCustomization) {
            if (type.getDeclaredAnnotation(MyCustomAnnotation.class) != null) {
                return typeCustomization.toBuilder()
                        .dateFormat("#.##")
                        .nillable(true)
                        .build();
            }
            return typeCustomization;
        }

    };

This ClassCustomizationVisitor would be there present as an alternative customization solution if one needs a bigger hammer. I am still not sure about the class name, but this is what I have so far. This is more or less the first draft of how it might look like. Does that sound better now? :-)

[rmannibucau]

@Verdent not in position to "snippet" right now - sorry about that - but if you move it to be consistent (API is called for classes but also for what the JSONB runtime reads instead of mixing called/pushed models) you will have a full visitor pattern without any literal which is what you try to avoid - I'm ok avoiding it while the overall solution solves the mentionned issues. The only issue is that you still miss the introspector/model provider point which is to enable to read a model in a simple reusable way. So at the end you must have a getAnnotation(Class) or alike in the visitor to solve all the mentionned issues with a single atomic API instead of creating 3 different API hard to understand and merge mentally IMHO. So the visitior must pull all data and the user/customization never push any model. It is really really close to jaxb. Then you just enable to access the default/current customizer and inject a customizer in JsonbConfig and you are done. Hope it makes sense.

[Verdent]

I think that so far this PR should be merged as is (with some more options I will push) and without the introspection stuff you brought up, since it is not even in the scope of this PR or the issue it is addressing. In terms of visitor I think it should not be added too right now.

The reason for both to be postponed, is to avoid exposing any sort of reflection executable components in the new API. There has been some talks with CDI guys in terms of separation of their class model to some new repository. That would allow as to use the model, even without having the hard dependency on the CDI itself and we will not be making any class models ourselves. This will also ensure that the model we are exposing is build time processing compatible and that is the direction we should be heading. I am not saying to not include the introspection and visitor stuff, I actually think both have their reasons to have them in. I would like to have them, but I would target those to some of the next releases, when the class model is separated.

[rmannibucau]

Hmm, I still see no reason to have such a model:

1. introspector does not require cdi - already, plain JDK works even if it requires to impl Type but most apps have it in general in a lib
2. introspector API is required anyway to solve other issues and solves this one without this model at all
3. introspector API is build time friendly

I'm fine targetting introspector for 3.0 or any other release, I'm not having a temporary API which 1. duplicates the existing one and 2. will be re-duplicated for a newt release.

So for me your last comment tend to not cover this use case in the spec - but feel free to add it in yasson - until we try to work on the introspector - but agree we can be short in time and a few other issues are worth a release more than that.

[Verdent]

This PR is mainly about 3rd party class customization and to be simple to use. To support introspector API properly, user would have to be able to obtain all the information about the fields, methods and also classes. That mean either having some sort of class/method etc. model or expose Method and Field instances. And that's no go.

And it is redundant to do the model ourselves if for example CDI does already have it. We have asked them if it is OK for them to make it as separate common spec and they had no objections to that. Other Jakarta EE specs were also claiming this would be useful for them to use. So that means we will not introduce any kind of model ourselves and we will postpone this PR till this separation is ready.

In terms of your proposed visitor, it requires information which will be provided by the model. That means we have to wait now even with visitor of yours and only the base PR we are having here can proceed now.

I think, that currently we cover large majority of the usecases and the only ones not covered are those done over visitor, which will be introduced in the future.

[rmannibucau]

This PR is mainly about 3rd party class customization and to be simple to use. To support introspector API properly, user would have to be able to obtain all the information about the fields, methods and also classes. That mean either having some sort of class/method etc. model or expose Method and Field instances. And that's no go.

Not sure what you exactly mean but for me current proposal sounds worse than that as explained.

And it is redundant to do the model ourselves if for example CDI does already have it. We have asked them if it is OK for them to make it as separate common spec and they had no objections to that. Other Jakarta EE specs were also claiming this would be useful for them to use. So that means we will not introduce any kind of model ourselves and we will postpone this PR till this separation is ready.

Fine, just means this feature will be postponed too.

In terms of your proposed visitor, it requires information which will be provided by the model. That means we have to wait now even with visitor of yours and only the base PR we are having here can proceed now.

As mentionned this means you do release R "feature F", release R+r "feature F was garbage, use feature F'", I want to avoid that.

I think, that currently we cover large majority of the usecases and the only ones not covered are those done over visitor, which will be introduced in the future.

As mentionned it covers a single customization case: final app one, it is very few of the use cases (libraries, global mapping changes etc.). So even if it is functional for this particular use case i'm more than happy to wait for a more global and simple solution and avoid a specific, single case, and verbose one. Jsonb is big enough as of today IMHO, no need to make it a big spec just by duplicating things IMHO.

[Verdent]

Fine, just means this feature will be postponed too.

You mean this PR?

As mentionned this means you do release R "feature F", release R+r "feature F was garbage, use feature F'", I want to avoid that.

I am not gonna be deprecating the runtime customization I am designing now. This is not something temporal. Visitor is not here to replace it. Just another way.

As mentionned it covers a single customization case: final app one, it is very few of the use cases (libraries, global mapping changes etc.). So even if it is functional for this particular use case i'm more than happy to wait for a more global and simple solution and avoid a specific, single case, and verbose one. Jsonb is big enough as of today IMHO, no need to make it a big spec just by duplicating things IMHO.

I simply don't agree with these arguments. I am still trying to make compromise and have proposed visitor with my customization objects, but to do that properly, we just have to wait. Using your "simple" annotation based solution is just unintuitive way of programming, since it simply doesn't help user in any way. That's not something regular programmer would want to do IMHO.

[rmannibucau]

Just another way.

This got proven a very bad way to do API. Lead to misleading and ambiguous experience to end users.

That's not something regular programmer would want to do IMHO.

Thought that when cdi 1.0 went out but I was wrong. It is exactly as your proposal, just a doc thing but industrializable without custom wrappers for 3rd parties so just even at that level it superseeds a fluent builder API.

[Verdent]

This got proven a very bad way to do API. Lead to misleading and ambiguous experience to end users.

What other suggestions do you have? I am really trying to make both worlds work together here, but I will simply not build my proposal on top of the literals or incorrect API. The reason for introducing visitor later is just because I want to make it right in terms of the API. I think it is time for you to fully introduce your desired solution in details, so we can review it.

Thought that when cdi 1.0 went out but I was wrong. It is exactly as your proposal, just a doc thing but industrializable without custom wrappers for 3rd parties so just even at that level it superseeds a fluent builder API.

CDI uses that approach and it is completely fine in terms of what they are doing, but it is something, what I do not believe, belong to the JSONB code and usage style.

[rmannibucau]

@Verdent i sent one proposal - based on johnzon - on our slack chan, but we can also reuse introspector kind of api for the high level idea, both work but miss the jsonb context - this is where visitor came from (think adapter, deserializer, which must be provided to the customizer before it contributes some customization for ex). So overall api can be something like:

public interface JsonbModelCustomizer {
  default void onType(TypeContext context) {}
  default void onProperty(PropertyContext context) {} // field and method merged view respecting visibility
}

Base context would be an inner interface:

public interface Context {
  <T extends Annotation> T getAnnotation(Class<T> a);
}

Type and property flavors will just expose, resp., the underlying getType()+skipProperty(String)+register(Annotation...)+registerProperty(Annotation...) and Optional+Optional as getters.

Nothing more needed, handles all types customizations including lists/maps, keep focused on our current model and finally is build time friendly since no reflection is required at runtime (even if build will stay simpler and way faster using it ;)). Also makes the API simpler to integrate with 3rd party since API surface is smaller.

[Verdent]

Well... it's not that simple as you are still claiming it to be. Alright lets do some tests here.

public class Pojo {

     private String valueOne;

    //getters and setters are also present
}

Show me how the usage of this API would look like, when I want to have a different name for serialization and for deserialization and for example @JsonbNillable to be also used on this class.

[rmannibucau]

public class MyJsonbModelCustomizer implements JsonbModelCustomizer {
  @Override
  public void onType(TypeContext context) { context.register(new JsonbNillable.Literal(); }
}

About the serial/deserial names, JSON-B does not support it until you use a custom deserializer/serializer (I hear you can put a setter and getter not asymmetric but not 100% sure it happens often enough) so at the end you ctx.registerProperty(new JsonbProperty.Literal("serialName"))registerProperty(new JsonbProperty.Literal("deserialName"));; and be it. If you really want to handle that case - I'm not convinced it is needed but fine if a big majority think it is - we just split onProperty i onSerializationProperty and onDeserializationProperty, does not dramatically change the proposal IMHO. I assume the default impl can call onProperty too if we want to stick to a simpler API?

[Verdent]

1. you are registering the names as a property?
2. This is not about how often something happens. This is about possibility to do the same customization in runtime as you can do in compile time.

3. This does not target only one class, but all of them. The same for properties since if we have this example, it will simply add the same annotations everywhere.

   public class Pojo {
    private String valueOne;
    private String valueTwo;
   
   //getters and setters are also present
   }

   So you are really trying to tell me, this is simpler, faster to work with and easier to use then my proposal? It gets messy and overly complicated with just a few properties. In fact 2 are enough to have unnecessary complexity of ifs or switch statements.

   
   public class MyJsonbModelCustomizer implements JsonbModelCustomizer {
   
   @Override
   public void onType(TypeContext context) {
       context.register(new JsonbNillable.Literal();
   }
   
   @Override
   public void onSerializationProperty(PropertyContext context) {
       context.registerProperty(new JsonbProperty.Literal("serialName"));
   }
   
   @Override
   public void onDeserializationProperty(PropertyContext context) {
       context.registerProperty(new JsonbProperty.Literal("deserialName"));
   }

}

[rmannibucau]

1. yes but I assume what you want is not a customizer but an actual ModelProvider which would also need a Supplier<T> and Consumer<T> to get/set the value of a virtual field (but it is another issue for me, just mentionning it cause the topic kind of change with exchanges)
2. I fully disagree on the last part since until now you claim you can't do it without your proposal but it is not true technically at all so I'm not sure how to refine the topic/comment there :thinking:
3. not sure why, context.getAnnotation(JsonbProperty.class )gives your the propery name, getField/getmethod the accessor so this is not true once again :s.

From my experience it is clearly easier to integrate with 3rd party libraries and classes than an explicit highly verbose API + you still didn't solve the model duplication.

[Verdent]

yes but I assume what you want is not a customizer but an actual ModelProvider which would also need a Supplier and Consumer to get/set the value of a virtual field (but it is another issue for me, just mentionning it cause the topic kind of change with exchanges)

Yes, if you are registering property, it would require more then just one annotation.

I fully disagree on the last part since until now you claim you can't do it without your proposal but it is not true technically at all so I'm not sure how to refine the topic/comment there 🤔

I am not claiming you cannot do it without my solution. I am claiming my solution is easier to use.

not sure why, context.getAnnotation(JsonbProperty.class )gives your the propery name, getField/getmethod the accessor so this is not true once again :s.

This is simply not true. Now try to demonstrate how it would look like for 3 or more different properties where you want to have different customization (for example names) for serialization and deserialization. And also try to make some mechanism which would execute your proposed solution only for a specific class and not for all. Because now it is basically widely used for each type.

From my experience it is clearly easier to integrate with 3rd party libraries and classes than an explicit highly verbose API + you still didn't solve the model duplication.

I don't believe there is any duplication, that means I do not see anything to fix. And once again I am not against visitor in general, I can see its reasoning. I am against the literals in our APIs and having visitor only without the easy way to do stuff. Visitor would be there as more of a hardcore part if user really needs that.

[rmannibucau]

Yes, if you are registering property, it would require more then just one annotation

If it is a virtual one it requires just the both functions (to be bidir), if we stick to customizations just annotations as in the example (but can be JsonbNumberFormat.Literal and others too indeed)

I am not claiming you cannot do it without my solution. I am claiming my solution is easier to use.

In one very particular case yes, in all others I don't think so. Plus, it duplicates the model which is a big issue for me.

This is simply not true. Now try to demonstrate how it would look like for 3 or more different properties where you want to have different customization (for example names) for serialization and deserialization. And also try to make some mechanism which would execute your proposed solution only for a specific class and not for all. Because now it is basically widely used for each type.

Well it is as you can see:

public class MyJsonbModelCustomizer implements JsonbModelCustomizer {
    @Override
    public void onProperty(PropertyContext context) { // impl ensures this request is fulfilled and falls back on member.getName() if needed
        context.registerProperty(new JsonbProperty.Literal(context.getAnnotation(JsonbProperty.class).value().equals("foo") ? "f" : "bar"));
    }
}

On your last point: I don't want to make a lot of convenient helper functions, this always lead to a noisy API which is more bothering and does not encourage people to learn proper ways to solve their issues. Once again, I would first solve the common needs when we manage to find an agreement (schema extraction enablement, customizations) then see if we need a more friendly DSL or not - can even be an optional part of the spec but ont a requirement IMHO.

[Verdent]

In one very particular case yes, in all others I don't think so. Plus, it duplicates the model which is a big issue for me.

That it duplicates something is just your opinion. From my point of view it is perfectly fine to have it like this. It is easier to use in most of the needed cases. And for those, it is not, visitor will be presented to be able to use. As I mentioned before.... a bigger hammer to do the stuff.

public class MyJsonbModelCustomizer implements JsonbModelCustomizer { @Override public void onProperty(PropertyContext context) { // impl ensures this request is fulfilled and falls back on member.getName() if needed context.registerProperty(new JsonbProperty.Literal(context.getAnnotation(JsonbProperty.class).value().equals("foo") ? "f" : "bar")); } }

Eh... I am not sure what to say here. I have several issues here. This still does not address multiple properties or specific class. Another is that JsonbProperty annotation doesn't have to be there and most likely will not be and that means it cannot be done simply like that. I know you are trying to put everything on a single line just to look it is simpler and less chatty. But I would prefer to do it correctly and with all cases covered.

Once again, I would first solve the common needs

That's exactly what my proposal is about. Visitor is for more extra needs.

I would be more then happy to come up with some sort of compromise. I have already introduced several of those, but literals are simply no go for me. I will go over that line.

[rmannibucau]

Visitor is for more extra needs.

You didnt get it. Your solution solves end user customization, period. My proposal solves this + 3rd party customizations + schema extraction with a single API and no duplication of the model. This is the common needs for me, not only your case which is not that common in regards of others IMHO.

Another is that JsonbProperty annotation doesn't have to be there and most likely will not be and that means it cannot be done simply like that.

It is as explained in the comment. Give a try if you doubt.

[Verdent]

only your case which is not that common in regards of others IMHO.

others -> you Common usecase is something common -> name change, number format, nillable, transient. That's common. And no we are not duplicating any model so there is no issue there. Sure I am covering customization only in terms of the base PR since that's what this is about. Introspection or advanced customization (visitor) will be also supported, but I have suggested to postpone it due to obvious reason I have described before.

It is as explained in the comment. Give a try if you doubt.

I did. So you think that this method should basically just fail with NullPointerException since otherwise I am not sure how the impl will even know to fall back.

[m0mus]

It definitely makes sense to use reflectionless Object Model API because we are targeting to Jakarta Core profile which is going to be build time processing friendly.

[m0mus]

Folks, this conversation becomes never ending. Both parties have strong opinion and no one wants to step back. It's getting close to October deadline and we may not have enough time to deal with the implementation and TCKs.

I see two options:

1. Merge the customization API as it was proposed without a visitor and an introspector. We'll add both in the next version of API when Object Model API is somehow (we don't know how) extracted from CDI.
2. Postpone the customization API to the next version of JSONB. We are still willing to add it to Yasson. So, users will be able to play with it and provide some feedback before it gets merged to the spec.

I would like all project committers to vote. I'll close the ballot in a week at Sep 23rd 13:00 CEST.

@njr-11 @Verdent @edbratt @jansupol @jimma @lukasj @otaviojava @pdudits @rmannibucau @tomas-langer @ivanjunckes please vote in the comments.

I hope I didn't forget anyone.

[otaviojava]

@m0mus I like MVPs, so I vote to merge as it is and then move to the next step.

[rmannibucau]

@m0mus without surprises, I think this misses the bbig opportunity to have an unified model and unified API solving > 3 issues (missing features) so I'd keep it as an extension not belonging to the spec requirements (= agree it can be a DSL end users like on top of the actual spec API but nothing more) so I'd be to not merge it.

[lukasj]

I'm for merging this.

[Verdent]

I am a bit puzzled here. I am for merging it, but I want to do it right. There is possible problem I can see and that is the moment, when we decide to add that visitor and introspector functionality, I have mentioned before. I would like to have these two features build on top of this customization PR and the problem might be, that we will need to change something in this customization design, but if it is already released and it is breaking change, we will not be able to. So I might vote for postponing and merge when visitor and introspector do have all the required parts ready and are fully designed. Does this thought make sense? or you think it will be fine to merge it now, since I do have the design of the visitor and introspector more or less ready and we might not need later to change the customization at all?

[jansupol]

The open question for Jakarta EE 11 is the possible replacement of Java reflection API with a metamodel for compile-time (annotation) processing. Such a thing would be required by multiple specs that would want to focus on speed (thank you Quarkus), starting with CDI lite, up to JSON-B. Introducing an API that handles the annotations directly can cause issues for EE 11 and the new API could have been immediately deprecated in EE 11. We do not know for certain what requirements this compile-time processing brings, and hence I agree with David to postpone this very API change until we have a more complete idea of the requirements. Then we will be able to design the API for introspection, too.

[rmannibucau]

I guess we can fully ignore reflectionless because:

1. It cant hit jakarta (platform) alone (only for sub parts of the specs and to reuse the only example - as of today, CDI - it is a full new spec from scratch with no link with current version
2. Technically you dont need anything new for cdi, jsonb, .... the jdk already provides all we need (indeed build time constraints are high enough to not be adopted by most users but technically we know how to do)
3. Build time always had been out of scope of EE, not sure it is sane to go that path - indeed it is a way to say it ;)

So long story short, the big risk there is to create an api not reaching enough consumer coverage and superseeded in a next version we already envision even if we dont have the api yet.

Side note: we dont have to await an ee version to release a major jsonb version, we can be faster if we reach a compromise. EE just gets the current last version in last release plan "sprints".

[jansupol]

@rmannibucau This issue is likely not to right place to discuss reflectionless. But it is a big topic right now for EE 11. I completely agree that it can be ignored, and the benefit not being used. I agree that JDK provides everything needed so we do not need anything new from CDI or whomever. We may not benefit from what is provided by other projects and we can reinvent the wheel. But I am not sure it is the right way to go. We may see where the EE world is heading soon; I expect a wide discussion once EE10 is dealt with.

Anyway, I understand that you agree with postponing the PR.

[Verdent]

@pdudits Thank you for your feedback. I definitely agree that javadoc should be improved and I will do that :-) It contains a lot of inconsistencies and unclear formulations, which I am working on getting rid of.