From 3ebee57ce8829b011ba6718629061fa8138b92d9 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 7 Apr 2025 17:25:24 +0530 Subject: [PATCH 1/6] Add generic api's for filtering, pagination in getAllConfigs call --- .../config/service/v1/config_service.proto | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto index 24c43262..c80f5124 100644 --- a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto +++ b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto @@ -88,11 +88,29 @@ message GetAllConfigsRequest { // required - namespace with which the config resource is associated string resource_namespace = 2; + + // optional - filtering criteria to narrow down the configs. + // Supports relational and logical operators on config fields. + optional Filter filter = 3; + + // optional - list of sorting conditions to order the results. + // Multiple SortBy entries are applied in the specified order of priority. + repeated SortBy sort_by = 4; + + // optional - pagination parameters to limit and offset the result set. + // Useful for retrieving configs in pages when total count is large. + optional Pagination pagination = 5; } message GetAllConfigsResponse { // list of config values along with the associated contexts, sorted in the descending order of their creation time repeated ContextSpecificConfig context_specific_configs = 1; + + // Total number of records matching the filter before pagination + int32 total_count = 2; + + // next offset to use for pagination + int32 next_offset = 3; } message ContextSpecificConfig { @@ -201,3 +219,25 @@ enum LogicalOperator { LOGICAL_OPERATOR_AND = 1; LOGICAL_OPERATOR_OR = 2; } + +message SortBy { + Selection selection = 1; + SortOrder sort_order = 2; +} + +message Pagination { + int32 limit = 1; + int32 offset = 2; +} + +message Selection { + oneof type { + string config_json_path = 1; + } +} + +enum SortOrder { + SORT_ORDER_UNSPECIFIED = 0; + SORT_ORDER_ASC = 1; + SORT_ORDER_DESC = 2; +} From 6c210448e638ae3ec9d0befbe7c2266dec253b2f Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 7 Apr 2025 21:55:51 +0530 Subject: [PATCH 2/6] Addressed comments --- .../config/service/v1/config_service.proto | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto index c80f5124..afc3930e 100644 --- a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto +++ b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto @@ -91,7 +91,7 @@ message GetAllConfigsRequest { // optional - filtering criteria to narrow down the configs. // Supports relational and logical operators on config fields. - optional Filter filter = 3; + Filter filter = 3; // optional - list of sorting conditions to order the results. // Multiple SortBy entries are applied in the specified order of priority. @@ -99,18 +99,18 @@ message GetAllConfigsRequest { // optional - pagination parameters to limit and offset the result set. // Useful for retrieving configs in pages when total count is large. - optional Pagination pagination = 5; + Pagination pagination = 5; + + // optional - include total count in the response + bool include_total = 6; } message GetAllConfigsResponse { // list of config values along with the associated contexts, sorted in the descending order of their creation time repeated ContextSpecificConfig context_specific_configs = 1; - // Total number of records matching the filter before pagination + // Total number of records matching the filter before pagination if include_total is true. int32 total_count = 2; - - // next offset to use for pagination - int32 next_offset = 3; } message ContextSpecificConfig { From b10aa5794f501c791acc963cae66e8f2bfe984ed Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 8 Apr 2025 09:00:43 +0530 Subject: [PATCH 3/6] Make total count optional --- .../proto/org/hypertrace/config/service/v1/config_service.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto index afc3930e..30cddf0a 100644 --- a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto +++ b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto @@ -110,7 +110,7 @@ message GetAllConfigsResponse { repeated ContextSpecificConfig context_specific_configs = 1; // Total number of records matching the filter before pagination if include_total is true. - int32 total_count = 2; + optional int32 total_count = 2; } message ContextSpecificConfig { From f1dc3508badb32d68bda4b0894da5b9b52960f13 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 14 Apr 2025 16:53:27 +0530 Subject: [PATCH 4/6] Add implementation Filtering, pagination, Sorting in getAllConfigsAPI --- .../config/service/v1/config_service.proto | 6 -- .../config/service/ConfigServiceGrpcImpl.java | 5 +- .../config/service/store/ConfigStore.java | 10 +- .../store/ConstantExpressionConverter.java | 48 +++++++++ .../service/store/DocumentConfigStore.java | 95 +++++++++++++++--- .../store/FilterExpressionBuilder.java | 98 +++++++++++++++++++ .../service/ConfigServiceGrpcImplTest.java | 91 ++++++++++++++++- .../store/DocumentConfigStoreTest.java | 7 +- 8 files changed, 334 insertions(+), 26 deletions(-) create mode 100644 config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java create mode 100644 config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java diff --git a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto index 30cddf0a..c3e480db 100644 --- a/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto +++ b/config-service-api/src/main/proto/org/hypertrace/config/service/v1/config_service.proto @@ -100,17 +100,11 @@ message GetAllConfigsRequest { // optional - pagination parameters to limit and offset the result set. // Useful for retrieving configs in pages when total count is large. Pagination pagination = 5; - - // optional - include total count in the response - bool include_total = 6; } message GetAllConfigsResponse { // list of config values along with the associated contexts, sorted in the descending order of their creation time repeated ContextSpecificConfig context_specific_configs = 1; - - // Total number of records matching the filter before pagination if include_total is true. - optional int32 total_count = 2; } message ContextSpecificConfig { diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java index 7f1aded5..60ac64f7 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceGrpcImpl.java @@ -118,7 +118,10 @@ public void getAllConfigs( List contextSpecificConfigList = configStore.getAllConfigs( new ConfigResource( - request.getResourceName(), request.getResourceNamespace(), getTenantId())); + request.getResourceName(), request.getResourceNamespace(), getTenantId()), + request.getFilter(), + request.getPagination(), + request.getSortByList()); responseObserver.onNext( GetAllConfigsResponse.newBuilder() .addAllContextSpecificConfigs(contextSpecificConfigList) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java index 68293ca4..2f7e4c8d 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigStore.java @@ -9,6 +9,9 @@ import org.hypertrace.config.service.ConfigResource; import org.hypertrace.config.service.ConfigResourceContext; import org.hypertrace.config.service.v1.ContextSpecificConfig; +import org.hypertrace.config.service.v1.Filter; +import org.hypertrace.config.service.v1.Pagination; +import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; @@ -57,10 +60,15 @@ Map getContextConfigs( * specified parameters, sorted in the descending order of their creation time. * * @param configResource + * @param filter + * @param pagination + * @param sortByList * @return * @throws IOException */ - List getAllConfigs(ConfigResource configResource) throws IOException; + List getAllConfigs( + ConfigResource configResource, Filter filter, Pagination pagination, List sortByList) + throws IOException; /** * Write each of the provided config value associated with the specified config resource to the diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java new file mode 100644 index 00000000..17c3ad44 --- /dev/null +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java @@ -0,0 +1,48 @@ +package org.hypertrace.config.service.store; + +import com.google.protobuf.Value; +import java.util.List; +import java.util.stream.Collectors; +import org.hypertrace.core.documentstore.expression.impl.ConstantExpression; + +public class ConstantExpressionConverter { + + public static ConstantExpression fromProtoValue(Value value) { + switch (value.getKindCase()) { + case STRING_VALUE: + return ConstantExpression.of(value.getStringValue()); + case NUMBER_VALUE: + return ConstantExpression.of(value.getNumberValue()); + case BOOL_VALUE: + return ConstantExpression.of(value.getBoolValue()); + case LIST_VALUE: + List values = value.getListValue().getValuesList(); + // Infer and cast list element types (assumes homogeneous list) + if (values.isEmpty()) { + throw new IllegalArgumentException("Empty list not supported in ConstantExpression"); + } + Value.KindCase elementType = values.get(0).getKindCase(); + switch (elementType) { + case STRING_VALUE: + return ConstantExpression.ofStrings( + values.stream().map(Value::getStringValue).collect(Collectors.toList())); + case NUMBER_VALUE: + return ConstantExpression.ofNumbers( + values.stream().map(Value::getNumberValue).collect(Collectors.toList())); + case BOOL_VALUE: + return ConstantExpression.ofBooleans( + values.stream().map(Value::getBoolValue).collect(Collectors.toList())); + default: + throw new UnsupportedOperationException( + "Unsupported list element type: " + elementType); + } + + case STRUCT_VALUE: + throw new UnsupportedOperationException("Struct not supported directly in this context"); + case NULL_VALUE: + case KIND_NOT_SET: + default: + throw new IllegalArgumentException("Unsupported or null value in ConstantExpression"); + } + } +} diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java index 9685162b..e6b083d6 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java @@ -24,11 +24,13 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.ConfigResource; import org.hypertrace.config.service.ConfigResourceContext; import org.hypertrace.config.service.ConfigServiceUtils; import org.hypertrace.config.service.v1.ContextSpecificConfig; +import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; import org.hypertrace.core.documentstore.CloseableIterator; @@ -58,12 +60,14 @@ public class DocumentConfigStore implements ConfigStore { private final Datastore datastore; private final Collection collection; private final FilterBuilder filterBuilder; + private final FilterExpressionBuilder filterExpressionBuilder; public DocumentConfigStore(Clock clock, Datastore datastore) { this.clock = clock; this.datastore = datastore; this.collection = this.datastore.getCollection(CONFIGURATIONS_COLLECTION); this.filterBuilder = new FilterBuilder(); + this.filterExpressionBuilder = new FilterExpressionBuilder(); } @Override @@ -208,30 +212,89 @@ public Map getContextConfigs( } @Override - public List getAllConfigs(ConfigResource configResource) + public List getAllConfigs( + ConfigResource configResource, + org.hypertrace.config.service.v1.Filter filter, + org.hypertrace.config.service.v1.Pagination pagination, + List sortByList) throws IOException { - Query query = - Query.builder() - .addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC) - .setFilter(getConfigResourceFilterTypeExpression(configResource)) - .build(); - List contextSpecificConfigList = new ArrayList<>(); + + Query query = buildQuery(configResource, filter, pagination, sortByList); + List configList = new ArrayList<>(); Set seenContexts = new HashSet<>(); + try (CloseableIterator documentIterator = collection.query(query, QueryOptions.DEFAULT_QUERY_OPTIONS)) { while (documentIterator.hasNext()) { - String documentString = documentIterator.next().toJson(); - ConfigDocument configDocument = ConfigDocument.fromJson(documentString); - String context = configDocument.getContext(); - if (seenContexts.add(context)) { - convertToContextSpecificConfig(configDocument).ifPresent(contextSpecificConfigList::add); - } + processDocument(documentIterator.next(), seenContexts, configList); } } - Collections.sort( - contextSpecificConfigList, + + configList.sort( Comparator.comparingLong(ContextSpecificConfig::getCreationTimestamp).reversed()); - return contextSpecificConfigList; + return configList; + } + + private Query buildQuery( + ConfigResource configResource, + org.hypertrace.config.service.v1.Filter filter, + org.hypertrace.config.service.v1.Pagination pagination, + List sortByList) { + + FilterTypeExpression combinedFilter = getCombinedFilter(configResource, filter); + Query.QueryBuilder queryBuilder = Query.builder().setFilter(combinedFilter); + if (pagination != null + && !pagination.equals(org.hypertrace.config.service.v1.Pagination.getDefaultInstance())) { + queryBuilder.setPagination( + Pagination.builder().offset(pagination.getOffset()).limit(pagination.getLimit()).build()); + } + + if (sortByList != null) { + sortByList.forEach( + sortBy -> + queryBuilder.addSort( + IdentifierExpression.of(sortBy.getSelection().getConfigJsonPath()), + convertSortOrder(sortBy))); + } + + queryBuilder.addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC); + return queryBuilder.build(); + } + + private FilterTypeExpression getCombinedFilter( + ConfigResource configResource, org.hypertrace.config.service.v1.Filter additionalFilter) { + + FilterTypeExpression resourceFilter = getConfigResourceFilterTypeExpression(configResource); + if (additionalFilter == null + || additionalFilter.equals(org.hypertrace.config.service.v1.Filter.getDefaultInstance())) { + return resourceFilter; + } + + FilterTypeExpression docStoreFilter = + filterExpressionBuilder.buildFilterTypeExpression(additionalFilter); + return LogicalExpression.and(resourceFilter, docStoreFilter); + } + + @SneakyThrows + private void processDocument( + Document document, Set seenContexts, List configList) { + + ConfigDocument configDocument = ConfigDocument.fromJson(document.toJson()); + String context = configDocument.getContext(); + + if (seenContexts.add(context)) { + convertToContextSpecificConfig(configDocument).ifPresent(configList::add); + } + } + + private static SortOrder convertSortOrder(SortBy sortBy) { + switch (sortBy.getSortOrder()) { + case SORT_ORDER_DESC: + return SortOrder.DESC; + case SORT_ORDER_ASC: + default: + return SortOrder.ASC; + } } @Override diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java new file mode 100644 index 00000000..6d60de99 --- /dev/null +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java @@ -0,0 +1,98 @@ +package org.hypertrace.config.service.store; + +import static org.hypertrace.config.service.store.ConfigDocument.CONFIG_FIELD_NAME; + +import io.grpc.Status; +import java.util.List; +import java.util.stream.Collectors; +import org.hypertrace.config.service.v1.Filter; +import org.hypertrace.config.service.v1.LogicalFilter; +import org.hypertrace.config.service.v1.RelationalFilter; +import org.hypertrace.core.documentstore.expression.impl.IdentifierExpression; +import org.hypertrace.core.documentstore.expression.impl.LogicalExpression; +import org.hypertrace.core.documentstore.expression.impl.RelationalExpression; +import org.hypertrace.core.documentstore.expression.operators.LogicalOperator; +import org.hypertrace.core.documentstore.expression.operators.RelationalOperator; +import org.hypertrace.core.documentstore.expression.type.FilterTypeExpression; + +public class FilterExpressionBuilder { + + public FilterTypeExpression buildFilterTypeExpression(Filter filter) { + switch (filter.getTypeCase()) { + case LOGICAL_FILTER: + return evaluateLogicalExpression(filter.getLogicalFilter()); + case RELATIONAL_FILTER: + return evaluateRelationalExpression(filter.getRelationalFilter()); + case TYPE_NOT_SET: + default: + throw Status.INVALID_ARGUMENT.withDescription("Filter type unset").asRuntimeException(); + } + } + + private FilterTypeExpression evaluateLogicalExpression(LogicalFilter logicalFilter) { + List childExpressions = + logicalFilter.getOperandsList().stream() + .map(this::buildFilterTypeExpression) + .collect(Collectors.toUnmodifiableList()); + + LogicalOperator operator; + switch (logicalFilter.getOperator()) { + case LOGICAL_OPERATOR_AND: + operator = LogicalOperator.AND; + break; + case LOGICAL_OPERATOR_OR: + operator = LogicalOperator.OR; + break; + case LOGICAL_OPERATOR_UNSPECIFIED: + default: + throw Status.INVALID_ARGUMENT + .withDescription("Unknown logical operator while building expression") + .asRuntimeException(); + } + return LogicalExpression.builder().operator(operator).operands(childExpressions).build(); + } + + private FilterTypeExpression evaluateRelationalExpression(RelationalFilter relationalFilter) { + RelationalOperator operator; + switch (relationalFilter.getOperator()) { + case RELATIONAL_OPERATOR_EQ: + operator = RelationalOperator.EQ; + break; + case RELATIONAL_OPERATOR_NEQ: + operator = RelationalOperator.NEQ; + break; + case RELATIONAL_OPERATOR_IN: + operator = RelationalOperator.IN; + break; + case RELATIONAL_OPERATOR_NOT_IN: + operator = RelationalOperator.NOT_IN; + break; + case RELATIONAL_OPERATOR_LT: + operator = RelationalOperator.LT; + break; + case RELATIONAL_OPERATOR_GT: + operator = RelationalOperator.GT; + break; + case RELATIONAL_OPERATOR_LTE: + operator = RelationalOperator.LTE; + break; + case RELATIONAL_OPERATOR_GTE: + operator = RelationalOperator.GTE; + break; + case UNRECOGNIZED: + default: + throw Status.INVALID_ARGUMENT + .withDescription("Unknown relational operator while building expression") + .asRuntimeException(); + } + + return RelationalExpression.of( + IdentifierExpression.of(buildConfigFieldPath(relationalFilter.getConfigJsonPath())), + operator, + ConstantExpressionConverter.fromProtoValue(relationalFilter.getValue())); + } + + private String buildConfigFieldPath(String configJsonPath) { + return String.format("%s.%s", CONFIG_FIELD_NAME, configJsonPath); + } +} diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java index d224e3b3..71421f31 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java @@ -28,6 +28,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -39,10 +40,17 @@ import org.hypertrace.config.service.v1.DeleteConfigsRequest; import org.hypertrace.config.service.v1.DeleteConfigsRequest.ConfigToDelete; import org.hypertrace.config.service.v1.DeleteConfigsResponse; +import org.hypertrace.config.service.v1.Filter; import org.hypertrace.config.service.v1.GetAllConfigsRequest; import org.hypertrace.config.service.v1.GetAllConfigsResponse; import org.hypertrace.config.service.v1.GetConfigRequest; import org.hypertrace.config.service.v1.GetConfigResponse; +import org.hypertrace.config.service.v1.Pagination; +import org.hypertrace.config.service.v1.RelationalFilter; +import org.hypertrace.config.service.v1.RelationalOperator; +import org.hypertrace.config.service.v1.Selection; +import org.hypertrace.config.service.v1.SortBy; +import org.hypertrace.config.service.v1.SortOrder; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; import org.hypertrace.config.service.v1.UpsertConfigResponse; @@ -175,7 +183,15 @@ void getAllConfigs() throws IOException { contextSpecificConfigList.add( ContextSpecificConfig.newBuilder().setContext(CONTEXT1).setConfig(config2).build()); when(configStore.getAllConfigs( - new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID))) + argThat( + resource -> + resource != null + && RESOURCE_NAME.equals(resource.getResourceName()) + && RESOURCE_NAMESPACE.equals(resource.getResourceNamespace()) + && TENANT_ID.equals(resource.getTenantId())), + eq(Filter.getDefaultInstance()), // for empty filter + eq(Pagination.getDefaultInstance()), + eq(Collections.emptyList()))) .thenReturn(contextSpecificConfigList); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -194,6 +210,79 @@ void getAllConfigs() throws IOException { assertEquals(contextSpecificConfigList, actualResponse.getContextSpecificConfigsList()); } + @Test + void getAllConfigs_withFilterPaginationAndSorting() throws IOException { + ConfigStore configStore = mock(ConfigStore.class); + + // Define filter + Filter filter = + Filter.newBuilder() + .setRelationalFilter( + RelationalFilter.newBuilder() + .setOperator(RelationalOperator.RELATIONAL_OPERATOR_EQ) + .setConfigJsonPath("spec.id") + .setValue(Value.newBuilder().setStringValue("id1").build()) + .build()) + .build(); + + // Define pagination + Pagination pagination = Pagination.newBuilder().setLimit(5).setOffset(10).build(); + + // Define sortBy + SortBy sortBy = + SortBy.newBuilder() + .setSelection(Selection.newBuilder().setConfigJsonPath("spec.id").build()) + .setSortOrder(SortOrder.SORT_ORDER_ASC) + .build(); + + // Mocked configs returned from store + List contextSpecificConfigList = + List.of( + ContextSpecificConfig.newBuilder().setConfig(config1).build(), + ContextSpecificConfig.newBuilder().setContext(CONTEXT1).setConfig(config2).build()); + + // Mock behavior + when(configStore.getAllConfigs( + argThat( + resource -> + resource != null + && RESOURCE_NAME.equals(resource.getResourceName()) + && RESOURCE_NAMESPACE.equals(resource.getResourceNamespace()) + && TENANT_ID.equals(resource.getTenantId())), + eq(filter), + eq(pagination), + eq(List.of(sortBy)))) + .thenReturn(contextSpecificConfigList); + + ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); + StreamObserver responseObserver = mock(StreamObserver.class); + + // Build request with all fields + GetAllConfigsRequest request = + GetAllConfigsRequest.newBuilder() + .setResourceName(RESOURCE_NAME) + .setResourceNamespace(RESOURCE_NAMESPACE) + .setFilter(filter) + .setPagination(pagination) + .addSortBy(sortBy) + .build(); + + // Execute in tenant context + Runnable runnable = () -> configServiceGrpc.getAllConfigs(request, responseObserver); + RequestContext.forTenantId(TENANT_ID).run(runnable); + + // Verify response + ArgumentCaptor responseCaptor = + ArgumentCaptor.forClass(GetAllConfigsResponse.class); + + verify(responseObserver).onNext(responseCaptor.capture()); + verify(responseObserver).onCompleted(); + verify(responseObserver, never()).onError(any()); + + GetAllConfigsResponse actualResponse = responseCaptor.getValue(); + assertEquals(contextSpecificConfigList, actualResponse.getContextSpecificConfigsList()); + } + @Test void deleteConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java index db61f41b..2bea1b30 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java @@ -34,6 +34,7 @@ import org.hypertrace.config.service.v1.Filter; import org.hypertrace.config.service.v1.LogicalFilter; import org.hypertrace.config.service.v1.LogicalOperator; +import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.RelationalFilter; import org.hypertrace.config.service.v1.RelationalOperator; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -268,7 +269,11 @@ void getAllConfigs() throws IOException { when(collection.query(any(Query.class), any())).thenReturn(iterator); List contextSpecificConfigList = - configStore.getAllConfigs(new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID)); + configStore.getAllConfigs( + new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID), + Filter.getDefaultInstance(), // for empty filter + Pagination.getDefaultInstance(), + Collections.emptyList()); assertEquals(2, contextSpecificConfigList.size()); assertEquals( ContextSpecificConfig.newBuilder() From ddcdf318318639722b1aaefb81fb53357bb2c806 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 15 Apr 2025 11:07:18 +0530 Subject: [PATCH 5/6] Addressed comments --- .../store/ConstantExpressionConverter.java | 21 ++++++++++++------- .../service/store/DocumentConfigStore.java | 20 +++++++++--------- .../store/FilterExpressionBuilder.java | 8 +++---- .../service/ConfigServiceGrpcImplTest.java | 7 +------ 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java index 17c3ad44..4d0cf848 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java @@ -1,6 +1,7 @@ package org.hypertrace.config.service.store; import com.google.protobuf.Value; +import io.grpc.Status; import java.util.List; import java.util.stream.Collectors; import org.hypertrace.core.documentstore.expression.impl.ConstantExpression; @@ -17,10 +18,12 @@ public static ConstantExpression fromProtoValue(Value value) { return ConstantExpression.of(value.getBoolValue()); case LIST_VALUE: List values = value.getListValue().getValuesList(); - // Infer and cast list element types (assumes homogeneous list) if (values.isEmpty()) { - throw new IllegalArgumentException("Empty list not supported in ConstantExpression"); + // Default to empty string list — or change logic based on expected behavior + return ConstantExpression.ofStrings(List.of()); } + + // Infer and cast list element types (assumes homogeneous list) Value.KindCase elementType = values.get(0).getKindCase(); switch (elementType) { case STRING_VALUE: @@ -33,16 +36,20 @@ public static ConstantExpression fromProtoValue(Value value) { return ConstantExpression.ofBooleans( values.stream().map(Value::getBoolValue).collect(Collectors.toList())); default: - throw new UnsupportedOperationException( - "Unsupported list element type: " + elementType); + throw Status.UNIMPLEMENTED + .withDescription("Unsupported list element type: " + elementType) + .asRuntimeException(); } - case STRUCT_VALUE: - throw new UnsupportedOperationException("Struct not supported directly in this context"); + throw Status.UNIMPLEMENTED + .withDescription("Struct not supported directly in ConstantExpression") + .asRuntimeException(); case NULL_VALUE: case KIND_NOT_SET: default: - throw new IllegalArgumentException("Unsupported or null value in ConstantExpression"); + throw Status.INVALID_ARGUMENT + .withDescription("Unsupported or null value in ConstantExpression") + .asRuntimeException(); } } } diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java index e6b083d6..dd5efa84 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java @@ -24,6 +24,7 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import lombok.NonNull; import lombok.SneakyThrows; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.ConfigResource; @@ -237,36 +238,35 @@ public List getAllConfigs( private Query buildQuery( ConfigResource configResource, - org.hypertrace.config.service.v1.Filter filter, - org.hypertrace.config.service.v1.Pagination pagination, + @NonNull org.hypertrace.config.service.v1.Filter filter, + @NonNull org.hypertrace.config.service.v1.Pagination pagination, List sortByList) { FilterTypeExpression combinedFilter = getCombinedFilter(configResource, filter); Query.QueryBuilder queryBuilder = Query.builder().setFilter(combinedFilter); - if (pagination != null - && !pagination.equals(org.hypertrace.config.service.v1.Pagination.getDefaultInstance())) { + if (!pagination.equals(org.hypertrace.config.service.v1.Pagination.getDefaultInstance())) { queryBuilder.setPagination( Pagination.builder().offset(pagination.getOffset()).limit(pagination.getLimit()).build()); } - if (sortByList != null) { + if (!sortByList.isEmpty()) { sortByList.forEach( sortBy -> queryBuilder.addSort( IdentifierExpression.of(sortBy.getSelection().getConfigJsonPath()), convertSortOrder(sortBy))); + } else { + queryBuilder.addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC); } - - queryBuilder.addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC); return queryBuilder.build(); } private FilterTypeExpression getCombinedFilter( - ConfigResource configResource, org.hypertrace.config.service.v1.Filter additionalFilter) { + ConfigResource configResource, + @NonNull org.hypertrace.config.service.v1.Filter additionalFilter) { FilterTypeExpression resourceFilter = getConfigResourceFilterTypeExpression(configResource); - if (additionalFilter == null - || additionalFilter.equals(org.hypertrace.config.service.v1.Filter.getDefaultInstance())) { + if (additionalFilter.equals(org.hypertrace.config.service.v1.Filter.getDefaultInstance())) { return resourceFilter; } diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java index 6d60de99..7ad61dde 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java @@ -20,16 +20,16 @@ public class FilterExpressionBuilder { public FilterTypeExpression buildFilterTypeExpression(Filter filter) { switch (filter.getTypeCase()) { case LOGICAL_FILTER: - return evaluateLogicalExpression(filter.getLogicalFilter()); + return buildLogicalExpression(filter.getLogicalFilter()); case RELATIONAL_FILTER: - return evaluateRelationalExpression(filter.getRelationalFilter()); + return buildRelationalExpression(filter.getRelationalFilter()); case TYPE_NOT_SET: default: throw Status.INVALID_ARGUMENT.withDescription("Filter type unset").asRuntimeException(); } } - private FilterTypeExpression evaluateLogicalExpression(LogicalFilter logicalFilter) { + private FilterTypeExpression buildLogicalExpression(LogicalFilter logicalFilter) { List childExpressions = logicalFilter.getOperandsList().stream() .map(this::buildFilterTypeExpression) @@ -52,7 +52,7 @@ private FilterTypeExpression evaluateLogicalExpression(LogicalFilter logicalFilt return LogicalExpression.builder().operator(operator).operands(childExpressions).build(); } - private FilterTypeExpression evaluateRelationalExpression(RelationalFilter relationalFilter) { + private FilterTypeExpression buildRelationalExpression(RelationalFilter relationalFilter) { RelationalOperator operator; switch (relationalFilter.getOperator()) { case RELATIONAL_OPERATOR_EQ: diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java index 71421f31..4e34f7f0 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java @@ -183,12 +183,7 @@ void getAllConfigs() throws IOException { contextSpecificConfigList.add( ContextSpecificConfig.newBuilder().setContext(CONTEXT1).setConfig(config2).build()); when(configStore.getAllConfigs( - argThat( - resource -> - resource != null - && RESOURCE_NAME.equals(resource.getResourceName()) - && RESOURCE_NAMESPACE.equals(resource.getResourceNamespace()) - && TENANT_ID.equals(resource.getTenantId())), + eq(new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID)), eq(Filter.getDefaultInstance()), // for empty filter eq(Pagination.getDefaultInstance()), eq(Collections.emptyList()))) From 6a3bdaa208874deec01dea10dea0e705286842cd Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 15 Apr 2025 19:44:14 +0530 Subject: [PATCH 6/6] Addressed comments --- .../config/service/store/ConstantExpressionConverter.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java index 4d0cf848..f74e7bf2 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java @@ -23,8 +23,14 @@ public static ConstantExpression fromProtoValue(Value value) { return ConstantExpression.ofStrings(List.of()); } - // Infer and cast list element types (assumes homogeneous list) Value.KindCase elementType = values.get(0).getKindCase(); + boolean isHomogeneous = values.stream().allMatch(v -> v.getKindCase() == elementType); + + if (!isHomogeneous) { + throw Status.INVALID_ARGUMENT + .withDescription("List contains mixed types. All elements must be of the same type.") + .asRuntimeException(); + } switch (elementType) { case STRING_VALUE: return ConstantExpression.ofStrings(