added internal flag to the AttributeMetadataFilter (#106)

* added internal flag to the AttributeMetadataFilter * added implementation for internal field * fixed some failing integration Tests * fixed unit tests * made the internal field in the filter optional * fixed failing snyk-scan * fix * fix2 * fix3 * added test for the case when internal flag is not set in the filter Co-authored-by: Ankit Choudhary <[email protected]>
hypertrace · Sep 23, 2021 · 8baa62e · 8baa62e
1 parent 2be10b0
commit 8baa62e
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 16 deletions.
diff --git a/.snyk b/.snyk
@@ -5,5 +5,5 @@ ignore:
   SNYK-JAVA-IONETTY-1042268:
     - '*':
         reason: No replacement available
-        expires: 2021-08-31T00:00:00.000Z
+        expires: 2021-10-31T00:00:00.000Z
 patch: {}
diff --git a/...vice-api/src/main/proto/org/hypertrace/core/attribute/service/v1/attribute_metadata.proto b/...vice-api/src/main/proto/org/hypertrace/core/attribute/service/v1/attribute_metadata.proto
@@ -136,6 +136,7 @@ message AttributeMetadataFilter {
   repeated AttributeScope scope = 2 [deprecated = true]; // Use scope_string instead
   repeated string key = 4;
   repeated string scope_string = 5;
+  optional bool internal = 6;
 }
 
 message Empty {

diff --git a/...ervice-impl/src/main/java/org/hypertrace/core/attribute/service/AttributeServiceImpl.java b/...ervice-impl/src/main/java/org/hypertrace/core/attribute/service/AttributeServiceImpl.java
@@ -37,6 +37,7 @@
 import org.hypertrace.core.documentstore.DatastoreProvider;
 import org.hypertrace.core.documentstore.Document;
 import org.hypertrace.core.documentstore.Filter;
+import org.hypertrace.core.documentstore.Filter.Op;
 import org.hypertrace.core.documentstore.JSONDocument;
 import org.hypertrace.core.documentstore.Key;
 import org.hypertrace.core.documentstore.Query;
@@ -53,6 +54,7 @@ public class AttributeServiceImpl extends AttributeServiceGrpc.AttributeServiceI
   private static final String ATTRIBUTE_SCOPE_KEY = "scope";
   private static final String ATTRIBUTE_SCOPE_STRING_KEY = "scope_string";
   private static final String ATTRIBUTE_KEY_KEY = "key";
+  private static final String ATTRIBUTE_INTERNAL_KEY = "internal";
   private static final String DOC_STORE_CONFIG_KEY = "document.store";
   private static final String DATA_STORE_TYPE = "dataStoreType";
   private static final String TENANT_ID_KEY = "tenant_id";
@@ -385,6 +387,11 @@ private Query getQueryForFilter(
       andFilters.add(new Filter(Filter.Op.IN, ATTRIBUTE_KEY_KEY, keyFilterRequest));
     }
 
+    if (attributeMetadataFilter.hasInternal()) {
+      andFilters.add(
+          new Filter(Op.EQ, ATTRIBUTE_INTERNAL_KEY, attributeMetadataFilter.getInternal()));
+    }
+
     Filter queryFilter = new Filter();
     if (!andFilters.isEmpty()) {
       queryFilter = andFilters.remove(0);

diff --git a/...ce-impl/src/test/java/org/hypertrace/core/attribute/service/AttributeServiceImplTest.java b/...ce-impl/src/test/java/org/hypertrace/core/attribute/service/AttributeServiceImplTest.java
@@ -150,7 +150,119 @@ public void testFindAllNoTenantId() {
   }
 
   @Test
-  public void testFindAttributes() {
+  public void testFindAttributesWithInternalFlag() {
+    RequestContext requestContext = mock(RequestContext.class);
+    when(requestContext.getTenantId()).thenReturn(Optional.of("test-tenant-id"));
+    Context ctx = Context.current().withValue(RequestContext.CURRENT, requestContext);
+
+    Context previous = ctx.attach();
+    try {
+      Collection collection =
+          mockCollectionReturningDocuments(
+              createMockDocument(
+                  "__root",
+                  "name",
+                  AttributeScope.EVENT,
+                  AttributeType.ATTRIBUTE,
+                  AttributeKind.TYPE_STRING),
+              createMockDocument(
+                  "__root",
+                  "duration",
+                  AttributeScope.EVENT,
+                  AttributeType.METRIC,
+                  AttributeKind.TYPE_INT64));
+      StreamObserver<AttributeMetadata> responseObserver = mock(StreamObserver.class);
+      AttributeServiceImpl attributeService = new AttributeServiceImpl(collection);
+
+      List<String> fqnList = List.of("EVENT.name", "EVENT.id");
+      List<String> keyList = List.of("name", "startTime", "duration");
+
+      AttributeMetadataFilter attributeMetadataFilter =
+          AttributeMetadataFilter.newBuilder()
+              .addAllFqn(fqnList)
+              .addAllKey(keyList)
+              .addAllScope(
+                  List.of(AttributeScope.TRACE, AttributeScope.EVENT, AttributeScope.BACKEND))
+              .addScopeString("OTHER")
+              .setInternal(true)
+              .build();
+
+      List<String> allScopes =
+          List.of(
+              "OTHER",
+              AttributeScope.TRACE.name(),
+              AttributeScope.EVENT.name(),
+              AttributeScope.BACKEND.name());
+
+      attributeService.findAttributes(attributeMetadataFilter, responseObserver);
+
+      ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class);
+      verify(collection, times(1)).search(queryCaptor.capture());
+
+      Filter filter = queryCaptor.getValue().getFilter();
+      // The structure of the filters is an and(^) filter chain that looks like this:
+      // ((((tenant_id ^ fqn) ^ (scope_string | scope)) ^ key) ^ internal)
+      Assertions.assertEquals(Filter.Op.AND, filter.getOp());
+      Assertions.assertEquals(Filter.Op.EQ, filter.getChildFilters()[1].getOp());
+      Assertions.assertEquals("internal", filter.getChildFilters()[1].getFieldName());
+      Assertions.assertEquals(true, filter.getChildFilters()[1].getValue());
+
+      Filter innerFilter = filter.getChildFilters()[0];
+      Assertions.assertEquals("key", innerFilter.getChildFilters()[1].getFieldName());
+      Assertions.assertEquals(keyList, innerFilter.getChildFilters()[1].getValue());
+
+      Assertions.assertEquals(Filter.Op.AND, innerFilter.getChildFilters()[0].getOp());
+      Filter scopeFilter = innerFilter.getChildFilters()[0].getChildFilters()[1];
+      Assertions.assertEquals(Filter.Op.OR, scopeFilter.getOp());
+
+      Assertions.assertEquals(Filter.Op.IN, scopeFilter.getChildFilters()[0].getOp());
+      Assertions.assertEquals("scope_string", scopeFilter.getChildFilters()[0].getFieldName());
+      Assertions.assertEquals(allScopes, scopeFilter.getChildFilters()[0].getValue());
+
+      Assertions.assertEquals(Filter.Op.IN, scopeFilter.getChildFilters()[1].getOp());
+      Assertions.assertEquals("scope", scopeFilter.getChildFilters()[1].getFieldName());
+      Assertions.assertEquals(allScopes, scopeFilter.getChildFilters()[1].getValue());
+
+      Assertions.assertEquals(
+          Filter.Op.AND, innerFilter.getChildFilters()[0].getChildFilters()[0].getOp());
+      Assertions.assertEquals(
+          Filter.Op.IN,
+          innerFilter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1].getOp());
+      Assertions.assertEquals(
+          "fqn",
+          innerFilter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1]
+              .getFieldName());
+      Assertions.assertEquals(
+          fqnList,
+          innerFilter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1].getValue());
+
+      Assertions.assertEquals(
+          Filter.Op.IN,
+          innerFilter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[0].getOp());
+      Assertions.assertEquals(
+          List.of("__root", "test-tenant-id"),
+          innerFilter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[0].getValue());
+
+      ArgumentCaptor<AttributeMetadata> attributeMetadataArgumentCaptor =
+          ArgumentCaptor.forClass(AttributeMetadata.class);
+      verify(responseObserver, times(2)).onNext(attributeMetadataArgumentCaptor.capture());
+
+      List<AttributeMetadata> attributeMetadataList =
+          attributeMetadataArgumentCaptor.getAllValues();
+      Assertions.assertEquals(2, attributeMetadataList.size());
+
+      Assertions.assertEquals(MOCK_EVENT_NAME_ATTRIBUTE, attributeMetadataList.get(0));
+      Assertions.assertEquals(MOCK_EVENT_DURATION_ATTRIBUTE, attributeMetadataList.get(1));
+
+      verify(responseObserver, times(1)).onCompleted();
+      verify(responseObserver, never()).onError(any(Throwable.class));
+    } finally {
+      ctx.detach(previous);
+    }
+  }
+
+  @Test
+  public void testFindAttributesWithNoInternalFlag() {
     RequestContext requestContext = mock(RequestContext.class);
     when(requestContext.getTenantId()).thenReturn(Optional.of("test-tenant-id"));
     Context ctx = Context.current().withValue(RequestContext.CURRENT, requestContext);
@@ -213,7 +325,6 @@ public void testFindAttributes() {
       Assertions.assertEquals(Filter.Op.IN, scopeFilter.getChildFilters()[0].getOp());
       Assertions.assertEquals("scope_string", scopeFilter.getChildFilters()[0].getFieldName());
       Assertions.assertEquals(allScopes, scopeFilter.getChildFilters()[0].getValue());
-
       Assertions.assertEquals(Filter.Op.IN, scopeFilter.getChildFilters()[1].getOp());
       Assertions.assertEquals("scope", scopeFilter.getChildFilters()[1].getFieldName());
       Assertions.assertEquals(allScopes, scopeFilter.getChildFilters()[1].getValue());
@@ -223,13 +334,6 @@ public void testFindAttributes() {
       Assertions.assertEquals(
           Filter.Op.IN,
           filter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1].getOp());
-      Assertions.assertEquals(
-          "fqn",
-          filter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1].getFieldName());
-      Assertions.assertEquals(
-          fqnList,
-          filter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[1].getValue());
-
       Assertions.assertEquals(
           Filter.Op.IN,
           filter.getChildFilters()[0].getChildFilters()[0].getChildFilters()[0].getOp());
@@ -240,14 +344,11 @@ public void testFindAttributes() {
       ArgumentCaptor<AttributeMetadata> attributeMetadataArgumentCaptor =
           ArgumentCaptor.forClass(AttributeMetadata.class);
       verify(responseObserver, times(2)).onNext(attributeMetadataArgumentCaptor.capture());
-
       List<AttributeMetadata> attributeMetadataList =
           attributeMetadataArgumentCaptor.getAllValues();
       Assertions.assertEquals(2, attributeMetadataList.size());
-
       Assertions.assertEquals(MOCK_EVENT_NAME_ATTRIBUTE, attributeMetadataList.get(0));
       Assertions.assertEquals(MOCK_EVENT_DURATION_ATTRIBUTE, attributeMetadataList.get(1));
-
       verify(responseObserver, times(1)).onCompleted();
       verify(responseObserver, never()).onError(any(Throwable.class));
     } finally {

diff --git a/attribute-service/build.gradle.kts b/attribute-service/build.gradle.kts
@@ -69,10 +69,10 @@ dependencies {
   // GRPC
   runtimeOnly("io.grpc:grpc-netty:1.40.0")
   constraints {
-    runtimeOnly("io.netty:netty-codec-http2:4.1.61.Final") {
+    runtimeOnly("io.netty:netty-codec-http2:4.1.68.Final") {
       because("https://snyk.io/vuln/SNYK-JAVA-IONETTY-1089809")
     }
-    runtimeOnly("io.netty:netty-handler-proxy:4.1.61.Final") {
+    runtimeOnly("io.netty:netty-handler-proxy:4.1.68.Final") {
       because("https://snyk.io/vuln/SNYK-JAVA-IONETTY-1089809")
     }
   }

diff --git a/.../src/integrationTest/java/org/hypertrace/core/attribute/service/AttributeServiceTest.java b/.../src/integrationTest/java/org/hypertrace/core/attribute/service/AttributeServiceTest.java
@@ -148,7 +148,6 @@ public void testCheckValidAttributeMetadata() {
             .setGroupable(true)
             .setDefinition(AttributeDefinition.newBuilder().setSourcePath("sourcepath-1"))
             .setScopeString(AttributeScope.EVENT.name())
-            .setInternal(true)
             .build();
 
     AttributeCreateRequest request =