Skip to content

Commit 05a65f9

Browse files
marko-bekhtamarko-bekhta
committed
HSEARCH-5255 Change how projection cardinality check is done on "request"
1 parent fce7e19 commit 05a65f9

File tree

9 files changed

+253
-10
lines changed

9 files changed

+253
-10
lines changed

‎backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/projection/impl/ElasticsearchFieldProjection.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,7 @@ public String toString() {
8181
@Override
8282
public ValueFieldExtractor<?> request(JsonObject requestBody, ProjectionRequestContext context) {
8383
ProjectionRequestContext innerContext = context.forField( absoluteFieldPath, absoluteFieldPathComponents );
84-
if ( requiredContextAbsoluteFieldPath != null
85-
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentFieldPath() ) ) {
84+
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
8685
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
8786
absoluteFieldPath, requiredContextAbsoluteFieldPath );
8887
}

‎backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/projection/impl/FieldProjectionRequestContext.java

+12
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,16 @@ public NamedValues queryParameters() {
8686
return root().queryParameters();
8787
}
8888

89+
@Override
90+
public boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath) {
91+
// requiredContextAbsoluteFieldPath generally speaking should be the path from root to the closest multi-valued field
92+
// (i.e. the very last multi-valued field in the current path)
93+
//
94+
// Hence if we have the context that includes that path or matches it exactly then we can assume that the cardinality was correctly handled:
95+
return requiredContextAbsoluteFieldPath == null
96+
|| requiredContextAbsoluteFieldPath.equals( absoluteCurrentFieldPath )
97+
|| ( absoluteCurrentFieldPath != null
98+
&& absoluteCurrentFieldPath.startsWith( requiredContextAbsoluteFieldPath + "." ) );
99+
}
100+
89101
}

‎backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/projection/impl/ProjectionRequestContext.java

+1
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,5 @@ public interface ProjectionRequestContext {
2323

2424
NamedValues queryParameters();
2525

26+
boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath);
2627
}

‎backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/query/impl/ElasticsearchSearchQueryRequestContext.java

+5
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,11 @@ public NamedValues queryParameters() {
126126
return parameters;
127127
}
128128

