From e9bad2ab29df3ef86eb5948bd8140cb5a5e264a2 Mon Sep 17 00:00:00 2001 From: Harshit Kumar Date: Sat, 24 Aug 2024 22:52:15 +0530 Subject: [PATCH] ENG-48723: Filter builder fix and object store changes (#230) * filter builder fix and store changes * nit * revert-change * refactor object building * review comment --- config-service-impl/build.gradle.kts | 1 + .../config/service/store/FilterBuilder.java | 45 ++++++++++--------- .../service/store/FilterBuilderTest.java | 20 ++++----- .../objectstore/IdentifiedObjectStore.java | 38 +++++++++++----- 4 files changed, 61 insertions(+), 43 deletions(-) diff --git a/config-service-impl/build.gradle.kts b/config-service-impl/build.gradle.kts index 0e041826..4d47df89 100644 --- a/config-service-impl/build.gradle.kts +++ b/config-service-impl/build.gradle.kts @@ -8,6 +8,7 @@ dependencies { api(projects.configServiceApi) implementation(projects.configServiceChangeEventGenerator) + implementation(projects.configProtoConverter) implementation(commonLibs.jackson.databind) implementation(commonLibs.guava) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterBuilder.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterBuilder.java index 51ed2807..760377d5 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterBuilder.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterBuilder.java @@ -2,12 +2,22 @@ import static org.hypertrace.config.service.store.ConfigDocument.CONFIG_FIELD_NAME; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.protobuf.Value; import io.grpc.Status; +import lombok.SneakyThrows; +import org.hypertrace.config.proto.converter.ConfigProtoConverter; import org.hypertrace.config.service.v1.LogicalFilter; import org.hypertrace.config.service.v1.RelationalFilter; import org.hypertrace.core.documentstore.Filter; class FilterBuilder { + private final ObjectMapper objectMapper; + + FilterBuilder() { + this.objectMapper = new ObjectMapper(); + } public Filter buildDocStoreFilter(org.hypertrace.config.service.v1.Filter filter) { switch (filter.getTypeCase()) { @@ -54,12 +64,11 @@ private Filter evaluateCompositeExpression(LogicalFilter logicalFilter) { } private Filter evaluateLeafExpression(RelationalFilter relationalFilter) { + Object value = this.convertValueToObject(relationalFilter.getValue()); switch (relationalFilter.getOperator()) { case RELATIONAL_OPERATOR_EQ: return new Filter( - Filter.Op.EQ, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.EQ, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_NEQ: return new Filter( Filter.Op.NEQ, @@ -67,34 +76,22 @@ private Filter evaluateLeafExpression(RelationalFilter relationalFilter) { relationalFilter.getValue()); case RELATIONAL_OPERATOR_IN: return new Filter( - Filter.Op.IN, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.IN, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_NOT_IN: return new Filter( - Filter.Op.NOT_IN, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.NOT_IN, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_LT: return new Filter( - Filter.Op.LT, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.LT, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_GT: return new Filter( - Filter.Op.GT, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.GT, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_LTE: return new Filter( - Filter.Op.LTE, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.LTE, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case RELATIONAL_OPERATOR_GTE: return new Filter( - Filter.Op.GTE, - buildConfigFieldPath(relationalFilter.getConfigJsonPath()), - relationalFilter.getValue()); + Filter.Op.GTE, buildConfigFieldPath(relationalFilter.getConfigJsonPath()), value); case UNRECOGNIZED: default: throw Status.INVALID_ARGUMENT @@ -106,4 +103,10 @@ private Filter evaluateLeafExpression(RelationalFilter relationalFilter) { private String buildConfigFieldPath(String configJsonPath) { return String.format("%s.%s", CONFIG_FIELD_NAME, configJsonPath); } + + @SneakyThrows + private Object convertValueToObject(Value value) { + return objectMapper.readValue( + ConfigProtoConverter.convertToJsonString(value), new TypeReference<>() {}); + } } diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/store/FilterBuilderTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/FilterBuilderTest.java index 76e36281..62bbfcad 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/store/FilterBuilderTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/store/FilterBuilderTest.java @@ -31,11 +31,11 @@ void buildDocStoreFilter1() { RelationalFilter.newBuilder() .setConfigJsonPath("key1") .setOperator(RelationalOperator.RELATIONAL_OPERATOR_EQ) - .setValue(Value.newBuilder().setStringValue("value1"))) + .setValue(Values.of("value1"))) .build(); org.hypertrace.core.documentstore.Filter docFilter = - org.hypertrace.core.documentstore.Filter.eq("config.key1", Values.of("value1")); + org.hypertrace.core.documentstore.Filter.eq("config.key1", "value1"); assertEquals(docFilter.toString(), filterBuilder.buildDocStoreFilter(filter).toString()); Filter filter2 = @@ -44,11 +44,11 @@ void buildDocStoreFilter1() { RelationalFilter.newBuilder() .setConfigJsonPath("key2") .setOperator(RelationalOperator.RELATIONAL_OPERATOR_EQ) - .setValue(Value.newBuilder().setNumberValue(300))) + .setValue(Values.of(300))) .build(); org.hypertrace.core.documentstore.Filter docFilter2 = - org.hypertrace.core.documentstore.Filter.eq("config.key2", Values.of(300)); + org.hypertrace.core.documentstore.Filter.eq("config.key2", 300.0); assertEquals(docFilter2.toString(), filterBuilder.buildDocStoreFilter(filter2).toString()); } @@ -76,12 +76,10 @@ void buildDocStoreFilter2() { .build(); org.hypertrace.core.documentstore.Filter docFilter = - org.hypertrace.core.documentstore.Filter.eq("config.key3", Values.of(true)) + org.hypertrace.core.documentstore.Filter.eq("config.key3", true) .and( new org.hypertrace.core.documentstore.Filter( - org.hypertrace.core.documentstore.Filter.Op.LTE, - "config.key4", - Values.of(100))); + org.hypertrace.core.documentstore.Filter.Op.LTE, "config.key4", 100.0)); assertEquals(docFilter.toString(), filterBuilder.buildDocStoreFilter(filter).toString()); } @@ -133,11 +131,11 @@ void buildDocStoreFilter3() { new org.hypertrace.core.documentstore.Filter( org.hypertrace.core.documentstore.Filter.Op.IN, "config.key5", - Values.of(List.of(Values.of("listValue1"), Values.of("listValue2")))) + List.of("listValue1", "listValue2")) .and( new org.hypertrace.core.documentstore.Filter( - org.hypertrace.core.documentstore.Filter.Op.GTE, "config.key6", Values.of(100))) - .or(org.hypertrace.core.documentstore.Filter.eq("config.key7", Values.of("value7"))); + org.hypertrace.core.documentstore.Filter.Op.GTE, "config.key6", 100.0)) + .or(org.hypertrace.core.documentstore.Filter.eq("config.key7", "value7")); assertEquals(docFilter.toString(), filterBuilder.buildDocStoreFilter(filter).toString()); } diff --git a/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index 72fdcec0..09aae8ad 100644 --- a/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -14,6 +14,7 @@ import org.hypertrace.config.service.v1.DeleteConfigRequest; import org.hypertrace.config.service.v1.DeleteConfigsRequest; import org.hypertrace.config.service.v1.DeleteConfigsRequest.ConfigToDelete; +import org.hypertrace.config.service.v1.Filter; import org.hypertrace.config.service.v1.GetAllConfigsRequest; import org.hypertrace.config.service.v1.GetConfigRequest; import org.hypertrace.config.service.v1.GetConfigResponse; @@ -153,24 +154,39 @@ public Optional getData(RequestContext context, String id) { return getObject(context, id).map(ConfigObject::getData); } - public ContextualConfigObject upsertObject(RequestContext context, T data) { + private ContextualConfigObject upsertObject( + RequestContext context, UpsertConfigRequest request) { UpsertConfigResponse response = context.call( - () -> - this.configServiceBlockingStub - .withDeadline(getDeadline()) - .upsertConfig( - UpsertConfigRequest.newBuilder() - .setResourceName(this.resourceName) - .setResourceNamespace(this.resourceNamespace) - .setContext(this.getContextFromData(data)) - .setConfig(this.buildValueFromData(data)) - .build())); + () -> this.configServiceBlockingStub.withDeadline(getDeadline()).upsertConfig(request)); return this.processUpsertResult(context, response) .orElseThrow(Status.INTERNAL::asRuntimeException); } + public ContextualConfigObject upsertObject(RequestContext context, T data) { + return this.upsertObject( + context, + UpsertConfigRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setContext(this.getContextFromData(data)) + .setConfig(this.buildValueFromData(data)) + .build()); + } + + protected ContextualConfigObject upsertObject(RequestContext context, T data, Filter filter) { + return this.upsertObject( + context, + UpsertConfigRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setContext(this.getContextFromData(data)) + .setConfig(this.buildValueFromData(data)) + .setUpsertCondition(filter) + .build()); + } + public Optional> deleteObject( RequestContext requestContext, String context) { try {