chore: add support to delete system label application rule (#236)

* chore: add support to delete system label application rule * fix build failure * address review comments --------- Co-authored-by: saxenakshitiz <[email protected]>
hypertrace · Sep 2, 2024 · b5cf2f4 · b5cf2f4
1 parent 834e663
commit b5cf2f4
Show file tree

Hide file tree

Showing 11 changed files with 196 additions and 29 deletions.
diff --git a/buf.work.yaml b/buf.work.yaml
@@ -3,6 +3,7 @@ directories:
   - alerting-config-service-api/src/main/proto
   - config-service-api/src/main/proto
   - label-application-rule-config-service-api/src/main/proto
+  - label-application-rule-config-service-impl/src/main/proto
   - labels-config-service-api/src/main/proto
   - notification-channel-config-service-api/src/main/proto
   - notification-rule-config-service-api/src/main/proto

diff --git a/config-service/src/main/resources/configs/common/application.conf b/config-service/src/main/resources/configs/common/application.conf
@@ -26,3 +26,8 @@ event.store {
     schema.registry.url = "http://localhost:8081"
   }
 }
+
+label.application.rule.config.service {
+  max.dynamic.label.application.rules.per.tenant = 100
+  system.label.application.rules = []
+}
diff --git a/label-application-rule-config-service-impl/build.gradle.kts b/label-application-rule-config-service-impl/build.gradle.kts
@@ -1,9 +1,16 @@
 plugins {
   `java-library`
+  alias(commonLibs.plugins.google.protobuf)
   jacoco
   alias(commonLibs.plugins.hypertrace.jacoco)
 }
 
