HSEARCH-2063 Review the pending TODOs

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/ProjectionConstants.java

@@ -18,5 +18,5 @@ public interface ProjectionConstants extends org.hibernate.search.engine.Project
 	 */
 	String SOURCE = "__HSearch_Source";
 
-	// TODO "Took" etc.?
+	// TODO HSEARCH-2257 add "took" etc.?
 }

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/JestClient.java

@@ -121,7 +121,7 @@ public void executeBulkRequest(List<BackendRequest<?>> actions, boolean refresh)
 		try {
 			BulkResult response = client.execute( request );
 
-			// TODO: Ideally I could just check on the status of the bulk, but for some reason the ES response is not
+			// Ideally I could just check on the status of the bulk, but for some reason the ES response is not
 			// always set to erroneous also if there is an erroneous item; I suppose that's a bug in ES? For now we are
 			// examining the result items and check if there is any erroneous
 			List<BackendRequest<?>> erroneousItems = getErroneousItems( actions, response );

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/impl/ElasticsearchHSQueryImpl.java

@@ -296,9 +296,11 @@ private IndexSearcher() {
 						);
 					}
 
-					// TODO will this be a problem when querying multiple entity types, with one using a field as id
-					// field and the other not; is that possible?
+					// TODO HSEARCH-2253 the id field name is used to detect if we should sort using the internal Elasticsearch id field
+					// as it's currently the only field in which we store the id of the entity.
+					// Thus the id fields must be consistent accross all the entity types when querying multiple ones.
 					idFieldName = binding.getDocumentBuilder().getIdentifierName();
+
 					ElasticsearchIndexManager esIndexManager = (ElasticsearchIndexManager) indexManager;
 					indexNames.add( esIndexManager.getActualIndexName() );
 				}
@@ -307,7 +309,6 @@ private IndexSearcher() {
 			}
 
 			// Query filters; always a type filter, possibly a tenant id filter;
-			// TODO feed in user-provided filters
 			JsonObject effectiveFilter = getEffectiveFilter( typeFilters );
 
 			JsonBuilder.Object completeQuery = JsonBuilder.object();
