Skip to content

Commit

Permalink
fix(structuredProperties): fixes underscore behavior in structured pr…
Browse files Browse the repository at this point in the history 
…operty names
  • Loading branch information
@RyanHolstien
RyanHolstien committed Oct 29, 2024
1 parent 6316e10 commit 4bc722e
Show file tree
Hide file tree
Showing 3 changed files with 202 additions and 2 deletions.
8 changes: 6 additions & 2 deletions entity-registry/src/main/java/com/linkedin/metadata/models/StructuredPropertyUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,17 @@ public static Optional<String> toStructuredPropertyFacetName(
lookupDefinitionFromFilterOrFacetName(
@Nonnull String fieldOrFacetName, @Nullable AspectRetriever aspectRetriever) {
if (fieldOrFacetName.startsWith(STRUCTURED_PROPERTY_MAPPING_FIELD + ".")) {
// Coming in from the UI this is structuredProperties.<FQN> + any particular specifier for
// subfield (.keyword etc)
String fqn =
fieldOrFacetName
.substring(STRUCTURED_PROPERTY_MAPPING_FIELD.length() + 1)
.replace(".keyword", "")
.replace(".delimited", "");

// FQN Maps directly to URN with urn:li:structuredProperties:FQN
Urn urn = toURNFromFQN(fqn);

Map<Urn, Map<String, Aspect>> result =
Objects.requireNonNull(aspectRetriever)
.getLatestAspectObjects(
Expand Down Expand Up @@ -224,8 +229,7 @@ public static void validateStructuredPropertyFQN(
* @return the expected structured property urn
*/
private static Urn toURNFromFQN(@Nonnull String fqn) {
return UrnUtils.getUrn(
String.join(":", "urn:li", STRUCTURED_PROPERTY_ENTITY_NAME, fqn.replace('_', '.')));
return UrnUtils.getUrn(String.join(":", "urn:li", STRUCTURED_PROPERTY_ENTITY_NAME, fqn));
}

public static void validateFilter(
Expand Down
112 changes: 112 additions & 0 deletions ...src/test/java/com/linkedin/metadata/search/query/request/AggregationQueryBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ public class AggregationQueryBuilderTest {
public void setup() throws RemoteInvocationException, URISyntaxException {
Urn helloUrn = Urn.createFromString("urn:li:structuredProperty:hello");
Urn abFghTenUrn = Urn.createFromString("urn:li:structuredProperty:ab.fgh.ten");
Urn underscoresAndDotsUrn =
Urn.createFromString("urn:li:structuredProperty:under.scores.and.dots_make_a_mess");

// legacy
aspectRetriever = mock(AspectRetriever.class);
Expand Down Expand Up @@ -75,6 +77,21 @@ public void setup() throws RemoteInvocationException, URISyntaxException {
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropAbFghTenDefinition.data()))));

StructuredPropertyDefinition structPropUnderscoresAndDotsDefinition =
new StructuredPropertyDefinition();
structPropUnderscoresAndDotsDefinition.setVersion(null, SetMode.REMOVE_IF_NULL);
structPropUnderscoresAndDotsDefinition.setValueType(
Urn.createFromString(DATA_TYPE_URN_PREFIX + "string"));
structPropUnderscoresAndDotsDefinition.setQualifiedName("under.scores.and.dots_make_a_mess");
structPropUnderscoresAndDotsDefinition.setDisplayName("under.scores.and.dots_make_a_mess");
when(aspectRetriever.getLatestAspectObjects(eq(Set.of(underscoresAndDotsUrn)), anySet()))
.thenReturn(
Map.of(
underscoresAndDotsUrn,
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropUnderscoresAndDotsDefinition.data()))));

// V1
aspectRetrieverV1 = mock(AspectRetriever.class);
when(aspectRetrieverV1.getEntityRegistry())
Expand Down Expand Up @@ -105,6 +122,21 @@ public void setup() throws RemoteInvocationException, URISyntaxException {
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropAbFghTenDefinitionV1.data()))));

StructuredPropertyDefinition structPropUnderscoresAndDotsDefinitionV1 =
new StructuredPropertyDefinition();
structPropUnderscoresAndDotsDefinitionV1.setVersion("00000000000001");
structPropUnderscoresAndDotsDefinitionV1.setValueType(
Urn.createFromString(DATA_TYPE_URN_PREFIX + "string"));
structPropUnderscoresAndDotsDefinitionV1.setQualifiedName("under.scores.and.dots_make_a_mess");
structPropUnderscoresAndDotsDefinitionV1.setDisplayName("under.scores.and.dots_make_a_mess");
when(aspectRetrieverV1.getLatestAspectObjects(eq(Set.of(underscoresAndDotsUrn)), anySet()))
.thenReturn(
Map.of(
underscoresAndDotsUrn,
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropUnderscoresAndDotsDefinitionV1.data()))));
}

