Skip to content
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

HSEARCH-3609 Replace bridge builders with *binders* #2034

Merged
merged 17 commits into from Jul 15, 2019

Conversation

@yrodiere
Copy link
Member

commented Jul 4, 2019

https://hibernate.atlassian.net//browse/HSEARCH-3609

Based on #2038 , which should be merged first. => Done

This is an attempt at making the bridge 2.0 API simpler for simple use cases, and not-too-complicated-yet-powerful for advanced use cases.

Due to the refactorings involved, this will not be easy to review, so I'll describe the result here instead. If that doesn't seem too bad to you, I'll merge. Then I'll work on documenting this... soon-ish...

Simple use case: ValueBridge only

To bind a single property to a single field, a custom ValueBridge can be used. It is similar to JPA's AttributeConverter.

Define the bridge class:

// Assuming "SKU" is a user-defined type
public static class SKUBridge implements ValueBridge<SKU, String> {
	@Override
	public String toIndexedValue(SKU value, ValueBridgeToIndexedValueContext context) {
		return value == null ? null : value.toString();
	}

	// Implementing this is optional (the default impl throws an exception), but useful if you need projections
	@Override
	public SKU fromIndexedValue(String value, ValueBridgeFromIndexedValueContext context) {
		return value == null ? null : SKU.fromString( value );
	}
}

Use it in the mapping:

@Entity
@Indexed
public class Product {

    // ...

    @Basic 
    @GenericField(valueBridge = @ValueBridgeRef(type = SKUBridge.class))
    private SKU sku;

    // ...
}

And that's it! Hibernate Search will automatically detect that the index field is of type String, and you'll be able to query the "sku" field as a text field.

More advanced use case: ValueBridge with a ValueBinder

The ValueBinder allows more fine-grained control over the index field type.

For example, if we wanted the sku fields to be normalized, we could do it that way:

@Entity
@Indexed
public class Product {

    // ...

    @Basic 
    @KeywordField(valueBridge = @ValueBridgeRef(type = SKUBridge.class), normalizer = "sku")
    private SKU sku;

    // ...
}

... but that's a bit redundant.

Another way of doing this would be to define a binder that applies the normalizer automatically.

Define the bridge and binder classes:

// Assuming "SKU" is a user-defined type
public static class SKUBridge implements ValueBridge<SKU, String> {
	@Override
	public String toIndexedValue(SKU value, ValueBridgeToIndexedValueContext context) {
		return value == null ? null : value.toString();
	}

	// Implementing this is optional (the default impl throws an exception), but useful if you need projections
	@Override
	public SKU fromIndexedValue(String value, ValueBridgeFromIndexedValueContext context) {
		return value == null ? null : SKU.fromString( value );
	}

	public static class Binder implements ValueBinder {
		public void bind(ValueBindingContext context) {
			context.setBridge(
				SKU.class, new SKUBridge(),
				context.getTypeFactory().asString().normalizer( "sku" )
			);
		}
	}
}

Use the binder in the mapping:

@Entity
@Indexed
public class Product {

    // ...

    @Basic 
    @GenericField(valueBinder = @ValueBinderRef(type = SKUBridge.Binder.class))
    private SKU sku;

    // ...
}

Note that ValueBinder is a bit more powerful than that: it also allows inspecting the input value type, which could allow the implementation of binders that create a different bridge depending on the input type. That's what we do for enums, in particular.

The bazooka: TypeBridge and PropertyBridge

If you need to bind multiple properties and combine them into a single field value, or to bind one or more properties to multiple fields in the same bridge, ValueBridge cannot help: one of its trade-offs is that it only allows one-to-one binding.

TypeBridge and PropertyBridge are here just for that. They are a bit more complex to set up, but far more powerful.

Note they allow things that were not possible in Search 5:

  • These bridges can define precisely the options of every index field they populate, including in particular analyzers and normalizers. In Search 5, you had to use a default analyzer for all fields except the "main" one (the one whose name was defined in the @Field annotation).
  • These bridges can access properties of the bridged element, yet preserve consistent reindexing, thanks to dependency declaration

