HSEARCH-3421 Make the Elasticsearch sort builders reusable

I.e. avoid side-effects in their build method. Mainly to be consistent with the predicate builders.
hibernate · Jan 31, 2019 · 73e8f75 · 73e8f75
1 parent cebfc01
commit 73e8f75
Show file tree

Hide file tree

Showing 6 changed files with 37 additions and 49 deletions.
diff --git a/...search/backend/elasticsearch/search/sort/impl/AbstractElasticsearchSearchSortBuilder.java b/...search/backend/elasticsearch/search/sort/impl/AbstractElasticsearchSearchSortBuilder.java
@@ -9,7 +9,6 @@
 import org.hibernate.search.backend.elasticsearch.gson.impl.JsonAccessor;
 import org.hibernate.search.engine.search.dsl.sort.SortOrder;
 import org.hibernate.search.engine.search.sort.spi.SearchSortBuilder;
-import org.hibernate.search.util.AssertionFailure;
 
 import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
@@ -26,9 +25,7 @@ abstract class AbstractElasticsearchSearchSortBuilder implements SearchSortBuild
 	private static final JsonPrimitive ASC_KEYWORD_JSON = new JsonPrimitive( "asc" );
 	private static final JsonPrimitive DESC_KEYWORD_JSON = new JsonPrimitive( "desc" );
 
