From 2e18bf8dd86f485c1a8f3da422e407f6f8bbc62a Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Wed, 29 May 2024 16:44:03 +0530 Subject: [PATCH 1/7] add user info details --- .../config/service/v1/config_service.proto | 14 ++++++++-- .../config/service/ConfigServiceGrpcImpl.java | 11 ++++++-- .../config/service/store/ConfigDocument.java | 6 ++++ .../config/service/store/ConfigStore.java | 15 ++++++++-- .../service/store/DocumentConfigStore.java | 28 ++++++++++++++----- .../service/ConfigServiceGrpcImplTest.java | 15 ++++++++-- .../service/store/ConfigDocumentTest.java | 2 ++ .../store/DocumentConfigStoreTest.java | 13 +++++++-- 8 files changed, 84 insertions(+), 20 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 d7ec9181..dbf93908 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 @@ -38,6 +38,8 @@ message UpsertConfigRequest { // optional - only required if config applies to a specific context. // If empty, specified config is associated with a default context. string context = 4; + + UserDetails user_details = 5; } message UpsertConfigResponse { @@ -52,6 +54,8 @@ message UpsertConfigResponse { // prev version of config value in the store optional google.protobuf.Value prev_config = 4; + + UserDetails user_details = 5; } message GetConfigRequest { @@ -141,7 +145,7 @@ message DeleteConfigsResponse { message UpsertAllConfigsRequest { repeated ConfigToUpsert configs = 1; - + UserDetails user_details = 2; message ConfigToUpsert { string resource_name = 1; string resource_namespace = 2; @@ -152,8 +156,8 @@ message UpsertAllConfigsRequest { message UpsertAllConfigsResponse { repeated UpsertedConfig upserted_configs = 1; - - message UpsertedConfig { + UserDetails user_details = 2; + message UpsertedConfig { string context = 1; google.protobuf.Value config = 2; int64 creation_timestamp = 3; @@ -161,3 +165,7 @@ message UpsertAllConfigsResponse { optional google.protobuf.Value prev_config = 5; } } + +message UserDetails { + string userEmail = 1; +} 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 bc6a2db6..32149629 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 @@ -51,9 +51,11 @@ public void upsertConfig( try { ConfigResourceContext configResourceContext = getConfigResourceContext(request); UpsertedConfig upsertedConfig = - configStore.writeConfig(configResourceContext, getUserId(), request.getConfig()); + configStore.writeConfig( + configResourceContext, getUserId(), request.getConfig(), request.getUserDetails()); UpsertConfigResponse.Builder builder = UpsertConfigResponse.newBuilder(); builder.setConfig(request.getConfig()); + builder.setUserDetails(request.getUserDetails()); builder.setCreationTimestamp(upsertedConfig.getCreationTimestamp()); builder.setUpdateTimestamp(upsertedConfig.getUpdateTimestamp()); if (upsertedConfig.hasPrevConfig()) { @@ -201,9 +203,12 @@ public void upsertAllConfigs( requestedUpsert.getConfig())) .collect(ImmutableMap.toImmutableMap(Entry::getKey, Entry::getValue)); List upsertedConfigs = - configStore.writeAllConfigs(valuesByContext, getUserId()); + configStore.writeAllConfigs(valuesByContext, getUserId(), request.getUserDetails()); responseObserver.onNext( - UpsertAllConfigsResponse.newBuilder().addAllUpsertedConfigs(upsertedConfigs).build()); + UpsertAllConfigsResponse.newBuilder() + .addAllUpsertedConfigs(upsertedConfigs) + .setUserDetails(request.getUserDetails()) + .build()); responseObserver.onCompleted(); } catch (Exception e) { log.error("Upsert all failed for request:{}", request, e); diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java index 27d9d0ca..7d6ceee5 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java @@ -37,6 +37,7 @@ public class ConfigDocument implements Document { public static final String CONTEXT_FIELD_NAME = "context"; public static final String VERSION_FIELD_NAME = "configVersion"; public static final String USER_ID_FIELD_NAME = "userId"; + public static final String USER_EMAIL_FIELD_NAME = "userEmail"; public static final String CONFIG_FIELD_NAME = "config"; public static final String CREATION_TIMESTAMP_FIELD_NAME = "creationTimestamp"; public static final String UPDATE_TIMESTAMP_FIELD_NAME = "updateTimestamp"; @@ -59,6 +60,9 @@ public class ConfigDocument implements Document { @JsonProperty(value = USER_ID_FIELD_NAME) String userId; + @JsonProperty(value = USER_EMAIL_FIELD_NAME) + String userEmail; + @JsonSerialize(using = ValueSerializer.class) @JsonDeserialize(using = ValueDeserializer.class) @JsonProperty(value = CONFIG_FIELD_NAME) @@ -78,6 +82,7 @@ public ConfigDocument( @JsonProperty(CONTEXT_FIELD_NAME) String context, @JsonProperty(VERSION_FIELD_NAME) long configVersion, @JsonProperty(USER_ID_FIELD_NAME) String userId, + @JsonProperty(USER_EMAIL_FIELD_NAME) String userEmail, @JsonProperty(CONFIG_FIELD_NAME) Value config, @JsonProperty(CREATION_TIMESTAMP_FIELD_NAME) long creationTimestamp, @JsonProperty(UPDATE_TIMESTAMP_FIELD_NAME) long updateTimestamp) { @@ -87,6 +92,7 @@ public ConfigDocument( this.context = context; this.configVersion = configVersion; this.userId = userId; + this.userEmail = userEmail; this.config = config; this.creationTimestamp = creationTimestamp; this.updateTimestamp = updateTimestamp; 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 3061d989..ef9a4dd2 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.UpsertAllConfigsResponse.UpsertedConfig; +import org.hypertrace.config.service.v1.UserDetails; /** * Abstraction for the backend which stores and serves the configuration data for multiple @@ -20,12 +21,16 @@ public interface ConfigStore { * Write the config value associated with the specified config resource to the store. * * @param configResourceContext - * @param userId * @param config + * @param userDetails * @return the config written to the store */ UpsertedConfig writeConfig( - ConfigResourceContext configResourceContext, String userId, Value config) throws IOException; + ConfigResourceContext configResourceContext, + String userId, + Value config, + UserDetails userDetails) + throws IOException; /** * Get the config with the latest version for the specified resource. @@ -63,10 +68,14 @@ Map getContextConfigs( * * @param resourceContextValueMap * @param userId + * @param userDetails * @return the upserted configs */ List writeAllConfigs( - Map resourceContextValueMap, String userId) throws IOException; + Map resourceContextValueMap, + String userId, + UserDetails userDetails) + throws IOException; /** * delete the config values associated with the specified config resources. 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 e7d0e4da..00c48ea0 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 @@ -29,6 +29,7 @@ import org.hypertrace.config.service.ConfigServiceUtils; import org.hypertrace.config.service.v1.ContextSpecificConfig; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; +import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.documentstore.CloseableIterator; import org.hypertrace.core.documentstore.Collection; import org.hypertrace.core.documentstore.Datastore; @@ -55,7 +56,10 @@ public DocumentConfigStore(Clock clock, Datastore datastore) { @Override public UpsertedConfig writeConfig( - ConfigResourceContext configResourceContext, String userId, Value latestConfig) + ConfigResourceContext configResourceContext, + String userId, + Value latestConfig, + UserDetails userDetails) throws IOException { Optional previousConfigDoc = getLatestVersionConfigDoc(configResourceContext); Optional optionalPreviousConfig = @@ -63,7 +67,8 @@ public UpsertedConfig writeConfig( Key latestDocKey = new ConfigDocumentKey(configResourceContext); ConfigDocument latestConfigDocument = - buildConfigDocument(configResourceContext, latestConfig, userId, previousConfigDoc); + buildConfigDocument( + configResourceContext, latestConfig, userId, previousConfigDoc, userDetails); collection.upsert(latestDocKey, latestConfigDocument); return optionalPreviousConfig @@ -72,7 +77,10 @@ public UpsertedConfig writeConfig( } private List writeConfigs( - Map resourceContextValueMap, String userId) throws IOException { + Map resourceContextValueMap, + String userId, + UserDetails userDetails) + throws IOException { Map> previousConfigDocs = getLatestVersionConfigDocs(resourceContextValueMap.keySet()); Map documentsToBeUpserted = new LinkedHashMap<>(); @@ -83,7 +91,8 @@ private List writeConfigs( } documentsToBeUpserted.put( new ConfigDocumentKey(key), - buildConfigDocument(key, resourceContextValueMap.get(key), userId, value)); + buildConfigDocument( + key, resourceContextValueMap.get(key), userId, value, userDetails)); }); boolean successfulBulkUpsertDocuments = collection.bulkUpsert(documentsToBeUpserted); @@ -109,7 +118,8 @@ private ConfigDocument buildConfigDocument( ConfigResourceContext configResourceContext, Value latestConfig, String userId, - Optional previousConfigDoc) { + Optional previousConfigDoc, + UserDetails userDetails) { long updateTimestamp = clock.millis(); long creationTimestamp = previousConfigDoc @@ -128,6 +138,7 @@ private ConfigDocument buildConfigDocument( configResourceContext.getContext(), newVersion, userId, + userDetails.getUserEmail(), latestConfig, creationTimestamp, updateTimestamp); @@ -135,8 +146,11 @@ private ConfigDocument buildConfigDocument( @Override public List writeAllConfigs( - Map resourceContextValueMap, String userId) throws IOException { - return this.writeConfigs(resourceContextValueMap, userId); + Map resourceContextValueMap, + String userId, + UserDetails userDetails) + throws IOException { + return this.writeConfigs(resourceContextValueMap, userId, userDetails); } @Override 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 f4b332a2..87fbb796 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 @@ -45,12 +45,14 @@ import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; import org.hypertrace.config.service.v1.UpsertConfigResponse; +import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.grpcutils.context.RequestContext; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; class ConfigServiceGrpcImplTest { + private static final String USER_EMAIL = "user@email.com"; private static Value config1 = getConfig1(); private static Value config2 = getConfig2(); private static Value mergedConfig = getExpectedMergedConfig(); @@ -61,7 +63,11 @@ class ConfigServiceGrpcImplTest { @Test void upsertConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); - when(configStore.writeConfig(any(ConfigResourceContext.class), anyString(), any(Value.class))) + when(configStore.writeConfig( + any(ConfigResourceContext.class), + anyString(), + any(Value.class), + any(UserDetails.class))) .thenAnswer( invocation -> { Value config = invocation.getArgument(2, Value.class); @@ -293,7 +299,11 @@ void deletingNonExistingConfigShouldThrowError() throws IOException { assertEquals(Status.NOT_FOUND, ((StatusException) throwable).getStatus()); verify(configStore, never()) - .writeConfig(any(ConfigResourceContext.class), anyString(), any(Value.class)); + .writeConfig( + any(ConfigResourceContext.class), + anyString(), + any(Value.class), + any(UserDetails.class)); verify(responseObserver, never()).onNext(any(DeleteConfigResponse.class)); verify(responseObserver, never()).onCompleted(); } @@ -304,6 +314,7 @@ private UpsertConfigRequest getUpsertConfigRequest(String context, Value config) .setResourceNamespace(RESOURCE_NAMESPACE) .setContext(context) .setConfig(config) + .setUserDetails(UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()) .build(); } diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java index 88e39095..275e426f 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java @@ -27,6 +27,7 @@ void convertToAndFromJson() throws IOException { "context", 15, "user1", + "user1@email.com", getConfig1(), timestamp, timestamp); @@ -45,6 +46,7 @@ void convertDocumentContainingNullValue() throws IOException { "context", 15, "user1", + "user1@email.com", nullValue, timestamp, timestamp); 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 29c7051f..1ca6c1ec 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 @@ -29,6 +29,7 @@ import org.hypertrace.config.service.ConfigResourceContext; import org.hypertrace.config.service.v1.ContextSpecificConfig; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; +import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.documentstore.CloseableIterator; import org.hypertrace.core.documentstore.Collection; import org.hypertrace.core.documentstore.Datastore; @@ -44,6 +45,7 @@ class DocumentConfigStoreTest { private static final long CONFIG_VERSION = 5; private static final String USER_ID = "user1"; + private static final String USER_EMAIL = "user@email.com"; private static final long TIMESTAMP1 = 100L; private static final long TIMESTAMP2 = 200L; private static final long TIMESTAMP3 = 300L; @@ -75,7 +77,11 @@ void writeConfig() throws IOException { when(collection.search(any(Query.class))).thenReturn(iterator); UpsertedConfig upsertedConfig = - configStore.writeConfig(configResourceContext, USER_ID, config1); + configStore.writeConfig( + configResourceContext, + USER_ID, + config1, + UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()); assertEquals(config1, upsertedConfig.getConfig()); assertEquals(TIMESTAMP1, upsertedConfig.getCreationTimestamp()); @@ -191,7 +197,9 @@ void writeAllConfigs() throws IOException { List upsertedConfigs = // Swap configs between contexts as an update configStore.writeAllConfigs( - ImmutableMap.of(resourceContext1, config2, resourceContext2, config1), USER_ID); + ImmutableMap.of(resourceContext1, config2, resourceContext2, config1), + USER_ID, + UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()); assertEquals( List.of( @@ -239,6 +247,7 @@ private static Document getConfigDocument( context, version, USER_ID, + USER_EMAIL, config, creationTimestamp, updateTimestamp); From 7e9b6bc302ae9383e2df0ca1a05d22d43d797cc8 Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Wed, 29 May 2024 16:47:35 +0530 Subject: [PATCH 2/7] resolve lint error --- .../org/hypertrace/config/service/v1/config_service.proto | 5 ++++- 1 file changed, 4 insertions(+), 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 dbf93908..5640c34d 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 @@ -39,6 +39,7 @@ message UpsertConfigRequest { // If empty, specified config is associated with a default context. string context = 4; + // user details UserDetails user_details = 5; } @@ -55,6 +56,7 @@ message UpsertConfigResponse { // prev version of config value in the store optional google.protobuf.Value prev_config = 4; + // user details UserDetails user_details = 5; } @@ -157,6 +159,7 @@ message UpsertAllConfigsRequest { message UpsertAllConfigsResponse { repeated UpsertedConfig upserted_configs = 1; UserDetails user_details = 2; + message UpsertedConfig { string context = 1; google.protobuf.Value config = 2; @@ -167,5 +170,5 @@ message UpsertAllConfigsResponse { } message UserDetails { - string userEmail = 1; + string user_email = 1; } From 3df5c4276ab2456f5be186033e54d093a5a94750 Mon Sep 17 00:00:00 2001 From: Vaibhav <120372639+Vaibhav090420@users.noreply.github.com> Date: Wed, 29 May 2024 19:27:29 +0530 Subject: [PATCH 3/7] Update config_service.proto --- .../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 5640c34d..106b2c98 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 @@ -160,7 +160,7 @@ message UpsertAllConfigsResponse { repeated UpsertedConfig upserted_configs = 1; UserDetails user_details = 2; - message UpsertedConfig { + message UpsertedConfig { string context = 1; google.protobuf.Value config = 2; int64 creation_timestamp = 3; From a12456119ccb4456a61b49bf92a9babab2178206 Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Thu, 30 May 2024 14:46:40 +0530 Subject: [PATCH 4/7] resolve comments --- .../config/service/v1/config_service.proto | 13 +-------- .../config/service/ConfigServiceGrpcImpl.java | 27 ++++++++++++------- .../config/service/store/ConfigStore.java | 14 +++------- .../service/store/DocumentConfigStore.java | 22 ++++++--------- .../service/ConfigServiceGrpcImplTest.java | 24 +++++++---------- .../store/DocumentConfigStoreTest.java | 9 ++----- 6 files changed, 43 insertions(+), 66 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 106b2c98..d7ec9181 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 @@ -38,9 +38,6 @@ message UpsertConfigRequest { // optional - only required if config applies to a specific context. // If empty, specified config is associated with a default context. string context = 4; - - // user details - UserDetails user_details = 5; } message UpsertConfigResponse { @@ -55,9 +52,6 @@ message UpsertConfigResponse { // prev version of config value in the store optional google.protobuf.Value prev_config = 4; - - // user details - UserDetails user_details = 5; } message GetConfigRequest { @@ -147,7 +141,7 @@ message DeleteConfigsResponse { message UpsertAllConfigsRequest { repeated ConfigToUpsert configs = 1; - UserDetails user_details = 2; + message ConfigToUpsert { string resource_name = 1; string resource_namespace = 2; @@ -158,7 +152,6 @@ message UpsertAllConfigsRequest { message UpsertAllConfigsResponse { repeated UpsertedConfig upserted_configs = 1; - UserDetails user_details = 2; message UpsertedConfig { string context = 1; @@ -168,7 +161,3 @@ message UpsertAllConfigsResponse { optional google.protobuf.Value prev_config = 5; } } - -message UserDetails { - string user_email = 1; -} 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 32149629..e566f262 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 @@ -52,10 +52,9 @@ public void upsertConfig( ConfigResourceContext configResourceContext = getConfigResourceContext(request); UpsertedConfig upsertedConfig = configStore.writeConfig( - configResourceContext, getUserId(), request.getConfig(), request.getUserDetails()); + configResourceContext, getUserId(), request.getConfig(), getUserEmail()); UpsertConfigResponse.Builder builder = UpsertConfigResponse.newBuilder(); builder.setConfig(request.getConfig()); - builder.setUserDetails(request.getUserDetails()); builder.setCreationTimestamp(upsertedConfig.getCreationTimestamp()); builder.setUpdateTimestamp(upsertedConfig.getUpdateTimestamp()); if (upsertedConfig.hasPrevConfig()) { @@ -203,12 +202,9 @@ public void upsertAllConfigs( requestedUpsert.getConfig())) .collect(ImmutableMap.toImmutableMap(Entry::getKey, Entry::getValue)); List upsertedConfigs = - configStore.writeAllConfigs(valuesByContext, getUserId(), request.getUserDetails()); + configStore.writeAllConfigs(valuesByContext, getUserId(), getUserEmail()); responseObserver.onNext( - UpsertAllConfigsResponse.newBuilder() - .addAllUpsertedConfigs(upsertedConfigs) - .setUserDetails(request.getUserDetails()) - .build()); + UpsertAllConfigsResponse.newBuilder().addAllUpsertedConfigs(upsertedConfigs).build()); responseObserver.onCompleted(); } catch (Exception e) { log.error("Upsert all failed for request:{}", request, e); @@ -275,8 +271,21 @@ private String getTenantId() { ::asRuntimeException); } - // TODO : get the userId from the context + private String getUserEmail() { + return RequestContext.CURRENT + .get() + .getEmail() + .orElseThrow( + Status.INVALID_ARGUMENT.withDescription("Email ID is missing in the request") + ::asRuntimeException); + } + private String getUserId() { - return ""; + return RequestContext.CURRENT + .get() + .getUserId() + .orElseThrow( + Status.INVALID_ARGUMENT.withDescription("User ID is missing in the request") + ::asRuntimeException); } } 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 ef9a4dd2..c5811fbe 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.UpsertAllConfigsResponse.UpsertedConfig; -import org.hypertrace.config.service.v1.UserDetails; /** * Abstraction for the backend which stores and serves the configuration data for multiple @@ -22,14 +21,11 @@ public interface ConfigStore { * * @param configResourceContext * @param config - * @param userDetails + * @param userEmail * @return the config written to the store */ UpsertedConfig writeConfig( - ConfigResourceContext configResourceContext, - String userId, - Value config, - UserDetails userDetails) + ConfigResourceContext configResourceContext, String userId, Value config, String userEmail) throws IOException; /** @@ -68,13 +64,11 @@ Map getContextConfigs( * * @param resourceContextValueMap * @param userId - * @param userDetails + * @param userEmail * @return the upserted configs */ List writeAllConfigs( - Map resourceContextValueMap, - String userId, - UserDetails userDetails) + Map resourceContextValueMap, String userId, String userEmail) 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 00c48ea0..b4126c1a 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 @@ -29,7 +29,6 @@ import org.hypertrace.config.service.ConfigServiceUtils; import org.hypertrace.config.service.v1.ContextSpecificConfig; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; -import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.documentstore.CloseableIterator; import org.hypertrace.core.documentstore.Collection; import org.hypertrace.core.documentstore.Datastore; @@ -59,7 +58,7 @@ public UpsertedConfig writeConfig( ConfigResourceContext configResourceContext, String userId, Value latestConfig, - UserDetails userDetails) + String userEmail) throws IOException { Optional previousConfigDoc = getLatestVersionConfigDoc(configResourceContext); Optional optionalPreviousConfig = @@ -68,7 +67,7 @@ public UpsertedConfig writeConfig( Key latestDocKey = new ConfigDocumentKey(configResourceContext); ConfigDocument latestConfigDocument = buildConfigDocument( - configResourceContext, latestConfig, userId, previousConfigDoc, userDetails); + configResourceContext, latestConfig, userId, previousConfigDoc, userEmail); collection.upsert(latestDocKey, latestConfigDocument); return optionalPreviousConfig @@ -77,9 +76,7 @@ public UpsertedConfig writeConfig( } private List writeConfigs( - Map resourceContextValueMap, - String userId, - UserDetails userDetails) + Map resourceContextValueMap, String userId, String userEmail) throws IOException { Map> previousConfigDocs = getLatestVersionConfigDocs(resourceContextValueMap.keySet()); @@ -91,8 +88,7 @@ private List writeConfigs( } documentsToBeUpserted.put( new ConfigDocumentKey(key), - buildConfigDocument( - key, resourceContextValueMap.get(key), userId, value, userDetails)); + buildConfigDocument(key, resourceContextValueMap.get(key), userId, value, userEmail)); }); boolean successfulBulkUpsertDocuments = collection.bulkUpsert(documentsToBeUpserted); @@ -119,7 +115,7 @@ private ConfigDocument buildConfigDocument( Value latestConfig, String userId, Optional previousConfigDoc, - UserDetails userDetails) { + String userEmail) { long updateTimestamp = clock.millis(); long creationTimestamp = previousConfigDoc @@ -138,7 +134,7 @@ private ConfigDocument buildConfigDocument( configResourceContext.getContext(), newVersion, userId, - userDetails.getUserEmail(), + userEmail, latestConfig, creationTimestamp, updateTimestamp); @@ -146,11 +142,9 @@ private ConfigDocument buildConfigDocument( @Override public List writeAllConfigs( - Map resourceContextValueMap, - String userId, - UserDetails userDetails) + Map resourceContextValueMap, String userId, String userEmail) throws IOException { - return this.writeConfigs(resourceContextValueMap, userId, userDetails); + return this.writeConfigs(resourceContextValueMap, userId, userEmail); } @Override 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 87fbb796..cb8aea37 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 @@ -16,6 +16,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -45,13 +46,13 @@ import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; import org.hypertrace.config.service.v1.UpsertConfigRequest; import org.hypertrace.config.service.v1.UpsertConfigResponse; -import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.grpcutils.context.RequestContext; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; class ConfigServiceGrpcImplTest { + private static final String USER_ID = "userId"; private static final String USER_EMAIL = "user@email.com"; private static Value config1 = getConfig1(); private static Value config2 = getConfig2(); @@ -64,10 +65,7 @@ class ConfigServiceGrpcImplTest { void upsertConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); when(configStore.writeConfig( - any(ConfigResourceContext.class), - anyString(), - any(Value.class), - any(UserDetails.class))) + any(ConfigResourceContext.class), anyString(), any(Value.class), anyString())) .thenAnswer( invocation -> { Value config = invocation.getArgument(2, Value.class); @@ -85,14 +83,17 @@ void upsertConfig() throws IOException { () -> configServiceGrpc.upsertConfig(getUpsertConfigRequest("", config1), responseObserver); Runnable runnableWithoutContext2 = () -> configServiceGrpc.upsertConfig(getUpsertConfigRequest("", config2), responseObserver); - RequestContext.forTenantId(TENANT_ID).run(runnableWithoutContext1); - RequestContext.forTenantId(TENANT_ID).run(runnableWithoutContext2); + RequestContext requestContext = spy(RequestContext.forTenantId(TENANT_ID)); + when(requestContext.getUserId()).thenReturn(Optional.of(USER_ID)); + when(requestContext.getEmail()).thenReturn(Optional.of(USER_EMAIL)); + requestContext.run(runnableWithoutContext1); + requestContext.run(runnableWithoutContext2); Runnable runnableWithContext = () -> configServiceGrpc.upsertConfig( getUpsertConfigRequest(CONTEXT1, config2), responseObserver); - RequestContext.forTenantId(TENANT_ID).run(runnableWithContext); + requestContext.run(runnableWithContext); ArgumentCaptor upsertConfigResponseCaptor = ArgumentCaptor.forClass(UpsertConfigResponse.class); @@ -299,11 +300,7 @@ void deletingNonExistingConfigShouldThrowError() throws IOException { assertEquals(Status.NOT_FOUND, ((StatusException) throwable).getStatus()); verify(configStore, never()) - .writeConfig( - any(ConfigResourceContext.class), - anyString(), - any(Value.class), - any(UserDetails.class)); + .writeConfig(any(ConfigResourceContext.class), anyString(), any(Value.class), anyString()); verify(responseObserver, never()).onNext(any(DeleteConfigResponse.class)); verify(responseObserver, never()).onCompleted(); } @@ -314,7 +311,6 @@ private UpsertConfigRequest getUpsertConfigRequest(String context, Value config) .setResourceNamespace(RESOURCE_NAMESPACE) .setContext(context) .setConfig(config) - .setUserDetails(UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()) .build(); } 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 1ca6c1ec..d4f26dbf 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 @@ -29,7 +29,6 @@ import org.hypertrace.config.service.ConfigResourceContext; import org.hypertrace.config.service.v1.ContextSpecificConfig; import org.hypertrace.config.service.v1.UpsertAllConfigsResponse.UpsertedConfig; -import org.hypertrace.config.service.v1.UserDetails; import org.hypertrace.core.documentstore.CloseableIterator; import org.hypertrace.core.documentstore.Collection; import org.hypertrace.core.documentstore.Datastore; @@ -77,11 +76,7 @@ void writeConfig() throws IOException { when(collection.search(any(Query.class))).thenReturn(iterator); UpsertedConfig upsertedConfig = - configStore.writeConfig( - configResourceContext, - USER_ID, - config1, - UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()); + configStore.writeConfig(configResourceContext, USER_ID, config1, USER_EMAIL); assertEquals(config1, upsertedConfig.getConfig()); assertEquals(TIMESTAMP1, upsertedConfig.getCreationTimestamp()); @@ -199,7 +194,7 @@ void writeAllConfigs() throws IOException { configStore.writeAllConfigs( ImmutableMap.of(resourceContext1, config2, resourceContext2, config1), USER_ID, - UserDetails.newBuilder().setUserEmail(USER_EMAIL).build()); + USER_EMAIL); assertEquals( List.of( From a43272ab56d2883d46d8384e8dd4d595f9a46e6a Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 31 May 2024 16:43:46 +0530 Subject: [PATCH 5/7] resolve comments --- .../config/service/ConfigServiceGrpcImpl.java | 16 ++++----------- .../config/service/store/ConfigDocument.java | 20 +++++++++---------- .../service/store/DocumentConfigStore.java | 18 ++++++++++------- 3 files changed, 25 insertions(+), 29 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 e566f262..d93c39df 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 @@ -39,6 +39,8 @@ @Slf4j public class ConfigServiceGrpcImpl extends ConfigServiceGrpc.ConfigServiceImplBase { + private static String DEFAULT_USER_ID = "Unknown"; + private static String DEFAULT_USER_EMAIL = "Unknown"; private final ConfigStore configStore; public ConfigServiceGrpcImpl(ConfigStore configStore) { @@ -272,20 +274,10 @@ private String getTenantId() { } private String getUserEmail() { - return RequestContext.CURRENT - .get() - .getEmail() - .orElseThrow( - Status.INVALID_ARGUMENT.withDescription("Email ID is missing in the request") - ::asRuntimeException); + return RequestContext.CURRENT.get().getEmail().orElse(DEFAULT_USER_EMAIL); } private String getUserId() { - return RequestContext.CURRENT - .get() - .getUserId() - .orElseThrow( - Status.INVALID_ARGUMENT.withDescription("User ID is missing in the request") - ::asRuntimeException); + return RequestContext.CURRENT.get().getUserId().orElse(DEFAULT_USER_ID); } } diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java index 7d6ceee5..9d4b6f0d 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java @@ -36,8 +36,8 @@ public class ConfigDocument implements Document { public static final String TENANT_ID_FIELD_NAME = "tenantId"; public static final String CONTEXT_FIELD_NAME = "context"; public static final String VERSION_FIELD_NAME = "configVersion"; - public static final String USER_ID_FIELD_NAME = "userId"; - public static final String USER_EMAIL_FIELD_NAME = "userEmail"; + public static final String LAST_UPDATED_USER_ID_FIELD_NAME = "lastUpdateUserId"; + public static final String LAST_UPDATED_USER_EMAIL_FIELD_NAME = "lastUpdatedUserEmail"; public static final String CONFIG_FIELD_NAME = "config"; public static final String CREATION_TIMESTAMP_FIELD_NAME = "creationTimestamp"; public static final String UPDATE_TIMESTAMP_FIELD_NAME = "updateTimestamp"; @@ -57,11 +57,11 @@ public class ConfigDocument implements Document { @JsonProperty(value = VERSION_FIELD_NAME) long configVersion; - @JsonProperty(value = USER_ID_FIELD_NAME) - String userId; + @JsonProperty(value = LAST_UPDATED_USER_ID_FIELD_NAME) + String lastUpdatedUserId; - @JsonProperty(value = USER_EMAIL_FIELD_NAME) - String userEmail; + @JsonProperty(value = LAST_UPDATED_USER_EMAIL_FIELD_NAME) + String lastUpdatedUserEmail; @JsonSerialize(using = ValueSerializer.class) @JsonDeserialize(using = ValueDeserializer.class) @@ -81,8 +81,8 @@ public ConfigDocument( @JsonProperty(TENANT_ID_FIELD_NAME) String tenantId, @JsonProperty(CONTEXT_FIELD_NAME) String context, @JsonProperty(VERSION_FIELD_NAME) long configVersion, - @JsonProperty(USER_ID_FIELD_NAME) String userId, - @JsonProperty(USER_EMAIL_FIELD_NAME) String userEmail, + @JsonProperty(LAST_UPDATED_USER_ID_FIELD_NAME) String lastUpdatedUserId, + @JsonProperty(LAST_UPDATED_USER_EMAIL_FIELD_NAME) String lastUpdatedUserEmail, @JsonProperty(CONFIG_FIELD_NAME) Value config, @JsonProperty(CREATION_TIMESTAMP_FIELD_NAME) long creationTimestamp, @JsonProperty(UPDATE_TIMESTAMP_FIELD_NAME) long updateTimestamp) { @@ -91,8 +91,8 @@ public ConfigDocument( this.tenantId = tenantId; this.context = context; this.configVersion = configVersion; - this.userId = userId; - this.userEmail = userEmail; + this.lastUpdatedUserId = lastUpdatedUserId; + this.lastUpdatedUserEmail = lastUpdatedUserEmail; this.config = config; this.creationTimestamp = creationTimestamp; this.updateTimestamp = updateTimestamp; 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 b4126c1a..8d96e8c1 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 @@ -56,9 +56,9 @@ public DocumentConfigStore(Clock clock, Datastore datastore) { @Override public UpsertedConfig writeConfig( ConfigResourceContext configResourceContext, - String userId, + String lastUpdatedUserId, Value latestConfig, - String userEmail) + String lastUpdatedUserEmail) throws IOException { Optional previousConfigDoc = getLatestVersionConfigDoc(configResourceContext); Optional optionalPreviousConfig = @@ -67,7 +67,11 @@ public UpsertedConfig writeConfig( Key latestDocKey = new ConfigDocumentKey(configResourceContext); ConfigDocument latestConfigDocument = buildConfigDocument( - configResourceContext, latestConfig, userId, previousConfigDoc, userEmail); + configResourceContext, + latestConfig, + lastUpdatedUserId, + previousConfigDoc, + lastUpdatedUserEmail); collection.upsert(latestDocKey, latestConfigDocument); return optionalPreviousConfig @@ -113,9 +117,9 @@ private List writeConfigs( private ConfigDocument buildConfigDocument( ConfigResourceContext configResourceContext, Value latestConfig, - String userId, + String lastUpdatedUserId, Optional previousConfigDoc, - String userEmail) { + String lastUpdatedUserEmail) { long updateTimestamp = clock.millis(); long creationTimestamp = previousConfigDoc @@ -133,8 +137,8 @@ private ConfigDocument buildConfigDocument( configResourceContext.getConfigResource().getTenantId(), configResourceContext.getContext(), newVersion, - userId, - userEmail, + lastUpdatedUserId, + lastUpdatedUserEmail, latestConfig, creationTimestamp, updateTimestamp); From 91f3f3b6e81c974de2844597fdd07b0441d0b243 Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Fri, 31 May 2024 22:48:34 +0530 Subject: [PATCH 6/7] add compatibility test --- .../config/service/store/ConfigDocument.java | 11 +++++- .../service/store/ConfigDocumentTest.java | 38 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java index 9d4b6f0d..245c2ae1 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java @@ -17,6 +17,8 @@ import com.google.protobuf.Value; import com.google.protobuf.util.JsonFormat; import java.io.IOException; +import java.util.Optional; +import lombok.Builder; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.ConfigServiceUtils; import org.hypertrace.core.documentstore.Document; @@ -26,11 +28,14 @@ */ @lombok.Value @Slf4j +@Builder public class ConfigDocument implements Document { private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + public static final String DEFAULT_LATEST_UPDATED_USER_ID = "Unknown"; + public static final String DEFAULT_LATEST_UPDATED_USER_EMAIL = "Unknown"; public static final String RESOURCE_FIELD_NAME = "resourceName"; public static final String RESOURCE_NAMESPACE_FIELD_NAME = "resourceNamespace"; public static final String TENANT_ID_FIELD_NAME = "tenantId"; @@ -91,8 +96,10 @@ public ConfigDocument( this.tenantId = tenantId; this.context = context; this.configVersion = configVersion; - this.lastUpdatedUserId = lastUpdatedUserId; - this.lastUpdatedUserEmail = lastUpdatedUserEmail; + this.lastUpdatedUserId = + Optional.ofNullable(lastUpdatedUserId).orElse(DEFAULT_LATEST_UPDATED_USER_ID); + this.lastUpdatedUserEmail = + Optional.ofNullable(lastUpdatedUserEmail).orElse(DEFAULT_LATEST_UPDATED_USER_EMAIL); this.config = config; this.creationTimestamp = creationTimestamp; this.updateTimestamp = updateTimestamp; diff --git a/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java b/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java index 275e426f..d36cfde7 100644 --- a/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java +++ b/config-service-impl/src/test/java/org/hypertrace/config/service/store/ConfigDocumentTest.java @@ -4,6 +4,7 @@ import static org.hypertrace.config.service.TestUtils.RESOURCE_NAMESPACE; import static org.hypertrace.config.service.TestUtils.TENANT_ID; import static org.hypertrace.config.service.TestUtils.getConfig1; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import com.google.common.io.Resources; @@ -70,4 +71,41 @@ void convertJsonStringWithoutTimestamp() throws IOException { assertEquals(0, configDocument.getCreationTimestamp()); assertEquals(0, configDocument.getUpdateTimestamp()); } + + @Test + @DisplayName("Test compatibility of json string when new field is added to ConfigDocument") + void testConfigDocumentCompatibility() throws IOException { + ConfigDocument configDocument = + new ConfigDocument( + "exampleResourceName", + "exampleResourceNamespace", + "exampleTenantId", + "exampleContext", + 1, + null, + null, + Value.newBuilder().build(), + 1622547800000L, + 1625149800000L); + assertDoesNotThrow(() -> configDocument.toJson()); + assertEquals("Unknown", configDocument.getLastUpdatedUserId()); + assertEquals("Unknown", configDocument.getLastUpdatedUserEmail()); + + String jsonString = + "{" + + "\"resourceName\":\"exampleResourceName\"," + + "\"userId\":\"userId\"," + + "\"resourceNamespace\":\"exampleResourceNamespace\"," + + "\"tenantId\":\"exampleTenantId\"," + + "\"context\":\"exampleContext\"," + + "\"configVersion\":1," + + "\"config\":null," + + "\"creationTimestamp\":1622547800000," + + "\"updateTimestamp\":1625149800000}"; + + ConfigDocument configDocument1 = ConfigDocument.fromJson(jsonString); + + assertEquals("Unknown", configDocument1.getLastUpdatedUserId()); + assertEquals("Unknown", configDocument1.getLastUpdatedUserEmail()); + } } From 261866e408410912653e7e570a06d3699064f84e Mon Sep 17 00:00:00 2001 From: Vaibhav Date: Sat, 1 Jun 2024 02:17:53 +0530 Subject: [PATCH 7/7] remove unnecessary annotation --- .../org/hypertrace/config/service/store/ConfigDocument.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java index 245c2ae1..9375a844 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/store/ConfigDocument.java @@ -18,7 +18,6 @@ import com.google.protobuf.util.JsonFormat; import java.io.IOException; import java.util.Optional; -import lombok.Builder; import lombok.extern.slf4j.Slf4j; import org.hypertrace.config.service.ConfigServiceUtils; import org.hypertrace.core.documentstore.Document; @@ -28,7 +27,6 @@ */ @lombok.Value @Slf4j -@Builder public class ConfigDocument implements Document { private static final ObjectMapper OBJECT_MAPPER =