The first step to define such a bridge is to define an annotation:

@PropertyBinding(binder = @PropertyBinderRef( type = LocalizedTextBridge.Binder.class ) )
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD, ElementType.METHOD })
@Documented
public @interface LocalizedTextBinding {
    String analyzerEn() default "text-en";
    String normalizerEn() default "text-en-sort";
    String analyzerFr() default "text-fr";
    String normalizerFr() default "text-fr-sort";
    String analyzerCrossLanguage() default "text-cross-language";
}

Then the bridge and binder classes:

public class LocalizedTextBridge implements PropertyBridge {

	private final IndexFieldReference<String> enFieldReference;
	private final IndexFieldReference<String> enSortFieldReference;
	private final IndexFieldReference<String> frFieldReference;
	private final IndexFieldReference<String> frSortFieldReference;
	private final IndexFieldReference<String> crossLanguageFieldReference;

	private LocalizedTextBridge(IndexSchemaElement schema, String fieldName,
			LocalizedTextBinding annotation) {
		this.enFieldReference = schema.field(
				fieldName + "_en",
				f -> f.asString().analyzer( annotation.analyzerEn() )
		).toReference();
		this.enSortFieldReference = schema.field(
				fieldName + "_en_sort",
				f -> f.asString().normalizer( annotation.normalizerEn() )
		).toReference();
		this.frFieldReference = schema.field(
				fieldName + "_fr",
				f -> f.asString().analyzer( annotation.analyzerFr() )
		).toReference();
		this.frSortFieldReference = schema.field(
				fieldName + "_fr_sort",
				f -> f.asString().normalizer( annotation.normalizerFr() )
		).toReference();
		// An extra field for cross-language search.
		// We want a different analyzer in that case:
		// neither the French one nor the English one will do.
		this.crossLanguageFieldReference = schema.field(
				fieldName,
				f -> f.asString().analyzer( annotation.analyzerCrossLanguage() )
		).multiValued().toReference();
	}

	@Override
	public void write(DocumentElement target, Object bridgedElement, TypeBridgeWriteContext context) {
		if ( bridgedElement == null ) {
			return;
		}

		LocalizedText text = (LocalizedText) bridgedElement;

		target.addValue( enFieldReference, text.getEn() );
		target.addValue( enSortFieldReference, text.getEn() );
		target.addValue( crossLanguageFieldReference, text.getEn() );
		target.addValue( frFieldReference, text.getFr() );
		target.addValue( frSortFieldReference, text.getFr() );
		target.addValue( crossLanguageFieldReference, text.getFr() );
	}

	public static class Binder implements PropertyBinder<LocalizedTextBinding> {
		private LocalizedTextBinding annotation;

		@Override
		public void initialize(LocalizedTextBinding annotation) {
			this.annotation = annotation;
		}

		@Override
		public void bind(PropertyBindingContext context) {
			// By declaring the properties we rely on,
			// we make sure that changes to these properties will trigger reindexing
			context.getDependencies().use( "fr" ).use( "en" );

			context.setBridge(
					new LocalizedTextBridge(
							context.getBridgedElement().getName(),
							context.getIndexSchemaElement(),
							annotation
					)
			);
		}
	}
}

And finally use the annotation in the mapping:

@Entity
@Indexed
public class Product {

    // ...

    @Embedded
    // Parameters can be passed to the binder through the annotation attributes
    @LocalizedTextBinding(analyzerCrossLanguage = "product-name-cross-language")
    private LocalizedText name;

    // ...
}

@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3609 branch 3 times, most recently from fc06f40 to 3374cb9 Jul 8, 2019

yrodiere added some commits Jul 2, 2019

HSEARCH-3609 Split BridgeBuilder into one interface per bridge type
This will be necessary to turn it into a binder, which takes
context specific to the bridge type in parameter of its bind() method.
HSEARCH-3609 Move projection conversion for ValueBridges to the bridg…
…e itself

... instead of requiring the bridge to implement bind().

We're about to move the bind() feature to another, more advanced class
(the bridge builder). This means setting the projection converter
explicitly will not be possible anymore without using a bridge builder.

