Extract notification methods in identified object store (#77)

SrikarMannepalli · web-flow · commit 777df107b014 · 2021-11-03T11:43:18.000+05:30
* extract notification methods

* refactor

* address review comments

* address review comments

* update DefaultObjectStore and ContextuallyIdentifiedObjectStore

* address review comments

* addres review comments

* address review comments

* remove unused object

* address review comments

* add tests
diff --git a/object-store/build.gradle.kts b/object-store/build.gradle.kts
@@ -10,6 +10,7 @@ dependencies {
   api(libs.hypertrace.grpcutils.context)
 
   implementation(projects.configServiceChangeEventGenerator)
+  implementation(libs.slf4j.api)
 
   annotationProcessor(libs.lombok)
   compileOnly(libs.lombok)
diff --git a/object-store/src/main/java/org/hypertrace/config/objectstore/ContextuallyIdentifiedObjectStore.java b/object-store/src/main/java/org/hypertrace/config/objectstore/ContextuallyIdentifiedObjectStore.java
@@ -38,6 +38,14 @@ protected ContextuallyIdentifiedObjectStore(
 
   protected abstract Value buildValueFromData(T data);
 
+  protected Value buildValueForChangeEvent(T data) {
+    return this.buildValueFromData(data);
+  }
+
+  protected String buildClassNameForChangeEvent(T data) {
+    return data.getClass().getName();
+  }
+
   protected abstract String getConfigContextFromRequestContext(RequestContext requestContext);
 
   private IdentifiedObjectStore<T> buildObjectStoreForContext(RequestContext context) {
@@ -86,6 +94,16 @@ protected Value buildValueFromData(T data) {
       return ContextuallyIdentifiedObjectStore.this.buildValueFromData(data);
     }
 
+    @Override
+    protected Value buildValueForChangeEvent(T data) {
+      return ContextuallyIdentifiedObjectStore.this.buildValueForChangeEvent(data);
+    }
+
+    @Override
+    protected String buildClassNameForChangeEvent(T data) {
+      return ContextuallyIdentifiedObjectStore.this.buildClassNameForChangeEvent(data);
+    }
+
     @Override
     protected String getContextFromData(T data) {
       return ContextuallyIdentifiedObjectStore.this.getConfigContextFromRequestContext(
diff --git a/object-store/src/main/java/org/hypertrace/config/objectstore/DefaultObjectStore.java b/object-store/src/main/java/org/hypertrace/config/objectstore/DefaultObjectStore.java
@@ -3,6 +3,7 @@
 import com.google.protobuf.Value;
 import io.grpc.Status;
 import java.util.Optional;
+import lombok.extern.slf4j.Slf4j;
 import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
 import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
 import org.hypertrace.config.service.v1.ContextSpecificConfig;
@@ -18,6 +19,7 @@
  *
  * @param <T>
  */
+@Slf4j
 public abstract class DefaultObjectStore<T> {
   private final ConfigServiceBlockingStub configServiceBlockingStub;
   private final String resourceNamespace;
@@ -49,6 +51,14 @@ protected DefaultObjectStore(
 
   protected abstract Value buildValueFromData(T data);
 
+  protected Value buildValueForChangeEvent(T data) {
+    return this.buildValueFromData(data);
+  }
+
+  protected String buildClassNameForChangeEvent(T data) {
+    return data.getClass().getName();
+  }
+
   public Optional<T> getData(RequestContext context) {
     try {
       Value value =
@@ -91,12 +101,22 @@ public ConfigObject<T> upsertObject(RequestContext context, T data) {
           if (response.hasPrevConfig()) {
             configChangeEventGenerator.sendUpdateNotification(
                 context,
-                upsertedObject.getData().getClass().getName(),
-                response.getPrevConfig(),
-                response.getConfig());
+                this.buildClassNameForChangeEvent(upsertedObject.getData()),
+                this.buildDataFromValue(response.getPrevConfig())
+                    .map(this::buildValueForChangeEvent)
+                    .orElseGet(
+                        () -> {
+                          log.error(
+                              "Unable to convert previousValue back to data for change event. Falling back to raw value {}",
+                              response.getPrevConfig());
+                          return response.getPrevConfig();
+                        }),
+                this.buildValueForChangeEvent(upsertedObject.getData()));
           } else {
             configChangeEventGenerator.sendCreateNotification(
-                context, upsertedObject.getData().getClass().getName(), response.getConfig());
+                context,
+                this.buildClassNameForChangeEvent(upsertedObject.getData()),
+                this.buildValueForChangeEvent(upsertedObject.getData()));
           }
         });
     return upsertedObject;
@@ -120,7 +140,9 @@ public Optional<ConfigObject<T>> deleteObject(RequestContext context) {
       configChangeEventGeneratorOptional.ifPresent(
           configChangeEventGenerator ->
               configChangeEventGenerator.sendDeleteNotification(
-                  context, object.getData().getClass().getName(), deletedConfig.getConfig()));
+                  context,
+                  this.buildClassNameForChangeEvent(object.getData()),
+                  this.buildValueForChangeEvent(object.getData())));
       return Optional.of(object);
     } catch (Exception exception) {
       if (Status.fromThrowable(exception).equals(Status.NOT_FOUND)) {
diff --git a/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java b/object-store/src/main/java/org/hypertrace/config/objectstore/IdentifiedObjectStore.java
@@ -5,6 +5,7 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
 import org.hypertrace.config.service.change.event.api.ConfigChangeEventGenerator;
 import org.hypertrace.config.service.v1.ConfigServiceGrpc.ConfigServiceBlockingStub;
 import org.hypertrace.config.service.v1.ContextSpecificConfig;
@@ -24,6 +25,7 @@
  *
  * @param <T>
  */
+@Slf4j
 public abstract class IdentifiedObjectStore<T> {
   private final ConfigServiceBlockingStub configServiceBlockingStub;
   private final String resourceNamespace;
@@ -57,6 +59,14 @@ protected IdentifiedObjectStore(
 
   protected abstract String getContextFromData(T data);
 
+  protected Value buildValueForChangeEvent(T data) {
+    return this.buildValueFromData(data);
+  }
+
+  protected String buildClassNameForChangeEvent(T data) {
+    return data.getClass().getName();
+  }
+
   protected List<ContextualConfigObject<T>> orderFetchedObjects(
       List<ContextualConfigObject<T>> objects) {
     return objects;
@@ -141,7 +151,10 @@ public Optional<ContextualConfigObject<T>> deleteObject(RequestContext context,
       configChangeEventGeneratorOptional.ifPresent(
           configChangeEventGenerator ->
               configChangeEventGenerator.sendDeleteNotification(
-                  context, object.getData().getClass().getName(), id, deletedConfig.getConfig()));
+                  context,
+                  this.buildClassNameForChangeEvent(object.getData()),
+                  id,
+                  this.buildValueForChangeEvent(object.getData())));
       return Optional.of(object);
     } catch (Exception exception) {
       if (Status.fromThrowable(exception).equals(Status.NOT_FOUND)) {
@@ -181,6 +194,8 @@ private Optional<ContextualConfigObject<T>> processUpsertResult(
     Optional<ContextualConfigObject<T>> optionalResult =
         ContextualConfigObjectImpl.tryBuild(
             response, this::buildDataFromValue, this::getContextFromData);
+
+    System.out.println(optionalResult);
     optionalResult.ifPresent(
         result -> {
           if (response.hasPrevConfig()) {
@@ -213,9 +228,9 @@ private void tryReportCreation(RequestContext requestContext, ContextualConfigOb
         configChangeEventGenerator ->
             configChangeEventGenerator.sendCreateNotification(
                 requestContext,
-                result.getData().getClass().getName(),
+                this.buildClassNameForChangeEvent(result.getData()),
                 result.getContext(),
-                this.buildValueFromData(result.getData())));
+                this.buildValueForChangeEvent(result.getData())));
   }
 
   private void tryReportUpdate(
@@ -224,9 +239,17 @@ private void tryReportUpdate(
         configChangeEventGenerator ->
             configChangeEventGenerator.sendUpdateNotification(
                 requestContext,
-                result.getData().getClass().getName(),
+                this.buildClassNameForChangeEvent(result.getData()),
                 result.getContext(),
-                previousValue,
-                this.buildValueFromData(result.getData())));
+                this.buildDataFromValue(previousValue)
+                    .map(this::buildValueForChangeEvent)
+                    .orElseGet(
+                        () -> {
+                          log.error(
+                              "Unable to convert previousValue back to data for change event. Falling back to raw value {}",
+                              previousValue);
+                          return previousValue;
+                        }),
+                this.buildValueForChangeEvent(result.getData())));
   }
 }
diff --git a/object-store/src/test/java/org/hypertrace/config/objectstore/DefaultObjectStoreTest.java b/object-store/src/test/java/org/hypertrace/config/objectstore/DefaultObjectStoreTest.java
@@ -2,11 +2,13 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.google.protobuf.Struct;
 import com.google.protobuf.Value;
 import com.google.protobuf.util.Values;
 import io.grpc.Status;
@@ -43,7 +45,7 @@ class DefaultObjectStoreTest {
   @Mock(answer = Answers.CALLS_REAL_METHODS)
   RequestContext mockRequestContext;
 
-  DefaultObjectStore<TestObject> store;
+  DefaultObjectStore<TestInternalObject> store;
 
   @BeforeEach
   void beforeEach() {
@@ -56,7 +58,8 @@ void generatesConfigReadRequestForGet() {
     when(this.mockStub.getConfig(any()))
         .thenReturn(GetConfigResponse.newBuilder().setConfig(Values.of("test")).build());
 
-    assertEquals(Optional.of(new TestObject("test")), this.store.getData(this.mockRequestContext));
+    assertEquals(
+        Optional.of(new TestInternalObject("test")), this.store.getData(this.mockRequestContext));
 
     verify(this.mockStub, times(1))
         .getConfig(
@@ -91,7 +94,7 @@ void generatesConfigDeleteRequest() {
     assertEquals(
         Optional.of(
             new ConfigObjectImpl<>(
-                new TestObject("test"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP)),
+                new TestInternalObject("test"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP)),
         this.store.deleteObject(mockRequestContext));
 
     verify(this.mockStub)
@@ -104,6 +107,15 @@ void generatesConfigDeleteRequest() {
     when(this.mockStub.deleteConfig(any())).thenThrow(Status.NOT_FOUND.asRuntimeException());
 
     assertEquals(Optional.empty(), this.store.deleteObject(mockRequestContext));
+
+    verify(this.configChangeEventGenerator, times(1))
+        .sendDeleteNotification(
+            eq(this.mockRequestContext),
+            eq(TestApiObject.class.getName()),
+            eq(
+                Value.newBuilder()
+                    .setStructValue(Struct.newBuilder().putFields("api_name", Values.of("test")))
+                    .build()));
   }
 
   @Test
@@ -115,38 +127,65 @@ void generatesConfigUpsertRequest() {
                 .setUpdateTimestamp(TEST_UPDATE_TIMESTAMP.toEpochMilli())
                 .setConfig(Values.of("updated"))
                 .build());
-    assertEquals(
+    ConfigObject configObject =
         new ConfigObjectImpl<>(
-            new TestObject("updated"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP),
-        this.store.upsertObject(this.mockRequestContext, new TestObject("updated")));
+            new TestInternalObject("updated"), TEST_CREATE_TIMESTAMP, TEST_UPDATE_TIMESTAMP);
+    assertEquals(
+        configObject,
+        this.store.upsertObject(this.mockRequestContext, new TestInternalObject("updated")));
     verify(this.mockStub, times(1))
         .upsertConfig(
             UpsertConfigRequest.newBuilder()
                 .setResourceName(TEST_RESOURCE_NAME)
                 .setResourceNamespace(TEST_RESOURCE_NAMESPACE)
                 .setConfig(Values.of("updated"))
                 .build());
+    verify(this.configChangeEventGenerator, times(1))
+        .sendCreateNotification(
+            eq(this.mockRequestContext),
+            eq(TestApiObject.class.getName()),
+            eq(
+                Value.newBuilder()
+                    .setStructValue(Struct.newBuilder().putFields("api_name", Values.of("updated")))
+                    .build()));
   }
 
   @lombok.Value
-  private static class TestObject {
+  private static class TestInternalObject {
     String name;
   }
 
-  private static class TestObjectStore extends DefaultObjectStore<TestObject> {
+  @lombok.Value
+  private static class TestApiObject {
+    String api_name;
+  }
+
+  private static class TestObjectStore extends DefaultObjectStore<TestInternalObject> {
     private TestObjectStore(
         ConfigServiceBlockingStub stub, ConfigChangeEventGenerator configChangeEventGenerator) {
       super(stub, TEST_RESOURCE_NAMESPACE, TEST_RESOURCE_NAME, configChangeEventGenerator);
     }
 
     @Override
-    protected Optional<TestObject> buildDataFromValue(Value value) {
-      return Optional.of(new TestObject(value.getStringValue()));
+    protected Optional<TestInternalObject> buildDataFromValue(Value value) {
+      return Optional.of(new TestInternalObject(value.getStringValue()));
     }
 
     @Override
-    protected Value buildValueFromData(TestObject object) {
+    protected Value buildValueFromData(TestInternalObject object) {
       return Values.of(object.getName());
     }
+
+    @Override
+    protected Value buildValueForChangeEvent(TestInternalObject object) {
+      return Value.newBuilder()
+          .setStructValue(Struct.newBuilder().putFields("api_name", Values.of(object.getName())))
+          .build();
+    }
+
+    @Override
+    protected String buildClassNameForChangeEvent(TestInternalObject object) {
+      return TestApiObject.class.getName();
+    }
   }
 }
diff --git a/object-store/src/test/java/org/hypertrace/config/objectstore/IdentifiedObjectStoreTest.java b/object-store/src/test/java/org/hypertrace/config/objectstore/IdentifiedObjectStoreTest.java
