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 3666f488..ef0c23f0 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 @@ -1,5 +1,6 @@ package org.hypertrace.config.service; +import static org.hypertrace.config.service.ConfigServiceUtils.emptyConfig; import static org.hypertrace.config.service.ConfigServiceUtils.filterNull; import static org.hypertrace.config.service.ConfigServiceUtils.merge; @@ -10,6 +11,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -69,17 +71,24 @@ public void upsertConfig( public void getConfig( GetConfigRequest request, StreamObserver responseObserver) { try { - ContextSpecificConfig config = configStore.getConfig(getConfigResourceContext(request)); - + ConfigResourceContext configResourceContext = getConfigResourceContext(request); + Optional maybeConfig = configStore.getConfig(configResourceContext); + // initialize empty config if not present to handle merges better + if (maybeConfig.isEmpty()) { + maybeConfig = Optional.of(emptyConfig(configResourceContext.getContext())); + } // get the configs for the contexts mentioned in the request and merge them in the specified // order for (String context : request.getContextsList()) { - ContextSpecificConfig contextSpecificConfig = + Optional contextSpecificConfig = configStore.getConfig(getConfigResourceContext(request, context)); - config = merge(config, contextSpecificConfig); + if (contextSpecificConfig.isEmpty()) { + continue; + } + maybeConfig = Optional.of(merge(maybeConfig.get(), contextSpecificConfig.get())); } - filterNull(config) + filterNull(maybeConfig.get()) .map( nonNullConfig -> GetConfigResponse.newBuilder() @@ -124,10 +133,10 @@ public void deleteConfig( DeleteConfigRequest request, StreamObserver responseObserver) { try { ConfigResourceContext configResourceContext = getConfigResourceContext(request); - ContextSpecificConfig configToDelete = configStore.getConfig(configResourceContext); + Optional maybeConfig = configStore.getConfig(configResourceContext); // if configToDelete is null/empty (i.e. config value doesn't exist or is already deleted), // then throw NOT_FOUND exception - if (configToDelete == null || ConfigServiceUtils.isNull(configToDelete.getConfig())) { + if (maybeConfig.isEmpty()) { responseObserver.onError(Status.NOT_FOUND.asException()); return; } @@ -135,7 +144,7 @@ public void deleteConfig( // delete the config for the specified config resource. configStore.deleteConfigs(Set.of(configResourceContext)); responseObserver.onNext( - DeleteConfigResponse.newBuilder().setDeletedConfig(configToDelete).build()); + DeleteConfigResponse.newBuilder().setDeletedConfig(maybeConfig.get()).build()); responseObserver.onCompleted(); } catch (Exception e) { log.error("Delete config failed for request:{}", request, e); @@ -160,7 +169,7 @@ public void deleteConfigs( .map(this::getConfigResourceContext) .collect(Collectors.toUnmodifiableSet()); Map configs = - configStore.getAllContextConfigs(configResourceContexts); + configStore.getContextConfigs(configResourceContexts); // delete the configs for the specified config resources. configStore.deleteConfigs(configResourceContexts); responseObserver.onNext( diff --git a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java index 691b8e84..624c126b 100644 --- a/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java +++ b/config-service-impl/src/main/java/org/hypertrace/config/service/ConfigServiceUtils.java @@ -31,6 +31,7 @@ private ConfigServiceUtils() { public static ContextSpecificConfig merge( ContextSpecificConfig defaultContextSpecificConfig, ContextSpecificConfig overridingContextSpecificConfig) { + Value defaultConfig = defaultContextSpecificConfig.getConfig(); Value overridingConfig = overridingContextSpecificConfig.getConfig(); if (isNull(defaultConfig)) { 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 2bdd4438..f1d1ea84 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 @@ -3,9 +3,10 @@ import com.google.protobuf.Value; import com.typesafe.config.Config; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Set; +import java.util.Optional; import org.hypertrace.config.service.ConfigResource; import org.hypertrace.config.service.ConfigResourceContext; import org.hypertrace.config.service.v1.ContextSpecificConfig; @@ -41,7 +42,8 @@ UpsertedConfig writeConfig( * @param configResourceContext * @return */ - ContextSpecificConfig getConfig(ConfigResourceContext configResourceContext) throws IOException; + Optional getConfig(ConfigResourceContext configResourceContext) + throws IOException; /** * Get all the configs with the latest version(along with the context to which it applies) for the @@ -51,8 +53,8 @@ UpsertedConfig writeConfig( * @return the configs * @throws IOException */ - Map getAllContextConfigs( - Set configResourceContexts) throws IOException; + Map getContextConfigs( + Collection configResourceContexts) throws IOException; /** * Get all the configs with the latest version(along with the context to which it applies) for the @@ -80,7 +82,7 @@ List writeAllConfigs( * * @param configResourceContexts */ - void deleteConfigs(Set configResourceContexts) throws IOException; + void deleteConfigs(Collection configResourceContexts) throws IOException; /** * Health check for the backend store 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 198331cf..95640c20 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 @@ -1,7 +1,6 @@ package org.hypertrace.config.service.store; import static com.google.common.collect.Streams.zip; -import static org.hypertrace.config.service.ConfigServiceUtils.emptyConfig; import static org.hypertrace.config.service.store.ConfigDocument.CONTEXT_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_FIELD_NAME; import static org.hypertrace.config.service.store.ConfigDocument.RESOURCE_NAMESPACE_FIELD_NAME; @@ -160,7 +159,7 @@ public List writeAllConfigs( } @Override - public void deleteConfigs(Set configResourceContexts) { + public void deleteConfigs(java.util.Collection configResourceContexts) { if (configResourceContexts.isEmpty()) { return; } @@ -168,28 +167,24 @@ public void deleteConfigs(Set configResourceContexts) { } @Override - public ContextSpecificConfig getConfig(ConfigResourceContext configResourceContext) + public Optional getConfig(ConfigResourceContext configResourceContext) throws IOException { return getLatestVersionConfigDoc(configResourceContext) - .flatMap(this::convertToContextSpecificConfig) - .orElseGet(() -> emptyConfig(configResourceContext.getContext())); + .flatMap(this::convertToContextSpecificConfig); } @Override - public Map getAllContextConfigs( - Set configResourceContexts) throws IOException { + public Map getContextConfigs( + java.util.Collection configResourceContexts) throws IOException { Map> configDocs = getLatestVersionConfigDocs(configResourceContexts); - return configDocs.entrySet().stream() + .filter(entry -> entry.getValue().isPresent()) .collect( Collectors.toUnmodifiableMap( Entry::getKey, entry -> - entry - .getValue() - .flatMap(this::convertToContextSpecificConfig) - .orElseGet(() -> emptyConfig(entry.getKey().getContext())))); + entry.getValue().flatMap(this::convertToContextSpecificConfig).orElseThrow())); } @Override @@ -238,7 +233,7 @@ private Optional getLatestVersionConfigDoc( } private Map> getLatestVersionConfigDocs( - Set configResourceContexts) throws IOException { + java.util.Collection configResourceContexts) throws IOException { if (configResourceContexts.isEmpty()) { return Collections.emptyMap(); } @@ -269,7 +264,7 @@ private Map> getLatestVersionCon } private Filter buildConfigResourceContextsFilter( - Set configResourceContexts) { + java.util.Collection configResourceContexts) { List childFilters = configResourceContexts.stream() .map(this::getConfigResourceFieldContextFilter) @@ -280,7 +275,11 @@ private Filter buildConfigResourceContextsFilter( Filter tenantIdFilter = Filter.eq( TENANT_ID_FIELD_NAME, - configResourceContexts.iterator().next().getConfigResource().getTenantId()); + configResourceContexts.stream() + .findFirst() + .orElseThrow() + .getConfigResource() + .getTenantId()); return tenantIdFilter.and(configResourceFieldContextFilter); } 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 0f61067a..8f2b41fc 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 @@ -29,6 +29,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.hypertrace.config.service.store.ConfigStore; import org.hypertrace.config.service.v1.ContextSpecificConfig; @@ -106,19 +107,21 @@ void getConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); when(configStore.getConfig(eq(configResourceWithoutContext))) .thenReturn( - ContextSpecificConfig.newBuilder() - .setConfig(config1) - .setCreationTimestamp(10L) - .setUpdateTimestamp(12L) - .build()); + Optional.of( + ContextSpecificConfig.newBuilder() + .setConfig(config1) + .setCreationTimestamp(10L) + .setUpdateTimestamp(12L) + .build())); when(configStore.getConfig(eq(configResourceWithContext))) .thenReturn( - ContextSpecificConfig.newBuilder() - .setConfig(config2) - .setContext(CONTEXT1) - .setCreationTimestamp(15L) - .setUpdateTimestamp(25L) - .build()); + Optional.of( + ContextSpecificConfig.newBuilder() + .setConfig(config2) + .setContext(CONTEXT1) + .setCreationTimestamp(15L) + .setUpdateTimestamp(25L) + .build())); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -186,7 +189,8 @@ void deleteConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); ContextSpecificConfig deletedConfig = ContextSpecificConfig.newBuilder().setConfig(config2).setContext(CONTEXT1).build(); - when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(deletedConfig); + when(configStore.getConfig(eq(configResourceWithContext))) + .thenReturn(Optional.of(deletedConfig)); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -222,7 +226,7 @@ void deleteConfigs() throws IOException { getConfigResourceContext(context2), emptyValue()); - when(configStore.getAllContextConfigs(writeAllConfigsRequest.keySet())) + when(configStore.getContextConfigs(writeAllConfigsRequest.keySet())) .thenReturn( Map.of( getConfigResourceContext(context1), @@ -259,7 +263,8 @@ void deleteDefaultContextConfig() throws IOException { ConfigStore configStore = mock(ConfigStore.class); ContextSpecificConfig deletedConfig = ContextSpecificConfig.newBuilder().setConfig(config2).build(); - when(configStore.getConfig(eq(configResourceWithoutContext))).thenReturn(deletedConfig); + when(configStore.getConfig(eq(configResourceWithoutContext))) + .thenReturn(Optional.of(deletedConfig)); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); @@ -280,7 +285,7 @@ void deleteDefaultContextConfig() throws IOException { void deletingNonExistingConfigShouldThrowError() throws IOException { ConfigStore configStore = mock(ConfigStore.class); ContextSpecificConfig emptyConfig = ConfigServiceUtils.emptyConfig(CONTEXT1); - when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(emptyConfig); + when(configStore.getConfig(eq(configResourceWithContext))).thenReturn(Optional.of(emptyConfig)); ConfigServiceGrpcImpl configServiceGrpc = new ConfigServiceGrpcImpl(configStore); StreamObserver responseObserver = mock(StreamObserver.class); 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 c4af8e1a..71a1c3c7 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 @@ -27,6 +27,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import org.hypertrace.config.service.ConfigResource; import org.hypertrace.config.service.ConfigResourceContext; @@ -123,7 +124,7 @@ void getConfigs() throws IOException { .setUpdateTimestamp(TIMESTAMP2) .build(); Map actualConfigs = - configStore.getAllContextConfigs(Set.of(configResourceContext)); + configStore.getContextConfigs(Set.of(configResourceContext)); assertEquals(Map.of(configResourceContext, expectedConfig), actualConfigs); } @@ -142,8 +143,8 @@ void getConfig() throws IOException { .setCreationTimestamp(TIMESTAMP1) .setUpdateTimestamp(TIMESTAMP2) .build(); - ContextSpecificConfig actualConfig = configStore.getConfig(configResourceContext); - assertEquals(expectedConfig, actualConfig); + Optional actualConfig = configStore.getConfig(configResourceContext); + assertEquals(Optional.of(expectedConfig), actualConfig); } @Test