But projection convertion is a very basic feature, which should
definitely be implementable simply by defining a ValueBridge, without
having to mess with bridge builders.

By adding a fromIndexedValue() method to ValueBridge, we solve this
exact problem.
HSEARCH-3609 Move ValueBridge's bind() method to the bridge builder, …
…replacing build()

So that:

1. The value bridge itself is simpler.
2. The builder can take advantage of reflection to build the resulting
bridge. See DefaultEnumValueBridge.Builder.
HSEARCH-3609 Move TypeBridge's and PropertyBridge's bind() method to …
…the bridge builder, replacing build()

So that:

1. The bridge itself is simpler.
2. The builder can take advantage of reflection to build the resulting
bridge, picking a different implementation based on the context.
3. Perhaps most importantly, the bridge is now immutable.
HSEARCH-3609 Move RoutingKeyBridge's bind() method to the bridge buil…
…der, replacing build()

So that:

1. The bridge itself is simpler.
2. The builder can take advantage of reflection to build the resulting
bridge, picking a different implementation based on the context.
3. Perhaps most importantly, the bridge is now immutable.
HSEARCH-3609 Make StartubStubBridge generic
It will be necessary for identifier bridges, where the bridge must match
the exact type of the identifier.
HSEARCH-3609 Move IdentifierBridge's bind() method to the bridge buil…
…der, replacing build()

So that:

1. The bridge itself is simpler.
2. The builder can take advantage of reflection to build the resulting
bridge. See DefaultEnumIdentifierBridge.Builder.
HSEARCH-3609 Rename bridge builders to binders
After all, they only define a bind() method, not a build() method
anymore.
HSEARCH-3609 Refactor MarkerBuilder to be consistent with bridges
Rename it to MarkerBinder, in particular.
HSEARCH-3609 Remove support for direct bridge reference for type, pro…
…perty and routing key bridges

The type bridge and property bridge are worthless without a declared
index field, and index fields can only be declared by the binder. So it
doesn't make sense to reference a bridge directly without a binder.

Routing key bridges could theoretically be used without a binder, but
only if they rely exclusively on the ID and entity type to generate the
routing key. It's doubtful such a simple bridge would be useful,
and routing key bridges are advanced usage anyway, so we'll remove
support for direct bridge reference for these too, so we're consistent
with type and property bridges.
HSEARCH-3609 Be more explicit about the fact @*Binding meta-annotatio…
…n reference binders, not bridges directly
HSEARCH-3609 Move binder references out of @ValueBridgeRef and @Ident…
…ifierBridgeRef to different annotations

Now that some bridges can only be referenced by their binder, this makes
more sense.

@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3609 branch from 3374cb9 to 3b3b255 Jul 12, 2019

@yrodiere yrodiere added the Urgent label Jul 12, 2019

HSEARCH-3609 Use a single naming scheme for all bindings: <prefix>Bin…
…ding for the annotation, <prefix>Bridge for the bridge
@yrodiere

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

@fax4ever I just pushed an additional commit to address a problem that @gsmet mentioned to me: the cast() method in Value and Identifier bridges was not necessary anymore, since after this PR the binders are expected to pass the expected type of values/identifiers to Hibernate Search, so we can perform the cast ourselves.
Anyway, it's fixed, and I updated the PR description to remove the weird-looking cast methods.

@fax4ever

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

I haven't reviewed the full code, I think that the new API is very powerful and it looks good to me. So +1 to merge from me.

HSEARCH-3609 Remove the cast() method from identifier and value bridges
It's no longer needed, now that the binder passes the expected value
type to Hibernate Search explicitly.

@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3609 branch from 2cc06c1 to 35d2d51 Jul 15, 2019

@yrodiere

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Thanks. I'll merge as soon as the Travis build succeeds.

@yrodiere yrodiere merged commit 91f0b5d into hibernate:master Jul 15, 2019

2 checks passed

LGTM analysis: Java No new or fixed alerts
Details
Travis CI - Pull Request Build Passed
Details

@yrodiere yrodiere deleted the yrodiere:HSEARCH-3609 branch Aug 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.