PR review changes QUES-36

Moved test utilities + deps to commons-testing project Got rid of unhelpful marker interfaces Add some testing (unit + integration) for SpringBootConfigStorage
patientsknowbest · Nov 10, 2020 · 6a952a7 · 6a952a7
1 parent 7d3eedb
commit 6a952a7
Show file tree

Hide file tree

Showing 17 changed files with 283 additions and 133 deletions.
diff --git a/config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java
@@ -0,0 +1,14 @@
+package com.pkb.common.config;
+
+public abstract class AbstractMutableConfigStorage extends AbstractBaseConfigStorage {
+
+    @Override
+    public void setValue(String key, String value) {
+        if (!MUTABLE_CONFIG_KEY.equals(key)) {
+            setValueInternal(key, value);
+        }
+    }
+
+    abstract void setValueInternal(String key, String value);
+
+}
diff --git a/config/src/main/java/com/pkb/common/config/ConfigStorage.java b/config/src/main/java/com/pkb/common/config/ConfigStorage.java
@@ -20,13 +20,14 @@ public interface ConfigStorage {
 
     Long getLong(String key, long defaultValue);
 
-    boolean isMutableConfigEnabled();
+    default boolean isMutableConfigEnabled() {
+        return getBoolean(MUTABLE_CONFIG_KEY, false);
+    }
 
     void setValue(String key, String value);
 
     OverrideRemovalResult removeOverrideAtKey(String key);
 
     void reset();
 
-    ImmutableConfigStorage getImmutableConfig();
 }
diff --git a/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java b/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java
@@ -10,7 +10,7 @@ public static ConfigStorage getConfigStorage() {
     }
 
 
