From 4bc722e7d7c53d6b996c13ebe0981140e76256a2 Mon Sep 17 00:00:00 2001 From: RyanHolstien Date: Tue, 29 Oct 2024 15:48:35 -0500 Subject: [PATCH] fix(structuredProperties): fixes underscore behavior in structured property names --- .../models/StructuredPropertyUtils.java | 8 +- .../request/AggregationQueryBuilderTest.java | 112 ++++++++++++++++++ .../metadata/search/utils/ESUtilsTest.java | 84 +++++++++++++ 3 files changed, 202 insertions(+), 2 deletions(-) diff --git a/entity-registry/src/main/java/com/linkedin/metadata/models/StructuredPropertyUtils.java b/entity-registry/src/main/java/com/linkedin/metadata/models/StructuredPropertyUtils.java index 885be5e00b945..1deba1d1af004 100644 --- a/entity-registry/src/main/java/com/linkedin/metadata/models/StructuredPropertyUtils.java +++ b/entity-registry/src/main/java/com/linkedin/metadata/models/StructuredPropertyUtils.java @@ -120,12 +120,17 @@ public static Optional toStructuredPropertyFacetName( lookupDefinitionFromFilterOrFacetName( @Nonnull String fieldOrFacetName, @Nullable AspectRetriever aspectRetriever) { if (fieldOrFacetName.startsWith(STRUCTURED_PROPERTY_MAPPING_FIELD + ".")) { + // Coming in from the UI this is structuredProperties. + 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> result = Objects.requireNonNull(aspectRetriever) .getLatestAspectObjects( @@ -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( diff --git a/metadata-io/src/test/java/com/linkedin/metadata/search/query/request/AggregationQueryBuilderTest.java b/metadata-io/src/test/java/com/linkedin/metadata/search/query/request/AggregationQueryBuilderTest.java index 1381e9560b7e5..cef463802a6b1 100644 --- a/metadata-io/src/test/java/com/linkedin/metadata/search/query/request/AggregationQueryBuilderTest.java +++ b/metadata-io/src/test/java/com/linkedin/metadata/search/query/request/AggregationQueryBuilderTest.java @@ -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); @@ -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()) @@ -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 @@ -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 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(); @@ -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 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 = diff --git a/metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java b/metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java index 6665faacae337..892f7088e7f61 100644 --- a/metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java +++ b/metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java @@ -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); @@ -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()) @@ -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 @@ -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() { @@ -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");