@Test
Expand Down Expand Up @@ -269,6 +301,46 @@ public void testAggregateOverStructuredProperty() {
Set.of("structuredProperties.ab_fgh_ten.keyword", "structuredProperties.hello.keyword"));
}

@Test
public void testAggregateOverStructuredPropertyNamespaced() {
SearchConfiguration config = new SearchConfiguration();
config.setMaxTermBucketSize(25);

AggregationQueryBuilder builder =
new AggregationQueryBuilder(
config, ImmutableMap.of(mock(EntitySpec.class), ImmutableList.of()));

List<AggregationBuilder> aggs =
builder.getAggregations(
TestOperationContexts.systemContextNoSearchAuthorization(aspectRetriever),
List.of("structuredProperties.under.scores.and.dots_make_a_mess"));
Assert.assertEquals(aggs.size(), 1);
AggregationBuilder aggBuilder = aggs.get(0);
Assert.assertTrue(aggBuilder instanceof TermsAggregationBuilder);
TermsAggregationBuilder agg = (TermsAggregationBuilder) aggBuilder;
// Check that field name is sanitized to correct field name
Assert.assertEquals(
agg.field(),
"structuredProperties.under_scores_and_dots_make_a_mess.keyword",
"Terms aggregate must be on a keyword or subfield keyword");

// Two structured properties
aggs =
builder.getAggregations(
TestOperationContexts.systemContextNoSearchAuthorization(aspectRetriever),
List.of(
"structuredProperties.under.scores.and.dots_make_a_mess",
"structuredProperties.hello"));
Assert.assertEquals(aggs.size(), 2);
Assert.assertEquals(
aggs.stream()
.map(aggr -> ((TermsAggregationBuilder) aggr).field())
.collect(Collectors.toSet()),
Set.of(
"structuredProperties.under_scores_and_dots_make_a_mess.keyword",
"structuredProperties.hello.keyword"));
}

@Test
public void testAggregateOverStructuredPropertyV1() {
SearchConfiguration config = new SearchConfiguration();
Expand Down Expand Up @@ -309,6 +381,46 @@ public void testAggregateOverStructuredPropertyV1() {
"structuredProperties._versioned.hello.00000000000001.string.keyword"));
}

@Test
public void testAggregateOverStructuredPropertyNamespacedV1() {
SearchConfiguration config = new SearchConfiguration();
config.setMaxTermBucketSize(25);

AggregationQueryBuilder builder =
new AggregationQueryBuilder(
config, ImmutableMap.of(mock(EntitySpec.class), ImmutableList.of()));

List<AggregationBuilder> aggs =
builder.getAggregations(
TestOperationContexts.systemContextNoSearchAuthorization(aspectRetrieverV1),
List.of("structuredProperties.under.scores.and.dots_make_a_mess"));
Assert.assertEquals(aggs.size(), 1);
AggregationBuilder aggBuilder = aggs.get(0);
Assert.assertTrue(aggBuilder instanceof TermsAggregationBuilder);
TermsAggregationBuilder agg = (TermsAggregationBuilder) aggBuilder;
// Check that field name is sanitized to correct field name
Assert.assertEquals(
agg.field(),
"structuredProperties._versioned.under_scores_and_dots_make_a_mess.00000000000001.string.keyword",
"Terms aggregation must be on a keyword field or subfield.");

// Two structured properties
aggs =
builder.getAggregations(
TestOperationContexts.systemContextNoSearchAuthorization(aspectRetrieverV1),
List.of(
"structuredProperties.under.scores.and.dots_make_a_mess",
"structuredProperties._versioned.hello.00000000000001.string"));
Assert.assertEquals(aggs.size(), 2);
Assert.assertEquals(
aggs.stream()
.map(aggr -> ((TermsAggregationBuilder) aggr).field())
.collect(Collectors.toSet()),
Set.of(
"structuredProperties._versioned.under_scores_and_dots_make_a_mess.00000000000001.string.keyword",
"structuredProperties._versioned.hello.00000000000001.string.keyword"));
}

@Test
public void testAggregateOverFieldsAndStructProp() {
SearchableAnnotation annotation1 =
Expand Down
84 changes: 84 additions & 0 deletions metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ public class ESUtilsTest {
@BeforeClass
public static void setup() throws RemoteInvocationException, URISyntaxException {
Urn abFghTenUrn = Urn.createFromString("urn:li:structuredProperty:ab.fgh.ten");
Urn underscoresAndDotsUrn =
Urn.createFromString("urn:li:structuredProperty:under.scores.and.dots_make_a_mess");

// legacy
aspectRetriever = mock(AspectRetriever.class);
Expand All @@ -62,6 +64,20 @@ public static void setup() throws RemoteInvocationException, URISyntaxException
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropAbFghTenDefinition.data()))));

