-
Notifications
You must be signed in to change notification settings - Fork 39
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
3rd party class customization #279
base: master
Are you sure you want to change the base?
Changes from 7 commits
0a6ffdb
9c6782a
4a072f2
88a18e7
a4d3c51
52ef94a
fb62caa
3733600
351b92e
eda10e1
0950a50
bcf0ad4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* | ||
* Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License v. 2.0, which is available at | ||
* http://www.eclipse.org/legal/epl-2.0. | ||
* | ||
* This Source Code may also be made available under the following Secondary | ||
* Licenses when the conditions for such availability set forth in the | ||
* Eclipse Public License v. 2.0 are satisfied: GNU General Public License, | ||
* version 2 with the GNU Classpath Exception, which is available at | ||
* https://www.gnu.org/software/classpath/license.html. | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
*/ | ||
|
||
package jakarta.json.bind.customization; | ||
|
||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
import jakarta.json.bind.spi.JsonbProvider; | ||
|
||
/** | ||
* Decorated creator customization. | ||
*/ | ||
public interface CreatorCustomization { | ||
|
||
/** | ||
* Create new {@link CreatorCustomizationBuilder} instance. | ||
* | ||
* Builder created via this method targets constructors of the class it is bound to. | ||
* | ||
* @return new creator customization builder instance | ||
*/ | ||
static CreatorCustomizationBuilder builder() { | ||
return JsonbProvider.provider().newCreatorCustomizationBuilder(); | ||
} | ||
|
||
/** | ||
* Create new {@link CreatorCustomizationBuilder} instance based on creator method name. | ||
* | ||
* @return new creator customization builder instance | ||
*/ | ||
static CreatorCustomizationBuilder builder(String methodName) { | ||
return JsonbProvider.provider().newCreatorCustomizationBuilder(methodName); | ||
} | ||
|
||
/** | ||
* Return creator method name if has been specified. | ||
* If the name is empty, it is handled as if it is null. | ||
* | ||
* @return specified creator method name, otherwise empty | ||
*/ | ||
Optional<String> factoryMethodName(); | ||
|
||
/** | ||
* Return {@link List} of the registered parameters. The order of the parameters needs to be the same as they were added. | ||
Verdent marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* If no parameters were added, empty {@link List} is returned. | ||
* | ||
* @return creator parameters | ||
*/ | ||
List<ParamCustomization> params(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License v. 2.0, which is available at | ||
* http://www.eclipse.org/legal/epl-2.0. | ||
* | ||
* This Source Code may also be made available under the following Secondary | ||
* Licenses when the conditions for such availability set forth in the | ||
* Eclipse Public License v. 2.0 are satisfied: GNU General Public License, | ||
* version 2 with the GNU Classpath Exception, which is available at | ||
* https://www.gnu.org/software/classpath/license.html. | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
*/ | ||
|
||
package jakarta.json.bind.customization; | ||
|
||
import java.util.function.Consumer; | ||
|
||
/** | ||
* Builder of the {@link CreatorCustomization} instance. | ||
* | ||
* When no creator method name is specified, it is assumed that creator is constructor. | ||
*/ | ||
public interface CreatorCustomizationBuilder { | ||
|
||
/** | ||
* Add {@link ParamCustomization} instance. | ||
* | ||
* All creator parameters are required to be added in exact order as they are on decorated factory method/constructor. | ||
* | ||
* @param creatorParam creator parameter | ||
* @return updated builder instance | ||
*/ | ||
CreatorCustomizationBuilder addParameter(ParamCustomization creatorParam); | ||
Verdent marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Add new {@link ParamCustomization} of the property. | ||
* <br> | ||
* Shortcut method to the {@link #addParameter(ParamCustomization)}. It is not required to create {@link CreatorCustomizationBuilder} | ||
* since this method will create {@link ParamCustomization} based on the provided parameter class and json name and by calling | ||
* {@link ParamCustomization#create(Class, String)} method. No further customizations will be applied. | ||
* <br> | ||
* All creator parameters are required to be added in exact order as they are on decorated factory method/constructor. | ||
* | ||
* @param parameterClass class of the parameter | ||
* @param jsonName json name of the parameter | ||
* @return updated builder instance | ||
*/ | ||
default CreatorCustomizationBuilder addParameter(Class<?> parameterClass, String jsonName) { | ||
return addParameter(ParamCustomization.create(parameterClass, jsonName)); | ||
} | ||
|
||
/** | ||
* Add new {@link PropertyCustomization} of the property. | ||
* <br> | ||
* Shortcut method to the {@link #addParameter(ParamCustomization)}. It is not required to create {@link CreatorCustomizationBuilder} | ||
* since this method will create is based on the provided parameter class and json name. Created builder is provided over | ||
* the paramBuilder. | ||
* <br> | ||
* All creator parameters are required to be added in exact order as they are on decorated factory method/constructor. | ||
* <br> | ||
* Example usage: | ||
* <pre>{@code | ||
* creatorBuilder.addParameter(String.class, "jsonName", paramBuilder -> paramBuilder.nillable(true)); | ||
* }</pre> | ||
* | ||
* @param parameterClass class of the parameter | ||
* @param jsonName json name of the parameter | ||
* @param paramBuilder builder used to customize parameter | ||
* @return updated builder instance | ||
*/ | ||
default CreatorCustomizationBuilder addParameter(Class<?> parameterClass, | ||
String jsonName, | ||
Consumer<ParamCustomizationBuilder> paramBuilder) { | ||
ParamCustomizationBuilder builder = ParamCustomization.builder(parameterClass, jsonName); | ||
paramBuilder.accept(builder); | ||
return addParameter(builder.build()); | ||
} | ||
|
||
/** | ||
* Build the new instance of the {@link CreatorCustomization}. | ||
* | ||
* @return new {@link CreatorCustomization} instance | ||
*/ | ||
CreatorCustomization build(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
* Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License v. 2.0, which is available at | ||
* http://www.eclipse.org/legal/epl-2.0. | ||
* | ||
* This Source Code may also be made available under the following Secondary | ||
* Licenses when the conditions for such availability set forth in the | ||
* Eclipse Public License v. 2.0 are satisfied: GNU General Public License, | ||
* version 2 with the GNU Classpath Exception, which is available at | ||
* https://www.gnu.org/software/classpath/license.html. | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
*/ | ||
|
||
package jakarta.json.bind.customization; | ||
|
||
import java.text.DateFormat; | ||
import java.text.NumberFormat; | ||
import java.util.Optional; | ||
|
||
import jakarta.json.bind.adapter.JsonbAdapter; | ||
import jakarta.json.bind.serializer.JsonbDeserializer; | ||
|
||
/** | ||
* Common interface for all the customizations. | ||
*/ | ||
public interface JsonbCustomization { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion the type hierarchy of the customizations does not serve any purpose for the user and makes API harder to navigate. I'd drop this entire hierarchy and limit public API just to the interfaces that should be used directly by the user. I can see how this can be convenient for implementor, but implementor is free to build its implementation class hierarchy instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm yes I kind of agree, but the whole hierarchy has been made this way to prevent having method duplication :-) Or do you agree with having it and just hide it as package private and expose just the end interfaces? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You got me reading Java Language Specification with this question, and it doesn't directly list the consequences. I believe it's better to inline the interfaces. |
||
|
||
/** | ||
* Return {@link NumberFormat} specified to the required {@link Scope}. | ||
* | ||
* @param scope required scope | ||
* @return specified {@link NumberFormat} instance, otherwise empty | ||
*/ | ||
Optional<NumberFormat> numberFormat(Scope scope); | ||
|
||
/** | ||
* Return {@link DateFormat} specified to the required {@link Scope}. | ||
* | ||
* @param scope required scope | ||
* @return specified {@link DateFormat} instance, otherwise empty | ||
*/ | ||
Optional<DateFormat> dateFormat(Scope scope); | ||
|
||
/** | ||
* Return {@link JsonbDeserializer} of the component. | ||
* | ||
* @return component deserializer instance, otherwise empty | ||
*/ | ||
Optional<JsonbDeserializer<?>> deserializer(); | ||
|
||
/** | ||
* Return {@link JsonbAdapter} of the component. | ||
* | ||
* @return component adapter instance, otherwise empty | ||
*/ | ||
Optional<JsonbAdapter<?, ?>> adapter(); | ||
|
||
/** | ||
* Return if the component can be nillable in the given {@link Scope}. | ||
* | ||
* @param scope required scope | ||
* @return property nillable state, otherwise empty | ||
*/ | ||
Optional<Boolean> nillable(Scope scope); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure the optional makes sense here. If empty, the spec still needs to define whether it is nillable or not when not set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional is here because the value didn't have to be set at all. Simply returned boolean is not enough in this case since we need 3 states. True, false and not set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tt is always true or false for the runtime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is not true. If user didn't call nillable method, it is equivalent to the situation where annotation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Verdent when @JsonbNillable is not on an attribute the attribute is nillable=true so true/false are strictly sufficient. Now I fully understand the intent of your API which is to let the user say "I don't care, use the (configured or not) default" but then the API is not designed for that case, it is reversed as mentionned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is true but if the user did he probably knows too since he has to use the jsonbconfig anyway with that solution so he can wire the default properly without any issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What user did? :-) User can set only true or false via builder and not Optional. I don't understand what is misleading here since it just says: User either set true/false by himself or didn't set anything at all since the method has not been called. In that case value from config will be used by implementation. There is simply no misleading part here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is never misleading for the people writing it, this is why reviews are not that bad ;) javadoc says There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eh... are you trying to tell me... that this whole discussion here was just about incorrect wording? OK, I will change that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hehe, no no, that even the comment is closer to the expectation. |
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we drop the two entry points from this class we can make this whole PR an appendix - same as proposing the introspector/annotation based solution - of the spec while there is not agreement on the final outcome.
Can enable to have API proposals and decide later (bean validation went that way in its time - for method validation).
Overall blocking point is to have something unified versus something duplicated for 3rd party libraries and I don't see us choosing before next release since both parties seems to either priviledge the interoperability and simplicity or the application API first (being said the introspector API enables to join the API point later if needed whereas decorator does not and keeps two concurrent models in the spec forever).
Wdyt? (trying to not block but not pollute the API too early without enabling 3rd party cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you mean by "drop the two entry points from this class". I am sorry, but I don't understand what you are trying to say. Please explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not make it explicit in the API (ie use yasson.decorators property without a constant nor a wither.