From 3ebee57ce8829b011ba6718629061fa8138b92d9 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 7 Apr 2025 17:25:24 +0530 Subject: [PATCH 01/30] 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 02/30] 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 03/30] 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 04/30] 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 05/30] 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 e838560dd37d11a47962c6bc5c0f1816249dda3d Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 15 Apr 2025 19:34:48 +0530 Subject: [PATCH 06/30] Updated object store with Filtering and Pagination --- ...IdentifiedFilterPushedDownObjectStore.java | 42 +++++++++++++++++++ .../objectstore/IdentifiedObjectStore.java | 29 +++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java new file mode 100644 index 00000000..d758f8a2 --- /dev/null +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -0,0 +1,42 @@ +package org.hypertrace.config.objectstore; + +import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator; +import org.hypertrace.config.service.v1.ConfigServiceGrpc; +import org.hypertrace.config.service.v1.Filter; +import org.hypertrace.config.service.v1.Pagination; +import org.hypertrace.config.service.v1.SortBy; +import org.hypertrace.core.grpcutils.context.RequestContext; + +import java.util.Collections; +import java.util.List; + +public abstract class IdentifiedFilterPushedDownObjectStore extends IdentifiedObjectStore { + + protected IdentifiedFilterPushedDownObjectStore( + ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub, + String resourceNamespace, + String resourceName, + ConfigChangeEventGenerator configChangeEventGenerator, + ClientConfig clientConfig) { + super( + configServiceBlockingStub, + resourceNamespace, + resourceName, + configChangeEventGenerator, + clientConfig); + } + + public List> getAllObjects( + RequestContext context, F filterInput, Pagination pagination) { + Filter filter = buildFilter(filterInput); + List sortByList = buildSortByList(filterInput); + return getAllObjectsWithFilter(context, filter, pagination, sortByList); + } + + protected abstract Filter buildFilter(F filterInput); + + /** Constructs a list of SortBy from client filter or returns an empty list. */ + protected List buildSortByList(F filterInput) { + return Collections.emptyList(); // Override if needed + } +} diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index 09aae8ad..acb42be9 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -18,6 +18,8 @@ import org.hypertrace.config.service.v1.GetAllConfigsRequest; 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.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest.ConfigToUpsert; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -114,6 +116,33 @@ public List> getAllObjects(RequestContext context) { Collectors.toUnmodifiableList(), this::orderFetchedObjects)); } + public List> getAllObjectsWithFilter( + RequestContext context, Filter filter, Pagination pagination, List sortByList) { + return context + .call( + () -> + this.configServiceBlockingStub + .withDeadline(getDeadline()) + .getAllConfigs( + GetAllConfigsRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setFilter(filter) + .setPagination(pagination) + .addAllSortBy(sortByList) + .build())) + .getContextSpecificConfigsList() + .stream() + .map( + contextSpecificConfig -> + ContextualConfigObjectImpl.tryBuild( + contextSpecificConfig, this::buildDataFromValue)) + .flatMap(Optional::stream) + .collect( + Collectors.collectingAndThen( + Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + } + public List getAllConfigData(RequestContext context) { return getAllObjects(context).stream() .map(ConfigObject::getData) From 6a3bdaa208874deec01dea10dea0e705286842cd Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 15 Apr 2025 19:44:14 +0530 Subject: [PATCH 07/30] 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( From b09be4d3a0f1f37dc0a2bfe9adaa21f8e5e977d7 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 15 Apr 2025 22:01:32 +0530 Subject: [PATCH 08/30] Addressed comments --- ...IdentifiedFilterPushedDownObjectStore.java | 51 ++++++++++++++----- .../objectstore/IdentifiedObjectStore.java | 2 +- 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index d758f8a2..a252d9e3 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -1,5 +1,8 @@ package org.hypertrace.config.objectstore; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator; import org.hypertrace.config.service.v1.ConfigServiceGrpc; import org.hypertrace.config.service.v1.Filter; @@ -7,10 +10,8 @@ import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.core.grpcutils.context.RequestContext; -import java.util.Collections; -import java.util.List; - -public abstract class IdentifiedFilterPushedDownObjectStore extends IdentifiedObjectStore { +public abstract class IdentifiedFilterPushedDownObjectStore + extends IdentifiedObjectStore { protected IdentifiedFilterPushedDownObjectStore( ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub, @@ -26,17 +27,43 @@ protected IdentifiedFilterPushedDownObjectStore( clientConfig); } - public List> getAllObjects( - RequestContext context, F filterInput, Pagination pagination) { + public List> getMatchingObjects( + RequestContext context, F filterInput, List sortInput, Pagination pagination) { Filter filter = buildFilter(filterInput); - List sortByList = buildSortByList(filterInput); - return getAllObjectsWithFilter(context, filter, pagination, sortByList); + List sortByList = buildSort(sortInput); + return getMatchingObjectsWithFilter(context, filter, pagination, sortByList); } - protected abstract Filter buildFilter(F filterInput); + public List getMatchingConfigData( + RequestContext context, F filterInput, List sortInput, Pagination pagination) { + return getMatchingObjects(context, filterInput, sortInput, pagination).stream() + .map(ConfigObject::getData) + .collect(Collectors.toUnmodifiableList()); + } + + public Optional> getObject( + RequestContext context, String id, F filter) { + return getObject(context, id).flatMap(configObject -> filterObject(configObject, filter)); + } - /** Constructs a list of SortBy from client filter or returns an empty list. */ - protected List buildSortByList(F filterInput) { - return Collections.emptyList(); // Override if needed + public Optional getData(RequestContext context, String id, F filter) { + return getObject(context, id, filter).map(ConfigObject::getData); } + + private Optional> filterObject( + ContextualConfigObject configObject, F filter) { + return filterConfigData(configObject.getData(), filter) + .map(filteredData -> updateConfigData(configObject, filteredData)); + } + + private ContextualConfigObject updateConfigData( + ContextualConfigObject configObject, T updatedData) { + return ((ContextualConfigObjectImpl) configObject).toBuilder().data(updatedData).build(); + } + + protected abstract Optional filterConfigData(T data, F filter); + + protected abstract Filter buildFilter(F filterInput); + + protected abstract List buildSort(List sortInput); } diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index acb42be9..75e2191e 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -116,7 +116,7 @@ public List> getAllObjects(RequestContext context) { Collectors.toUnmodifiableList(), this::orderFetchedObjects)); } - public List> getAllObjectsWithFilter( + List> getMatchingObjectsWithFilter( RequestContext context, Filter filter, Pagination pagination, List sortByList) { return context .call( From 1065b9d8e5c1b72b57152a7d20721d26e96c2372 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Wed, 16 Apr 2025 16:43:03 +0530 Subject: [PATCH 09/30] Addressed comments --- ...IdentifiedFilterPushedDownObjectStore.java | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index a252d9e3..e7ce6088 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -1,5 +1,6 @@ package org.hypertrace.config.objectstore; +import io.grpc.Status; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -30,40 +31,34 @@ protected IdentifiedFilterPushedDownObjectStore( public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput, Pagination pagination) { Filter filter = buildFilter(filterInput); - List sortByList = buildSort(sortInput); + List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); return getMatchingObjectsWithFilter(context, filter, pagination, sortByList); } - public List getMatchingConfigData( + public List getMatchingData( RequestContext context, F filterInput, List sortInput, Pagination pagination) { return getMatchingObjects(context, filterInput, sortInput, pagination).stream() .map(ConfigObject::getData) .collect(Collectors.toUnmodifiableList()); } - public Optional> getObject( - RequestContext context, String id, F filter) { - return getObject(context, id).flatMap(configObject -> filterObject(configObject, filter)); + public Optional> getMatchingObject( + RequestContext context, F filterInput, List sortInput) { + List> results = + getMatchingObjects(context, filterInput, sortInput, Pagination.getDefaultInstance()); + if (results.size() > 1) { + throw Status.FAILED_PRECONDITION + .withDescription("Multiple objects found when expecting at most one") + .asRuntimeException(); + } + return results.stream().findFirst(); } - public Optional getData(RequestContext context, String id, F filter) { - return getObject(context, id, filter).map(ConfigObject::getData); + public Optional getMatchingData(RequestContext context, F filterInput, List sortInput) { + return getMatchingObject(context, filterInput, sortInput).map(ConfigObject::getData); } - private Optional> filterObject( - ContextualConfigObject configObject, F filter) { - return filterConfigData(configObject.getData(), filter) - .map(filteredData -> updateConfigData(configObject, filteredData)); - } - - private ContextualConfigObject updateConfigData( - ContextualConfigObject configObject, T updatedData) { - return ((ContextualConfigObjectImpl) configObject).toBuilder().data(updatedData).build(); - } - - protected abstract Optional filterConfigData(T data, F filter); + protected abstract SortBy buildSort(S sortInput); protected abstract Filter buildFilter(F filterInput); - - protected abstract List buildSort(List sortInput); } From 0f245519c6809ab8f953e0fb54e86be2962cb975 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Wed, 16 Apr 2025 23:52:07 +0530 Subject: [PATCH 10/30] Addressed comments --- ...IdentifiedFilterPushedDownObjectStore.java | 54 +++++++++++++++++-- .../objectstore/IdentifiedObjectStore.java | 29 ---------- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index e7ce6088..f4425841 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -7,6 +7,7 @@ import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator; import org.hypertrace.config.service.v1.ConfigServiceGrpc; import org.hypertrace.config.service.v1.Filter; +import org.hypertrace.config.service.v1.GetAllConfigsRequest; import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.core.grpcutils.context.RequestContext; @@ -14,6 +15,10 @@ public abstract class IdentifiedFilterPushedDownObjectStore extends IdentifiedObjectStore { + private final ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub; + private final String resourceName; + private final String resourceNamespace; + protected IdentifiedFilterPushedDownObjectStore( ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub, String resourceNamespace, @@ -26,13 +31,23 @@ protected IdentifiedFilterPushedDownObjectStore( resourceName, configChangeEventGenerator, clientConfig); + this.configServiceBlockingStub = configServiceBlockingStub; + this.resourceName = resourceName; + this.resourceNamespace = resourceNamespace; } public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput, Pagination pagination) { Filter filter = buildFilter(filterInput); List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjectsWithFilter(context, filter, pagination, sortByList); + return getMatchingObjectsWithFilter(context, filter, sortByList, pagination); + } + + public List> getMatchingObjects( + RequestContext context, F filterInput, List sortInput) { + Filter filter = buildFilter(filterInput); + List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); + return getMatchingObjectsWithFilter(context, filter, sortByList, null); } public List getMatchingData( @@ -44,8 +59,7 @@ public List getMatchingData( public Optional> getMatchingObject( RequestContext context, F filterInput, List sortInput) { - List> results = - getMatchingObjects(context, filterInput, sortInput, Pagination.getDefaultInstance()); + List> results = getMatchingObjects(context, filterInput, sortInput); if (results.size() > 1) { throw Status.FAILED_PRECONDITION .withDescription("Multiple objects found when expecting at most one") @@ -58,6 +72,40 @@ public Optional getMatchingData(RequestContext context, F filterInput, List> getMatchingObjectsWithFilter( + RequestContext context, Filter filter, List sortByList, Pagination pagination) { + return context + .call( + () -> + this.configServiceBlockingStub + .withDeadline(getDeadline()) + .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) + .getContextSpecificConfigsList() + .stream() + .map( + contextSpecificConfig -> + ContextualConfigObjectImpl.tryBuild( + contextSpecificConfig, this::buildDataFromValue)) + .flatMap(Optional::stream) + .collect( + Collectors.collectingAndThen( + Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + } + + private GetAllConfigsRequest buildGetAllConfigsRequest( + Filter filter, List sortByList, Pagination pagination) { + GetAllConfigsRequest.Builder getAllConfigsRequest = + GetAllConfigsRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setFilter(filter) + .addAllSortBy(sortByList); + if (pagination != null) { + getAllConfigsRequest.setPagination(pagination); + } + return getAllConfigsRequest.build(); + } + protected abstract SortBy buildSort(S sortInput); protected abstract Filter buildFilter(F filterInput); diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index 75e2191e..09aae8ad 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -18,8 +18,6 @@ import org.hypertrace.config.service.v1.GetAllConfigsRequest; 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.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest.ConfigToUpsert; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -116,33 +114,6 @@ public List> getAllObjects(RequestContext context) { Collectors.toUnmodifiableList(), this::orderFetchedObjects)); } - List> getMatchingObjectsWithFilter( - RequestContext context, Filter filter, Pagination pagination, List sortByList) { - return context - .call( - () -> - this.configServiceBlockingStub - .withDeadline(getDeadline()) - .getAllConfigs( - GetAllConfigsRequest.newBuilder() - .setResourceName(this.resourceName) - .setResourceNamespace(this.resourceNamespace) - .setFilter(filter) - .setPagination(pagination) - .addAllSortBy(sortByList) - .build())) - .getContextSpecificConfigsList() - .stream() - .map( - contextSpecificConfig -> - ContextualConfigObjectImpl.tryBuild( - contextSpecificConfig, this::buildDataFromValue)) - .flatMap(Optional::stream) - .collect( - Collectors.collectingAndThen( - Collectors.toUnmodifiableList(), this::orderFetchedObjects)); - } - public List getAllConfigData(RequestContext context) { return getAllObjects(context).stream() .map(ConfigObject::getData) From 952956370bb263ee6cd7fc3491589517b0d495f8 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Thu, 17 Apr 2025 07:38:04 +0530 Subject: [PATCH 11/30] Addressed comments --- ...IdentifiedFilterPushedDownObjectStore.java | 51 ++----------------- .../objectstore/IdentifiedObjectStore.java | 36 +++++++++++++ 2 files changed, 41 insertions(+), 46 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index f4425841..056c2251 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -4,10 +4,10 @@ import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import javax.annotation.Nullable; import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator; import org.hypertrace.config.service.v1.ConfigServiceGrpc; import org.hypertrace.config.service.v1.Filter; -import org.hypertrace.config.service.v1.GetAllConfigsRequest; import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.core.grpcutils.context.RequestContext; @@ -15,10 +15,6 @@ public abstract class IdentifiedFilterPushedDownObjectStore extends IdentifiedObjectStore { - private final ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub; - private final String resourceName; - private final String resourceNamespace; - protected IdentifiedFilterPushedDownObjectStore( ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub, String resourceNamespace, @@ -31,27 +27,24 @@ protected IdentifiedFilterPushedDownObjectStore( resourceName, configChangeEventGenerator, clientConfig); - this.configServiceBlockingStub = configServiceBlockingStub; - this.resourceName = resourceName; - this.resourceNamespace = resourceNamespace; } public List> getMatchingObjects( - RequestContext context, F filterInput, List sortInput, Pagination pagination) { + RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { Filter filter = buildFilter(filterInput); List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjectsWithFilter(context, filter, sortByList, pagination); + return getMatchingObjects(context, filter, sortByList, pagination); } public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput) { Filter filter = buildFilter(filterInput); List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjectsWithFilter(context, filter, sortByList, null); + return getMatchingObjects(context, filter, sortByList, null); } public List getMatchingData( - RequestContext context, F filterInput, List sortInput, Pagination pagination) { + RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { return getMatchingObjects(context, filterInput, sortInput, pagination).stream() .map(ConfigObject::getData) .collect(Collectors.toUnmodifiableList()); @@ -72,40 +65,6 @@ public Optional getMatchingData(RequestContext context, F filterInput, List> getMatchingObjectsWithFilter( - RequestContext context, Filter filter, List sortByList, Pagination pagination) { - return context - .call( - () -> - this.configServiceBlockingStub - .withDeadline(getDeadline()) - .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) - .getContextSpecificConfigsList() - .stream() - .map( - contextSpecificConfig -> - ContextualConfigObjectImpl.tryBuild( - contextSpecificConfig, this::buildDataFromValue)) - .flatMap(Optional::stream) - .collect( - Collectors.collectingAndThen( - Collectors.toUnmodifiableList(), this::orderFetchedObjects)); - } - - private GetAllConfigsRequest buildGetAllConfigsRequest( - Filter filter, List sortByList, Pagination pagination) { - GetAllConfigsRequest.Builder getAllConfigsRequest = - GetAllConfigsRequest.newBuilder() - .setResourceName(this.resourceName) - .setResourceNamespace(this.resourceNamespace) - .setFilter(filter) - .addAllSortBy(sortByList); - if (pagination != null) { - getAllConfigsRequest.setPagination(pagination); - } - return getAllConfigsRequest.build(); - } - protected abstract SortBy buildSort(S sortInput); protected abstract Filter buildFilter(F filterInput); diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index 09aae8ad..3a049ff5 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -18,6 +18,8 @@ import org.hypertrace.config.service.v1.GetAllConfigsRequest; 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.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest.ConfigToUpsert; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -114,6 +116,40 @@ public List> getAllObjects(RequestContext context) { Collectors.toUnmodifiableList(), this::orderFetchedObjects)); } + List> getMatchingObjects( + RequestContext context, Filter filter, List sortByList, Pagination pagination) { + return context + .call( + () -> + this.configServiceBlockingStub + .withDeadline(getDeadline()) + .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) + .getContextSpecificConfigsList() + .stream() + .map( + contextSpecificConfig -> + ContextualConfigObjectImpl.tryBuild( + contextSpecificConfig, this::buildDataFromValue)) + .flatMap(Optional::stream) + .collect( + Collectors.collectingAndThen( + Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + } + + private GetAllConfigsRequest buildGetAllConfigsRequest( + Filter filter, List sortByList, Pagination pagination) { + GetAllConfigsRequest.Builder getAllConfigsRequest = + GetAllConfigsRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setFilter(filter) + .addAllSortBy(sortByList); + if (pagination != null) { + getAllConfigsRequest.setPagination(pagination); + } + return getAllConfigsRequest.build(); + } + public List getAllConfigData(RequestContext context) { return getAllObjects(context).stream() .map(ConfigObject::getData) From 7b276c3e523e61bf5a5c86f8d9e30c49fabefb02 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Thu, 17 Apr 2025 07:58:30 +0530 Subject: [PATCH 12/30] Addressed comments --- .../objectstore/IdentifiedFilterPushedDownObjectStore.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index 056c2251..b2a2d6d3 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -38,9 +38,7 @@ public List> getMatchingObjects( public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput) { - Filter filter = buildFilter(filterInput); - List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjects(context, filter, sortByList, null); + return getMatchingObjects(context, filterInput, sortInput, null); } public List getMatchingData( From 272b14456a0ba0ef3cc4eb089cd14f694f6d3edf Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Thu, 17 Apr 2025 12:10:28 +0530 Subject: [PATCH 13/30] Addressed comments --- ...IdentifiedFilterPushedDownObjectStore.java | 42 +++++++++++++++++++ .../objectstore/IdentifiedObjectStore.java | 36 ---------------- 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index b2a2d6d3..36aa3464 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -8,6 +8,7 @@ import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator; import org.hypertrace.config.service.v1.ConfigServiceGrpc; import org.hypertrace.config.service.v1.Filter; +import org.hypertrace.config.service.v1.GetAllConfigsRequest; import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.core.grpcutils.context.RequestContext; @@ -15,6 +16,10 @@ public abstract class IdentifiedFilterPushedDownObjectStore extends IdentifiedObjectStore { + private final ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub; + private final String resourceNamespace; + private final String resourceName; + protected IdentifiedFilterPushedDownObjectStore( ConfigServiceGrpc.ConfigServiceBlockingStub configServiceBlockingStub, String resourceNamespace, @@ -27,6 +32,9 @@ protected IdentifiedFilterPushedDownObjectStore( resourceName, configChangeEventGenerator, clientConfig); + this.configServiceBlockingStub = configServiceBlockingStub; + this.resourceNamespace = resourceNamespace; + this.resourceName = resourceName; } public List> getMatchingObjects( @@ -63,6 +71,40 @@ public Optional getMatchingData(RequestContext context, F filterInput, List> getMatchingObjects( + RequestContext context, Filter filter, List sortByList, Pagination pagination) { + return context + .call( + () -> + this.configServiceBlockingStub + .withDeadline(getDeadline()) + .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) + .getContextSpecificConfigsList() + .stream() + .map( + contextSpecificConfig -> + ContextualConfigObjectImpl.tryBuild( + contextSpecificConfig, this::buildDataFromValue)) + .flatMap(Optional::stream) + .collect( + Collectors.collectingAndThen( + Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + } + + private GetAllConfigsRequest buildGetAllConfigsRequest( + Filter filter, List sortByList, Pagination pagination) { + GetAllConfigsRequest.Builder getAllConfigsRequest = + GetAllConfigsRequest.newBuilder() + .setResourceName(this.resourceName) + .setResourceNamespace(this.resourceNamespace) + .setFilter(filter) + .addAllSortBy(sortByList); + if (pagination != null) { + getAllConfigsRequest.setPagination(pagination); + } + return getAllConfigsRequest.build(); + } + protected abstract SortBy buildSort(S sortInput); protected abstract Filter buildFilter(F filterInput); diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java index 3a049ff5..09aae8ad 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java @@ -18,8 +18,6 @@ import org.hypertrace.config.service.v1.GetAllConfigsRequest; 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.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest; import org.hypertrace.config.service.v1.UpsertAllConfigsRequest.ConfigToUpsert; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -116,40 +114,6 @@ public List> getAllObjects(RequestContext context) { Collectors.toUnmodifiableList(), this::orderFetchedObjects)); } - List> getMatchingObjects( - RequestContext context, Filter filter, List sortByList, Pagination pagination) { - return context - .call( - () -> - this.configServiceBlockingStub - .withDeadline(getDeadline()) - .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) - .getContextSpecificConfigsList() - .stream() - .map( - contextSpecificConfig -> - ContextualConfigObjectImpl.tryBuild( - contextSpecificConfig, this::buildDataFromValue)) - .flatMap(Optional::stream) - .collect( - Collectors.collectingAndThen( - Collectors.toUnmodifiableList(), this::orderFetchedObjects)); - } - - private GetAllConfigsRequest buildGetAllConfigsRequest( - Filter filter, List sortByList, Pagination pagination) { - GetAllConfigsRequest.Builder getAllConfigsRequest = - GetAllConfigsRequest.newBuilder() - .setResourceName(this.resourceName) - .setResourceNamespace(this.resourceNamespace) - .setFilter(filter) - .addAllSortBy(sortByList); - if (pagination != null) { - getAllConfigsRequest.setPagination(pagination); - } - return getAllConfigsRequest.build(); - } - public List getAllConfigData(RequestContext context) { return getAllObjects(context).stream() .map(ConfigObject::getData) From 4a6ae7a38710081b87caa26417fd46a3630cacef Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Thu, 17 Apr 2025 14:31:12 +0530 Subject: [PATCH 14/30] Add optional total_count for config service api's --- .../config/service/v1/config_service.proto | 6 ++++ .../config/service/ConfigServiceGrpcImpl.java | 10 +++--- .../config/service/store/ConfigStore.java | 10 ++++-- .../service/store/DocumentConfigStore.java | 31 +++++++++++++++++-- .../service/ConfigServiceGrpcImplTest.java | 25 +++++++++++---- .../store/DocumentConfigStoreTest.java | 9 ++++-- 6 files changed, 72 insertions(+), 19 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 c3e480db..27d94559 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,11 +100,17 @@ 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 int64 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 60ac64f7..f53b9624 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 @@ -115,17 +115,15 @@ public void getConfig( public void getAllConfigs( GetAllConfigsRequest request, StreamObserver responseObserver) { try { - List contextSpecificConfigList = + GetAllConfigsResponse getAllConfigsResponse = configStore.getAllConfigs( new ConfigResource( request.getResourceName(), request.getResourceNamespace(), getTenantId()), request.getFilter(), request.getPagination(), - request.getSortByList()); - responseObserver.onNext( - GetAllConfigsResponse.newBuilder() - .addAllContextSpecificConfigs(contextSpecificConfigList) - .build()); + request.getSortByList(), + request.getIncludeTotal()); + responseObserver.onNext(getAllConfigsResponse); responseObserver.onCompleted(); } catch (Exception e) { log.error("Get all configs failed for request:{}", request, e); 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 2f7e4c8d..f2ecca28 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 @@ -10,6 +10,7 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -63,11 +64,16 @@ Map getContextConfigs( * @param filter * @param pagination * @param sortByList + * @param includeTotal * @return * @throws IOException */ - List getAllConfigs( - ConfigResource configResource, Filter filter, Pagination pagination, List sortByList) + GetAllConfigsResponse getAllConfigs( + ConfigResource configResource, + Filter filter, + Pagination pagination, + List sortByList, + boolean includeTotal) throws IOException; /** 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 dd5efa84..0dfcbf6c 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 @@ -1,6 +1,7 @@ package org.hypertrace.config.service.store; import static com.google.common.collect.Streams.zip; +import static java.util.Collections.emptyList; import static org.hypertrace.config.service.store.ConfigDocument.CONTEXT_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_NAMESPACE_FIELD_NAME; @@ -23,6 +24,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; import lombok.NonNull; import lombok.SneakyThrows; @@ -31,6 +33,7 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; @@ -51,6 +54,7 @@ import org.hypertrace.core.documentstore.model.options.QueryOptions; import org.hypertrace.core.documentstore.query.Pagination; import org.hypertrace.core.documentstore.query.Query; +import org.hypertrace.core.documentstore.query.transform.TransformedQueryBuilder; /** Document store which stores and serves the configurations. */ @Slf4j @@ -213,17 +217,25 @@ public Map getContextConfigs( } @Override - public List getAllConfigs( + public GetAllConfigsResponse getAllConfigs( ConfigResource configResource, org.hypertrace.config.service.v1.Filter filter, org.hypertrace.config.service.v1.Pagination pagination, - List sortByList) + List sortByList, + boolean includeTotal) throws IOException { Query query = buildQuery(configResource, filter, pagination, sortByList); List configList = new ArrayList<>(); Set seenContexts = new HashSet<>(); + CompletableFuture totalCountFuture = null; + + if (includeTotal) { + Query countQuery = clearSortingAndPagination(query); + totalCountFuture = CompletableFuture.supplyAsync(() -> collection.count(countQuery)); + } + try (CloseableIterator documentIterator = collection.query(query, QueryOptions.DEFAULT_QUERY_OPTIONS)) { while (documentIterator.hasNext()) { @@ -233,7 +245,20 @@ public List getAllConfigs( configList.sort( Comparator.comparingLong(ContextSpecificConfig::getCreationTimestamp).reversed()); - return configList; + + GetAllConfigsResponse.Builder responseBuilder = + GetAllConfigsResponse.newBuilder().addAllContextSpecificConfigs(configList); + + if (includeTotal) { + long totalCount = totalCountFuture.join(); // Wait for it if needed + responseBuilder.setTotalCount(totalCount); + } + + return responseBuilder.build(); + } + + private Query clearSortingAndPagination(final Query query) { + return new TransformedQueryBuilder(query).setSorts(emptyList()).setPagination(null).build(); } private Query buildQuery( 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 4e34f7f0..17353d7d 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 @@ -186,8 +186,12 @@ void getAllConfigs() throws IOException { eq(new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID)), eq(Filter.getDefaultInstance()), // for empty filter eq(Pagination.getDefaultInstance()), - eq(Collections.emptyList()))) - .thenReturn(contextSpecificConfigList); + eq(Collections.emptyList()), + eq(false))) + .thenReturn( + GetAllConfigsResponse.newBuilder() + .addAllContextSpecificConfigs(contextSpecificConfigList) + .build()); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -206,7 +210,7 @@ void getAllConfigs() throws IOException { } @Test - void getAllConfigs_withFilterPaginationAndSorting() throws IOException { + void getAllConfigs_withFilterPaginationSortingAndTotalCount() throws IOException { ConfigStore configStore = mock(ConfigStore.class); // Define filter @@ -236,6 +240,8 @@ void getAllConfigs_withFilterPaginationAndSorting() throws IOException { ContextSpecificConfig.newBuilder().setConfig(config1).build(), ContextSpecificConfig.newBuilder().setContext(CONTEXT1).setConfig(config2).build()); + long mockedTotalCount = 42L; + // Mock behavior when(configStore.getAllConfigs( argThat( @@ -246,13 +252,18 @@ void getAllConfigs_withFilterPaginationAndSorting() throws IOException { && TENANT_ID.equals(resource.getTenantId())), eq(filter), eq(pagination), - eq(List.of(sortBy)))) - .thenReturn(contextSpecificConfigList); + eq(List.of(sortBy)), + eq(true))) + .thenReturn( + GetAllConfigsResponse.newBuilder() + .addAllContextSpecificConfigs(contextSpecificConfigList) + .setTotalCount(mockedTotalCount) + .build()); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); - // Build request with all fields + // Build request with all fields and includeTotal = true GetAllConfigsRequest request = GetAllConfigsRequest.newBuilder() .setResourceName(RESOURCE_NAME) @@ -260,6 +271,7 @@ void getAllConfigs_withFilterPaginationAndSorting() throws IOException { .setFilter(filter) .setPagination(pagination) .addSortBy(sortBy) + .setIncludeTotal(true) .build(); // Execute in tenant context @@ -276,6 +288,7 @@ void getAllConfigs_withFilterPaginationAndSorting() throws IOException { GetAllConfigsResponse actualResponse = responseCaptor.getValue(); assertEquals(contextSpecificConfigList, actualResponse.getContextSpecificConfigsList()); + assertEquals(mockedTotalCount, actualResponse.getTotalCount()); } @Test 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 2bea1b30..d56bb62b 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 @@ -32,6 +32,7 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.LogicalFilter; import org.hypertrace.config.service.v1.LogicalOperator; import org.hypertrace.config.service.v1.Pagination; @@ -268,12 +269,16 @@ void getAllConfigs() throws IOException { getConfigDocument("context", CONFIG_VERSION - 1, config2, TIMESTAMP1, TIMESTAMP1))); when(collection.query(any(Query.class), any())).thenReturn(iterator); - List contextSpecificConfigList = + GetAllConfigsResponse response = configStore.getAllConfigs( new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID), Filter.getDefaultInstance(), // for empty filter Pagination.getDefaultInstance(), - Collections.emptyList()); + Collections.emptyList(), + false); + + List contextSpecificConfigList = + response.getContextSpecificConfigsList(); assertEquals(2, contextSpecificConfigList.size()); assertEquals( ContextSpecificConfig.newBuilder() From 9f8528e02577aedffeae2d6f5b96f931808a0935 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Fri, 18 Apr 2025 21:15:34 +0530 Subject: [PATCH 15/30] Addressed comments --- .../config/service/ConfigServiceGrpcImpl.java | 54 +++++++++++++++---- .../config/service/store/ConfigStore.java | 12 ++--- .../service/store/DocumentConfigStore.java | 41 +++++++------- .../service/ConfigServiceGrpcImplTest.java | 20 +++---- .../store/DocumentConfigStoreTest.java | 8 +-- 5 files changed, 75 insertions(+), 60 deletions(-) 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 f53b9624..853e4379 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 @@ -8,11 +8,14 @@ import com.google.protobuf.Value; import io.grpc.Status; import io.grpc.stub.StreamObserver; +import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.store.ConfigStore; @@ -115,18 +118,49 @@ public void getConfig( public void getAllConfigs( GetAllConfigsRequest request, StreamObserver responseObserver) { try { - GetAllConfigsResponse getAllConfigsResponse = - configStore.getAllConfigs( - new ConfigResource( - request.getResourceName(), request.getResourceNamespace(), getTenantId()), - request.getFilter(), - request.getPagination(), - request.getSortByList(), - request.getIncludeTotal()); - responseObserver.onNext(getAllConfigsResponse); + ConfigResource configResource = + new ConfigResource( + request.getResourceName(), request.getResourceNamespace(), getTenantId()); + + CompletableFuture> configListFuture = + CompletableFuture.supplyAsync( + () -> { + try { + return configStore.getAllConfigs( + configResource, + request.getFilter(), + request.getPagination(), + request.getSortByList()); + } catch (IOException e) { + throw new CompletionException(e); + } + }); + + CompletableFuture totalCountFuture = null; + if (request.getIncludeTotal()) { + totalCountFuture = + CompletableFuture.supplyAsync( + () -> configStore.getMatchingConfigsCount(configResource, request.getFilter())); + } + + // Wait for both futures to complete + List configList = configListFuture.join(); + GetAllConfigsResponse.Builder responseBuilder = + GetAllConfigsResponse.newBuilder().addAllContextSpecificConfigs(configList); + + if (totalCountFuture != null) { + long totalCount = totalCountFuture.join(); + responseBuilder.setTotalCount(totalCount); + } + + responseObserver.onNext(responseBuilder.build()); responseObserver.onCompleted(); + } catch (CompletionException ce) { + Throwable cause = ce.getCause() instanceof IOException ? ce.getCause() : ce; + log.error("Get all configs failed for request: {}", request, cause); + responseObserver.onError(cause); } catch (Exception e) { - log.error("Get all configs failed for request:{}", request, e); + log.error("Get all configs failed for request: {}", request, e); responseObserver.onError(e); } } 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 f2ecca28..a8801be5 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 @@ -10,7 +10,6 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; @@ -64,18 +63,15 @@ Map getContextConfigs( * @param filter * @param pagination * @param sortByList - * @param includeTotal * @return * @throws IOException */ - GetAllConfigsResponse getAllConfigs( - ConfigResource configResource, - Filter filter, - Pagination pagination, - List sortByList, - boolean includeTotal) + List getAllConfigs( + ConfigResource configResource, Filter filter, Pagination pagination, List sortByList) throws IOException; + long getMatchingConfigsCount(ConfigResource configResource, Filter filter); + /** * Write each of the provided config value associated with the specified config resource to the * store. 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 0dfcbf6c..61da0c08 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,7 +24,6 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; import lombok.NonNull; import lombok.SneakyThrows; @@ -33,7 +32,6 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; @@ -217,44 +215,41 @@ public Map getContextConfigs( } @Override - public GetAllConfigsResponse getAllConfigs( + public List getAllConfigs( ConfigResource configResource, org.hypertrace.config.service.v1.Filter filter, org.hypertrace.config.service.v1.Pagination pagination, - List sortByList, - boolean includeTotal) + List sortByList) throws IOException { Query query = buildQuery(configResource, filter, pagination, sortByList); List configList = new ArrayList<>(); Set seenContexts = new HashSet<>(); - CompletableFuture totalCountFuture = null; - - if (includeTotal) { - Query countQuery = clearSortingAndPagination(query); - totalCountFuture = CompletableFuture.supplyAsync(() -> collection.count(countQuery)); - } - try (CloseableIterator documentIterator = collection.query(query, QueryOptions.DEFAULT_QUERY_OPTIONS)) { while (documentIterator.hasNext()) { processDocument(documentIterator.next(), seenContexts, configList); } } - - configList.sort( - Comparator.comparingLong(ContextSpecificConfig::getCreationTimestamp).reversed()); - - GetAllConfigsResponse.Builder responseBuilder = - GetAllConfigsResponse.newBuilder().addAllContextSpecificConfigs(configList); - - if (includeTotal) { - long totalCount = totalCountFuture.join(); // Wait for it if needed - responseBuilder.setTotalCount(totalCount); + // When there is no requested sort by, sort by creation timestamp default. + if (sortByList.isEmpty()) { + configList.sort( + Comparator.comparingLong(ContextSpecificConfig::getCreationTimestamp).reversed()); } + return configList; + } - return responseBuilder.build(); + @Override + public long getMatchingConfigsCount( + ConfigResource configResource, org.hypertrace.config.service.v1.Filter filter) { + Query query = + buildQuery( + configResource, + filter, + org.hypertrace.config.service.v1.Pagination.getDefaultInstance(), + Collections.emptyList()); + return collection.count(query); } private Query clearSortingAndPagination(final Query query) { 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 17353d7d..fd13ddf0 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 @@ -186,12 +186,8 @@ void getAllConfigs() throws IOException { eq(new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID)), eq(Filter.getDefaultInstance()), // for empty filter eq(Pagination.getDefaultInstance()), - eq(Collections.emptyList()), - eq(false))) - .thenReturn( - GetAllConfigsResponse.newBuilder() - .addAllContextSpecificConfigs(contextSpecificConfigList) - .build()); + eq(Collections.emptyList()))) + .thenReturn(contextSpecificConfigList); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -252,13 +248,11 @@ void getAllConfigs_withFilterPaginationSortingAndTotalCount() throws IOException && TENANT_ID.equals(resource.getTenantId())), eq(filter), eq(pagination), - eq(List.of(sortBy)), - eq(true))) - .thenReturn( - GetAllConfigsResponse.newBuilder() - .addAllContextSpecificConfigs(contextSpecificConfigList) - .setTotalCount(mockedTotalCount) - .build()); + eq(List.of(sortBy)))) + .thenReturn(contextSpecificConfigList); + when(configStore.getMatchingConfigsCount( + eq(new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID)), eq(filter))) + .thenReturn(mockedTotalCount); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.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 d56bb62b..f82c6f8c 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 @@ -32,7 +32,6 @@ 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.GetAllConfigsResponse; import org.hypertrace.config.service.v1.LogicalFilter; import org.hypertrace.config.service.v1.LogicalOperator; import org.hypertrace.config.service.v1.Pagination; @@ -269,16 +268,13 @@ void getAllConfigs() throws IOException { getConfigDocument("context", CONFIG_VERSION - 1, config2, TIMESTAMP1, TIMESTAMP1))); when(collection.query(any(Query.class), any())).thenReturn(iterator); - GetAllConfigsResponse response = + List contextSpecificConfigList = configStore.getAllConfigs( new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID), Filter.getDefaultInstance(), // for empty filter Pagination.getDefaultInstance(), - Collections.emptyList(), - false); + Collections.emptyList()); - List contextSpecificConfigList = - response.getContextSpecificConfigsList(); assertEquals(2, contextSpecificConfigList.size()); assertEquals( ContextSpecificConfig.newBuilder() From b196bf4b7631152df7fc2c087386d8f163fbe099 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Fri, 18 Apr 2025 21:19:40 +0530 Subject: [PATCH 16/30] Addressed comments --- .../hypertrace/config/service/store/DocumentConfigStore.java | 4 ---- 1 file changed, 4 deletions(-) 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 61da0c08..cb35af51 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 @@ -252,10 +252,6 @@ public long getMatchingConfigsCount( return collection.count(query); } - private Query clearSortingAndPagination(final Query query) { - return new TransformedQueryBuilder(query).setSorts(emptyList()).setPagination(null).build(); - } - private Query buildQuery( ConfigResource configResource, @NonNull org.hypertrace.config.service.v1.Filter filter, From 0c7ab600349a6701c9ff11e68c7bf9e9dc3905ed Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Fri, 18 Apr 2025 21:31:47 +0530 Subject: [PATCH 17/30] Addressed comments --- .../hypertrace/config/service/store/DocumentConfigStore.java | 2 -- 1 file changed, 2 deletions(-) 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 cb35af51..f94b217f 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 @@ -1,7 +1,6 @@ package org.hypertrace.config.service.store; import static com.google.common.collect.Streams.zip; -import static java.util.Collections.emptyList; import static org.hypertrace.config.service.store.ConfigDocument.CONTEXT_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_NAMESPACE_FIELD_NAME; @@ -52,7 +51,6 @@ import org.hypertrace.core.documentstore.model.options.QueryOptions; import org.hypertrace.core.documentstore.query.Pagination; import org.hypertrace.core.documentstore.query.Query; -import org.hypertrace.core.documentstore.query.transform.TransformedQueryBuilder; /** Document store which stores and serves the configurations. */ @Slf4j From 9b8b6a6de6d41f85f6092a79d0aa4d75fdc4d98a Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Sat, 19 Apr 2025 20:49:32 +0530 Subject: [PATCH 18/30] Addressed comments --- .../config/service/ConfigServiceGrpcImpl.java | 65 ++++++++++++------- .../service/store/DocumentConfigStore.java | 11 +--- 2 files changed, 43 insertions(+), 33 deletions(-) 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 853e4379..74f49120 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 @@ -7,15 +7,16 @@ import com.google.common.collect.ImmutableMap; import com.google.protobuf.Value; import io.grpc.Status; +import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; -import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionException; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.store.ConfigStore; @@ -45,6 +46,15 @@ public class ConfigServiceGrpcImpl extends ConfigServiceGrpc.ConfigServiceImplBa private static String DEFAULT_USER_ID = "Unknown"; private static String DEFAULT_USER_EMAIL = "Unknown"; private final ConfigStore configStore; + private static final Executor configExecutor = + Executors.newFixedThreadPool( + 4, + runnable -> { + Thread thread = new Thread(runnable); + thread.setName("config-executor"); + thread.setDaemon(true); + return thread; + }); public ConfigServiceGrpcImpl(ConfigStore configStore) { this.configStore = configStore; @@ -122,46 +132,51 @@ public void getAllConfigs( new ConfigResource( request.getResourceName(), request.getResourceNamespace(), getTenantId()); - CompletableFuture> configListFuture = - CompletableFuture.supplyAsync( - () -> { - try { - return configStore.getAllConfigs( - configResource, - request.getFilter(), - request.getPagination(), - request.getSortByList()); - } catch (IOException e) { - throw new CompletionException(e); - } - }); - CompletableFuture totalCountFuture = null; + + // Start count query first (in parallel) if (request.getIncludeTotal()) { totalCountFuture = CompletableFuture.supplyAsync( - () -> configStore.getMatchingConfigsCount(configResource, request.getFilter())); + () -> { + try { + return configStore.getMatchingConfigsCount(configResource, request.getFilter()); + } catch (Exception e) { + throw Status.INTERNAL + .withCause(e) + .withDescription("Failed to fetch total count") + .asRuntimeException(); + } + }, + configExecutor); } - // Wait for both futures to complete - List configList = configListFuture.join(); + // 🔵 Then run getAllConfigs on the current thread + List configList = + configStore.getAllConfigs( + configResource, + request.getFilter(), + request.getPagination(), + request.getSortByList()); + + // 🧱 Build the response GetAllConfigsResponse.Builder responseBuilder = GetAllConfigsResponse.newBuilder().addAllContextSpecificConfigs(configList); if (totalCountFuture != null) { - long totalCount = totalCountFuture.join(); + long totalCount = totalCountFuture.join(); // Wait if not finished responseBuilder.setTotalCount(totalCount); } responseObserver.onNext(responseBuilder.build()); responseObserver.onCompleted(); - } catch (CompletionException ce) { - Throwable cause = ce.getCause() instanceof IOException ? ce.getCause() : ce; - log.error("Get all configs failed for request: {}", request, cause); - responseObserver.onError(cause); - } catch (Exception e) { + } catch (StatusRuntimeException e) { log.error("Get all configs failed for request: {}", request, e); responseObserver.onError(e); + } catch (Exception e) { + log.error("Get all configs failed for request: {}", request, e); + responseObserver.onError( + Status.INTERNAL.withCause(e).withDescription("Unexpected error").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 f94b217f..69292852 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 @@ -2,6 +2,7 @@ import static com.google.common.collect.Streams.zip; import static org.hypertrace.config.service.store.ConfigDocument.CONTEXT_FIELD_NAME; +import static org.hypertrace.config.service.store.ConfigDocument.CREATION_TIMESTAMP_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_NAMESPACE_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.TENANT_ID_FIELD_NAME; @@ -15,7 +16,6 @@ import java.time.Clock; import java.util.ArrayList; import java.util.Collections; -import java.util.Comparator; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -230,11 +230,6 @@ public List getAllConfigs( processDocument(documentIterator.next(), seenContexts, configList); } } - // When there is no requested sort by, sort by creation timestamp default. - if (sortByList.isEmpty()) { - configList.sort( - Comparator.comparingLong(ContextSpecificConfig::getCreationTimestamp).reversed()); - } return configList; } @@ -262,7 +257,7 @@ private Query buildQuery( queryBuilder.setPagination( Pagination.builder().offset(pagination.getOffset()).limit(pagination.getLimit()).build()); } - + queryBuilder.addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC); if (!sortByList.isEmpty()) { sortByList.forEach( sortBy -> @@ -270,7 +265,7 @@ private Query buildQuery( IdentifierExpression.of(sortBy.getSelection().getConfigJsonPath()), convertSortOrder(sortBy))); } else { - queryBuilder.addSort(IdentifierExpression.of(VERSION_FIELD_NAME), SortOrder.DESC); + queryBuilder.addSort(IdentifierExpression.of(CREATION_TIMESTAMP_FIELD_NAME), SortOrder.DESC); } return queryBuilder.build(); } From 0df3b6e4bd124730e3e23243f373387a10bf1df4 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Sat, 19 Apr 2025 20:51:12 +0530 Subject: [PATCH 19/30] Addressed comments --- .../org/hypertrace/config/service/ConfigServiceGrpcImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 74f49120..136dd58d 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 @@ -151,7 +151,7 @@ public void getAllConfigs( configExecutor); } - // 🔵 Then run getAllConfigs on the current thread + // Then run getAllConfigs on the current thread List configList = configStore.getAllConfigs( configResource, @@ -159,7 +159,7 @@ public void getAllConfigs( request.getPagination(), request.getSortByList()); - // 🧱 Build the response + // Build the response GetAllConfigsResponse.Builder responseBuilder = GetAllConfigsResponse.newBuilder().addAllContextSpecificConfigs(configList); From ddacdda14d014de102b8abe7d4ec545fe827f5db Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Sun, 20 Apr 2025 20:29:13 +0530 Subject: [PATCH 20/30] Addressed comments --- .../hypertrace/config/service/ConfigServiceGrpcImpl.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) 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 136dd58d..e709c05c 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 @@ -7,7 +7,6 @@ import com.google.common.collect.ImmutableMap; import com.google.protobuf.Value; import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import java.util.List; import java.util.Map; @@ -170,13 +169,9 @@ public void getAllConfigs( responseObserver.onNext(responseBuilder.build()); responseObserver.onCompleted(); - } catch (StatusRuntimeException e) { - log.error("Get all configs failed for request: {}", request, e); - responseObserver.onError(e); } catch (Exception e) { log.error("Get all configs failed for request: {}", request, e); - responseObserver.onError( - Status.INTERNAL.withCause(e).withDescription("Unexpected error").asRuntimeException()); + responseObserver.onError(Status.fromThrowable(e).withCause(e).asRuntimeException()); } } From 0940f88f434794a4ba97cab3b108d81e9abe43c2 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Sun, 20 Apr 2025 20:33:18 +0530 Subject: [PATCH 21/30] Addressed comments --- .../hypertrace/config/service/ConfigServiceGrpcImpl.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 e709c05c..caf58f00 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 @@ -5,6 +5,7 @@ import static org.hypertrace.config.service.ConfigServiceUtils.merge; import com.google.common.collect.ImmutableMap; +import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.protobuf.Value; import io.grpc.Status; import io.grpc.stub.StreamObserver; @@ -48,12 +49,7 @@ public class ConfigServiceGrpcImpl extends ConfigServiceGrpc.ConfigServiceImplBa private static final Executor configExecutor = Executors.newFixedThreadPool( 4, - runnable -> { - Thread thread = new Thread(runnable); - thread.setName("config-executor"); - thread.setDaemon(true); - return thread; - }); + new ThreadFactoryBuilder().setNameFormat("config-executor-%d").setDaemon(true).build()); public ConfigServiceGrpcImpl(ConfigStore configStore) { this.configStore = configStore; From 7728b42f2c8d0c38376d494f32cff541a5622287 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 21 Apr 2025 13:01:08 +0530 Subject: [PATCH 22/30] Added api in IdentifiedObjectsStorePushdown --- .../config/objectstore/ConfigsResponse.java | 9 +++++ .../objectstore/ConfigsResponseImpl.java | 24 +++++++++++++ ...IdentifiedFilterPushedDownObjectStore.java | 35 +++++++++++++++++-- 3 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java create mode 100644 config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java new file mode 100644 index 00000000..39a35e93 --- /dev/null +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java @@ -0,0 +1,9 @@ +package org.hypertrace.config.objectstore; + +import java.util.List; + +public interface ConfigsResponse { + List> getContextualConfigObjects(); + + long totalCount(); +} diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java new file mode 100644 index 00000000..d05728b4 --- /dev/null +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java @@ -0,0 +1,24 @@ +package org.hypertrace.config.objectstore; + +import java.util.List; + +public class ConfigsResponseImpl implements ConfigsResponse { + private final List> contextualConfigObjects; + private final long totalCount; + + public ConfigsResponseImpl( + List> contextualConfigObjects, long totalCount) { + this.contextualConfigObjects = contextualConfigObjects; + this.totalCount = totalCount; + } + + @Override + public List> getContextualConfigObjects() { + return contextualConfigObjects; + } + + @Override + public long totalCount() { + return totalCount; + } +} diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index 36aa3464..6b79c0af 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -9,6 +9,7 @@ import org.hypertrace.config.service.v1.ConfigServiceGrpc; 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.Pagination; import org.hypertrace.config.service.v1.SortBy; import org.hypertrace.core.grpcutils.context.RequestContext; @@ -71,6 +72,32 @@ public Optional getMatchingData(RequestContext context, F filterInput, List getMatchingObjectsWithTotalCount( + RequestContext context, F filterInput, List sortInput, Pagination pagination) { + List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); + GetAllConfigsResponse getAllConfigsResponse = + context.call( + () -> + this.configServiceBlockingStub + .withDeadline(getDeadline()) + .getAllConfigs( + buildGetAllConfigsRequest( + buildFilter(filterInput), sortByList, pagination, true))); + List> contextualConfigObjects = + getAllConfigsResponse.getContextSpecificConfigsList().stream() + .map( + contextSpecificConfig -> + ContextualConfigObjectImpl.tryBuild( + contextSpecificConfig, this::buildDataFromValue)) + .flatMap(Optional::stream) + .collect( + Collectors.collectingAndThen( + Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + + return new ConfigsResponseImpl<>( + contextualConfigObjects, getAllConfigsResponse.getTotalCount()); + } + List> getMatchingObjects( RequestContext context, Filter filter, List sortByList, Pagination pagination) { return context @@ -78,7 +105,8 @@ List> getMatchingObjects( () -> this.configServiceBlockingStub .withDeadline(getDeadline()) - .getAllConfigs(buildGetAllConfigsRequest(filter, sortByList, pagination))) + .getAllConfigs( + buildGetAllConfigsRequest(filter, sortByList, pagination, false))) .getContextSpecificConfigsList() .stream() .map( @@ -92,7 +120,7 @@ List> getMatchingObjects( } private GetAllConfigsRequest buildGetAllConfigsRequest( - Filter filter, List sortByList, Pagination pagination) { + Filter filter, List sortByList, Pagination pagination, boolean totalIncluded) { GetAllConfigsRequest.Builder getAllConfigsRequest = GetAllConfigsRequest.newBuilder() .setResourceName(this.resourceName) @@ -102,6 +130,9 @@ private GetAllConfigsRequest buildGetAllConfigsRequest( if (pagination != null) { getAllConfigsRequest.setPagination(pagination); } + if (totalIncluded) { + getAllConfigsRequest.setIncludeTotal(true); + } return getAllConfigsRequest.build(); } From 5862939c5c0b7238618b34f89136d18db1f3a2cd Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 21 Apr 2025 18:34:15 +0530 Subject: [PATCH 23/30] Addressed comments --- .../config/objectstore/ConfigsResponse.java | 2 +- .../objectstore/ConfigsResponseImpl.java | 9 ++-- ...IdentifiedFilterPushedDownObjectStore.java | 52 ++++++++----------- .../config/service/ConfigServiceGrpcImpl.java | 2 +- 4 files changed, 27 insertions(+), 38 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java index 39a35e93..23375910 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponse.java @@ -3,7 +3,7 @@ import java.util.List; public interface ConfigsResponse { - List> getContextualConfigObjects(); + List getContextualConfigObjects(); long totalCount(); } diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java index d05728b4..8677f282 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/ConfigsResponseImpl.java @@ -2,18 +2,17 @@ import java.util.List; -public class ConfigsResponseImpl implements ConfigsResponse { - private final List> contextualConfigObjects; +class ConfigsResponseImpl implements ConfigsResponse { + private final List contextualConfigObjects; private final long totalCount; - public ConfigsResponseImpl( - List> contextualConfigObjects, long totalCount) { + public ConfigsResponseImpl(List contextualConfigObjects, long totalCount) { this.contextualConfigObjects = contextualConfigObjects; this.totalCount = totalCount; } @Override - public List> getContextualConfigObjects() { + public List getContextualConfigObjects() { return contextualConfigObjects; } diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index 6b79c0af..ebc9d78f 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -42,7 +42,9 @@ public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { Filter filter = buildFilter(filterInput); List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjects(context, filter, sortByList, pagination); + ConfigsResponse> configsResponse = + getMatchingObjects(context, filter, sortByList, pagination, false); + return configsResponse.getContextualConfigObjects(); } public List> getMatchingObjects( @@ -72,19 +74,29 @@ public Optional getMatchingData(RequestContext context, F filterInput, List getMatchingObjectsWithTotalCount( - RequestContext context, F filterInput, List sortInput, Pagination pagination) { + public ConfigsResponse> getMatchingObjectsWithTotalCount( + RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { + Filter filter = buildFilter(filterInput); List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - GetAllConfigsResponse getAllConfigsResponse = + return getMatchingObjects(context, filter, sortByList, pagination, true); + } + + ConfigsResponse> getMatchingObjects( + RequestContext context, + Filter filter, + List sortByList, + Pagination pagination, + boolean totalIncluded) { + GetAllConfigsResponse getConfigsResponse = context.call( () -> this.configServiceBlockingStub .withDeadline(getDeadline()) .getAllConfigs( - buildGetAllConfigsRequest( - buildFilter(filterInput), sortByList, pagination, true))); - List> contextualConfigObjects = - getAllConfigsResponse.getContextSpecificConfigsList().stream() + buildGetAllConfigsRequest(filter, sortByList, pagination, totalIncluded))); + + List> contextualConfigObjectsList = + getConfigsResponse.getContextSpecificConfigsList().stream() .map( contextSpecificConfig -> ContextualConfigObjectImpl.tryBuild( @@ -93,30 +105,8 @@ public ConfigsResponse getMatchingObjectsWithTotalCount( .collect( Collectors.collectingAndThen( Collectors.toUnmodifiableList(), this::orderFetchedObjects)); - return new ConfigsResponseImpl<>( - contextualConfigObjects, getAllConfigsResponse.getTotalCount()); - } - - List> getMatchingObjects( - RequestContext context, Filter filter, List sortByList, Pagination pagination) { - return context - .call( - () -> - this.configServiceBlockingStub - .withDeadline(getDeadline()) - .getAllConfigs( - buildGetAllConfigsRequest(filter, sortByList, pagination, false))) - .getContextSpecificConfigsList() - .stream() - .map( - contextSpecificConfig -> - ContextualConfigObjectImpl.tryBuild( - contextSpecificConfig, this::buildDataFromValue)) - .flatMap(Optional::stream) - .collect( - Collectors.collectingAndThen( - Collectors.toUnmodifiableList(), this::orderFetchedObjects)); + contextualConfigObjectsList, getConfigsResponse.getTotalCount()); } private GetAllConfigsRequest buildGetAllConfigsRequest( 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 caf58f00..66fc9e4a 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 @@ -167,7 +167,7 @@ public void getAllConfigs( responseObserver.onCompleted(); } catch (Exception e) { log.error("Get all configs failed for request: {}", request, e); - responseObserver.onError(Status.fromThrowable(e).withCause(e).asRuntimeException()); + responseObserver.onError(Status.fromThrowable(e).asRuntimeException()); } } From c84c4c1f436cb4c2eb98da08dedaefd1bf3265b9 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 21 Apr 2025 19:06:27 +0530 Subject: [PATCH 24/30] Addressed comments --- .../IdentifiedFilterPushedDownObjectStore.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index ebc9d78f..dbc0bf03 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -40,10 +40,8 @@ protected IdentifiedFilterPushedDownObjectStore( public List> getMatchingObjects( RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { - Filter filter = buildFilter(filterInput); - List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); ConfigsResponse> configsResponse = - getMatchingObjects(context, filter, sortByList, pagination, false); + getMatchingObjects(context, filterInput, sortInput, pagination, false); return configsResponse.getContextualConfigObjects(); } @@ -74,19 +72,19 @@ public Optional getMatchingData(RequestContext context, F filterInput, List> getMatchingObjectsWithTotalCount( + public ConfigsResponse> getMatchingDataWithTotalCount( RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { - Filter filter = buildFilter(filterInput); - List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); - return getMatchingObjects(context, filter, sortByList, pagination, true); + return getMatchingObjects(context, filterInput, sortInput, pagination, true); } ConfigsResponse> getMatchingObjects( RequestContext context, - Filter filter, - List sortByList, + F filterInput, + List sortInput, Pagination pagination, boolean totalIncluded) { + Filter filter = buildFilter(filterInput); + List sortByList = sortInput.stream().map(this::buildSort).collect(Collectors.toList()); GetAllConfigsResponse getConfigsResponse = context.call( () -> From a70375c95f26cb9e46947caf73031d22513badb6 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Mon, 21 Apr 2025 19:25:50 +0530 Subject: [PATCH 25/30] Added data and objects api separately --- .../IdentifiedFilterPushedDownObjectStore.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java index dbc0bf03..d8c13836 100644 --- a/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java +++ b/config-object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedFilterPushedDownObjectStore.java @@ -72,11 +72,22 @@ public Optional getMatchingData(RequestContext context, F filterInput, List> getMatchingDataWithTotalCount( + public ConfigsResponse> getMatchingObjectsWithTotalCount( RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { return getMatchingObjects(context, filterInput, sortInput, pagination, true); } + public ConfigsResponse getMatchingDataWithTotalCount( + RequestContext context, F filterInput, List sortInput, @Nullable Pagination pagination) { + ConfigsResponse> configsResponse = + getMatchingObjects(context, filterInput, sortInput, pagination, true); + return new ConfigsResponseImpl<>( + configsResponse.getContextualConfigObjects().stream() + .map(ConfigObject::getData) + .collect(Collectors.toUnmodifiableList()), + configsResponse.totalCount()); + } + ConfigsResponse> getMatchingObjects( RequestContext context, F filterInput, From 43eec29120050ada8dd426f666aa5a39028f64cc Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 22 Apr 2025 16:00:46 +0530 Subject: [PATCH 26/30] Bugfix user filters --- .../service/store/DocumentConfigStore.java | 6 +- .../store/FilterExpressionBuilder.java | 2 +- .../store/DocumentConfigStoreTest.java | 108 ++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) 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 69292852..d46da11a 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 @@ -245,7 +245,7 @@ public long getMatchingConfigsCount( return collection.count(query); } - private Query buildQuery( + Query buildQuery( ConfigResource configResource, @NonNull org.hypertrace.config.service.v1.Filter filter, @NonNull org.hypertrace.config.service.v1.Pagination pagination, @@ -262,7 +262,9 @@ private Query buildQuery( sortByList.forEach( sortBy -> queryBuilder.addSort( - IdentifierExpression.of(sortBy.getSelection().getConfigJsonPath()), + IdentifierExpression.of( + filterExpressionBuilder.buildConfigFieldPath( + sortBy.getSelection().getConfigJsonPath())), convertSortOrder(sortBy))); } else { queryBuilder.addSort(IdentifierExpression.of(CREATION_TIMESTAMP_FIELD_NAME), SortOrder.DESC); 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 7ad61dde..788a1bf0 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 @@ -92,7 +92,7 @@ private FilterTypeExpression buildRelationalExpression(RelationalFilter relation ConstantExpressionConverter.fromProtoValue(relationalFilter.getValue())); } - private String buildConfigFieldPath(String configJsonPath) { + 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/store/DocumentConfigStoreTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java index f82c6f8c..2e845399 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 @@ -9,6 +9,7 @@ import static org.hypertrace.config.service.TestUtils.getConfigResourceContext; import static org.hypertrace.config.service.store.DocumentConfigStore.CONFIGURATIONS_COLLECTION; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -37,6 +38,7 @@ 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.SortOrder; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; import org.hypertrace.core.documentstore.CloseableIterator; @@ -45,6 +47,11 @@ import org.hypertrace.core.documentstore.Document; import org.hypertrace.core.documentstore.Key; import org.hypertrace.core.documentstore.UpdateResult; +import org.hypertrace.core.documentstore.expression.impl.ConstantExpression; +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.type.FilterTypeExpression; import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider; import org.hypertrace.core.documentstore.query.Query; import org.junit.jupiter.api.BeforeEach; @@ -358,6 +365,107 @@ void writeAllConfigs() throws IOException { updateTime))); } + @Test + void buildQuery_withDefaultPagination() { + ConfigResource configResource = + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); + Filter filter = Filter.getDefaultInstance(); + Pagination pagination = Pagination.getDefaultInstance(); + List sortByList = Collections.emptyList(); + + Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + + // Verify default sorting is applied + assertEquals(2, query.getSorts().size()); + assertEquals( + "configVersion", + ((IdentifierExpression) query.getSorts().get(0).getExpression()).getName()); + assertEquals( + "creationTimestamp", + ((IdentifierExpression) query.getSorts().get(1).getExpression()).getName()); + } + + @Test + void buildQuery_withCustomPagination() { + ConfigResource configResource = + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); + Filter filter = Filter.getDefaultInstance(); + Pagination pagination = Pagination.newBuilder().setOffset(10).setLimit(20).build(); + List sortByList = Collections.emptyList(); + + Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + + assertEquals(10, query.getPagination().get().getOffset()); + assertEquals(20, query.getPagination().get().getLimit()); + } + + @Test + void buildQuery_withCustomSorting() { + ConfigResource configResource = + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); + Filter filter = Filter.getDefaultInstance(); + Pagination pagination = Pagination.getDefaultInstance(); + + org.hypertrace.config.service.v1.Selection selection = + org.hypertrace.config.service.v1.Selection.newBuilder() + .setConfigJsonPath("test.path") + .build(); + org.hypertrace.config.service.v1.SortBy sortBy = + org.hypertrace.config.service.v1.SortBy.newBuilder() + .setSelection(selection) + .setSortOrder(SortOrder.SORT_ORDER_DESC) + .build(); + List sortByList = Collections.singletonList(sortBy); + + Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + + assertEquals(2, query.getSorts().size()); + assertEquals( + "configVersion", + ((IdentifierExpression) query.getSorts().get(0).getExpression()).getName()); + assertEquals( + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC, + query.getSorts().get(0).getOrder()); + assertEquals( + "config.test.path", + ((IdentifierExpression) query.getSorts().get(1).getExpression()).getName()); + assertEquals( + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC, + query.getSorts().get(1).getOrder()); + } + + @Test + void buildQuery_withFilter() { + ConfigResource configResource = + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); + + RelationalFilter relationalFilter = + RelationalFilter.newBuilder() + .setConfigJsonPath("test.path") + .setOperator(RelationalOperator.RELATIONAL_OPERATOR_EQ) + .setValue(Values.of("test-value")) + .build(); + + Filter filter = Filter.newBuilder().setRelationalFilter(relationalFilter).build(); + + Pagination pagination = Pagination.getDefaultInstance(); + List sortByList = Collections.emptyList(); + + Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + + // Verify filter is present in the query + assertNotNull(query.getFilter()); + + FilterTypeExpression userFilter = + ((LogicalExpression) query.getFilter().get()).getOperands().get(1); + assertEquals( + "config.test.path", + ((IdentifierExpression) ((RelationalExpression) userFilter).getLhs()).getName()); + assertEquals( + "test-value", + ((ConstantExpression) ((RelationalExpression) userFilter).getRhs()).getValue().toString()); + } + private static Document getConfigDocument( String context, long version, Value config, long creationTimestamp, long updateTimestamp) { return new ConfigDocument( From 420cb4bad7167470e25b211fc1aa1e872bb0f553 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Tue, 22 Apr 2025 21:54:51 +0530 Subject: [PATCH 27/30] Addressed comments --- .../config/service/ConfigServiceUtils.java | 6 +++++ .../service/store/DocumentConfigStore.java | 2 +- .../store/FilterExpressionBuilder.java | 10 +++----- .../store/DocumentConfigStoreTest.java | 24 +++++++++---------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java index 624c126b..1c803f93 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java @@ -1,5 +1,7 @@ package org.hypertrace.config.service; +import static org.hypertrace.config.service.store.ConfigDocument.CONFIG_FIELD_NAME; + import com.google.protobuf.NullValue; import com.google.protobuf.Struct; import com.google.protobuf.Value; @@ -108,6 +110,10 @@ public static ContextSpecificConfig emptyConfig(String context) { .build(); } + public static String buildConfigFieldPath(String configJsonPath) { + return String.format("%s.%s", CONFIG_FIELD_NAME, configJsonPath); + } + private static ContextSpecificConfig buildContextSpecificConfig( Value config, long creationTimestamp, long updateTimestamp) { return ContextSpecificConfig.newBuilder() 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 d46da11a..bf6bb77a 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 @@ -263,7 +263,7 @@ Query buildQuery( sortBy -> queryBuilder.addSort( IdentifierExpression.of( - filterExpressionBuilder.buildConfigFieldPath( + ConfigServiceUtils.buildConfigFieldPath( sortBy.getSelection().getConfigJsonPath())), convertSortOrder(sortBy))); } else { 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 788a1bf0..eb5c400b 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 @@ -1,10 +1,9 @@ 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.ConfigServiceUtils; import org.hypertrace.config.service.v1.Filter; import org.hypertrace.config.service.v1.LogicalFilter; import org.hypertrace.config.service.v1.RelationalFilter; @@ -87,12 +86,9 @@ private FilterTypeExpression buildRelationalExpression(RelationalFilter relation } return RelationalExpression.of( - IdentifierExpression.of(buildConfigFieldPath(relationalFilter.getConfigJsonPath())), + IdentifierExpression.of( + ConfigServiceUtils.buildConfigFieldPath(relationalFilter.getConfigJsonPath())), operator, ConstantExpressionConverter.fromProtoValue(relationalFilter.getValue())); } - - 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/store/DocumentConfigStoreTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java index 2e845399..548a0f37 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 @@ -23,6 +23,7 @@ import io.grpc.StatusRuntimeException; import java.io.IOException; import java.time.Clock; +import java.util.Arrays; import java.util.Collections; import java.util.Iterator; import java.util.List; @@ -54,6 +55,7 @@ import org.hypertrace.core.documentstore.expression.type.FilterTypeExpression; import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider; import org.hypertrace.core.documentstore.query.Query; +import org.hypertrace.core.documentstore.query.SortingSpec; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -420,18 +422,16 @@ void buildQuery_withCustomSorting() { Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); assertEquals(2, query.getSorts().size()); - assertEquals( - "configVersion", - ((IdentifierExpression) query.getSorts().get(0).getExpression()).getName()); - assertEquals( - org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC, - query.getSorts().get(0).getOrder()); - assertEquals( - "config.test.path", - ((IdentifierExpression) query.getSorts().get(1).getExpression()).getName()); - assertEquals( - org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC, - query.getSorts().get(1).getOrder()); + List expectedSorts = + Arrays.asList( + SortingSpec.of( + IdentifierExpression.of("configVersion"), + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC), + SortingSpec.of( + IdentifierExpression.of("config.test.path"), + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC)); + + assertEquals(expectedSorts, query.getSorts()); } @Test From 3b1e687d60bb89d7a05c25eb2aaca6a961432158 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Wed, 23 Apr 2025 10:19:30 +0530 Subject: [PATCH 28/30] Fix test cases using public methods --- .../service/store/DocumentConfigStore.java | 2 +- .../store/DocumentConfigStoreTest.java | 136 ++++++++++++++---- 2 files changed, 106 insertions(+), 32 deletions(-) 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 bf6bb77a..7693a053 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 @@ -245,7 +245,7 @@ public long getMatchingConfigsCount( return collection.count(query); } - Query buildQuery( + private Query buildQuery( ConfigResource configResource, @NonNull org.hypertrace.config.service.v1.Filter filter, @NonNull org.hypertrace.config.service.v1.Pagination pagination, 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 548a0f37..c84dfbb3 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 @@ -54,6 +54,7 @@ import org.hypertrace.core.documentstore.expression.impl.RelationalExpression; import org.hypertrace.core.documentstore.expression.type.FilterTypeExpression; import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider; +import org.hypertrace.core.documentstore.model.options.QueryOptions; import org.hypertrace.core.documentstore.query.Query; import org.hypertrace.core.documentstore.query.SortingSpec; import org.junit.jupiter.api.BeforeEach; @@ -368,41 +369,75 @@ void writeAllConfigs() throws IOException { } @Test - void buildQuery_withDefaultPagination() { + void buildQuery_withDefaultPagination() throws Exception { + // Arrange ConfigResource configResource = new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); Filter filter = Filter.getDefaultInstance(); - Pagination pagination = Pagination.getDefaultInstance(); - List sortByList = Collections.emptyList(); + Pagination pagination = Pagination.getDefaultInstance(); // Default pagination - Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + // Create a mock for CloseableIterator + CloseableIterator mockIterator = mock(CloseableIterator.class); + when(mockIterator.hasNext()).thenReturn(false); // No documents to process - // Verify default sorting is applied - assertEquals(2, query.getSorts().size()); - assertEquals( - "configVersion", - ((IdentifierExpression) query.getSorts().get(0).getExpression()).getName()); - assertEquals( - "creationTimestamp", - ((IdentifierExpression) query.getSorts().get(1).getExpression()).getName()); + // Mock the collection.query to return the mock iterator + when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); + + // Act + configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList()); + + // Capture the Query object passed to collection.query + ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); + verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); + + // Assert the properties of the captured Query + Query capturedQuery = queryCaptor.getValue(); + assertNotNull(capturedQuery); + + List expectedSorts = + Arrays.asList( + SortingSpec.of( + IdentifierExpression.of("configVersion"), + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC), + SortingSpec.of( + IdentifierExpression.of("creationTimestamp"), + org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC)); + assertEquals(expectedSorts, capturedQuery.getSorts()); } @Test - void buildQuery_withCustomPagination() { + void buildQuery_withCustomPagination() throws IOException { ConfigResource configResource = new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); Filter filter = Filter.getDefaultInstance(); Pagination pagination = Pagination.newBuilder().setOffset(10).setLimit(20).build(); List sortByList = Collections.emptyList(); - Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + // Create a mock for CloseableIterator + CloseableIterator mockIterator = mock(CloseableIterator.class); + when(mockIterator.hasNext()).thenReturn(false); // No documents to process + + // Mock the collection.query to return the mock iterator + when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); - assertEquals(10, query.getPagination().get().getOffset()); - assertEquals(20, query.getPagination().get().getLimit()); + // Act + configStore.getAllConfigs(configResource, filter, pagination, sortByList); + + // Capture the Query object passed to collection.query + ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); + verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); + + // Assert the properties of the captured Query + Query capturedQuery = queryCaptor.getValue(); + assertNotNull(capturedQuery); + + org.hypertrace.core.documentstore.query.Pagination expectedPagination = + org.hypertrace.core.documentstore.query.Pagination.builder().offset(10).limit(20).build(); + assertEquals(expectedPagination, capturedQuery.getPagination().get()); } @Test - void buildQuery_withCustomSorting() { + void buildQuery_withCustomSorting() throws IOException { ConfigResource configResource = new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); Filter filter = Filter.getDefaultInstance(); @@ -418,10 +453,23 @@ void buildQuery_withCustomSorting() { .setSortOrder(SortOrder.SORT_ORDER_DESC) .build(); List sortByList = Collections.singletonList(sortBy); + // Create a mock for CloseableIterator + CloseableIterator mockIterator = mock(CloseableIterator.class); + when(mockIterator.hasNext()).thenReturn(false); // No documents to process + + // Mock the collection.query to return the mock iterator + when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); - Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + // Act + configStore.getAllConfigs(configResource, filter, pagination, sortByList); - assertEquals(2, query.getSorts().size()); + // Capture the Query object passed to collection.query + ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); + verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); + + // Assert the properties of the captured Query + Query capturedQuery = queryCaptor.getValue(); + assertNotNull(capturedQuery); List expectedSorts = Arrays.asList( SortingSpec.of( @@ -431,11 +479,11 @@ void buildQuery_withCustomSorting() { IdentifierExpression.of("config.test.path"), org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC)); - assertEquals(expectedSorts, query.getSorts()); + assertEquals(expectedSorts, capturedQuery.getSorts()); } @Test - void buildQuery_withFilter() { + void buildQuery_withFilter() throws IOException { ConfigResource configResource = new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); @@ -451,19 +499,45 @@ void buildQuery_withFilter() { Pagination pagination = Pagination.getDefaultInstance(); List sortByList = Collections.emptyList(); - Query query = configStore.buildQuery(configResource, filter, pagination, sortByList); + // Create a mock for CloseableIterator + CloseableIterator mockIterator = mock(CloseableIterator.class); + when(mockIterator.hasNext()).thenReturn(false); // No documents to process + + // Mock the collection.query to return the mock iterator + when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); + + // Act + configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList()); + + // Capture the Query object passed to collection.query + ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); + verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); + + // Assert the properties of the captured Query + Query capturedQuery = queryCaptor.getValue(); + assertNotNull(capturedQuery); // Verify filter is present in the query - assertNotNull(query.getFilter()); + assertNotNull(capturedQuery.getFilter()); - FilterTypeExpression userFilter = - ((LogicalExpression) query.getFilter().get()).getOperands().get(1); - assertEquals( - "config.test.path", - ((IdentifierExpression) ((RelationalExpression) userFilter).getLhs()).getName()); - assertEquals( - "test-value", - ((ConstantExpression) ((RelationalExpression) userFilter).getRhs()).getValue().toString()); + // Compare the actual filter with the expected filter + FilterTypeExpression actualFilter = + ((LogicalExpression) capturedQuery.getFilter().get()).getOperands().get(1); + FilterTypeExpression expectedFilter = createExpectedFilter(); + + assertEquals(expectedFilter, actualFilter); // Compare the entire filter expressions + } + + private FilterTypeExpression createExpectedFilter() { + // Create the expected left-hand side and right-hand side expressions + IdentifierExpression lhs = IdentifierExpression.of("config.test.path"); + ConstantExpression rhs = ConstantExpression.of("test-value"); + + // Build the relational expression + + // Assuming you have a logical expression that contains this relational expression + return RelationalExpression.of( + lhs, org.hypertrace.core.documentstore.expression.operators.RelationalOperator.EQ, rhs); } private static Document getConfigDocument( From 21d8de9a8679e6602309d9799f84ce556073f787 Mon Sep 17 00:00:00 2001 From: Aaron Steinfeld <45047841+aaron-steinfeld@users.noreply.github.com> Date: Wed, 23 Apr 2025 12:34:47 -0400 Subject: [PATCH 29/30] test: simplify tests (#290) --- config-service-impl/build.gradle.kts | 1 + config-service-impl/gradle.lockfile | 20 +- .../store/DocumentConfigStoreTest.java | 180 ++++++------------ 3 files changed, 67 insertions(+), 134 deletions(-) diff --git a/config-service-impl/build.gradle.kts b/config-service-impl/build.gradle.kts index 4a84e25a..501cadc9 100644 --- a/config-service-impl/build.gradle.kts +++ b/config-service-impl/build.gradle.kts @@ -24,6 +24,7 @@ dependencies { compileOnly(commonLibs.lombok) testImplementation(commonLibs.junit.jupiter) + testImplementation(commonLibs.mockito.junit) testImplementation(commonLibs.mockito.core) testImplementation(commonLibs.hypertrace.grpcutils.client) } diff --git a/config-service-impl/gradle.lockfile b/config-service-impl/gradle.lockfile index 5ff44502..065e711b 100644 --- a/config-service-impl/gradle.lockfile +++ b/config-service-impl/gradle.lockfile @@ -82,16 +82,22 @@ org.hypertrace.core.kafkastreams.framework:kafka-bom:0.5.4=compileClasspath,runt org.hypertrace.core.serviceframework:docstore-metrics:0.1.86=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath org.hypertrace.core.serviceframework:platform-metrics:0.1.86=runtimeClasspath,testRuntimeClasspath org.hypertrace.core.serviceframework:service-framework-spi:0.1.86=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath -org.junit.jupiter:junit-jupiter-api:5.10.0=testCompileClasspath,testRuntimeClasspath -org.junit.jupiter:junit-jupiter-engine:5.10.0=testRuntimeClasspath -org.junit.jupiter:junit-jupiter-params:5.10.0=testCompileClasspath,testRuntimeClasspath -org.junit.jupiter:junit-jupiter:5.10.0=testCompileClasspath,testRuntimeClasspath -org.junit.platform:junit-platform-commons:1.10.0=testCompileClasspath,testRuntimeClasspath -org.junit.platform:junit-platform-engine:1.10.0=testRuntimeClasspath -org.junit:junit-bom:5.10.0=testCompileClasspath,testRuntimeClasspath +org.junit.jupiter:junit-jupiter-api:5.10.0=testCompileClasspath +org.junit.jupiter:junit-jupiter-api:5.10.1=testRuntimeClasspath +org.junit.jupiter:junit-jupiter-engine:5.10.1=testRuntimeClasspath +org.junit.jupiter:junit-jupiter-params:5.10.0=testCompileClasspath +org.junit.jupiter:junit-jupiter-params:5.10.1=testRuntimeClasspath +org.junit.jupiter:junit-jupiter:5.10.0=testCompileClasspath +org.junit.jupiter:junit-jupiter:5.10.1=testRuntimeClasspath +org.junit.platform:junit-platform-commons:1.10.0=testCompileClasspath +org.junit.platform:junit-platform-commons:1.10.1=testRuntimeClasspath +org.junit.platform:junit-platform-engine:1.10.1=testRuntimeClasspath +org.junit:junit-bom:5.10.0=testCompileClasspath +org.junit:junit-bom:5.10.1=testRuntimeClasspath org.latencyutils:LatencyUtils:2.0.3=runtimeClasspath,testRuntimeClasspath org.lz4:lz4-java:1.8.0=runtimeClasspath,testRuntimeClasspath org.mockito:mockito-core:5.8.0=testCompileClasspath,testRuntimeClasspath +org.mockito:mockito-junit-jupiter:5.8.0=testCompileClasspath,testRuntimeClasspath org.mongodb:bson-record-codec:5.2.0=runtimeClasspath,testRuntimeClasspath org.mongodb:bson:5.2.0=runtimeClasspath,testRuntimeClasspath org.mongodb:mongodb-driver-core:5.2.0=runtimeClasspath,testRuntimeClasspath 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 c84dfbb3..0643b3f3 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 @@ -1,5 +1,6 @@ package org.hypertrace.config.service.store; +import static java.util.Collections.emptyList; import static org.hypertrace.config.service.TestUtils.CONTEXT1; import static org.hypertrace.config.service.TestUtils.RESOURCE_NAME; import static org.hypertrace.config.service.TestUtils.RESOURCE_NAMESPACE; @@ -12,6 +13,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -53,14 +55,19 @@ import org.hypertrace.core.documentstore.expression.impl.LogicalExpression; import org.hypertrace.core.documentstore.expression.impl.RelationalExpression; import org.hypertrace.core.documentstore.expression.type.FilterTypeExpression; -import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider; import org.hypertrace.core.documentstore.model.options.QueryOptions; import org.hypertrace.core.documentstore.query.Query; import org.hypertrace.core.documentstore.query.SortingSpec; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Answers; import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) class DocumentConfigStoreTest { private static final long CONFIG_VERSION = 5; @@ -72,17 +79,19 @@ class DocumentConfigStoreTest { private static Value config1 = getConfig1(); private static Value config2 = getConfig2(); private static ConfigResourceContext configResourceContext = getConfigResourceContext(); - private static Collection collection; - private DocumentConfigStore configStore; - private Clock mockClock; - private FilterBuilder filterBuilder; - @BeforeEach() + private Collection collection; + @Mock private Clock mockClock; + @Mock private FilterBuilder filterBuilder; + + @Mock(answer = Answers.RETURNS_DEEP_STUBS) + private Datastore mockDatastore; + + @InjectMocks private DocumentConfigStore configStore; + + @BeforeEach void beforeEach() { - collection = mock(Collection.class); - this.mockClock = mock(Clock.class); - this.filterBuilder = mock(FilterBuilder.class); - this.configStore = new DocumentConfigStore(mockClock, new MockDatastore()); + this.collection = mockDatastore.getCollection(CONFIGURATIONS_COLLECTION); } @Test @@ -192,10 +201,7 @@ void WriteConfigForUpdateWithUpsertCondition() throws IOException { .setValue(Values.of("v2"))))) .build(); - org.hypertrace.core.documentstore.Filter docFilter = - new org.hypertrace.core.documentstore.Filter(); when(request.getUpsertCondition()).thenReturn(upsertCondition); - when(filterBuilder.buildDocStoreFilter(upsertCondition)).thenReturn(docFilter); UpsertedConfig upsertedConfig = configStore.writeConfig(configResourceContext, USER_ID, request, USER_EMAIL); assertEquals(config2, upsertedConfig.getConfig()); @@ -222,7 +228,6 @@ void WriteConfigForUpdateWithUpsertCondition() throws IOException { document); // failed upsert condition - when(updateResult.getUpdatedCount()).thenReturn(0L); assertThrows( StatusRuntimeException.class, () -> configStore.writeConfig(configResourceContext, USER_ID, request, USER_EMAIL)); @@ -283,7 +288,7 @@ void getAllConfigs() throws IOException { new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID), Filter.getDefaultInstance(), // for empty filter Pagination.getDefaultInstance(), - Collections.emptyList()); + emptyList()); assertEquals(2, contextSpecificConfigList.size()); assertEquals( @@ -370,30 +375,6 @@ void writeAllConfigs() throws IOException { @Test void buildQuery_withDefaultPagination() throws Exception { - // Arrange - ConfigResource configResource = - new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); - Filter filter = Filter.getDefaultInstance(); - Pagination pagination = Pagination.getDefaultInstance(); // Default pagination - - // Create a mock for CloseableIterator - CloseableIterator mockIterator = mock(CloseableIterator.class); - when(mockIterator.hasNext()).thenReturn(false); // No documents to process - - // Mock the collection.query to return the mock iterator - when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); - - // Act - configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList()); - - // Capture the Query object passed to collection.query - ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); - verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); - - // Assert the properties of the captured Query - Query capturedQuery = queryCaptor.getValue(); - assertNotNull(capturedQuery); - List expectedSorts = Arrays.asList( SortingSpec.of( @@ -402,47 +383,39 @@ void buildQuery_withDefaultPagination() throws Exception { SortingSpec.of( IdentifierExpression.of("creationTimestamp"), org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC)); - assertEquals(expectedSorts, capturedQuery.getSorts()); + + configStore.getAllConfigs( + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"), + Filter.getDefaultInstance(), + Pagination.getDefaultInstance(), + emptyList()); + + verify(collection) + .query( + argThat(query -> query.getSorts().equals(expectedSorts)), + any(QueryOptions.class)); } @Test void buildQuery_withCustomPagination() throws IOException { - ConfigResource configResource = - new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); - Filter filter = Filter.getDefaultInstance(); Pagination pagination = Pagination.newBuilder().setOffset(10).setLimit(20).build(); - List sortByList = Collections.emptyList(); - - // Create a mock for CloseableIterator - CloseableIterator mockIterator = mock(CloseableIterator.class); - when(mockIterator.hasNext()).thenReturn(false); // No documents to process - - // Mock the collection.query to return the mock iterator - when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); - - // Act - configStore.getAllConfigs(configResource, filter, pagination, sortByList); - - // Capture the Query object passed to collection.query - ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); - verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); - - // Assert the properties of the captured Query - Query capturedQuery = queryCaptor.getValue(); - assertNotNull(capturedQuery); - org.hypertrace.core.documentstore.query.Pagination expectedPagination = org.hypertrace.core.documentstore.query.Pagination.builder().offset(10).limit(20).build(); - assertEquals(expectedPagination, capturedQuery.getPagination().get()); + + configStore.getAllConfigs( + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"), + Filter.getDefaultInstance(), + pagination, + emptyList()); + + verify(collection) + .query( + argThat(query -> query.getPagination().orElseThrow().equals(expectedPagination)), + any(QueryOptions.class)); } @Test void buildQuery_withCustomSorting() throws IOException { - ConfigResource configResource = - new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"); - Filter filter = Filter.getDefaultInstance(); - Pagination pagination = Pagination.getDefaultInstance(); - org.hypertrace.config.service.v1.Selection selection = org.hypertrace.config.service.v1.Selection.newBuilder() .setConfigJsonPath("test.path") @@ -452,24 +425,7 @@ void buildQuery_withCustomSorting() throws IOException { .setSelection(selection) .setSortOrder(SortOrder.SORT_ORDER_DESC) .build(); - List sortByList = Collections.singletonList(sortBy); - // Create a mock for CloseableIterator - CloseableIterator mockIterator = mock(CloseableIterator.class); - when(mockIterator.hasNext()).thenReturn(false); // No documents to process - - // Mock the collection.query to return the mock iterator - when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); - - // Act - configStore.getAllConfigs(configResource, filter, pagination, sortByList); - // Capture the Query object passed to collection.query - ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); - verify(collection).query(queryCaptor.capture(), any(QueryOptions.class)); - - // Assert the properties of the captured Query - Query capturedQuery = queryCaptor.getValue(); - assertNotNull(capturedQuery); List expectedSorts = Arrays.asList( SortingSpec.of( @@ -479,7 +435,14 @@ void buildQuery_withCustomSorting() throws IOException { IdentifierExpression.of("config.test.path"), org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC)); - assertEquals(expectedSorts, capturedQuery.getSorts()); + configStore.getAllConfigs( + new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"), + Filter.getDefaultInstance(), + Pagination.getDefaultInstance(), + Collections.singletonList(sortBy)); + + verify(collection) + .query(argThat(query -> query.getSorts().equals(expectedSorts)), any(QueryOptions.class)); } @Test @@ -497,7 +460,7 @@ void buildQuery_withFilter() throws IOException { Filter filter = Filter.newBuilder().setRelationalFilter(relationalFilter).build(); Pagination pagination = Pagination.getDefaultInstance(); - List sortByList = Collections.emptyList(); + List sortByList = emptyList(); // Create a mock for CloseableIterator CloseableIterator mockIterator = mock(CloseableIterator.class); @@ -507,7 +470,7 @@ void buildQuery_withFilter() throws IOException { when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator); // Act - configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList()); + configStore.getAllConfigs(configResource, filter, pagination, emptyList()); // Capture the Query object passed to collection.query ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(Query.class); @@ -540,7 +503,7 @@ private FilterTypeExpression createExpectedFilter() { lhs, org.hypertrace.core.documentstore.expression.operators.RelationalOperator.EQ, rhs); } - private static Document getConfigDocument( + private Document getConfigDocument( String context, long version, Value config, long creationTimestamp, long updateTimestamp) { return new ConfigDocument( RESOURCE_NAME, @@ -577,41 +540,4 @@ public Document next() { return documentIterator.next(); } } - - public static class MockDatastore implements Datastore { - - @Override - public Set listCollections() { - return Collections.singleton(CONFIGURATIONS_COLLECTION); - } - - @Override - public Collection getCollection(String s) { - return collection; - } - - // default implementation for other methods as they are unused - @Override - public boolean createCollection(String s, Map map) { - return false; - } - - @Override - public boolean deleteCollection(String s) { - return false; - } - - @Override - public boolean healthCheck() { - return false; - } - - @Override - public DocStoreMetricProvider getDocStoreMetricProvider() { - return null; - } - - @Override - public void close() {} - } } From 1ea83136fda59ae3efa245d5463c3cebd15b2118 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Kolamuri Date: Wed, 23 Apr 2025 22:15:08 +0530 Subject: [PATCH 30/30] spotlessapply --- .../config/service/store/DocumentConfigStoreTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 0643b3f3..e062b766 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 @@ -391,9 +391,7 @@ void buildQuery_withDefaultPagination() throws Exception { emptyList()); verify(collection) - .query( - argThat(query -> query.getSorts().equals(expectedSorts)), - any(QueryOptions.class)); + .query(argThat(query -> query.getSorts().equals(expectedSorts)), any(QueryOptions.class)); } @Test