diff --git a/docs/how/updating-datahub.md b/docs/how/updating-datahub.md index c21d197de2..10173ccd4a 100644 --- a/docs/how/updating-datahub.md +++ b/docs/how/updating-datahub.md @@ -38,7 +38,7 @@ This file documents any backwards-incompatible changes in DataHub and assists pe ### Breaking Changes -- #11486 - Deprecated Criterion filters using `value`. Use `values` instead. This also deprecates the ability to use comma delimited string to represent multiple values using `value`. +- #11486 - Criterion's `value` parameter has been previously deprecated. Use of `value` instead of `values` is no longer supported and will be completely removed on the next major version. - #11484 - Metadata service authentication enabled by default - #11484 - Rest API authorization enabled by default - #10472 - `SANDBOX` added as a FabricType. No rollbacks allowed once metadata with this fabric type is added without manual cleanups in databases. diff --git a/docs/managed-datahub/release-notes/v_0_3_7.md b/docs/managed-datahub/release-notes/v_0_3_7.md index 19cb04e9f5..a151462976 100644 --- a/docs/managed-datahub/release-notes/v_0_3_7.md +++ b/docs/managed-datahub/release-notes/v_0_3_7.md @@ -32,7 +32,7 @@ If you are using an older CLI/SDK version, then please upgrade it. This applies datahub: timezone: 'America/Los_Angeles' ``` - - #11486 - Deprecated Criterion filters using `value`. Use `values` instead. This also deprecates the ability to use comma delimited string to represent multiple values using `value`. + - #11486 - Criterion's `value` parameter has been previously deprecated. Use of `value` instead of `values` is no longer supported and will be completely removed on the next major version. - #10472 - `SANDBOX` added as a FabricType. No rollbacks allowed once metadata with this fabric type is added without manual cleanups in databases. - #11619 - schema field/column paths can no longer be empty strings - #11619 - schema field/column paths can no longer be duplicated within the schema diff --git a/li-utils/src/main/java/com/linkedin/metadata/Constants.java b/li-utils/src/main/java/com/linkedin/metadata/Constants.java index f1f096640b..077e0e2b66 100644 --- a/li-utils/src/main/java/com/linkedin/metadata/Constants.java +++ b/li-utils/src/main/java/com/linkedin/metadata/Constants.java @@ -10,6 +10,7 @@ public class Constants { public static final String INTERNAL_DELEGATED_FOR_ACTOR_HEADER_NAME = "X-DataHub-Delegated-For"; public static final String INTERNAL_DELEGATED_FOR_ACTOR_TYPE = "X-DataHub-Delegated-For-"; + public static final String URN_LI_PREFIX = "urn:li:"; public static final String DATAHUB_ACTOR = "urn:li:corpuser:datahub"; // Super user. public static final String SYSTEM_ACTOR = "urn:li:corpuser:__datahub_system"; // DataHub internal service principal. diff --git a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/analytics/Analytics.java b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/analytics/Analytics.java index 9bbe1bb35f..94da6308ed 100644 --- a/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/analytics/Analytics.java +++ b/metadata-service/restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/analytics/Analytics.java @@ -5,7 +5,6 @@ import com.datahub.authentication.AuthenticationContext; import com.datahub.authorization.AuthUtil; import com.datahub.plugins.auth.authorization.Authorizer; import com.linkedin.analytics.GetTimeseriesAggregatedStatsResponse; -import com.linkedin.metadata.authorization.PoliciesConfig; import com.linkedin.metadata.query.filter.Filter; import com.linkedin.metadata.resources.restli.RestliUtils; import com.linkedin.metadata.timeseries.TimeseriesAspectService; @@ -14,7 +13,6 @@ import com.linkedin.restli.common.HttpStatus; import com.linkedin.restli.server.RestLiServiceException; import com.linkedin.restli.server.annotations.Action; import com.linkedin.restli.server.annotations.ActionParam; -import com.linkedin.restli.server.annotations.Context; import com.linkedin.restli.server.annotations.Optional; import com.linkedin.restli.server.annotations.RestLiSimpleResource; import com.linkedin.restli.server.resources.SimpleResourceTemplate; @@ -24,12 +22,10 @@ import com.linkedin.timeseries.GenericTable; import com.linkedin.timeseries.GroupingBucket; import com.linkedin.timeseries.GroupingBucketArray; import java.util.Arrays; -import java.util.List; import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.inject.Inject; import javax.inject.Named; -import javax.servlet.http.HttpServletRequest; import io.datahubproject.metadata.context.OperationContext; import io.datahubproject.metadata.context.RequestContext; @@ -38,6 +34,7 @@ import lombok.extern.slf4j.Slf4j; import static com.datahub.authorization.AuthUtil.isAPIAuthorized; import static com.linkedin.metadata.authorization.ApiGroup.TIMESERIES; import static com.linkedin.metadata.authorization.ApiOperation.READ; +import static com.linkedin.metadata.utils.CriterionUtils.validateAndConvert; /** Rest.li entry point: /analytics */ @Slf4j @@ -90,8 +87,9 @@ public class Analytics extends SimpleResourceTemplate { - SearchResult result = searchService.searchAcrossEntities(opContext, entityList, input, filter, sortCriterionList, start, count); + SearchResult result = searchService.searchAcrossEntities(opContext, entityList, input, validateAndConvert(filter), sortCriterionList, start, count); if (!isAPIAuthorizedResult( opContext, result)) { @@ -514,7 +515,7 @@ public class EntityResource extends CollectionResourceTaskTemplate sortCriterionList = getSortCriteria(sortCriteria, sortCriterion); - log.info("GET LIST RESULTS for {} with filter {}", entityName, filter); + final Filter finalFilter = validateAndConvert(filter); + log.info("GET LIST RESULTS for {} with filter {}", entityName, finalFilter); return RestliUtils.toTask( () -> { - SearchResult result = entitySearchService.filter(opContext, entityName, filter, sortCriterionList, start, count); + SearchResult result = entitySearchService.filter(opContext, entityName, finalFilter, sortCriterionList, start, count); if (!AuthUtil.isAPIAuthorizedResult( opContext, result)) { diff --git a/metadata-utils/src/main/java/com/linkedin/metadata/utils/CriterionUtils.java b/metadata-utils/src/main/java/com/linkedin/metadata/utils/CriterionUtils.java index e40c4af1e0..f8e138487f 100644 --- a/metadata-utils/src/main/java/com/linkedin/metadata/utils/CriterionUtils.java +++ b/metadata-utils/src/main/java/com/linkedin/metadata/utils/CriterionUtils.java @@ -1,17 +1,81 @@ package com.linkedin.metadata.utils; +import static com.linkedin.metadata.Constants.URN_LI_PREFIX; + import com.linkedin.data.template.StringArray; import com.linkedin.metadata.query.filter.Condition; import com.linkedin.metadata.query.filter.Criterion; +import com.linkedin.metadata.query.filter.Filter; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.List; import java.util.stream.Collectors; import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import lombok.extern.slf4j.Slf4j; +@Slf4j public class CriterionUtils { private CriterionUtils() {} + /** + * This function is meant to validate and correct Filter input for rest.li endpoints. + * + * @param inputFilter the rest.li filter parameter + * @return validated and corrected filter + */ + @Nullable + public static Filter validateAndConvert(@Nullable Filter inputFilter) { + if (inputFilter != null) { + List invalidCriterion = new ArrayList<>(); + if (inputFilter.hasCriteria()) { + invalidCriterion.addAll( + inputFilter.getCriteria().stream() + .filter( + criterion -> + (criterion.hasValue() && !criterion.getValue().isEmpty()) + || !criterion.hasValue()) + .collect(Collectors.toList())); + } + if (inputFilter.hasOr()) { + invalidCriterion.addAll( + inputFilter.getOr().stream() + .flatMap(c -> c.getAnd().stream()) + .filter( + criterion -> + (criterion.hasValue() && !criterion.getValue().isEmpty()) + || !criterion.hasValue()) + .collect(Collectors.toList())); + } + + for (Criterion criterion : invalidCriterion) { + if (criterion.hasValue()) { + if ((criterion.getValue().contains(",") + && !criterion.getValue().startsWith(URN_LI_PREFIX)) + || criterion.getValue().contains(")," + URN_LI_PREFIX)) { + throw new IllegalArgumentException( + "Criterion `value` is deprecated and contains an ambiguous comma. Please use `values`."); + } + if (criterion.hasValues() && !criterion.getValue().equals(criterion.getValues().get(0))) { + throw new IllegalArgumentException( + "Criterion `value` is deprecated and `values`[0] is populated with a conflicting value."); + } + // auto-convert + if (!criterion.hasValues()) { + log.error( + "Deprecated use of a filter using Criterion's `value` has been detected and corrected. Please migrate to `values` instead."); + criterion.setValues(new StringArray(criterion.getValue())); + } + } + // must be set per required field + criterion.setValue(""); + } + } + return inputFilter; + } + public static Criterion buildExistsCriterion(@Nonnull String field) { return buildCriterion(field, Condition.EXISTS, false, Collections.emptyList()); } diff --git a/metadata-utils/src/test/java/com/linkedin/metadata/utils/CriterionUtilsTest.java b/metadata-utils/src/test/java/com/linkedin/metadata/utils/CriterionUtilsTest.java new file mode 100644 index 0000000000..e2f22dd665 --- /dev/null +++ b/metadata-utils/src/test/java/com/linkedin/metadata/utils/CriterionUtilsTest.java @@ -0,0 +1,274 @@ +package com.linkedin.metadata.utils; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertTrue; + +import com.linkedin.data.template.StringArray; +import com.linkedin.metadata.query.filter.ConjunctiveCriterion; +import com.linkedin.metadata.query.filter.ConjunctiveCriterionArray; +import com.linkedin.metadata.query.filter.Criterion; +import com.linkedin.metadata.query.filter.CriterionArray; +import com.linkedin.metadata.query.filter.Filter; +import org.testng.annotations.Test; + +public class CriterionUtilsTest { + @Test + public void testNullFilter() { + Filter result = CriterionUtils.validateAndConvert(null); + assertNull(result); + } + + @Test + public void testEmptyFilter() { + Filter input = new Filter(); + Filter result = CriterionUtils.validateAndConvert(input); + assertNotNull(result); + assertFalse(result.hasCriteria()); + assertFalse(result.hasOr()); + } + + @Test + public void testSimpleCriterionConversion() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue("testValue"); + input.setCriteria(new CriterionArray(criterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getCriteria().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertTrue(convertedCriterion.hasValues()); + assertEquals("testValue", convertedCriterion.getValues().get(0)); + } + + @Test + public void testOrClauseCriterionConversion() { + Filter input = new Filter(); + + // Create OR clause with AND criteria + Criterion criterion = new Criterion(); + criterion.setValue("orValue"); + + ConjunctiveCriterion conjunctive = new ConjunctiveCriterion(); + conjunctive.setAnd(new CriterionArray(criterion)); + + input.setOr(new ConjunctiveCriterionArray(conjunctive)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getOr().get(0).getAnd().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertTrue(convertedCriterion.hasValues()); + assertEquals("orValue", convertedCriterion.getValues().get(0)); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testCommaInValueThrowsException() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue("value1,value2"); + input.setCriteria(new CriterionArray(criterion)); + + CriterionUtils.validateAndConvert(input); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testConflictingValuesThrowsException() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue("value1"); + criterion.setValues(new StringArray("differentValue")); + input.setCriteria(new CriterionArray(criterion)); + + CriterionUtils.validateAndConvert(input); + } + + @Test + public void testExistingValuesNotModified() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue("value1"); + criterion.setValues(new StringArray("value1")); // Same value, should not throw exception + input.setCriteria(new CriterionArray(criterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getCriteria().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertTrue(convertedCriterion.hasValues()); + assertEquals("value1", convertedCriterion.getValues().get(0)); + } + + @Test + public void testMultipleCriteriaConversion() { + Filter input = new Filter(); + + Criterion criterion1 = new Criterion(); + criterion1.setValue("value1"); + + Criterion criterion2 = new Criterion(); + criterion2.setValue("value2"); + + input.setCriteria(new CriterionArray(criterion1, criterion2)); + + Filter result = CriterionUtils.validateAndConvert(input); + + assertEquals(2, result.getCriteria().size()); + + for (Criterion c : result.getCriteria()) { + assertEquals(c.getValue(), ""); + assertTrue(c.hasValues()); + assertTrue(c.getValues().get(0).equals("value1") || c.getValues().get(0).equals("value2")); + } + } + + @Test + public void testMixedCriteriaAndOrClause() { + Filter input = new Filter(); + + // Add direct criteria + Criterion criterion1 = new Criterion(); + criterion1.setValue("directValue"); + input.setCriteria(new CriterionArray(criterion1)); + + // Add OR clause with AND criteria + Criterion criterion2 = new Criterion(); + criterion2.setValue("orValue"); + ConjunctiveCriterion conjunctive = new ConjunctiveCriterion(); + conjunctive.setAnd(new CriterionArray(criterion2)); + input.setOr(new ConjunctiveCriterionArray(conjunctive)); + + Filter result = CriterionUtils.validateAndConvert(input); + + // Check direct criterion + Criterion convertedDirect = result.getCriteria().get(0); + assertEquals(convertedDirect.getValue(), ""); + assertTrue(convertedDirect.hasValues()); + assertEquals("directValue", convertedDirect.getValues().get(0)); + + // Check OR clause criterion + Criterion convertedOr = result.getOr().get(0).getAnd().get(0); + assertEquals(convertedOr.getValue(), ""); + assertTrue(convertedOr.hasValues()); + assertEquals("orValue", convertedOr.getValues().get(0)); + } + + @Test + public void testEmptyStringValueNotConverted() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue(""); // Empty string value + input.setCriteria(new CriterionArray(criterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getCriteria().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertFalse(convertedCriterion.hasValues()); // Should not be converted since value was empty + } + + @Test + public void testMixedEmptyAndNonEmptyValues() { + Filter input = new Filter(); + + Criterion emptyCriterion = new Criterion(); + emptyCriterion.setValue(""); + + Criterion nonEmptyCriterion = new Criterion(); + nonEmptyCriterion.setValue("value1"); + + input.setCriteria(new CriterionArray(emptyCriterion, nonEmptyCriterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + assertEquals(2, result.getCriteria().size()); + + // Check empty criterion + Criterion convertedEmpty = result.getCriteria().get(0); + assertEquals(convertedEmpty.getValue(), ""); + assertFalse(convertedEmpty.hasValues()); + + // Check non-empty criterion + Criterion convertedNonEmpty = result.getCriteria().get(1); + assertEquals(convertedNonEmpty.getValue(), ""); + assertTrue(convertedNonEmpty.hasValues()); + assertEquals(convertedNonEmpty.getValues().get(0), "value1"); + } + + @Test + public void testOrClauseWithEmptyValues() { + Filter input = new Filter(); + + // Create OR clause with mixed empty and non-empty criteria + Criterion emptyCriterion = new Criterion(); + emptyCriterion.setValue(""); + + Criterion nonEmptyCriterion = new Criterion(); + nonEmptyCriterion.setValue("orValue"); + + ConjunctiveCriterion conjunctive = new ConjunctiveCriterion(); + conjunctive.setAnd(new CriterionArray(emptyCriterion, nonEmptyCriterion)); + + input.setOr(new ConjunctiveCriterionArray(conjunctive)); + + Filter result = CriterionUtils.validateAndConvert(input); + + // Check empty criterion + Criterion convertedEmpty = result.getOr().get(0).getAnd().get(0); + assertEquals(convertedEmpty.getValue(), ""); + assertFalse(convertedEmpty.hasValues()); + + // Check non-empty criterion + Criterion convertedNonEmpty = result.getOr().get(0).getAnd().get(1); + assertEquals(convertedNonEmpty.getValue(), ""); + assertTrue(convertedNonEmpty.hasValues()); + assertEquals(convertedNonEmpty.getValues().get(0), "orValue"); + } + + @Test + public void testCriterionWithOnlyValues() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValues(new StringArray("value1")); // Only has values, no value field set + input.setCriteria(new CriterionArray(criterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getCriteria().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertTrue(convertedCriterion.hasValues()); + assertEquals(convertedCriterion.getValues().get(0), "value1"); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testMultiUrnThrowsException() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue( + "urn:li:dataset:(urn:li:dataPlatform:postgres,foo,PROD),urn:li:dataset:(urn:li:dataPlatform:postgres,foo,PROD)"); + input.setCriteria(new CriterionArray(criterion)); + + CriterionUtils.validateAndConvert(input); + } + + @Test + public void testUrnConversion() { + Filter input = new Filter(); + Criterion criterion = new Criterion(); + criterion.setValue("urn:li:dataset:(urn:li:dataPlatform:postgres,foo,PROD)"); + input.setCriteria(new CriterionArray(criterion)); + + Filter result = CriterionUtils.validateAndConvert(input); + + Criterion convertedCriterion = result.getCriteria().get(0); + assertEquals(convertedCriterion.getValue(), ""); + assertTrue(convertedCriterion.hasValues()); + assertEquals( + "urn:li:dataset:(urn:li:dataPlatform:postgres,foo,PROD)", + convertedCriterion.getValues().get(0)); + } +}