-	private final JsonObject innerObject = new JsonObject();
-
-	private boolean built;
+	private SortOrder order;
 
 	@Override
 	public ElasticsearchSearchSortBuilder toImplementation() {
@@ -37,32 +34,25 @@ public ElasticsearchSearchSortBuilder toImplementation() {
 
 	@Override
 	public void order(SortOrder order) {
-		switch ( order ) {
-			case ASC:
-				ORDER.set( getInnerObject(), ASC_KEYWORD_JSON );
-				break;
-			case DESC:
-				ORDER.set( getInnerObject(), DESC_KEYWORD_JSON );
-				break;
-		}
+		this.order = order;
 	}
 
 	@Override
 	public final void buildAndAddTo(ElasticsearchSearchSortCollector collector) {
-		if ( built ) {
-			// we must never call a builder twice. Building may have side-effects.
-			throw new AssertionFailure(
-					"A sort builder was called twice. There is a bug in Hibernate Search, please report it."
-			);
+		JsonObject innerObject = new JsonObject();
+		if ( order != null ) {
+			switch ( order ) {
+				case ASC:
+					ORDER.set( innerObject, ASC_KEYWORD_JSON );
+					break;
+				case DESC:
+					ORDER.set( innerObject, DESC_KEYWORD_JSON );
+					break;
+			}
 		}
-		built = true;
-		doBuildAndAddTo( collector );
-	}
-
-	protected final JsonObject getInnerObject() {
-		return innerObject;
+		doBuildAndAddTo( collector, innerObject );
 	}
 
-	protected abstract void doBuildAndAddTo(ElasticsearchSearchSortCollector collector);
+	protected abstract void doBuildAndAddTo(ElasticsearchSearchSortCollector collector, JsonObject innerObject);
 
 }
diff --git a/...rnate/search/backend/elasticsearch/search/sort/impl/ElasticsearchDistanceSortBuilder.java b/...rnate/search/backend/elasticsearch/search/sort/impl/ElasticsearchDistanceSortBuilder.java
@@ -28,11 +28,11 @@ public ElasticsearchDistanceSortBuilder(String absoluteFieldPath, GeoPoint locat
 	}
 
 	@Override
-	protected void doBuildAndAddTo(ElasticsearchSearchSortCollector collector) {
-		getInnerObject().add( absoluteFieldPath, ElasticsearchGeoPointFieldCodec.INSTANCE.encode( location ) );
+	protected void doBuildAndAddTo(ElasticsearchSearchSortCollector collector, JsonObject innerObject) {
+		innerObject.add( absoluteFieldPath, ElasticsearchGeoPointFieldCodec.INSTANCE.encode( location ) );
 
 		JsonObject outerObject = new JsonObject();
-		GEO_DISTANCE.add( outerObject, getInnerObject() );
+		GEO_DISTANCE.add( outerObject, innerObject );
 		collector.collectDistanceSort( outerObject, absoluteFieldPath, location );
 	}
 }
diff --git a/...ibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchFieldSortBuilder.java b/...ibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchFieldSortBuilder.java
@@ -36,6 +36,8 @@ public class ElasticsearchFieldSortBuilder<F> extends AbstractElasticsearchSearc
 	private final ToDocumentFieldValueConverter<?, ? extends F> dslToIndexConverter;
 	private final ElasticsearchFieldCodec<F> codec;
 
+	private JsonElement missing;
+
 	public ElasticsearchFieldSortBuilder(ElasticsearchSearchContext searchContext,
 			String absoluteFieldPath,
 			ToDocumentFieldValueConverter<?, ? extends F> dslToIndexConverter,
@@ -48,32 +50,33 @@ public ElasticsearchFieldSortBuilder(ElasticsearchSearchContext searchContext,
 
 	@Override
 	public void missingFirst() {
-		MISSING.set( getInnerObject(), MISSING_FIRST_KEYWORD_JSON );
+		this.missing = MISSING_FIRST_KEYWORD_JSON;
 	}
 
 	@Override
 	public void missingLast() {
-		MISSING.set( getInnerObject(), MISSING_LAST_KEYWORD_JSON );
+		this.missing = MISSING_LAST_KEYWORD_JSON;
 	}
 
 	@Override
 	public void missingAs(Object value) {
-		JsonElement element;
 		try {
 			F converted = dslToIndexConverter.convertUnknown( value, searchContext.getToDocumentFieldValueConvertContext() );
-			element = codec.encode( converted );
+			this.missing = codec.encode( converted );
 		}
 		catch (RuntimeException e) {
 			throw log.cannotConvertDslParameter(
 					e.getMessage(), e, EventContexts.fromIndexFieldAbsolutePath( absoluteFieldPath )
 			);
 		}
-		MISSING.set( getInnerObject(), element );
 	}
 
 	@Override
-	public void doBuildAndAddTo(ElasticsearchSearchSortCollector collector) {
-		JsonObject innerObject = getInnerObject();
+	public void doBuildAndAddTo(ElasticsearchSearchSortCollector collector, JsonObject innerObject) {
+		if ( missing != null ) {
+			MISSING.set( innerObject, missing );
+		}
+
 		if ( innerObject.size() == 0 ) {
 			collector.collectSort( new JsonPrimitive( absoluteFieldPath ) );
 		}

diff --git a/...ibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchScoreSortBuilder.java b/...ibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchScoreSortBuilder.java
@@ -18,8 +18,7 @@ class ElasticsearchScoreSortBuilder extends AbstractElasticsearchSearchSortBuild
 	private static final JsonPrimitive SCORE_SORT_KEYWORD_JSON = new JsonPrimitive( SCORE_SORT_KEYWORD );
 
 	@Override
-	public void doBuildAndAddTo(ElasticsearchSearchSortCollector collector) {
-		JsonObject innerObject = getInnerObject();
+	public void doBuildAndAddTo(ElasticsearchSearchSortCollector collector, JsonObject innerObject) {
 		if ( innerObject.size() == 0 ) {
 			collector.collectSort( SCORE_SORT_KEYWORD_JSON );
 		}

diff --git a/.../org/hibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchSearchSort.java b/.../org/hibernate/search/backend/elasticsearch/search/sort/impl/ElasticsearchSearchSort.java
@@ -6,21 +6,23 @@
  */
 package org.hibernate.search.backend.elasticsearch.search.sort.impl;
 
-import org.hibernate.search.engine.search.SearchSort;
+import java.util.List;
 
-import com.google.gson.JsonArray;
+import org.hibernate.search.engine.search.SearchSort;
 
 class ElasticsearchSearchSort implements SearchSort, ElasticsearchSearchSortBuilder {
 
-	private final JsonArray jsonArray;
+	private final List<ElasticsearchSearchSortBuilder> delegates;
 
-	ElasticsearchSearchSort(JsonArray jsonArray) {
-		this.jsonArray = jsonArray;
+	ElasticsearchSearchSort(List<ElasticsearchSearchSortBuilder> delegates) {
+		this.delegates = delegates;
 	}
 
 	@Override
 	public void buildAndAddTo(ElasticsearchSearchSortCollector collector) {
-		collector.collectSort( jsonArray );
+		for ( ElasticsearchSearchSortBuilder delegate : delegates ) {
+			delegate.buildAndAddTo( collector );
+		}
 	}
 
 }
diff --git a/...rch/backend/elasticsearch/search/sort/impl/ElasticsearchSearchSortBuilderFactoryImpl.java b/...rch/backend/elasticsearch/search/sort/impl/ElasticsearchSearchSortBuilderFactoryImpl.java
@@ -13,7 +13,6 @@
 import org.hibernate.search.backend.elasticsearch.document.model.impl.ElasticsearchIndexSchemaFieldNode;
 import org.hibernate.search.backend.elasticsearch.logging.impl.Log;
 import org.hibernate.search.backend.elasticsearch.search.impl.ElasticsearchSearchContext;
-import org.hibernate.search.backend.elasticsearch.search.impl.ElasticsearchSearchQueryElementCollector;
 import org.hibernate.search.backend.elasticsearch.search.impl.ElasticsearchSearchTargetModel;
 import org.hibernate.search.backend.elasticsearch.search.impl.IndexSchemaFieldNodeComponentRetrievalStrategy;
 import org.hibernate.search.backend.elasticsearch.types.sort.impl.ElasticsearchFieldSortBuilderFactory;
@@ -51,12 +50,7 @@ public ElasticsearchSearchSortBuilderFactoryImpl(ElasticsearchSearchContext sear
 
 	@Override
 	public SearchSort toSearchSort(List<ElasticsearchSearchSortBuilder> builders) {
-		// FIXME we should not pass null here... it works but it's dodgy.
-		ElasticsearchSearchQueryElementCollector collector = new ElasticsearchSearchQueryElementCollector( null );
-		for ( ElasticsearchSearchSortBuilder builder : builders ) {
-			builder.buildAndAddTo( collector );
-		}
-		return new ElasticsearchSearchSort( collector.toJsonSort() );
+		return new ElasticsearchSearchSort( builders );
 	}
 
 	@Override