Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store user details in config objects in mongo #212

Merged
merged 7 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ 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;
aaron-steinfeld marked this conversation as resolved.
Show resolved Hide resolved
}

message UpsertConfigResponse {
Expand All @@ -52,6 +55,9 @@ message UpsertConfigResponse {

// prev version of config value in the store
optional google.protobuf.Value prev_config = 4;

// user details
UserDetails user_details = 5;
aaron-steinfeld marked this conversation as resolved.
Show resolved Hide resolved
}

message GetConfigRequest {
Expand Down Expand Up @@ -141,7 +147,7 @@ message DeleteConfigsResponse {

message UpsertAllConfigsRequest {
repeated ConfigToUpsert configs = 1;

UserDetails user_details = 2;
message ConfigToUpsert {
string resource_name = 1;
string resource_namespace = 2;
Expand All @@ -152,12 +158,17 @@ message UpsertAllConfigsRequest {

message UpsertAllConfigsResponse {
repeated UpsertedConfig upserted_configs = 1;
UserDetails user_details = 2;

message UpsertedConfig {
message UpsertedConfig {
aaron-steinfeld marked this conversation as resolved.
Show resolved Hide resolved
string context = 1;
google.protobuf.Value config = 2;
int64 creation_timestamp = 3;
int64 update_timestamp = 4;
optional google.protobuf.Value prev_config = 5;
}
}

message UserDetails {
string user_email = 1;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -201,9 +203,12 @@ public void upsertAllConfigs(
requestedUpsert.getConfig()))
.collect(ImmutableMap.toImmutableMap(Entry::getKey, Entry::getValue));
List<UpsertedConfig> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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)
Expand All @@ -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) {
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -63,10 +68,14 @@ Map<ConfigResourceContext, ContextSpecificConfig> getContextConfigs(
*
* @param resourceContextValueMap
* @param userId
* @param userDetails
* @return the upserted configs
*/
List<UpsertedConfig> writeAllConfigs(
Map<ConfigResourceContext, Value> resourceContextValueMap, String userId) throws IOException;
Map<ConfigResourceContext, Value> resourceContextValueMap,
String userId,
UserDetails userDetails)
throws IOException;

/**
* delete the config values associated with the specified config resources.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -55,15 +56,19 @@ 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<ConfigDocument> previousConfigDoc = getLatestVersionConfigDoc(configResourceContext);
Optional<ContextSpecificConfig> optionalPreviousConfig =
previousConfigDoc.flatMap(this::convertToContextSpecificConfig);

Key latestDocKey = new ConfigDocumentKey(configResourceContext);
ConfigDocument latestConfigDocument =
buildConfigDocument(configResourceContext, latestConfig, userId, previousConfigDoc);
buildConfigDocument(
configResourceContext, latestConfig, userId, previousConfigDoc, userDetails);

collection.upsert(latestDocKey, latestConfigDocument);
return optionalPreviousConfig
Expand All @@ -72,7 +77,10 @@ public UpsertedConfig writeConfig(
}

private List<UpsertedConfig> writeConfigs(
Map<ConfigResourceContext, Value> resourceContextValueMap, String userId) throws IOException {
Map<ConfigResourceContext, Value> resourceContextValueMap,
String userId,
UserDetails userDetails)
throws IOException {
Map<ConfigResourceContext, Optional<ConfigDocument>> previousConfigDocs =
getLatestVersionConfigDocs(resourceContextValueMap.keySet());
Map<Key, Document> documentsToBeUpserted = new LinkedHashMap<>();
Expand All @@ -83,7 +91,8 @@ private List<UpsertedConfig> 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);
Expand All @@ -109,7 +118,8 @@ private ConfigDocument buildConfigDocument(
ConfigResourceContext configResourceContext,
Value latestConfig,
String userId,
Optional<ConfigDocument> previousConfigDoc) {
Optional<ConfigDocument> previousConfigDoc,
UserDetails userDetails) {
long updateTimestamp = clock.millis();
long creationTimestamp =
previousConfigDoc
Expand All @@ -128,15 +138,19 @@ private ConfigDocument buildConfigDocument(
configResourceContext.getContext(),
newVersion,
userId,
userDetails.getUserEmail(),
latestConfig,
creationTimestamp,
updateTimestamp);
}

@Override
public List<UpsertedConfig> writeAllConfigs(
Map<ConfigResourceContext, Value> resourceContextValueMap, String userId) throws IOException {
return this.writeConfigs(resourceContextValueMap, userId);
Map<ConfigResourceContext, Value> resourceContextValueMap,
String userId,
UserDetails userDetails)
throws IOException {
return this.writeConfigs(resourceContextValueMap, userId, userDetails);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "[email protected]";
private static Value config1 = getConfig1();
private static Value config2 = getConfig2();
private static Value mergedConfig = getExpectedMergedConfig();
Expand All @@ -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);
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ void convertToAndFromJson() throws IOException {
"context",
15,
"user1",
"[email protected]",
getConfig1(),
timestamp,
timestamp);
Expand All @@ -45,6 +46,7 @@ void convertDocumentContainingNullValue() throws IOException {
"context",
15,
"user1",
"[email protected]",
nullValue,
timestamp,
timestamp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 = "[email protected]";
private static final long TIMESTAMP1 = 100L;
private static final long TIMESTAMP2 = 200L;
private static final long TIMESTAMP3 = 300L;
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -191,7 +197,9 @@ void writeAllConfigs() throws IOException {
List<UpsertedConfig> 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(
Expand Down Expand Up @@ -239,6 +247,7 @@ private static Document getConfigDocument(
context,
version,
USER_ID,
USER_EMAIL,
config,
creationTimestamp,
updateTimestamp);
Expand Down
Loading