Bugfix user filters

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

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

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()

config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java

@@ -262,7 +262,9 @@ private Query buildQuery(
       sortByList.forEach(
           sortBy ->
               queryBuilder.addSort(
-                  IdentifierExpression.of(sortBy.getSelection().getConfigJsonPath()),
+                  IdentifierExpression.of(
+                      ConfigServiceUtils.buildConfigFieldPath(
+                          sortBy.getSelection().getConfigJsonPath())),
                   convertSortOrder(sortBy)));
     } else {
       queryBuilder.addSort(IdentifierExpression.of(CREATION_TIMESTAMP_FIELD_NAME), SortOrder.DESC);

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()));
   }
-
-  private String buildConfigFieldPath(String configJsonPath) {
-    return String.format("%s.%s", CONFIG_FIELD_NAME, configJsonPath);
-  }
 }

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;
@@ -9,8 +10,10 @@
 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.ArgumentMatchers.argThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -22,6 +25,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;
@@ -37,6 +41,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,12 +50,24 @@
 import org.hypertrace.core.documentstore.Document;
 import org.hypertrace.core.documentstore.Key;
 import org.hypertrace.core.documentstore.UpdateResult;
-import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider;
+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.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;
@@ -62,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
@@ -182,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());
@@ -212,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));
@@ -273,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(
@@ -358,7 +373,135 @@ void writeAllConfigs() throws IOException {
                     updateTime)));
   }
 
-  private static Document getConfigDocument(
+  @Test
+  void buildQuery_withDefaultPagination() throws Exception {
+    List<SortingSpec> 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));
+
+    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 {
+    Pagination pagination = Pagination.newBuilder().setOffset(10).setLimit(20).build();
+    org.hypertrace.core.documentstore.query.Pagination expectedPagination =
+        org.hypertrace.core.documentstore.query.Pagination.builder().offset(10).limit(20).build();
+
+    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 {
+    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<SortingSpec> 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));
+
+    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
+  void buildQuery_withFilter() throws IOException {
+    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<org.hypertrace.config.service.v1.SortBy> sortByList = emptyList();
+
+    // Create a mock for CloseableIterator<Document>
+    CloseableIterator<Document> 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, emptyList());
+
+    // Capture the Query object passed to collection.query
+    ArgumentCaptor<Query> 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(capturedQuery.getFilter());
+
+    // 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 Document getConfigDocument(
       String context, long version, Value config, long creationTimestamp, long updateTimestamp) {
     return new ConfigDocument(
         RESOURCE_NAME,
@@ -395,41 +538,4 @@ public Document next() {
       return documentIterator.next();
     }
   }
-
-  public static class MockDatastore implements Datastore {
-
-    @Override
-    public Set<String> 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<String, String> 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() {}
-  }
 }