129+
@Override
130+
public boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath) {
131+
return requiredContextAbsoluteFieldPath == null;
132+
}
133+
129134
@Override
130135
public ElasticsearchSearchHighlighter highlighter(String highlighterName) {
131136
if ( highlighterName == null ) {

‎backend/lucene/src/main/java/org/hibernate/search/backend/lucene/search/projection/impl/LuceneDistanceToFieldProjection.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,7 @@ public Extractor<?, P> request(ProjectionRequestContext context) {
9494
}
9595
else {
9696
context.checkValidField( absoluteFieldPath );
97-
if ( requiredContextAbsoluteFieldPath != null
98-
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
97+
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
9998
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
10099
absoluteFieldPath, requiredContextAbsoluteFieldPath );
101100
}

‎backend/lucene/src/main/java/org/hibernate/search/backend/lucene/search/projection/impl/LuceneFieldProjection.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,7 @@ public String toString() {
7777
@Override
7878
public ValueFieldExtractor<?> request(ProjectionRequestContext context) {
7979
context.checkValidField( absoluteFieldPath );
80-
if ( requiredContextAbsoluteFieldPath != null
81-
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
80+
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
8281
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
8382
absoluteFieldPath, requiredContextAbsoluteFieldPath );
8483
}

‎backend/lucene/src/main/java/org/hibernate/search/backend/lucene/search/projection/impl/LuceneObjectProjection.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,7 @@ public String toString() {
7676
@Override
7777
public Extractor<?, P> request(ProjectionRequestContext context) {
7878
ProjectionRequestContext innerContext = context.forField( absoluteFieldPath, nested );
79-
if ( requiredContextAbsoluteFieldPath != null
80-
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
79+
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
8180
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
8281
absoluteFieldPath, requiredContextAbsoluteFieldPath );
8382
}

‎backend/lucene/src/main/java/org/hibernate/search/backend/lucene/search/projection/impl/ProjectionRequestContext.java

+9
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ public String absoluteCurrentNestedFieldPath() {
9595
return absoluteCurrentNestedFieldPath;
9696
}
9797

98+
public boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath) {
99+
String absoluteCurrentNestedFieldPath = absoluteCurrentNestedFieldPath();
100+
return requiredContextAbsoluteFieldPath == null
101+
|| requiredContextAbsoluteFieldPath.equals( absoluteCurrentNestedFieldPath )
102+
|| ( absoluteCurrentNestedFieldPath != null
103+
&& absoluteCurrentNestedFieldPath.startsWith( requiredContextAbsoluteFieldPath + "." ) );
104+
105+
}
106+
98107
public String absoluteCurrentFieldPath() {
99108
return absoluteCurrentFieldPath;
100109
}

‎integrationtest/backend/tck/src/main/java/org/hibernate/search/integrationtest/backend/tck/search/projection/FieldProjectionBaseIT.java

+222-2
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,19 @@
44
*/
55
package org.hibernate.search.integrationtest.backend.tck.search.projection;
66

7+
import static org.assertj.core.api.Assertions.assertThat;
8+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
9+
710
import java.math.BigDecimal;
811
import java.util.ArrayList;
912
import java.util.Comparator;
1013
import java.util.List;
1114

15+
import org.hibernate.search.engine.backend.document.DocumentElement;
16+
import org.hibernate.search.engine.backend.document.model.dsl.IndexSchemaElement;
17+
import org.hibernate.search.engine.backend.document.model.dsl.IndexSchemaObjectField;
1218
import org.hibernate.search.engine.backend.types.ObjectStructure;
19+
import org.hibernate.search.engine.backend.types.Projectable;
1320
import org.hibernate.search.engine.backend.types.dsl.SearchableProjectableIndexFieldTypeOptionsStep;
1421
import org.hibernate.search.engine.search.projection.ProjectionCollector;
1522
import org.hibernate.search.engine.search.projection.dsl.ProjectionFinalStep;
@@ -18,11 +25,13 @@
1825
import org.hibernate.search.integrationtest.backend.tck.testsupport.types.StandardFieldTypeDescriptor;
1926
import org.hibernate.search.integrationtest.backend.tck.testsupport.util.TckConfiguration;
2027
import org.hibernate.search.integrationtest.backend.tck.testsupport.util.extension.SearchSetupHelper;
28+
import org.hibernate.search.util.common.SearchException;
2129
import org.hibernate.search.util.impl.integrationtest.mapper.stub.BulkIndexer;
2230
import org.hibernate.search.util.impl.integrationtest.mapper.stub.SimpleMappedIndex;
2331

2432
import org.junit.jupiter.api.BeforeAll;
2533
import org.junit.jupiter.api.Nested;
34+
import org.junit.jupiter.api.Test;
2635
import org.junit.jupiter.api.extension.RegisterExtension;
2736
import org.junit.jupiter.params.provider.Arguments;
2837

@@ -53,7 +62,9 @@ static void setup() {
5362
InObjectProjectionConfigured.missingLevel2SingleValuedFieldIndex,
5463
InvalidFieldConfigured.index,
5564
ProjectableConfigured.projectableDefaultIndex, ProjectableConfigured.projectableYesIndex,
56-
ProjectableConfigured.projectableNoIndex )
65+
ProjectableConfigured.projectableNoIndex,
66+
CardinalityCheckConfigured.index,
67+
CardinalityCheckConfigured.containing )
5768
.setup();
5869

5970
BulkIndexer compositeForEachMainIndexer = InObjectProjectionConfigured.mainIndex.bulkIndexer();
@@ -74,7 +85,12 @@ static void setup() {
7485

7586
compositeForEachMainIndexer.join( compositeForEachMissingLevel1Indexer,
7687
compositeForEachMissingLevel1SingleValuedFieldIndexer, compositeForEachMissingLevel2Indexer,
77-
compositeForEachMissingLevel2SingleValuedFieldIndexer );
88+
compositeForEachMissingLevel2SingleValuedFieldIndexer,
89+
CardinalityCheckConfigured.containing.binding()
90+
.contribute( CardinalityCheckConfigured.containing.bulkIndexer() ),
91+
CardinalityCheckConfigured.index.binding()
92+
.contribute( CardinalityCheckConfigured.index.bulkIndexer() )
93+
);
7894
}
7995

8096
@Nested
@@ -211,4 +227,208 @@ protected String projectionTrait() {
211227
return "projection:field";
212228
}
213229
}
230+
231+
@Nested
232+
class CardinalityCheckIT extends CardinalityCheckConfigured {
233+
// JDK 11 does not allow static fields in non-static inner class and JUnit does not allow running @Nested tests in static inner classes...
234+
}
235+
236+
abstract static class CardinalityCheckConfigured {
237+
private static final SimpleMappedIndex<IndexContaining> containing =
238+
SimpleMappedIndex.of( IndexContaining::new ).name( "cardinality-containing" );
239+
private static final SimpleMappedIndex<Index> index = SimpleMappedIndex.of( Index::new ).name( "cardinality-index" );
240+
241+
/**
242+
* In this case we build a projection with object/fields projections recreating the entity tree structure.
243+
* Hence, it should be close to what a projection constructor is doing.
244+
*/
245+
@Test
246+
void testAsIfItWasProjectionConstructors() {
247+
List<List<?>> hits = index.createScope().query()
248+
.select( f -> f.composite(
249+
f.id(),
250+
f.field( "string" ),
251+
f.object( "contained" ).from(
252+
f.field( "contained.id" ),
253+
f.field( "contained.containedString" ),
254+
f.object( "contained.deeperContained" ).from(
255+
f.field( "contained.deeperContained.id" ),
256+
f.field( "contained.deeperContained.deeperContainedString" )
257+
).asList()
258+
).asList().list()
259+
) )
260+
.where( f -> f.matchAll() )
261+
.fetchAllHits();
262+
263+
assertThat( hits ).hasSize( 1 )
264+
.contains(
265+
List.of(
266+
"1",
267+
"string",
268+
List.of(
269+
List.of( "10", "containedString1",
270+
List.of( "100", "deeperContainedString1" ) ),
271+
List.of( "20", "containedString2",
272+
List.of( "200", "deeperContainedString2" ) )
273+
)
274+
)
275+
);
276+
}
277+
278+
/**
279+
* We want to make sure that cardinality is correctly checked when we request a projection for a single field with a "long" path
280+
* containing both single and multi fields in it.
281+
* <p>
282+
* Since there is a multi-`contained` the resulting caridnality of the projected field is expected to be also multi
283+
*/
284+
@Test
285+
void testFieldProjectionLongPath_correctCardinality() {
286+
List<List<Object>> hits = index.createScope().query()
287+
.select( f -> f.field( "contained.deeperContained.id" ).list() )
288+
.where( f -> f.matchAll() )
289+
.fetchAllHits();
290+
291+
assertThat( hits ).hasSize( 1 )
292+
.contains( List.of( "100", "200" ) );
293+
}
294+
295+
@Test
296+
void testFieldProjectionLongPath_incorrectCardinality() {
297+
assertThatThrownBy( () -> index.createScope().query()
298+
.select( f -> f.field( "contained.deeperContained.id" ) )
299+
.where( f -> f.matchAll() )
300+
.fetchAllHits() )
301+
.isInstanceOf( SearchException.class )
302+
.hasMessageContaining( "Invalid cardinality for projection on field 'contained.deeperContained.id'" );
303+
}
304+
305+
@Test
306+
void testFieldProjectionLongPath_correctCardinality_multiAtDifferentLevelInPath() {
307+
assertThat( containing.createScope().query()
308+
.select( f -> f.field( "single.contained.deeperContained.id" ).list() )
309+
.where( f -> f.matchAll() )
310+
.fetchAllHits()
311+
).hasSize( 1 )
312+
.contains( List.of( "100", "200" ) );
313+
}
314+
315+
@Test
316+
void testFieldProjectionLongPath_correctCardinality_multiAtDifferentLevelInPath_multipleMultis() {
317+
assertThat( containing.createScope().query()
318+
.select( f -> f.field( "list.contained.deeperContained.id" ).list() )
319+
.where( f -> f.matchAll() )
320+
.fetchAllHits() ).hasSize( 1 )
321+
.contains( List.of( "100", "200" ) );
322+
}
323+
324+
/**
325+
* Here we take all multi fields as object projections with expected cardinalities and then
326+
* add the "deepest" field as a simple single-valued field:
327+
*/
328+
@Test
329+
void testFieldProjectionLongPath_correctCardinality_multiFieldsAsObjects() {
330+
assertThat( containing.createScope().query()
331+
.select( f -> f.object( "list" )
332+
.from( f.object( "list.contained" )
333+
.from( f.field( "list.contained.deeperContained.id" ) ).asList().list() )
334+
.asList().list() )
335+
.where( f -> f.matchAll() )
336+
.fetchAllHits() ).hasSize( 1 )
337+
.contains(
338+
List.of( List.of( List.of( List.of( "100" ), List.of( "200" ) ) ) ) );
339+
}
340+
341+
@Test
342+
void testFieldProjectionLongPath_incorrectCardinality_multiAtDifferentLevelInPath_multipleMultis() {
343+
assertThatThrownBy( () -> containing.createScope().query()
344+
.select( f -> f.field( "list.contained.deeperContained.id" ) )
345+
.where( f -> f.matchAll() )
346+
.fetchAllHits() )
347+
.isInstanceOf( SearchException.class )
348+
.hasMessageContaining(
349+
"Invalid cardinality for projection on field 'list.contained.deeperContained.id'" );
350+
}
351+
352+
@Test
353+
void testFieldProjectionLongPath_incorrectCardinality_multiAtDifferentLevelInPath() {
354+
assertThatThrownBy( () -> containing.createScope().query()
355+
.select( f -> f.field( "single.contained.deeperContained.id" ) )
356+
.where( f -> f.matchAll() )
357+
.fetchAllHits() )
358+
.isInstanceOf( SearchException.class )
359+
.hasMessageContaining(
360+
"Invalid cardinality for projection on field 'single.contained.deeperContained.id'" );
361+
}
362+
363+
public static final class IndexContaining extends IndexBase {
364+
public IndexContaining(IndexSchemaElement root) {
365+
IndexSchemaObjectField single = root.objectField( "single", ObjectStructure.NESTED );
366+
addIndexFields( single );
367+
single.toReference();
368+
369+
IndexSchemaObjectField list = root.objectField( "list", ObjectStructure.NESTED ).multiValued();
370+
addIndexFields( list );
371+
list.toReference();
372+
}
373+
374+
BulkIndexer contribute(BulkIndexer indexer) {
375+
indexer.add( "1", d -> {
376+
contribute( d.addObject( "single" ) );
377+
contribute( d.addObject( "list" ) );
378+
} );
379+
return indexer;
380+
}
381+
}
382+
383+
public static class Index extends IndexBase {
384+
385+
public Index(IndexSchemaElement root) {
386+
addIndexFields( root );
387+
}
388+
389+
BulkIndexer contribute(BulkIndexer indexer) {
390+
indexer.add( "1", this::contribute );
391+
return indexer;
392+
}
393+
}
394+
395+
public abstract static class IndexBase {
396+
protected void addIndexFields(IndexSchemaElement el) {
397+
el.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
398+
el.field( "string", f -> f.asString().projectable( Projectable.YES ) ).toReference();
399+
400+
IndexSchemaObjectField contained = el.objectField( "contained", ObjectStructure.NESTED ).multiValued();
401+
contained.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
402+
contained.field( "containedString", f -> f.asString().projectable( Projectable.YES ) ).toReference();
403+
404+
IndexSchemaObjectField deeperContained = contained.objectField( "deeperContained", ObjectStructure.NESTED );
405+
deeperContained.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
406+
deeperContained.field( "deeperContainedString", f -> f.asString().projectable( Projectable.YES ) )
407+
.toReference();
408+
409+
deeperContained.toReference();
410+
contained.toReference();
411+
}
412+
413+
protected void contribute(DocumentElement element) {
414+
element.addValue( "id", "1" );
415+
element.addValue( "string", "string" );
416+
417+
DocumentElement contained = element.addObject( "contained" );
418+
contained.addValue( "id", "10" );
419+
contained.addValue( "containedString", "containedString1" );
420+
DocumentElement deeperContained = contained.addObject( "deeperContained" );
421+
deeperContained.addValue( "id", "100" );
422+
deeperContained.addValue( "deeperContainedString", "deeperContainedString1" );
423+
424+
contained = element.addObject( "contained" );
425+
contained.addValue( "id", "20" );
426+
contained.addValue( "containedString", "containedString2" );
427+
deeperContained = contained.addObject( "deeperContained" );
428+
deeperContained.addValue( "id", "200" );
429+
deeperContained.addValue( "deeperContainedString", "deeperContainedString2" );
430+
431+
}
432+
}
433+
}
214434
}

0 commit comments

Comments
 (0)