Skip to content

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

Closed
wants to merge 9 commits into from
Closed

Conversation

christophstrobl
Copy link
Member

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 a ValueConversionContext that gives access to the target property itself and may be enhanced by the store specific implementation.

class Person {
  // ...
  @ValueConverter(EncryptedValueConverter.class)
  String ssn;
}

class EncryptedValueConverter implements PropertyValueConverter<String, byte[], ValueConversionContext> {

  @Override
  public String read(byte[] value, ValueConversionContext context) {
    // ...
  }

  @Override
  public byte[] write(String value, ValueConversionContext context) {
    // ...
  }
}

In addition to the annotation based approach it is possible to programatically register converters for certain properties

PropertyValueConverterRegistrar registrar = new PropertyValueConverterRegistrar();

// untyped registration
registrar.registerConverter(Address.class, "street", new PropertyValueConverter() { ... });

// type safe registration
registrar.registerConverter(Person.class, Person::getSsn())
  .writing(value -> encrypt(value))
  .reading(value -> decrypt(value));

christophstrobl and others added 7 commits February 25, 2022 06:48
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.
@christophstrobl christophstrobl linked an issue Mar 1, 2022 that may be closed by this pull request
* @since 2.7
*/
@Nullable
public <A, B, C extends PersistentProperty<C>, D extends ValueConversionContext<C>> PropertyValueConverter<A, B, D> getPropertyValueConverter(
Copy link
Member

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) {
Copy link
Member

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(
Copy link
Member

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(
Copy link
Member

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);
Copy link
Member

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(
Copy link
Member

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>> {
Copy link
Member

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;
Copy link
Member

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);
Copy link
Member

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() {
Copy link
Member

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;
Copy link
Member

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.
@mp911de mp911de marked this pull request as ready for review March 18, 2022 08:25
@mp911de mp911de changed the title Add support for property specific converters Add support for property-specific converters Mar 18, 2022
@mp911de mp911de added this to the 2.7 M4 (2021.2.0) milestone Mar 18, 2022
@mp911de mp911de added the type: enhancement A general enhancement label Mar 18, 2022
mp911de pushed a commit that referenced this pull request Mar 18, 2022
mp911de pushed a commit that referenced this pull request Mar 18, 2022
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.
mp911de pushed a commit that referenced this pull request Mar 18, 2022
mp911de added a commit that referenced this pull request Mar 18, 2022
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.
mp911de pushed a commit that referenced this pull request Mar 18, 2022
mp911de pushed a commit that referenced this pull request Mar 18, 2022
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.
mp911de pushed a commit that referenced this pull request Mar 18, 2022
mp911de added a commit that referenced this pull request Mar 18, 2022
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.
@mp911de
Copy link
Member

mp911de commented Mar 18, 2022

That's merged, polished, and forward-ported now.

@mp911de mp911de closed this Mar 18, 2022
@mp911de mp911de deleted the issue/1484 branch March 18, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add API for property specific converters [DATACMNS-1036]
3 participants