-
Notifications
You must be signed in to change notification settings - Fork 682
Add support for property-specific converters #2566
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
Conversation
Introduce a builder API to register PropertyValueConverters using simple (Bi)Functions. Additional methods on ValueConversionContext to enable advanced use cases in converter implementation. Tweak generics of VCC to be able to expose store-specific PersistentProperty implementations via the context.
* @since 2.7 | ||
*/ | ||
@Nullable | ||
public <A, B, C extends PersistentProperty<C>, D extends ValueConversionContext<C>> PropertyValueConverter<A, B, D> getPropertyValueConverter( |
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.
Let's make the getPropertyValueConverter()
method non-nullable as we have hasPropertyValueConverter(…)
methods to check for the presence of converters.
*/ | ||
public ConverterConfiguration(StoreConversions storeConversions, List<?> userConverters, | ||
Predicate<ConvertiblePair> converterRegistrationFilter, | ||
@Nullable PropertyValueConversions propertyValueConversions) { |
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.
Having a null-object for PropertyValueConversions
would allow to avoid nullable constructor args.
* @return the suitable {@link PropertyValueConverter} or {@literal null} if none available. | ||
*/ | ||
@Nullable | ||
<A, B, C extends PersistentProperty<C>, D extends ValueConversionContext<C>> PropertyValueConverter<A, B, D> getValueConverter( |
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.
Let's make the getPropertyValueConverter()
method non-nullable as we have hasPropertyValueConverter(…)
methods to check for the presence of converters.
* Helper that allows to create {@link PropertyValueConversions} instance with the configured | ||
* {@link PropertyValueConverter converters} provided via the given callback. | ||
*/ | ||
static <P extends PersistentProperty<P>> PropertyValueConversions simple( |
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.
How about naming this method create
? simple
implies there could be also a complex
variant.
* @return the converted value. Can be {@literal null}. | ||
*/ | ||
@Nullable | ||
A read(@Nullable B value, C context); |
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.
Didn't we want to have default
methods or similar to avoid nullability for the value to read/to write?
* @return | ||
*/ | ||
@Nullable | ||
<A, B, C extends ValueConversionContext<?>> PropertyValueConverter<A, B, C> getConverter( |
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.
Let's make the getPropertyValueConverter()
method non-nullable and introduce a hasConverter
method to check for the presence of converters.
* | ||
* @author Oliver Drotbohm | ||
*/ | ||
static class WritingConverterRegistrationBuilder<T, S, P extends PersistentProperty<P>> { |
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.
We need to increase visibility so that builders can be used from outside of the package.
* | ||
* @return the configured {@link PropertyValueConverter}. {@link ObjectToObjectPropertyValueConverter} by default. | ||
*/ | ||
Class<? extends PropertyValueConverter> value() default ObjectToObjectPropertyValueConverter.class; |
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.
Can we make the value
mandatory?
* @return {@literal null} if no converter present for the given type/path combination. | ||
*/ | ||
@Nullable | ||
<S, T> PropertyValueConverter<S, T, ? extends ValueConversionContext<P>> getConverter(Class<?> type, String path); |
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.
Let's make the getConverter()
method non-nullable as we have containsConverterFor(…)
methods to check for the presence of converters.
* @param <T> | ||
* @return new instance of {@link ValueConverterRegistry}. | ||
*/ | ||
static <T extends PersistentProperty<T>> ValueConverterRegistry<T> simple() { |
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.
Let's call this create
as we have no other variants.
*/ | ||
static class ReadingConverterRegistrationBuilder<T, S, R, P extends PersistentProperty<P>> { | ||
|
||
private WritingConverterRegistrationBuilder<T, S, P> origin; |
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.
Generics seem off here as T
refers to the underlying type. Later on, we call registration.accept
where generics do not match.
Tweak Javadoc. Increase visibility of converter builders.
Introduce a builder API to register PropertyValueConverters using simple (Bi)Functions. Additional methods on ValueConversionContext to enable advanced use cases in converter implementation. Tweak generics of VCC to be able to expose store-specific PersistentProperty implementations via the context. See #1484 Original pull request: #2566.
Increase visibility of converter builders. Refine generics naming towards DV/SV instead of A/B to easier identify store-native values and domains-specific values in the API. Refactor PropertyValueConversions.hasValueConverter into a non-default method. Tweak documentation. See #1484 Original pull request: #2566.
Introduce a builder API to register PropertyValueConverters using simple (Bi)Functions. Additional methods on ValueConversionContext to enable advanced use cases in converter implementation. Tweak generics of VCC to be able to expose store-specific PersistentProperty implementations via the context. See #1484 Original pull request: #2566.
Increase visibility of converter builders. Refine generics naming towards DV/SV instead of A/B to easier identify store-native values and domains-specific values in the API. Refactor PropertyValueConversions.hasValueConverter into a non-default method. Tweak documentation. See #1484 Original pull request: #2566.
That's merged, polished, and forward-ported now. |
This PR will introduce common infrastructure that allows identifying and serving converters that are property specific. Other than existing converters configured via custom conversions, the property based variant will only be applied to a specific property of a type, instead of targeting all properties of a certain type.
PropertyValueConverter
implementations will be provided aValueConversionContext
that gives access to the target property itself and may be enhanced by the store specific implementation.In addition to the annotation based approach it is possible to programatically register converters for certain properties