Add mutable spring-backed config QUES-36 #43

symposion · 2020-10-26T14:46:43Z

Add mutable configuration for spring environment to allow easier integration between Spring Boot apps and PKB e2e properties framework

r-oylo

Looks good!

r-oylo · 2020-11-05T09:52:39Z

spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java

+
+import java.lang.annotation.*;
+
+@Target({ElementType.TYPE, ElementType.METHOD})


Why do we need it to be on both type and method?

Actually, I think I copied this from someone on the web somewhere. I guess in principle you could have individual tests that are marked as integration tests, but that seems like a bad fit with our use cases. I think I might remove the METHOD option

…t apps QUES-36 Also move Spring boot tools into their own module to avoid pulling in unnecessary dependencies to non-spring-boot apps.

MFAshby

LGTM, thanks for the extensive documentation comments.

MFAshby · 2020-11-09T10:32:15Z

spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java

+ * 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.


👍 thanks for the explanation

r-oylo

Well done, thanks!

karsaig · 2020-11-09T19:37:42Z

pom.xml

+                <groupId>org.springframework</groupId>
+                <artifactId>spring-test</artifactId>
+                <version>${spring.version}</version>
+                <scope>compile</scope>


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

karsaig

Forgot to push submit...

karsaig · 2020-11-09T19:39:24Z

spring-boot-infrastructure/pom.xml

+        <dependency>
+            <groupId>org.springframework</groupId>
+            <artifactId>spring-test</artifactId>
+            <scope>compile</scope>


karsaig · 2020-11-09T19:40:34Z

spring-boot-infrastructure/pom.xml

+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <scope>compile</scope>


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

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

karsaig · 2020-11-09T20:00:16Z

spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java

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


Instead of copy pasting the constant pls use ConfigStorage#MUTABLE_CONFIG_KEY

karsaig · 2020-11-09T23:39:38Z

spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java

+ * infrastructure that may have been wired together according to their values - are dynamically updated.
+ */
+@ParametersAreNonnullByDefault
+public class SpringBootConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage {


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

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.

karsaig · 2020-11-09T23:40:51Z

spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java

+ * infrastructure that may have been wired together according to their values - are dynamically updated.
+ */
+@ParametersAreNonnullByDefault
+public class SpringBootConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage {


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

Moved test utilities + deps to commons-testing project Got rid of unhelpful marker interfaces Add some testing (unit + integration) for SpringBootConfigStorage

config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java

...frastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java

…QUES-36

r-oylo

Thanks for making the changes!

Add mutable spring-backed config QUES-36

symposion force-pushed the feature/ques-36 branch from dace286 to 415e92f Compare November 3, 2020 18:24

symposion requested a review from karsaig November 5, 2020 09:41

r-oylo previously approved these changes Nov 5, 2020

View reviewed changes

symposion added dont-merge-yet Don't merge this yet and removed dont-merge-yet Don't merge this yet labels Nov 6, 2020

symposion dismissed r-oylo’s stale review via bda34f5 November 6, 2020 15:11

symposion changed the title ~~Add integration test annotation for Spring Boot apps QUES-36~~ Add integration test annotation +mutable spring-backed config QUES-36 Nov 6, 2020

Add integration test annotation for Spring Boot apps QUES-36

10f46d2

symposion force-pushed the feature/ques-36 branch 2 times, most recently from c05e5b3 to 15861e4 Compare November 6, 2020 15:23

symposion added 2 commits November 6, 2020 16:33

Add SpringBootConfigStorage to allow mutable properties in Spring Boo…

379748d

…t apps QUES-36 Also move Spring boot tools into their own module to avoid pulling in unnecessary dependencies to non-spring-boot apps.

Fix Owasp QUES-38

7d3eedb

symposion force-pushed the feature/ques-36 branch from 15861e4 to 7d3eedb Compare November 6, 2020 15:34

MFAshby previously approved these changes Nov 9, 2020

View reviewed changes

r-oylo previously approved these changes Nov 9, 2020

View reviewed changes

karsaig suggested changes Nov 9, 2020

View reviewed changes

PR review changes QUES-36

6a952a7

Moved test utilities + deps to commons-testing project Got rid of unhelpful marker interfaces Add some testing (unit + integration) for SpringBootConfigStorage

symposion dismissed stale reviews from r-oylo and MFAshby via 6a952a7 November 10, 2020 11:17

Refix OWASP QUES-36

0168455

symposion changed the title ~~Add integration test annotation +mutable spring-backed config QUES-36~~ Add mutable spring-backed config QUES-36 Nov 10, 2020

Set Spring deps to provided to avoid version conflicts QUES-36

451246e

karsaig suggested changes Nov 10, 2020

View reviewed changes

config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java Show resolved Hide resolved

karsaig reviewed Nov 10, 2020

View reviewed changes

...frastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java Show resolved Hide resolved

symposion added 2 commits November 10, 2020 17:05

Fix incorrect method visibility of internal method. QUES-36

cbaa928

Add another test for immutability of mutableConfig.enabled property. …

cf475a7

…QUES-36

r-oylo approved these changes Nov 10, 2020

View reviewed changes

karsaig approved these changes Nov 10, 2020

View reviewed changes

karsaig merged commit a65ddd9 into master Nov 10, 2020

karsaig deleted the feature/ques-36 branch November 10, 2020 18:38

MFAshby pushed a commit that referenced this pull request Jul 5, 2022

Merge pull request #43 from patientsknowbest/feature/ques-36

7f9e33b

Add mutable spring-backed config QUES-36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mutable spring-backed config QUES-36 #43

Add mutable spring-backed config QUES-36 #43

symposion commented Oct 26, 2020 •

edited

Loading

r-oylo left a comment

r-oylo Nov 5, 2020

symposion Nov 6, 2020

MFAshby left a comment

MFAshby Nov 9, 2020

r-oylo left a comment

karsaig Nov 9, 2020

karsaig left a comment

karsaig Nov 9, 2020

karsaig Nov 9, 2020

symposion Nov 10, 2020

karsaig Nov 9, 2020

karsaig Nov 9, 2020

symposion Nov 10, 2020

karsaig Nov 9, 2020

r-oylo left a comment


		import java.lang.annotation.*;

		@Target({ElementType.TYPE, ElementType.METHOD})

Add mutable spring-backed config QUES-36 #43

Add mutable spring-backed config QUES-36 #43

Conversation

symposion commented Oct 26, 2020 • edited Loading

r-oylo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MFAshby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-oylo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karsaig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-oylo left a comment

Choose a reason for hiding this comment

symposion commented Oct 26, 2020 •

edited

Loading