Skip to content

Commit

Permalink
HSEARCH-5255 Change how projection cardinality check is done on "requ…
Browse files Browse the repository at this point in the history
…est"
  • Loading branch information
marko-bekhta committed Nov 21, 2024
1 parent 581d792 commit 8c1ddde
Show file tree
Hide file tree
Showing 9 changed files with 253 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ public String toString() {
@Override
public ValueFieldExtractor<?> request(JsonObject requestBody, ProjectionRequestContext context) {
ProjectionRequestContext innerContext = context.forField( absoluteFieldPath, absoluteFieldPathComponents );
if ( requiredContextAbsoluteFieldPath != null
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentFieldPath() ) ) {
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
absoluteFieldPath, requiredContextAbsoluteFieldPath );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,16 @@ public NamedValues queryParameters() {
return root().queryParameters();
}

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ public interface ProjectionRequestContext {

NamedValues queryParameters();

boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath);
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ public NamedValues queryParameters() {
return parameters;
}

@Override
public boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath) {
return requiredContextAbsoluteFieldPath == null;
}

@Override
public ElasticsearchSearchHighlighter highlighter(String highlighterName) {
if ( highlighterName == null ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ public Extractor<?, P> request(ProjectionRequestContext context) {
}
else {
context.checkValidField( absoluteFieldPath );
if ( requiredContextAbsoluteFieldPath != null
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
absoluteFieldPath, requiredContextAbsoluteFieldPath );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ public String toString() {
@Override
public ValueFieldExtractor<?> request(ProjectionRequestContext context) {
context.checkValidField( absoluteFieldPath );
if ( requiredContextAbsoluteFieldPath != null
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
absoluteFieldPath, requiredContextAbsoluteFieldPath );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ public String toString() {
@Override
public Extractor<?, P> request(ProjectionRequestContext context) {
ProjectionRequestContext innerContext = context.forField( absoluteFieldPath, nested );
if ( requiredContextAbsoluteFieldPath != null
&& !requiredContextAbsoluteFieldPath.equals( context.absoluteCurrentNestedFieldPath() ) ) {
if ( !context.projectionCardinalityCorrectlyAddressed( requiredContextAbsoluteFieldPath ) ) {
throw log.invalidSingleValuedProjectionOnValueFieldInMultiValuedObjectField(
absoluteFieldPath, requiredContextAbsoluteFieldPath );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ public String absoluteCurrentNestedFieldPath() {
return absoluteCurrentNestedFieldPath;
}

public boolean projectionCardinalityCorrectlyAddressed(String requiredContextAbsoluteFieldPath) {
String absoluteCurrentNestedFieldPath = absoluteCurrentNestedFieldPath();
return requiredContextAbsoluteFieldPath == null
|| requiredContextAbsoluteFieldPath.equals( absoluteCurrentNestedFieldPath )
|| ( absoluteCurrentNestedFieldPath != null
&& absoluteCurrentNestedFieldPath.startsWith( requiredContextAbsoluteFieldPath + "." ) );

}

public String absoluteCurrentFieldPath() {
return absoluteCurrentFieldPath;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,33 @@
*/
package org.hibernate.search.integrationtest.backend.tck.search.projection;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;

import org.hibernate.search.engine.backend.document.DocumentElement;
import org.hibernate.search.engine.backend.document.model.dsl.IndexSchemaElement;
import org.hibernate.search.engine.backend.document.model.dsl.IndexSchemaObjectField;
import org.hibernate.search.engine.backend.types.ObjectStructure;
import org.hibernate.search.engine.backend.types.Projectable;
import org.hibernate.search.engine.backend.types.dsl.SearchableProjectableIndexFieldTypeOptionsStep;
import org.hibernate.search.engine.search.projection.dsl.ProjectionFinalStep;
import org.hibernate.search.engine.search.projection.dsl.SearchProjectionFactory;
import org.hibernate.search.integrationtest.backend.tck.testsupport.types.FieldTypeDescriptor;
import org.hibernate.search.integrationtest.backend.tck.testsupport.types.StandardFieldTypeDescriptor;
import org.hibernate.search.integrationtest.backend.tck.testsupport.util.TckConfiguration;
import org.hibernate.search.integrationtest.backend.tck.testsupport.util.extension.SearchSetupHelper;
import org.hibernate.search.util.common.SearchException;
import org.hibernate.search.util.impl.integrationtest.mapper.stub.BulkIndexer;
import org.hibernate.search.util.impl.integrationtest.mapper.stub.SimpleMappedIndex;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.provider.Arguments;

Expand Down Expand Up @@ -52,7 +61,9 @@ static void setup() {
InObjectProjectionConfigured.missingLevel2SingleValuedFieldIndex,
InvalidFieldConfigured.index,
ProjectableConfigured.projectableDefaultIndex, ProjectableConfigured.projectableYesIndex,
ProjectableConfigured.projectableNoIndex )
ProjectableConfigured.projectableNoIndex,
CardinalityCheckConfigured.index,
CardinalityCheckConfigured.containing )
.setup();

BulkIndexer compositeForEachMainIndexer = InObjectProjectionConfigured.mainIndex.bulkIndexer();
Expand All @@ -73,7 +84,12 @@ static void setup() {

compositeForEachMainIndexer.join( compositeForEachMissingLevel1Indexer,
compositeForEachMissingLevel1SingleValuedFieldIndexer, compositeForEachMissingLevel2Indexer,
compositeForEachMissingLevel2SingleValuedFieldIndexer );
compositeForEachMissingLevel2SingleValuedFieldIndexer,
CardinalityCheckConfigured.containing.binding()
.contribute( CardinalityCheckConfigured.containing.bulkIndexer() ),
CardinalityCheckConfigured.index.binding()
.contribute( CardinalityCheckConfigured.index.bulkIndexer() )
);
}

@Nested
Expand Down Expand Up @@ -210,4 +226,208 @@ protected String projectionTrait() {
return "projection:field";
}
}

@Nested
class CardinalityCheckIT extends CardinalityCheckConfigured {
// JDK 11 does not allow static fields in non-static inner class and JUnit does not allow running @Nested tests in static inner classes...
}

abstract static class CardinalityCheckConfigured {
private static final SimpleMappedIndex<IndexContaining> containing =
SimpleMappedIndex.of( IndexContaining::new ).name( "cardinality-containing" );
private static final SimpleMappedIndex<Index> index = SimpleMappedIndex.of( Index::new ).name( "cardinality-index" );

/**
* In this case we build a projection with object/fields projections recreating the entity tree structure.
* Hence, it should be close to what a projection constructor is doing.
*/
@Test
void testAsIfItWasProjectionConstructors() {
List<List<?>> hits = index.createScope().query()
.select( f -> f.composite(
f.id(),
f.field( "string" ),
f.object( "contained" ).from(
f.field( "contained.id" ),
f.field( "contained.containedString" ),
f.object( "contained.deeperContained" ).from(
f.field( "contained.deeperContained.id" ),
f.field( "contained.deeperContained.deeperContainedString" )
).asList()
).asList().multi()
) )
.where( f -> f.matchAll() )
.fetchAllHits();

assertThat( hits ).hasSize( 1 )
.contains(
List.of(
"1",
"string",
List.of(
List.of( "10", "containedString1",
List.of( "100", "deeperContainedString1" ) ),
List.of( "20", "containedString2",
List.of( "200", "deeperContainedString2" ) )
)
)
);
}

/**
* We want to make sure that cardinality is correctly checked when we request a projection for a single field with a "long" path
* containing both single and multi fields in it.
* <p>
* Since there is a multi-`contained` the resulting caridnality of the projected field is expected to be also multi
*/
@Test
void testFieldProjectionLongPath_correctCardinality() {
List<List<Object>> hits = index.createScope().query()
.select( f -> f.field( "contained.deeperContained.id" ).multi() )
.where( f -> f.matchAll() )
.fetchAllHits();

assertThat( hits ).hasSize( 1 )
.contains( List.of( "100", "200" ) );
}

@Test
void testFieldProjectionLongPath_incorrectCardinality() {
assertThatThrownBy( () -> index.createScope().query()
.select( f -> f.field( "contained.deeperContained.id" ) )
.where( f -> f.matchAll() )
.fetchAllHits() )
.isInstanceOf( SearchException.class )
.hasMessageContaining( "Invalid cardinality for projection on field 'contained.deeperContained.id'" );
}

@Test
void testFieldProjectionLongPath_correctCardinality_multiAtDifferentLevelInPath() {
assertThat( containing.createScope().query()
.select( f -> f.field( "single.contained.deeperContained.id" ).multi() )
.where( f -> f.matchAll() )
.fetchAllHits()
).hasSize( 1 )
.contains( List.of( "100", "200" ) );
}

@Test
void testFieldProjectionLongPath_correctCardinality_multiAtDifferentLevelInPath_multipleMultis() {
assertThat( containing.createScope().query()
.select( f -> f.field( "list.contained.deeperContained.id" ).multi() )
.where( f -> f.matchAll() )
.fetchAllHits() ).hasSize( 1 )
.contains( List.of( "100", "200" ) );
}

/**
* Here we take all multi fields as object projections with expected cardinalities and then
* add the "deepest" field as a simple single-valued field:
*/
@Test
void testFieldProjectionLongPath_correctCardinality_multiFieldsAsObjects() {
assertThat( containing.createScope().query()
.select( f -> f.object( "list" )
.from( f.object( "list.contained" )
.from( f.field( "list.contained.deeperContained.id" ) ).asList().multi() )
.asList().multi() )
.where( f -> f.matchAll() )
.fetchAllHits() ).hasSize( 1 )
.contains(
List.of( List.of( List.of( List.of( "100" ), List.of( "200" ) ) ) ) );
}

@Test
void testFieldProjectionLongPath_incorrectCardinality_multiAtDifferentLevelInPath_multipleMultis() {
assertThatThrownBy( () -> containing.createScope().query()
.select( f -> f.field( "list.contained.deeperContained.id" ) )
.where( f -> f.matchAll() )
.fetchAllHits() )
.isInstanceOf( SearchException.class )
.hasMessageContaining(
"Invalid cardinality for projection on field 'list.contained.deeperContained.id'" );
}

@Test
void testFieldProjectionLongPath_incorrectCardinality_multiAtDifferentLevelInPath() {
assertThatThrownBy( () -> containing.createScope().query()
.select( f -> f.field( "single.contained.deeperContained.id" ) )
.where( f -> f.matchAll() )
.fetchAllHits() )
.isInstanceOf( SearchException.class )
.hasMessageContaining(
"Invalid cardinality for projection on field 'single.contained.deeperContained.id'" );
}

public static final class IndexContaining extends IndexBase {
public IndexContaining(IndexSchemaElement root) {
IndexSchemaObjectField single = root.objectField( "single", ObjectStructure.NESTED );
addIndexFields( single );
single.toReference();

IndexSchemaObjectField list = root.objectField( "list", ObjectStructure.NESTED ).multiValued();
addIndexFields( list );
list.toReference();
}

BulkIndexer contribute(BulkIndexer indexer) {
indexer.add( "1", d -> {
contribute( d.addObject( "single" ) );
contribute( d.addObject( "list" ) );
} );
return indexer;
}
}

public static class Index extends IndexBase {

public Index(IndexSchemaElement root) {
addIndexFields( root );
}

BulkIndexer contribute(BulkIndexer indexer) {
indexer.add( "1", this::contribute );
return indexer;
}
}

public abstract static class IndexBase {
protected void addIndexFields(IndexSchemaElement el) {
el.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
el.field( "string", f -> f.asString().projectable( Projectable.YES ) ).toReference();

IndexSchemaObjectField contained = el.objectField( "contained", ObjectStructure.NESTED ).multiValued();
contained.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
contained.field( "containedString", f -> f.asString().projectable( Projectable.YES ) ).toReference();

IndexSchemaObjectField deeperContained = contained.objectField( "deeperContained", ObjectStructure.NESTED );
deeperContained.field( "id", f -> f.asString().projectable( Projectable.YES ) ).toReference();
deeperContained.field( "deeperContainedString", f -> f.asString().projectable( Projectable.YES ) )
.toReference();

deeperContained.toReference();
contained.toReference();
}

protected void contribute(DocumentElement element) {
element.addValue( "id", "1" );
element.addValue( "string", "string" );

DocumentElement contained = element.addObject( "contained" );
contained.addValue( "id", "10" );
contained.addValue( "containedString", "containedString1" );
DocumentElement deeperContained = contained.addObject( "deeperContained" );
deeperContained.addValue( "id", "100" );
deeperContained.addValue( "deeperContainedString", "deeperContainedString1" );

contained = element.addObject( "contained" );
contained.addValue( "id", "20" );
contained.addValue( "containedString", "containedString2" );
deeperContained = contained.addObject( "deeperContained" );
deeperContained.addValue( "id", "200" );
deeperContained.addValue( "deeperContainedString", "deeperContainedString2" );

}
}
}
}

0 comments on commit 8c1ddde

Please sign in to comment.