-    public static ConfigStorage getConfigStorage(ImmutableConfigStorage baseStorage) {
+    public static ConfigStorage getConfigStorage(ConfigStorage baseStorage) {
         if (baseStorage.isMutableConfigEnabled()) {
             return new MutableRawConfigStorage(baseStorage);
         }

diff --git a/config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java
diff --git a/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java b/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java
@@ -1,12 +1,12 @@
 package com.pkb.common.config;
 
-import java.util.HashMap;
-import java.util.Map;
-
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.unmodifiableMap;
 
-public final class ImmutableRawConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage {
+import java.util.HashMap;
+import java.util.Map;
+
+public final class ImmutableRawConfigStorage extends AbstractBaseConfigStorage {
 
     public static final ImmutableRawConfigStorage EMPTY = new ImmutableRawConfigStorage(emptyMap());
 
@@ -36,4 +36,19 @@ public String getString(String key, String defaultValue) {
         return storage.getOrDefault(key, defaultValue);
     }
 
+    @Override
+    public void setValue(String key, String value) {
+        //No-op
+    }
+
+    @Override
+    public OverrideRemovalResult removeOverrideAtKey(String key) {
+        return OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE;
+    }
+
+    @Override
+    public void reset() {
+        //No-op
+    }
+
 }
diff --git a/config/src/main/java/com/pkb/common/config/MutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/MutableConfigStorage.java
diff --git a/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java b/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java
@@ -4,12 +4,12 @@
 import java.util.Map;
 import java.util.function.Supplier;
 
-public final class MutableRawConfigStorage extends AbstractBaseConfigStorage implements MutableConfigStorage {
+public final class MutableRawConfigStorage extends AbstractMutableConfigStorage {
 
     private final Map<String, String> overrideMap = new HashMap<>();
-    private final ImmutableConfigStorage configStorage;
+    private final ConfigStorage configStorage;
 
-    MutableRawConfigStorage(ImmutableConfigStorage configStorage) {
+    MutableRawConfigStorage(ConfigStorage configStorage) {
         this.configStorage = configStorage;
     }
 
@@ -24,10 +24,8 @@ public String getString(String key, String defaultValue) {
     }
 
     @Override
-    public void setValue(String key, String value) {
-        if (!MUTABLE_CONFIG_KEY.equals(key)) {
-            overrideMap.put(key, value);
-        }
+    void setValueInternal(String key, String value) {
+        overrideMap.put(key, value);
     }
 
     @Override
@@ -45,15 +43,15 @@ public void reset() {
         overrideMap.clear();
     }
 
-    @Override
-    public ImmutableConfigStorage getImmutableConfig() {
-        return configStorage;
-    }
-
     private String getOverriddenOrOriginalValue(String key, Supplier<String> originalSupplier) {
         if (overrideMap.containsKey(key)) {
             return overrideMap.get(key);
         }
         return originalSupplier.get();
     }
+
+    @Override
+    public boolean isMutableConfigEnabled() {
+        return true;
+    }
 }
diff --git a/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java b/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java
@@ -4,7 +4,6 @@
 import static com.github.karsaig.approvalcrest.jupiter.MatcherAssert.assertThrows;
 import static com.github.karsaig.approvalcrest.jupiter.matcher.Matchers.sameJsonAsApproved;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.HashMap;
@@ -197,9 +196,4 @@ public void removingNonExistentKey() {
         assertEquals(OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE, removalResult);
     }
 
-    @Test
-    void getImmutableConfigReturnsSelf() {
-        ConfigStorage configStorage = underTest.getImmutableConfig();
-        assertThat(configStorage, sameInstance(underTest));
-    }
 }
diff --git a/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java b/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java
@@ -4,7 +4,6 @@
 import static com.github.karsaig.approvalcrest.jupiter.MatcherAssert.assertThrows;
 import static com.github.karsaig.approvalcrest.jupiter.matcher.Matchers.sameJsonAsApproved;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.util.HashMap;
@@ -195,11 +194,6 @@ void mutableConfigCannotBeOverridden() {
         assertThat(underTest.isMutableConfigEnabled(), is(true));
     }
 
-    @Test
-    void getImmutableConfigReturnsOriginalImmutableConfig() {
-        ConfigStorage configStorage = underTest.getImmutableConfig();
-        assertThat(configStorage, sameInstance(immutableRawConfigStorage));
-    }
 
     @DisplayName("getInt returns original value without override")
     @Test

diff --git a/pom.xml b/pom.xml
@@ -56,6 +56,7 @@
         <commons.testing.version>20-789c720-63866</commons.testing.version>
         <spring-cloud.version>Hoxton.SR8</spring-cloud.version>
         <hibernate-core.version>5.3.15.Final</hibernate-core.version>
+        <spring.boot.version>2.3.5.RELEASE</spring.boot.version>
     </properties>
 
     <modules>
@@ -254,17 +255,22 @@
                 <artifactId>spring-context</artifactId>
                 <version>${spring.version}</version>
             </dependency>
-
             <dependency>
                 <groupId>org.springframework</groupId>
-                <artifactId>spring-test</artifactId>
+                <artifactId>spring-beans</artifactId>
                 <version>${spring.version}</version>
-                <scope>compile</scope>
+            </dependency>
+            <dependency>
+                <groupId>org.springframework.boot</groupId>
+                <artifactId>spring-boot-test</artifactId>
+                <version>${spring.boot.version}</version>
+                <scope>test</scope>
             </dependency>
             <dependency>
                 <groupId>org.springframework</groupId>
-                <artifactId>spring-beans</artifactId>
+                <artifactId>spring-test</artifactId>
                 <version>${spring.version}</version>
+                <scope>test</scope>
             </dependency>
         </dependencies>
     </dependencyManagement>

diff --git a/spring-boot-infrastructure/pom.xml b/spring-boot-infrastructure/pom.xml
@@ -14,27 +14,22 @@
     <dependencies>
         <dependency>
             <groupId>org.springframework</groupId>
-            <artifactId>spring-test</artifactId>
-            <scope>compile</scope>
+            <artifactId>spring-beans</artifactId>
         </dependency>
         <dependency>
-            <groupId>org.springframework</groupId>
-            <artifactId>spring-beans</artifactId>
+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-starter</artifactId>
+            <version>${spring.boot.version}</version>
+            <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.springframework.cloud</groupId>
-            <artifactId>spring-cloud-context</artifactId>
-            <exclusions>
-                <exclusion>
-                    <groupId>org.springframework.security</groupId>
-                    <artifactId>spring-security-crypto</artifactId>
-                </exclusion>
-            </exclusions>
+            <artifactId>spring-cloud-starter</artifactId>
+            <scope>test</scope>
         </dependency>
         <dependency>
-            <groupId>org.junit.jupiter</groupId>
-            <artifactId>junit-jupiter-api</artifactId>
-            <scope>compile</scope>
+            <groupId>org.springframework.cloud</groupId>
+            <artifactId>spring-cloud-context</artifactId>
         </dependency>
         <dependency>
             <groupId>com.pkb.pkbcommon</groupId>
@@ -44,6 +39,30 @@
             <groupId>org.springframework</groupId>
             <artifactId>spring-context</artifactId>
         </dependency>
+        <dependency>
+            <groupId>com.github.karsaig</groupId>
+            <artifactId>approvalcrest-junit-jupiter</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-test</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.springframework</groupId>
+            <artifactId>spring-test</artifactId>
+        </dependency>
     </dependencies>
 
 </project>
diff --git a/...ture/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java b/...ture/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java
@@ -1,21 +1,22 @@
 package com.pkb.common.config;
 
+import java.util.Optional;
+
+import javax.annotation.ParametersAreNonnullByDefault;
+
 import org.springframework.aop.scope.ScopedProxyUtils;
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanDefinitionHolder;
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
-import org.springframework.beans.factory.config.Scope;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor;
 import org.springframework.beans.factory.support.GenericBeanDefinition;
+import org.springframework.context.annotation.Scope;
 import org.springframework.context.annotation.ScopedProxyMode;
 import org.springframework.core.annotation.AnnotationAttributes;
 import org.springframework.core.type.AnnotationMetadata;
 
-import javax.annotation.ParametersAreNonnullByDefault;
-import java.util.Optional;
-
 /**
  * This post-processor works around a bug/limitation of Spring Boot/Spring Cloud whereby
  * beans discovered by the @EnableConfigurationProperties or @ConfigurationPropertiesScan annotations
@@ -31,13 +32,17 @@
 @ParametersAreNonnullByDefault
 public class ConfigurationPropertiesScopingPostProcessor implements BeanDefinitionRegistryPostProcessor {
 
+    private static final String SCOPE_ANNOTATION = Scope.class.getName();
+
     @Override
     public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException {
         for (String name : registry.getBeanDefinitionNames()) {
             BeanDefinition beanDefinition = registry.getBeanDefinition(name);
             if (beanDefinition.getClass().getName().equals("org.springframework.boot.context.properties.ConfigurationPropertiesValueObjectBeanDefinition")) {
                 GenericBeanDefinition gbd = (GenericBeanDefinition) beanDefinition;
-                Optional<AnnotationAttributes> annAttrs = Optional.ofNullable(AnnotationAttributes.fromMap(AnnotationMetadata.introspect(gbd.getBeanClass()).getAnnotationAttributes(Scope.class.getName(), false)));
+                Optional<AnnotationAttributes> annAttrs = Optional
+                        .ofNullable(AnnotationAttributes.fromMap(AnnotationMetadata.introspect(gbd.getBeanClass()).getAnnotationAttributes(
+                                SCOPE_ANNOTATION, false)));
                 ScopedProxyMode scopedProxyMode = annAttrs.map(attrs -> attrs.<ScopedProxyMode>getEnum("proxyMode")).orElse(ScopedProxyMode.NO);
                 gbd.setScope(annAttrs.map(attrs -> attrs.getString("value")).orElse(gbd.getScope()));
                 if (scopedProxyMode == ScopedProxyMode.NO) {