StructuredPropertyDefinition structPropUnderscoresAndDotsDefinition =
new StructuredPropertyDefinition();
structPropUnderscoresAndDotsDefinition.setVersion(null, SetMode.REMOVE_IF_NULL);
structPropUnderscoresAndDotsDefinition.setValueType(
Urn.createFromString(DATA_TYPE_URN_PREFIX + "string"));
structPropUnderscoresAndDotsDefinition.setQualifiedName("under.scores.and.dots_make_a_mess");
when(aspectRetriever.getLatestAspectObjects(eq(Set.of(underscoresAndDotsUrn)), anySet()))
.thenReturn(
Map.of(
underscoresAndDotsUrn,
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropUnderscoresAndDotsDefinition.data()))));

// V1
aspectRetrieverV1 = mock(AspectRetriever.class);
when(aspectRetrieverV1.getEntityRegistry())
Expand All @@ -80,6 +96,20 @@ public static void setup() throws RemoteInvocationException, URISyntaxException
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropAbFghTenDefinitionV1.data()))));

StructuredPropertyDefinition structPropUnderscoresAndDotsDefinitionV1 =
new StructuredPropertyDefinition();
structPropUnderscoresAndDotsDefinitionV1.setVersion("00000000000001");
structPropUnderscoresAndDotsDefinitionV1.setValueType(
Urn.createFromString(DATA_TYPE_URN_PREFIX + "string"));
structPropUnderscoresAndDotsDefinitionV1.setQualifiedName("under.scores.and.dots_make_a_mess");
when(aspectRetrieverV1.getLatestAspectObjects(eq(Set.of(underscoresAndDotsUrn)), anySet()))
.thenReturn(
Map.of(
underscoresAndDotsUrn,
Map.of(
STRUCTURED_PROPERTY_DEFINITION_ASPECT_NAME,
new Aspect(structPropUnderscoresAndDotsDefinitionV1.data()))));
}

@Test
Expand Down Expand Up @@ -703,6 +733,31 @@ public void testGetQueryBuilderFromStructPropEqualsValue() {
Assert.assertEquals(result.toString(), expected);
}

@Test
public void testGetQueryBuilderFromNamespacedStructPropEqualsValue() {

final Criterion singleValueCriterion =
buildCriterion(
"structuredProperties.under.scores.and.dots_make_a_mess", Condition.EQUAL, "value1");

OperationContext opContext = mock(OperationContext.class);
when(opContext.getAspectRetriever()).thenReturn(aspectRetriever);
QueryBuilder result =
ESUtils.getQueryBuilderFromCriterion(
singleValueCriterion, false, new HashMap<>(), opContext, QueryFilterRewriteChain.EMPTY);
String expected =
"{\n"
+ " \"terms\" : {\n"
+ " \"structuredProperties.under_scores_and_dots_make_a_mess.keyword\" : [\n"
+ " \"value1\"\n"
+ " ],\n"
+ " \"boost\" : 1.0,\n"
+ " \"_name\" : \"structuredProperties.under.scores.and.dots_make_a_mess\"\n"
+ " }\n"
+ "}";
Assert.assertEquals(result.toString(), expected);
}

@Test
public void testGetQueryBuilderFromStructPropEqualsValueV1() {

Expand Down Expand Up @@ -731,6 +786,35 @@ public void testGetQueryBuilderFromStructPropEqualsValueV1() {
Assert.assertEquals(result.toString(), expected);
}

@Test
public void testGetQueryBuilderFromNamespacedStructPropEqualsValueV1() {

final Criterion singleValueCriterion =
buildCriterion(
"structuredProperties.under.scores.and.dots_make_a_mess", Condition.EQUAL, "value1");

OperationContext opContextV1 = mock(OperationContext.class);
when(opContextV1.getAspectRetriever()).thenReturn(aspectRetrieverV1);
QueryBuilder result =
ESUtils.getQueryBuilderFromCriterion(
singleValueCriterion,
false,
new HashMap<>(),
opContextV1,
QueryFilterRewriteChain.EMPTY);
String expected =
"{\n"
+ " \"terms\" : {\n"
+ " \"structuredProperties._versioned.under_scores_and_dots_make_a_mess.00000000000001.string.keyword\" : [\n"
+ " \"value1\"\n"
+ " ],\n"
+ " \"boost\" : 1.0,\n"
+ " \"_name\" : \"structuredProperties.under.scores.and.dots_make_a_mess\"\n"
+ " }\n"
+ "}";
Assert.assertEquals(result.toString(), expected);
}

@Test
public void testGetQueryBuilderFromStructPropExists() {
final Criterion singleValueCriterion = buildExistsCriterion("structuredProperties.ab.fgh.ten");
Expand Down

0 comments on commit 4bc722e

Please sign in to comment.