Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Add mutable spring-backed config QUES-36 #43

Merged
merged 8 commits into from
Nov 10, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
40 changes: 40 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
<phr-pulsar.version>12-a115018-86851</phr-pulsar.version>
<restassured.version>3.0.7-PKB</restassured.version>

<spring.version>5.2.3.RELEASE</spring.version> <!-- copied from PHR -->
<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>
</properties>

Expand All @@ -64,10 +67,23 @@
<module>spring-infrastructure</module>
<module>testlogging</module>
<module>testsupport</module>
<module>spring-boot-infrastructure</module>
</modules>

<dependencyManagement>
<dependencies>

<dependency>
<groupId>com.pkb.pkbcommon</groupId>
<artifactId>infrastructure</artifactId>
<version>${revision}${changelist}</version>
</dependency>
<dependency>
<groupId>com.pkb</groupId>
<artifactId>commons-testing-base</artifactId>
<version>${commons.testing.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
Expand Down Expand Up @@ -210,6 +226,13 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-dependencies</artifactId>
<version>${spring-cloud.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-core</artifactId>
Expand All @@ -226,6 +249,23 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<version>${spring.version}</version>
</dependency>

<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
<version>${spring.version}</version>
<scope>compile</scope>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think spring Test shouldn't end up in prod code, compile is the default

</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-beans</artifactId>
<version>${spring.version}</version>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down
49 changes: 49 additions & 0 deletions spring-boot-infrastructure/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<parent>
<artifactId>pkb-common</artifactId>
<groupId>com.pkb.pkbcommon</groupId>
<version>${revision}${changelist}</version>
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>spring-boot-infrastructure</artifactId>

<dependencies>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
<scope>compile</scope>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-beans</artifactId>
</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>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>compile</scope>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test related deps should be test scoped, or in another module which is only used as test scope. Test related deps are in commons-testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this tag should be in commons-testing then? Obviously it can't have test scope because it's a library for use in other tests. I can move it to a module in commons-testing, that's fine

</dependency>
<dependency>
<groupId>com.pkb.pkbcommon</groupId>
<artifactId>config</artifactId>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package com.pkb.common.config;

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.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
* aren't ever put into custom scopes like the RefreshScope. This post-processor mimics what the
* standard Spring bean component scanning does by reading the scope attributes and then creating
* a scoped proxy if the requested scope requires it. It also ensure that the bean definition
* has the appropriate scope marked on it.
*
* To use this, define a static method on Configuration class that returns an instance of this as a Bean.
* You should probably give it an @Order attribute as well to ensure that this executes as early as possible
* so that other standard scope processing steps can operate on the corrected bean definition.
*/
@ParametersAreNonnullByDefault
public class ConfigurationPropertiesScopingPostProcessor implements BeanDefinitionRegistryPostProcessor {

@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)));
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) {
continue;
}
boolean proxyTargetClass = scopedProxyMode == ScopedProxyMode.TARGET_CLASS;
registry.removeBeanDefinition(name);
BeanDefinitionHolder scopedProxy = ScopedProxyUtils.createScopedProxy(new BeanDefinitionHolder(gbd, name), registry, proxyTargetClass);
registry.registerBeanDefinition(scopedProxy.getBeanName(), scopedProxy.getBeanDefinition());
}
}
}