+protobuf {
+  protoc {
+    artifact = "com.google.protobuf:protoc:${commonLibs.versions.protoc.get()}"
+  }
+}
+
 dependencies {
   api(projects.labelApplicationRuleConfigServiceApi)
   implementation(projects.configServiceApi)

diff --git a/...ertrace/label/application/rule/config/service/DeletedSystemLabelApplicationRuleStore.java b/...ertrace/label/application/rule/config/service/DeletedSystemLabelApplicationRuleStore.java
@@ -0,0 +1,50 @@
+package org.hypertrace.label.application.rule.config.service;
+
+import com.google.protobuf.Value;
+import java.util.Optional;
+import lombok.SneakyThrows;
+import org.hypertrace.config.objectstore.IdentifiedObjectStore;
+import org.hypertrace.config.proto.converter.ConfigProtoConverter;
+import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
+import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
+import org.hypertrace.label.application.rule.config.service.impl.v1.DeletedSystemLabelApplicationRule;
+
+class DeletedSystemLabelApplicationRuleStore
+    extends IdentifiedObjectStore<DeletedSystemLabelApplicationRule> {
+
+  private static final String DELETED_SYSTEM_LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAME =
+      "deleted-system-label-application-rule-config";
+  private static final String LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAMESPACE = "labels";
+
+  DeletedSystemLabelApplicationRuleStore(
+      ConfigServiceBlockingStub stub, ConfigChangeEventGenerator configChangeEventGenerator) {
+    super(
+        stub,
+        LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAMESPACE,
+        DELETED_SYSTEM_LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAME,
+        configChangeEventGenerator);
+  }
+
+  @Override
+  protected Optional<DeletedSystemLabelApplicationRule> buildDataFromValue(Value value) {
+    try {
+      DeletedSystemLabelApplicationRule.Builder builder =
+          DeletedSystemLabelApplicationRule.newBuilder();
+      ConfigProtoConverter.mergeFromValue(value, builder);
+      return Optional.of(builder.build());
+    } catch (Exception e) {
+      return Optional.empty();
+    }
+  }
+
+  @SneakyThrows
+  @Override
+  protected Value buildValueFromData(DeletedSystemLabelApplicationRule object) {
+    return ConfigProtoConverter.convertToValue(object);
+  }
+
+  @Override
+  protected String getContextFromData(DeletedSystemLabelApplicationRule object) {
+    return object.getId();
+  }
+}
diff --git a/...java/org/hypertrace/label/application/rule/config/service/LabelApplicationRuleConfig.java b/...java/org/hypertrace/label/application/rule/config/service/LabelApplicationRuleConfig.java
@@ -9,6 +9,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import lombok.Getter;
 import lombok.SneakyThrows;
@@ -24,7 +25,7 @@ public class LabelApplicationRuleConfig {
 
   @Getter private final int maxDynamicLabelApplicationRulesAllowed;
   @Getter private final List<LabelApplicationRule> systemLabelApplicationRules;
-  @Getter private final Map<String, LabelApplicationRule> systemLabelApplicationRulesMap;
+  private final Map<String, LabelApplicationRule> systemLabelApplicationRulesMap;
 
   public LabelApplicationRuleConfig(Config config) {
     Config labelApplicationRuleConfig =
@@ -64,4 +65,8 @@ private static LabelApplicationRule buildLabelApplicationRuleFromConfig(
     JsonFormat.parser().merge(jsonString, builder);
     return builder.build();
   }
+
+  public Optional<LabelApplicationRule> getSystemLabelApplicationRule(String id) {
+    return Optional.ofNullable(systemLabelApplicationRulesMap.get(id));
+  }
 }
diff --git a/...pertrace/label/application/rule/config/service/LabelApplicationRuleConfigServiceImpl.java b/...pertrace/label/application/rule/config/service/LabelApplicationRuleConfigServiceImpl.java
@@ -2,7 +2,6 @@
 
 import io.grpc.Channel;
 import io.grpc.Status;
-import io.grpc.StatusRuntimeException;
 import io.grpc.stub.StreamObserver;
 import java.util.List;
 import java.util.Optional;
@@ -16,6 +15,7 @@
 import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
 import org.hypertrace.core.grpcutils.client.RequestContextClientCallCredsProviderFactory;
 import org.hypertrace.core.grpcutils.context.RequestContext;
+import org.hypertrace.label.application.rule.config.service.impl.v1.DeletedSystemLabelApplicationRule;
 import org.hypertrace.label.application.rule.config.service.v1.CreateLabelApplicationRuleRequest;
 import org.hypertrace.label.application.rule.config.service.v1.CreateLabelApplicationRuleResponse;
 import org.hypertrace.label.application.rule.config.service.v1.DeleteLabelApplicationRuleRequest;
@@ -30,6 +30,8 @@
 public class LabelApplicationRuleConfigServiceImpl
     extends LabelApplicationRuleConfigServiceGrpc.LabelApplicationRuleConfigServiceImplBase {
   private final IdentifiedObjectStore<LabelApplicationRule> labelApplicationRuleStore;
+  private final IdentifiedObjectStore<DeletedSystemLabelApplicationRule>
+      deletedSystemLabelApplicationRuleStore;
   private final LabelApplicationRuleValidator requestValidator;
   private final LabelApplicationRuleConfig labelApplicationRuleConfig;
 
@@ -45,6 +47,9 @@ public LabelApplicationRuleConfigServiceImpl(
                 RequestContextClientCallCredsProviderFactory.getClientCallCredsProvider().get());
     this.labelApplicationRuleStore =
         new LabelApplicationRuleStore(configServiceBlockingStub, configChangeEventGenerator);
+    this.deletedSystemLabelApplicationRuleStore =
+        new DeletedSystemLabelApplicationRuleStore(
+            configServiceBlockingStub, configChangeEventGenerator);
     this.requestValidator = new LabelApplicationRuleValidatorImpl();
   }
 
@@ -90,9 +95,12 @@ public void getLabelApplicationRules(
           labelApplicationRules.stream()
               .map(LabelApplicationRule::getId)
               .collect(Collectors.toUnmodifiableSet());
+      Set<String> deletedSystemLabelApplicationRuleIds =
+          getDeletedSystemLabelApplicationRuleIds(requestContext);
       List<LabelApplicationRule> filteredSystemLabelApplicationRules =
           this.labelApplicationRuleConfig.getSystemLabelApplicationRules().stream()
               .filter(rule -> !labelApplicationRuleIds.contains(rule.getId()))
+              .filter(rule -> !deletedSystemLabelApplicationRuleIds.contains(rule.getId()))
               .collect(Collectors.toUnmodifiableList());
       responseObserver.onNext(
           GetLabelApplicationRulesResponse.newBuilder()
@@ -115,12 +123,7 @@ public void updateLabelApplicationRule(
       LabelApplicationRule existingRule =
           this.labelApplicationRuleStore
               .getData(requestContext, request.getId())
-              .or(
-                  () ->
-                      Optional.ofNullable(
-                          this.labelApplicationRuleConfig
-                              .getSystemLabelApplicationRulesMap()
-                              .get(request.getId())))
+              .or(() -> getSystemLabelApplicationRule(requestContext, request.getId()))
               .orElseThrow(Status.NOT_FOUND::asRuntimeException);
       LabelApplicationRule updateLabelApplicationRule =
           existingRule.toBuilder().setData(request.getData()).build();
@@ -146,18 +149,20 @@ public void deleteLabelApplicationRule(
       RequestContext requestContext = RequestContext.CURRENT.get();
       this.requestValidator.validateOrThrow(requestContext, request);
       String labelApplicationRuleId = request.getId();
-      if (this.labelApplicationRuleConfig
-          .getSystemLabelApplicationRulesMap()
-          .containsKey(labelApplicationRuleId)) {
-        // Deleting a system label application rule is not allowed
-        responseObserver.onError(new StatusRuntimeException(Status.INVALID_ARGUMENT));
-        return;
+      boolean customRuleDeleted =
+          this.labelApplicationRuleStore.deleteObject(requestContext, request.getId()).isPresent();
+      Optional<LabelApplicationRule> systemLabelApplicationRule =
+          getSystemLabelApplicationRule(requestContext, labelApplicationRuleId);
+      if (systemLabelApplicationRule.isPresent()) {
+        deletedSystemLabelApplicationRuleStore.upsertObject(
+            requestContext,
+            DeletedSystemLabelApplicationRule.newBuilder().setId(labelApplicationRuleId).build());
       }
-      this.labelApplicationRuleStore
-          .deleteObject(requestContext, request.getId())
-          .orElseThrow(Status.NOT_FOUND::asRuntimeException);
-      responseObserver.onNext(DeleteLabelApplicationRuleResponse.getDefaultInstance());
-      responseObserver.onCompleted();
+      if (customRuleDeleted || systemLabelApplicationRule.isPresent()) {
+        responseObserver.onNext(DeleteLabelApplicationRuleResponse.getDefaultInstance());
+        responseObserver.onCompleted();
+      }
+      throw Status.NOT_FOUND.asRuntimeException();
     } catch (Exception e) {
       responseObserver.onError(e);
     }
@@ -180,4 +185,20 @@ private void checkRequestForDynamicLabelsLimit(
       }
     }
   }
+
+  private Set<String> getDeletedSystemLabelApplicationRuleIds(RequestContext requestContext) {
+    return this.deletedSystemLabelApplicationRuleStore.getAllObjects(requestContext).stream()
+        .map(ConfigObject::getData)
+        .map(DeletedSystemLabelApplicationRule::getId)
+        .collect(Collectors.toUnmodifiableSet());
+  }
+
+  private Optional<LabelApplicationRule> getSystemLabelApplicationRule(
+      RequestContext requestContext, String id) {
+    return this.labelApplicationRuleConfig
+        .getSystemLabelApplicationRule(id)
+        .filter(
+            unused ->
+                this.deletedSystemLabelApplicationRuleStore.getData(requestContext, id).isEmpty());
+  }
 }
diff --git a/.../java/org/hypertrace/label/application/rule/config/service/LabelApplicationRuleStore.java b/.../java/org/hypertrace/label/application/rule/config/service/LabelApplicationRuleStore.java
@@ -9,7 +9,7 @@
 import org.hypertrace.config.service.v1.ConfigServiceGrpc;
 import org.hypertrace.label.application.rule.config.service.v1.LabelApplicationRule;
 
-public class LabelApplicationRuleStore extends IdentifiedObjectStore<LabelApplicationRule> {
+class LabelApplicationRuleStore extends IdentifiedObjectStore<LabelApplicationRule> {
   private static final String LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAME =
       "label-application-rule-config";
   private static final String LABEL_APPLICATION_RULE_CONFIG_RESOURCE_NAMESPACE = "labels";

diff --git a/label-application-rule-config-service-impl/src/main/proto/buf.yaml b/label-application-rule-config-service-impl/src/main/proto/buf.yaml
@@ -0,0 +1,8 @@
+version: v1
+breaking:
+  use:
+    - PACKAGE
+    - WIRE_JSON
+lint:
+  use:
+    - DEFAULT
diff --git a/...label/application/rule/config/service/impl/v1/deleted_system_label_application_rule.proto b/...label/application/rule/config/service/impl/v1/deleted_system_label_application_rule.proto
@@ -0,0 +1,9 @@
+syntax="proto3";
+
+package org.hypertrace.label.application.rule.config.service.impl.v1;
+
+option java_multiple_files = true;
+
+message DeletedSystemLabelApplicationRule {
+  string id = 1;
+}
diff --git a/...race/label/application/rule/config/service/LabelApplicationRuleConfigServiceImplTest.java b/...race/label/application/rule/config/service/LabelApplicationRuleConfigServiceImplTest.java
@@ -1,5 +1,6 @@
 package org.hypertrace.label.application.rule.config.service;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -14,6 +15,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
@@ -59,8 +61,8 @@ void setUp() throws InvalidProtocolBufferException {
             .build();
     LabelApplicationRuleConfig config = mock(LabelApplicationRuleConfig.class);
     when(config.getSystemLabelApplicationRules()).thenReturn(List.of(systemLabelApplicationRule));
-    when(config.getSystemLabelApplicationRulesMap())
-        .thenReturn(Map.of(systemLabelApplicationRule.getId(), systemLabelApplicationRule));
+    when(config.getSystemLabelApplicationRule(systemLabelApplicationRule.getId()))
+        .thenReturn(Optional.of(systemLabelApplicationRule));
     when(config.getMaxDynamicLabelApplicationRulesAllowed()).thenReturn(2);
     mockGenericConfigService
         .addService(
@@ -204,13 +206,66 @@ void deleteSystemApplicationRuleError() {
         DeleteLabelApplicationRuleRequest.newBuilder()
             .setId(systemLabelApplicationRule.getId())
             .build();
+    assertDoesNotThrow(
+        () -> labelApplicationRuleConfigServiceBlockingStub.deleteLabelApplicationRule(request));
+    // deleting an already deleted system label application rule should throw error
     Throwable exception =
         assertThrows(
             StatusRuntimeException.class,
-            () -> {
-              labelApplicationRuleConfigServiceBlockingStub.deleteLabelApplicationRule(request);
-            });
-    assertEquals(Status.INVALID_ARGUMENT, Status.fromThrowable(exception));
+            () ->
+                labelApplicationRuleConfigServiceBlockingStub.deleteLabelApplicationRule(request));
+    assertEquals(Status.NOT_FOUND, Status.fromThrowable(exception));
+  }
+
+  @Test
+  void updateDeletedSystemLabelApplicationRule() {
+    DeleteLabelApplicationRuleRequest request =
+        DeleteLabelApplicationRuleRequest.newBuilder()
+            .setId(systemLabelApplicationRule.getId())
+            .build();
+    assertDoesNotThrow(
+        () -> labelApplicationRuleConfigServiceBlockingStub.deleteLabelApplicationRule(request));
+    // updating an already deleted system label application rule should throw error
+    LabelApplicationRuleData expectedData = buildSimpleRuleData("auth", "not-valid");
+    UpdateLabelApplicationRuleRequest updateRequest =
+        UpdateLabelApplicationRuleRequest.newBuilder()
+            .setId(systemLabelApplicationRule.getId())
+            .setData(expectedData)
+            .build();
+    Throwable exception =
+        assertThrows(
+            StatusRuntimeException.class,
+            () ->
+                labelApplicationRuleConfigServiceBlockingStub.updateLabelApplicationRule(
+                    updateRequest));
+    assertEquals(Status.NOT_FOUND, Status.fromThrowable(exception));
+  }
+
+  @Test
+  void getLabelApplicationRulesAfterDeletingSystemLabelApplicationRule() {
+    LabelApplicationRule simpleRule = createSimpleRule("auth", "valid");
+    LabelApplicationRule compositeRule = createCompositeRule();
+    Set<LabelApplicationRule> expectedRules =
+        Set.of(simpleRule, compositeRule, systemLabelApplicationRule);
+    GetLabelApplicationRulesResponse response =
+        labelApplicationRuleConfigServiceBlockingStub.getLabelApplicationRules(
+            GetLabelApplicationRulesRequest.getDefaultInstance());
+    assertEquals(
+        expectedRules,
+        response.getLabelApplicationRulesList().stream().collect(Collectors.toUnmodifiableSet()));
+    DeleteLabelApplicationRuleRequest request =
+        DeleteLabelApplicationRuleRequest.newBuilder()
+            .setId(systemLabelApplicationRule.getId())
+            .build();
+    assertDoesNotThrow(
+        () -> labelApplicationRuleConfigServiceBlockingStub.deleteLabelApplicationRule(request));
+    expectedRules = Set.of(simpleRule, compositeRule);
+    response =
+        labelApplicationRuleConfigServiceBlockingStub.getLabelApplicationRules(
+            GetLabelApplicationRulesRequest.getDefaultInstance());
+    assertEquals(
+        expectedRules,
+        response.getLabelApplicationRulesList().stream().collect(Collectors.toUnmodifiableSet()));
   }
 
   private LabelApplicationRuleData buildCompositeRuleData() {

diff --git a/.../org/hypertrace/label/application/rule/config/service/LabelApplicationRuleConfigTest.java b/.../org/hypertrace/label/application/rule/config/service/LabelApplicationRuleConfigTest.java
@@ -1,6 +1,7 @@
 package org.hypertrace.label.application.rule.config.service;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.util.JsonFormat;
@@ -9,6 +10,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import org.hypertrace.label.application.rule.config.service.v1.LabelApplicationRule;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -53,8 +55,12 @@ void test_getSystemLabelApplicationRules() {
 
   @Test
   void test_getSystemLabelApplicationRulesMap() {
-    assertEquals(
-        stringLabelApplicationRuleMap,
-        labelApplicationRuleConfig.getSystemLabelApplicationRulesMap());
+    Optional<LabelApplicationRule> rule =
+        labelApplicationRuleConfig.getSystemLabelApplicationRule(
+            systemLabelApplicationRule.getId());
+    assertTrue(rule.isPresent());
+    assertEquals(systemLabelApplicationRule, rule.get());
+    assertTrue(
+        labelApplicationRuleConfig.getSystemLabelApplicationRule("id-does-not-exist").isEmpty());
   }
 }