fix(rest.li): fix use of Criterion in rest.li filters (#11932)

This commit is contained in:
david-leifker 2024-11-22 23:47:56 -06:00 committed by GitHub
parent 2878c65ceb
commit 794ac2b4e8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 357 additions and 22 deletions

View File

@ -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.

View File

@ -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

View File

@ -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.

View File

@ -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<GetTimeseriesAggregatedSta
resp.setEntityName(entityName);
resp.setAspectName(aspectName);
resp.setAggregationSpecs(new AggregationSpecArray(Arrays.asList(aggregationSpecs)));
if (filter != null) {
resp.setFilter(filter);
final Filter finalFilter = validateAndConvert(filter);
if (finalFilter != null) {
resp.setFilter(finalFilter);
}
if (groupingBuckets != null) {
resp.setGroupingBuckets(new GroupingBucketArray(Arrays.asList(groupingBuckets)));
@ -99,7 +97,7 @@ public class Analytics extends SimpleResourceTemplate<GetTimeseriesAggregatedSta
GenericTable aggregatedStatsTable =
timeseriesAspectService.getAggregatedStats(opContext,
entityName, aspectName, aggregationSpecs, filter, groupingBuckets);
entityName, aspectName, aggregationSpecs, finalFilter, groupingBuckets);
resp.setTable(aggregatedStatsTable);
return resp;
});

View File

@ -11,6 +11,7 @@ import static com.linkedin.metadata.authorization.ApiGroup.TIMESERIES;
import static com.linkedin.metadata.authorization.ApiOperation.READ;
import static com.linkedin.metadata.resources.operations.OperationsResource.*;
import static com.linkedin.metadata.resources.restli.RestliConstants.*;
import static com.linkedin.metadata.utils.CriterionUtils.validateAndConvert;
import com.codahale.metrics.MetricRegistry;
import com.datahub.authentication.Authentication;
@ -22,14 +23,12 @@ import com.linkedin.common.AuditStamp;
import com.linkedin.common.urn.Urn;
import com.linkedin.metadata.aspect.EnvelopedAspectArray;
import com.linkedin.metadata.aspect.VersionedAspect;
import com.linkedin.metadata.aspect.batch.BatchItem;
import com.linkedin.metadata.authorization.PoliciesConfig;
import com.linkedin.metadata.entity.EntityService;
import com.linkedin.metadata.entity.IngestResult;
import com.linkedin.metadata.entity.ebean.batch.AspectsBatchImpl;
import com.linkedin.metadata.aspect.batch.AspectsBatch;
import com.linkedin.metadata.entity.validation.ValidationException;
import com.linkedin.metadata.models.registry.EntityRegistry;
import com.linkedin.metadata.query.filter.Filter;
import com.linkedin.metadata.query.filter.SortCriterion;
import com.linkedin.metadata.resources.operations.Utils;
@ -38,7 +37,6 @@ import com.linkedin.metadata.search.EntitySearchService;
import com.linkedin.metadata.timeseries.TimeseriesAspectService;
import com.linkedin.mxe.GenericAspect;
import com.linkedin.mxe.MetadataChangeProposal;
import com.linkedin.mxe.SystemMetadata;
import com.linkedin.parseq.Task;
import com.linkedin.restli.common.HttpStatus;
import com.linkedin.restli.internal.server.methods.AnyRecord;
@ -59,8 +57,6 @@ import java.nio.charset.StandardCharsets;
import java.time.Clock;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
@ -239,7 +235,7 @@ public class AspectResource extends CollectionResourceTaskTemplate<String, Versi
startTimeMillis,
endTimeMillis,
limit,
filter,
validateAndConvert(filter),
sort)));
return response;
},

View File

@ -12,6 +12,7 @@ import static com.linkedin.metadata.entity.validation.ValidationApiUtils.validat
import static com.linkedin.metadata.entity.validation.ValidationUtils.*;
import static com.linkedin.metadata.resources.restli.RestliConstants.*;
import static com.linkedin.metadata.search.utils.SearchUtils.*;
import static com.linkedin.metadata.utils.CriterionUtils.validateAndConvert;
import static com.linkedin.metadata.utils.PegasusUtils.*;
import static com.linkedin.metadata.utils.SystemMetadataUtils.generateSystemMetadataIfEmpty;
@ -401,7 +402,7 @@ public class EntityResource extends CollectionResourceTaskTemplate<String, Entit
// This API is not used by the frontend for search bars so we default to structured
result =
entitySearchService.search(opContext,
List.of(entityName), input, filter, sortCriterionList, start, count);
List.of(entityName), input, validateAndConvert(filter), sortCriterionList, start, count);
if (!isAPIAuthorizedResult(
opContext,
@ -448,7 +449,7 @@ public class EntityResource extends CollectionResourceTaskTemplate<String, Entit
log.info("GET SEARCH RESULTS ACROSS ENTITIES for {} with query {}", entityList, input);
return RestliUtils.toTask(
() -> {
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<String, Entit
opContext,
entityList,
input,
filter,
validateAndConvert(filter),
sortCriterionList,
scrollId,
keepAlive,
@ -583,7 +584,7 @@ public class EntityResource extends CollectionResourceTaskTemplate<String, Entit
entityList,
input,
maxHops,
filter,
validateAndConvert(filter),
sortCriterionList,
start,
count),
@ -648,7 +649,7 @@ public class EntityResource extends CollectionResourceTaskTemplate<String, Entit
entityList,
input,
maxHops,
filter,
validateAndConvert(filter),
sortCriterionList,
scrollId,
keepAlive,
@ -683,10 +684,11 @@ public class EntityResource extends CollectionResourceTaskTemplate<String, Entit
List<SortCriterion> 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)) {

View File

@ -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<Criterion> 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());
}

View File

@ -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));
}
}