@Override
public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException {
//No-op
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
package com.pkb.common.config;

import org.springframework.cloud.context.scope.refresh.RefreshScope;
import org.springframework.core.env.ConfigurableEnvironment;
import org.springframework.core.env.MapPropertySource;

import javax.annotation.ParametersAreNonnullByDefault;
import java.util.LinkedHashMap;
import java.util.Map;

/**
* This class provides an implementation of the PKB ConfigStorage interface that interacts
* helpfully with the Spring environment. This class can be passed to both the TestSupportAgent
* and any PKB config objects that require a ConfigStorage. It will read properties from the
* Spring environment, allowing Spring Boot applications to be be configured using all the standard
* Spring properties mechanisms such as application.properties and the OS environment - so you don't
* need to provide an additional config.properties file just to control shared PKB components that
* use PKB Common configuration classes
*
* Additionally, this class maintains an override map that is loaded into the Spring environment
* as a high-priority property source. If mutations to properties are requested (e.g. from the
* TestSupportAgent) these overrides will be visible in the spring environment.
*
* In a typical Spring Boot application, properties are not accessed directly from the environment but
* are bound to type-safe classes annotated with @ConfigurationProperties. Since this binding happens when
* the Spring context is loading, in order for later changes to the Spring environment to be visible
* to these classes and their clients, it is necessary to do a selective refresh of the spring context.
* ConfigurationProperties beans that contain mutable properties that may need to be updated at runtime
* should be annotated with @RefreshScope. Additionally, you should include the {@link ConfigurationPropertiesScopingPostProcessor}
* bean in your context to ensure that all ConfigurationProperties classes get included in the RefreshScope,
* irrespective of how they are constructed. Once you do this, this class will ensure that configuration
* beans are dynamically re-bound at runtime when properties change.
*
* It is recommended to use @ConstructorBinding for ConfigurationProperties classes; this can be used with
* kotlin data classes, java value objects or at least a POJO with final fields and no setters. This ensures
* that properties present an immutable interface to clients and can only be changed by the framework.
*
*
* NB This class and the above-described mechanism only ensure that the relevant properties classes will
* return the updated values on the next method call to one of the property getters. As yet there is no
* mechanism for notifying clients that an update has occured (although in principle this would not be difficult)
* nor does it make any attempt to ensure that any dependencies of these properties beans - including spring
* infrastructure that may have been wired together according to their values - are dynamically updated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for the explanation

*/
@ParametersAreNonnullByDefault
public class SpringBootConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mutable store, then why does it implement ImmutableConfigStorage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't really sure what to do here. This can be both a mutable and immutable store. Honestly, I'm not sure that the interface separation between the two is actually doing much for us. In practice every client of the existing code uses the factory, which creates an implementation based on the value of the mutable config property. In particular, I'm not sure what the getImmutableConfigStorage method does for us - AFAICT nothing in any project ever calls this. I was thinking about just removing the interfaces and having a single ConfigStorage interface with the two RawConfigStorage implementations + this one... but then I thought about updating PKB Common everywhere and I chickened out. I guess i can look at this again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few integration test would be good, to prevent any unintended regression early on.

private final ConfigurableEnvironment environment;
private final RefreshScope refreshScope;
private final Map<String, Object> overrides = new LinkedHashMap<>();

public SpringBootConfigStorage(ConfigurableEnvironment environment, RefreshScope refreshScope) {
this.environment = environment;
this.refreshScope = refreshScope;
environment.getPropertySources().addFirst(new MapPropertySource("TestSupportOverrides", overrides));
}


@Override
public String getString(String key) {
return environment.getProperty(key);
}

@Override
public String getString(String key, String defaultValue) {
return environment.getProperty(key, defaultValue);
}

@Override
public boolean isMutableConfigEnabled() {
return getBoolean("mutableConfig.enabled", false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copy pasting the constant pls use ConfigStorage#MUTABLE_CONFIG_KEY

}

@Override
public void setValue(String key, String value) {
if (isMutableConfigEnabled()) {
overrides.put(key, value);
refreshScope.refreshAll();
}
}

@Override
public OverrideRemovalResult removeOverrideAtKey(String key) {
if (!isMutableConfigEnabled()) {
return OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE;
}
if (overrides.remove(key) == null) {
return OverrideRemovalResult.KEY_NOT_FOUND;
}
refreshScope.refreshAll();
return OverrideRemovalResult.REMOVED;
}

@Override
public void reset() {
if (isMutableConfigEnabled()) {
overrides.clear();
refreshScope.refreshAll();
}
}

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.pkb.common.testing;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.junit.jupiter.api.Tag;
import org.springframework.test.context.ActiveProfiles;

/**
* This tag helps organise integration tests for spring boot applications.
* When applied to a junit 5 test, it enables it to be filtered in maven
* using "groups" in the surefire/failsafe configuration.
*
* In addition, it applies the spring profile "integration-test" allowing
* selective configuration overrides.
*/
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Inherited
@ActiveProfiles("integration-test")
@Documented
@Tag("integration-test")
public @interface IntegrationTest {
}
13 changes: 0 additions & 13 deletions spring-infrastructure/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@
<artifactId>spring-infrastructure</artifactId>
<name>pkb-common/spring-infrastructure</name>

<properties>
<spring.version>5.2.3.RELEASE</spring.version> <!-- copied from PHR -->
<commons.testing.version>20-789c720-63866</commons.testing.version>
</properties>

<dependencies>
<dependency>
<groupId>com.google.guava</groupId>
Expand All @@ -25,12 +20,10 @@
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-context</artifactId>
<version>${spring.version}</version>
</dependency>
<dependency>
<groupId>com.pkb.pkbcommon</groupId>
<artifactId>infrastructure</artifactId>
<version>${revision}${changelist}</version>
</dependency>
<dependency>
<groupId>com.github.karsaig</groupId>
Expand All @@ -39,17 +32,11 @@
<dependency>
<groupId>com.pkb</groupId>
<artifactId>commons-testing-base</artifactId>
<version>${commons.testing.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
</dependency>
<dependency>
<groupId>com.pkb</groupId>
<artifactId>rest-assured</artifactId>
Expand Down

This file was deleted.

7 changes: 7 additions & 0 deletions suppression.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@
</suppress>
<suppress until="2020-12-01Z">
<notes><![CDATA[
file name: pulsar-client-2.5.2.jar (shaded: org.eclipse.jetty:jetty-util:9.4.20.v20190813)
]]></notes>
<packageUrl regex="true">^pkg:maven/org\.eclipse\.jetty/jetty\-util@.*$</packageUrl>
<cve>CVE-2020-27216</cve>
</suppress>
<suppress until="2020-12-01Z">
<notes><![CDATA[
file name: protobuf-shaded-2.1.0-incubating.jar (shaded: com.google.protobuf:protobuf-java:2.4.1)
]]></notes>
<sha1>d25481cf27aab12be6e7ba11eca5eea3f0bc4be2</sha1>
Expand Down