@@ -341,7 +342,7 @@ private IndexSearcher() {
 			// given, I take as much as possible, as by default only 10 rows would be returned
 			search.setParameter( "size", maxResults != null ? maxResults : MAX_RESULT_WINDOW_SIZE - firstResult );
 
-			// TODO: embedded fields (see https://github.com/searchbox-io/Jest/issues/304)
+			// TODO: HSEARCH-2254 embedded fields (see https://github.com/searchbox-io/Jest/issues/304)
 			if ( sort != null ) {
 				validateSortFields( extendedIntegrator, queriedEntityTypes );
 				for ( SortField sortField : sort.getSort() ) {
@@ -620,7 +621,7 @@ else if ( fieldBridge instanceof TwoWayFieldBridge ) {
 			}
 			// Should only be the case for custom bridges
 			else {
-				// TODO: should we do it?
+				// TODO: HSEARCH-2255 should we do it?
 				if ( !value.isJsonPrimitive() ) {
 					throw new SearchException( "Projection of non-JSON-primitive field values is not supported: " + value );
 				}
@@ -631,15 +632,15 @@ else if ( fieldBridge instanceof TwoWayFieldBridge ) {
 					return primitive.getAsBoolean();
 				}
 				else if ( primitive.isNumber() ) {
-					// TODO this will expose a Gson-specific Number implementation; Can we somehow return an Integer,
+					// TODO HSEARCH-2255 this will expose a Gson-specific Number implementation; Can we somehow return an Integer,
 					// Long... etc. instead?
 					return primitive.getAsNumber();
 				}
 				else if ( primitive.isString() ) {
 					return primitive.getAsString();
 				}
 				else {
-					// TODO Better raise an exception?
+					// TODO HSEARCH-2255 Better raise an exception?
 					return primitive.toString();
 				}
 			}
@@ -740,7 +741,7 @@ private List<Facet> updateStringFacets(JsonObject aggregations, DiscreteFacetReq
 	}
 
 	private boolean isNested(DiscreteFacetRequest facetRequest) {
-		//TODO Drive through meta-data
+		//TODO HSEARCH-2097 Drive through meta-data
 //		return FieldHelper.isEmbeddedField( facetRequest.getFieldName() );
 		return false;
 	}
@@ -757,7 +758,7 @@ private boolean isPartOfProjectedFields(String projectionName) {
 		return false;
 	}
 
-	// TODO: Investigate scrolling API:
+	// TODO: HSEARCH-2189  Investigate scrolling API:
 	// https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-scroll.html
 	private class ElasticsearchDocumentExtractor implements DocumentExtractor {
 

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/impl/ElasticsearchIndexManager.java

@@ -21,6 +21,7 @@
 import org.hibernate.search.backend.BackendFactory;
 import org.hibernate.search.backend.IndexingMonitor;
 import org.hibernate.search.backend.LuceneWork;
+import org.hibernate.search.backend.OptimizeLuceneWork;
 import org.hibernate.search.backend.elasticsearch.cfg.ElasticsearchEnvironment;
 import org.hibernate.search.backend.elasticsearch.cfg.IndexManagementStrategy;
 import org.hibernate.search.backend.elasticsearch.client.impl.JestClient;
@@ -264,7 +265,7 @@ private void deleteIndexIfExisting() {
 		}
 	}
 
-	// TODO
+	// TODO HSEARCH-2260
 	// What happens if mappings already exist? We need an option similar to hbm2ddl
 	// What happens if several nodes in a cluster try to create the mappings?
 	private void createIndexMappings() {
@@ -276,9 +277,8 @@ private void createIndexMappings() {
 			JsonObject properties = new JsonObject();
 			payload.add( "properties", properties );
 
-			// Add field for tenant id
-			// TODO At this point we don't know yet whether it's actually going to be needed
-			// Should we make this configurable?
+			// Add field for tenant id though we don't know at this point if it's actually going to be needed.
+			// TODO HSEARCH-2256 Should we make this configurable?
 			JsonObject field = new JsonObject();
 			field.addProperty( "type", "string" );
 			field.addProperty( "index", "not_analyzed" );
@@ -355,7 +355,7 @@ private void addFieldMapping(JsonObject payload, EntityIndexBinding descriptor,
 		}
 
 		if ( fieldMetadata.indexNullAs() != null ) {
-			// TODO Validate the type; Supported types are converted transparently by ES
+			// TODO HSEARCH-2262 Validate the type; Supported types are converted transparently by ES
 			field.addProperty( "null_value", fieldMetadata.indexNullAs() );
 		}
 
@@ -552,7 +552,7 @@ private JsonObject getOrCreateProperties(JsonObject mapping, String fieldName) {
 			if ( property == null ) {
 				property = new JsonObject();
 
-				// TODO enable nested mapping as needed:
+				// TODO HSEARCH-2263 enable nested mapping as needed:
 				// * only needed for embedded *-to-many with more than one field
 				// * for these, the user should be able to opt out (nested would be the safe default mapping in this
 				// case, but they could want to opt out when only ever querying on single fields of the embeddable)
@@ -666,7 +666,7 @@ public void performStreamOperation(LuceneWork singleOperation, IndexingMonitor m
 
 	@Override
 	public void optimize() {
-		// TODO Is there such thing for ES?
+		performStreamOperation( OptimizeLuceneWork.INSTANCE, null, false );
 	}
 
 	@Override

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/impl/ElasticsearchIndexWorkVisitor.java

@@ -95,15 +95,13 @@ public BackendRequest<?> visitDeleteWork(DeleteLuceneWork work, Boolean refresh)
 
 	@Override
 	public BackendRequest<?> visitOptimizeWork(OptimizeLuceneWork work, Boolean refresh) {
-		// TODO implement
+		// TODO HSEARCH-2092 implement
 		LOG.warn( "Optimize work is not yet supported for Elasticsearch backend, ignoring it" );
 		return null;
 	}
 
 	@Override
 	public BackendRequest<?> visitPurgeAllWork(PurgeAllLuceneWork work, Boolean refresh) {
-		// TODO This requires the delete-by-query plug-in on ES 2.0 and beyond; Alternatively
-		// the type mappings could be deleted, think about implications for concurrent access
 		String query = work.getTenantId() != null ?
 				String.format( Locale.ENGLISH, DELETE_ALL_FOR_TENANT_QUERY, work.getTenantId() ) :
 				DELETE_ALL_QUERY;
@@ -266,7 +264,7 @@ else if ( FieldHelper.isBoolean( indexBinding, field.name() ) ) {
 					addPropertyOfPotentiallyMultipleCardinality( parent, jsonPropertyName,
 							value != null ? new JsonPrimitive( value ) : null );
 				}
-				// TODO falling back for now to checking actual Field type to cover numeric fields created by custom
+				// TODO HSEARCH-2261 falling back for now to checking actual Field type to cover numeric fields created by custom
 				// bridges
 				else if ( FieldHelper.isNumeric( documentFieldMetadata ) || isNumeric( field ) ) {
 					// Explicitly propagate null in case value is not given and let ES handle the default token set

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/impl/FieldHelper.java

@@ -37,7 +37,7 @@ class FieldHelper {
 	private FieldHelper() {
 	}
 
-	// TODO make it work with fields embedded types
+	// TODO HSEARCH-2259 make it work with fields embedded types
 	static NumericEncodingType getNumericEncodingType(EntityIndexBinding indexBinding, DocumentFieldMetadata field) {
 		NumericEncodingType numericEncodingType = field.getNumericEncodingType();
 

elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/impl/IndexNameNormalizer.java

@@ -21,8 +21,8 @@ private IndexNameNormalizer() {
 	public static String getElasticsearchIndexName(String indexName) {
 		String esIndexName = indexName.toLowerCase( Locale.ENGLISH );
 		if ( !esIndexName.equals( indexName ) ) {
-			// TODO LOG
-			// TODO: if index lowercasing introduces a possible ambiguity in the ES case, maybe we should validate for this
+			// TODO LOG (not sure a log is useful if we validate everything at bootstrap.)
+			// TODO HSEARCH-2258 if index lowercasing introduces a possible ambiguity in the ES case, maybe we should validate for this
 			// at the root of all IndexManagers during bootstrap?
 		}
 

elasticsearch/src/test/java/org/hibernate/search/backend/elasticsearch/test/ElasticsearchNullValueIT.java

@@ -64,7 +64,6 @@ public void deleteTestData() {
 		FullTextSession session = Search.getFullTextSession( s );
 		Transaction tx = s.beginTransaction();
 
-		//TODO verify this is no longer needed after we implement the delete operations
 		QueryDescriptor query = ElasticsearchQueries.fromJson( "{ 'query': { 'match_all' : {} } }" );
 		List<?> result = session.createFullTextQuery( query ).list();
 

elasticsearch/src/test/java/org/hibernate/search/backend/elasticsearch/test/ElasticsearchSpatialIT.java

@@ -59,7 +59,6 @@ public void deleteTestData() {
 		FullTextSession session = Search.getFullTextSession( s );
 		Transaction tx = s.beginTransaction();
 
-		//TODO verify this is no longer needed after we implement the delete operations
 		QueryDescriptor query = ElasticsearchQueries.fromJson( "{ 'query': { 'match_all' : {} } }" );
 		List<?> result = session.createFullTextQuery( query ).list();