resolved embedded value in annotation @ApolloConfigChangeListener #3349

Anilople · 2020-10-23T14:53:44Z

What's the purpose of this PR

Make ApolloConfigChangeListener's value more dynamic.

Which issue(s) this PR fixes:

Brief changelog

The value in ApolloConfigChangeListener can be resolved now.

One can write like follow:

  @Configuration
  @EnableApolloConfig
  static class TestResolveExpressionFromSystemPropertyConfiguration {

    @ApolloConfigChangeListener(value = {ConfigConsts.NAMESPACE_APPLICATION,
        "${redis.namespace}"})
    private void onChange(ConfigChangeEvent event) {
    }
  }

"${redis.namespace}" will be resolved.

If there is property

redis.namespace=xxx

then "${redis.namespace}" will get xxx

If there is property

redis.namespace=yyy

then "${redis.namespace}" will get yyy

Also one can write code like follow:

  @Configuration
  @EnableApolloConfig
  static class TestResolveExpressionSimpleConfiguration {

    @ApolloConfigChangeListener("${simple.application:abc}")
    private void onChange(ConfigChangeEvent event) {
    }
  }

"${simple.application:abc}" will be resolved to abc if there is no property for key simple.application

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.

…value in ApolloConfigChangeListener

…ener

nobodyiam · 2020-10-30T00:45:13Z

Looks good! Will take a detailed look soon

nobodyiam · 2020-11-01T14:50:07Z

@Anilople I'm thinking is it necessary to support embedded value for @EnableApolloConfig and @ApolloConfig?

Anilople · 2020-11-01T15:17:41Z

@Anilople I'm thinking is it necessary to support embedded value for @EnableApolloConfig and @ApolloConfig?

Implement BeanFactoryAware for DefaultApolloConfigRegistrarHelper to process @EnableApolloConfig.

But I'm not sure it will work.

Anilople · 2020-11-01T15:21:00Z

@Anilople I'm thinking is it necessary to support embedded value for @EnableApolloConfig and @ApolloConfig?

Implement BeanFactoryAware for DefaultApolloConfigRegistrarHelper to process @EnableApolloConfig.

But I'm not sure it will work.

Meet java.lang.NullPointerException... Let me try another way.

...-client/src/main/java/com/ctrip/framework/apollo/spring/spi/ApolloConfigRegistrarHelper.java

codecov-io · 2020-11-02T13:41:39Z

Codecov Report

Merging #3349 (811f42a) into master (bd388e8) will increase coverage by 0.07%.
The diff coverage is 91.48%.

@@             Coverage Diff              @@
##             master    #3349      +/-   ##
============================================
+ Coverage     51.43%   51.51%   +0.07%     
- Complexity     2301     2307       +6     
============================================
  Files           442      441       -1     
  Lines         13741    13762      +21     
  Branches       1399     1400       +1     
============================================
+ Hits           7068     7089      +21     
  Misses         6185     6185              
  Partials        488      488

Impacted Files	Coverage Δ	Complexity Δ
...i/DefaultConfigPropertySourcesProcessorHelper.java	`100.00% <ø> (ø)`	`4.00 <0.00> (ø)`
...o/spring/annotation/ApolloAnnotationProcessor.java	`91.08% <89.18%> (-2.86%)`	`21.00 <13.00> (+12.00)`	⬇️
...pollo/spring/annotation/ApolloConfigRegistrar.java	`100.00% <100.00%> (ø)`	`3.00 <2.00> (+1.00)`
...amework/apollo/spring/config/NamespaceHandler.java	`86.95% <100.00%> (+1.95%)`	`3.00 <1.00> (ø)`
...spring/spi/DefaultApolloConfigRegistrarHelper.java	`100.00% <100.00%> (ø)`	`6.00 <3.00> (+3.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd388e8...811f42a. Read the comment docs.

…e resolved

.../src/main/java/com/ctrip/framework/apollo/spring/spi/DefaultApolloConfigRegistrarHelper.java

…loConfig' instead of ConfigurableBeanFactory

nobodyiam

Please see some comments below.
BTW is it possible to make it work for xml configuration? i.e. to resolve the namespace placeholder in com.ctrip.framework.apollo.spring.config.NamespaceHandler.BeanParser#doParse

...t/src/main/java/com/ctrip/framework/apollo/spring/annotation/ApolloConfigChangeListener.java

...va/com/ctrip/framework/apollo/spring/JavaConfigAnnotationApolloConfigChangeListenerTest.java

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/annotation/ApolloConfigTest.java

...lient/src/test/java/com/ctrip/framework/apollo/spring/annotation/EnableApolloConfigTest.java

Co-authored-by: Jason Song <[email protected]>

…ystem property

Sometime test failure in DefaultConfigTest#testGetIntPropertyMultipleTimesWithShortExpireTime, because system property is global

nobodyiam

This version looks much better, but I still have some comments as below, please take a look.
BTW, I noticed you created many inner classes and xmls for the tests, however I think many of them are quite similar, so maybe we could try to reuse them instead of creating new ones.

nobodyiam · 2020-11-22T07:23:02Z

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java

+    assertEquals(someValue, configuration.getSomeKey());
+    verify(yyyConfig, times(1)).getProperty(eq(someKey), anyString());
+
+    System.clearProperty(propertyKey);


Please clear the state in tearDown method

Use

@After public void javaConfigAnnotationTestTearDown() { // clear the system property clearSystemPropertiesDefineWithStaticStringField(SystemPropertyKeyConstants.class); }

to do it.

This is a little complex, I think we could simply assign some variables and just use System.clearProperty(xxx) in tear down method.

Change to code like follow:

/** * forbidden to override the method {@link super#tearDown()}. */ @After public void javaConfigAnnotationTestTearDown() { // clear the system properties System.clearProperty(SystemPropertyKeyConstants.SIMPLE_NAMESPACE); System.clearProperty(SystemPropertyKeyConstants.REDIS_NAMESPACE); System.clearProperty(SystemPropertyKeyConstants.FROM_SYSTEM_NAMESPACE); System.clearProperty(SystemPropertyKeyConstants.FROM_SYSTEM_YAML_NAMESPACE); System.clearProperty(SystemPropertyKeyConstants.FROM_NAMESPACE_APPLICATION_KEY); System.clearProperty(SystemPropertyKeyConstants.FROM_NAMESPACE_APPLICATION_KEY_YAML); }

Clear system properties manually instead of using reflection.

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java

nobodyiam · 2020-11-22T07:34:25Z

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java

+    // no using
+    verify(ignoreConfig, never()).addChangeListener(any(ConfigChangeListener.class));
+
+    // strange, it invoke 2 times


because there is an auto update feature enabled: com.ctrip.framework.apollo.spring.config.PropertySourcesProcessor#initializeAutoUpdatePropertiesFeature

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XmlConfigPlaceholderTest.java

nobodyiam · 2020-11-22T07:50:14Z

apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XmlConfigPlaceholderTest.java

-  private void check(String xmlLocation, int expectedTimeout, int expectedBatch) {
+
+  @Test
+  public void testResolveNamespacesWithDefaultValue() throws Exception {


this case failed in my machine, as there is no value for xxx.from.system.property

btw, this case looks like the same as testUnresolvedNamespaces

Fixed it in commit b5eb6f0

…figChangeListenerResolveExpressionFromApplicationNamespace.

…d config

…rty'. Merge similar xml file.

Anilople · 2020-11-22T09:22:52Z

In my machine(Win10), there is a test Failure in DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime:280.

Results :

Failed tests: 
  DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime:280 
properties.getProperty("someKey");
Wanted 2 times:
-> at com.ctrip.framework.apollo.internals.DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime(DefaultConfigTest.java:280)
But was 1 time:
-> at com.ctrip.framework.apollo.internals.DefaultConfig.getProperty(DefaultConfig.java:77)

Tests in error: 
  RemoteConfigRepositoryTest.testLongPollingRefresh:298 ? Timeout Waited 5000 mi...

Tests run: 293, Failures: 1, Errors: 1, Skipped: 0

I don't know why.

Maybe a bug in mockito? Or exists some concurrent problem between tests?

It seems ok on Linux platform.

…geListener

nobodyiam · 2020-11-22T09:53:42Z

In my machine(Win10), there is a test Failure in DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime:280.

Results :

Failed tests: 
  DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime:280 
properties.getProperty("someKey");
Wanted 2 times:
-> at com.ctrip.framework.apollo.internals.DefaultConfigTest.testGetIntPropertyMultipleTimesWithShortExpireTime(DefaultConfigTest.java:280)
But was 1 time:
-> at com.ctrip.framework.apollo.internals.DefaultConfig.getProperty(DefaultConfig.java:77)

Tests in error: 
  RemoteConfigRepositoryTest.testLongPollingRefresh:298 ? Timeout Waited 5000 mi...

Tests run: 293, Failures: 1, Errors: 1, Skipped: 0

I don't know why.

Maybe a bug in mockito? Or exists some concurrent problem between tests?

It seems ok on Linux platform.

This test was not well written, I'll fix it later.

nobodyiam

LGTM

Anilople added 3 commits October 23, 2020 22:40

feat(apollo-client): use ConfigurableBeanFactory to resolve embedded …

b801014

…value in ApolloConfigChangeListener

test(apollo-client): resolve embedded value in ApolloConfigChangeList…

6513539

…ener

docs(apollo-client): ApolloConfigChangeListener's embedded value usage

7823bda

Anilople added 2 commits November 1, 2020 23:04

feat(apollo-client): resolve embedded value in '@apolloconfig'

d5407cd

feat(apollo-client): resolve embedded value in '@EnableApolloConfig'

7bb0147

fix(apollo-client): implement BeanFactoryAware in ApolloConfigRegistrar

b096d8f

Anilople commented Nov 2, 2020

View reviewed changes

...-client/src/main/java/com/ctrip/framework/apollo/spring/spi/ApolloConfigRegistrarHelper.java Outdated Show resolved Hide resolved

Anilople added 4 commits November 3, 2020 21:44

refactor(apollo-client): process annotation ApolloConfig

d5670b5

refactor(apollo-client): move '@ApolloJsonValue' processor

ac672e6

refactor(apollo-client): move field

2e1616c

test(apollo-client): embedded value in '@EnableApolloConfig' cannot b…

66389c7

…e resolved

Anilople commented Nov 4, 2020

View reviewed changes

.../src/main/java/com/ctrip/framework/apollo/spring/spi/DefaultApolloConfigRegistrarHelper.java Outdated Show resolved Hide resolved

Anilople added 2 commits November 6, 2020 21:43

feat(apollo-client): use Environment to resolve value in '@EnableApol…

ee934e3

…loConfig' instead of ConfigurableBeanFactory

test(apollo-client): '@EnableApolloConfig' resolving testing

cc1bbf7

nobodyiam mentioned this pull request Nov 7, 2020

在bean的XML中无法注入环境变量 #3367

Closed

Anilople requested a review from nobodyiam November 7, 2020 03:08

nobodyiam requested changes Nov 8, 2020

View reviewed changes

Anilople and others added 8 commits November 8, 2020 15:06

docs(apollo-client): ApolloConfigChangeListener usage

bd3c5de

Co-authored-by: Jason Song <[email protected]>

docs(apollo-client): '@apolloconfig' usage.

4170a99

docs(apollo-client): '@EnableApolloConfig' usage.

4c0e7de

feat(apollo-client): use Environment to resolve placeholder

44d96bb

test(apollo-client): placeholder in '@apolloconfig'

a8b029a

test(apollo-client): resolve placeholder in '@EnableApolloConfig'

2f8ba8c

test(apollo-client): rename

cab3dfe

docs(apollo-client): '@EnableApolloConfig' usage

936784a

Anilople added 6 commits November 14, 2020 16:18

test(apollo-client): '@EnableApolloConfig' mock value to field for check

9c5f8d9

test(apollo-client): '@EnableApolloConfig' resolve placeholder from s…

cd0491b

…ystem property

test(apollo-client): reset system propery after each test case

138a50e

test(apollo-client): change the way to clear system propery.

280ca0f

Sometime test failure in DefaultConfigTest#testGetIntPropertyMultipleTimesWithShortExpireTime, because system property is global

test(apollo-client): fix concurrent problem

a550b34

test(apollo-client): change "someKey" to forbid confliction

1f3e6f4

Anilople requested a review from nobodyiam November 14, 2020 11:06

nobodyiam requested changes Nov 22, 2020

View reviewed changes

Anilople added 6 commits November 22, 2020 16:04

test(apollo-client): clear custom system property after each test case

2ff60d5

test(apollo-client): delete unnecessary test case.

363c727

test(apollo-client): delete redundant code in test case testApolloCon…

593ce02

…figChangeListenerResolveExpressionFromApplicationNamespace.

test(apollo-client): change assertEquals to assertSame to check mocke…

28e34e6

…d config

test(apollo-client): clear system property after test

27c043d

test(apollo-client): fix 'there is no value for xxx.from.system.prope…

b5eb6f0

…rty'. Merge similar xml file.

Merge branch 'master' into feature/embedded-value-in-ApolloConfigChan…

de5ca86

…geListener

Anilople requested a review from nobodyiam November 22, 2020 09:38

Anilople and others added 3 commits November 22, 2020 18:00

test(apollo-client): clear system property manually

f18b4f8

Update JavaConfigAnnotationTest.java

c4f0665

Update JavaConfigAnnotationTest.java

811f42a

nobodyiam approved these changes Nov 22, 2020

View reviewed changes

nobodyiam merged commit 6ce0932 into apolloconfig:master Nov 22, 2020

Anilople added feature Categorizes issue as related to a new feature. area/client apollo-client labels Dec 23, 2020

nobodyiam added this to the 1.8.0 milestone Feb 11, 2021

Anilople mentioned this pull request Dec 14, 2021

1.9.0&1.9.1版本client集成dubbo 2.6.11 property-placeholder 不能生效 #4152

Closed

nobodyiam mentioned this pull request Dec 25, 2021

NPE occurred when using EnableApolloConfig with Spring 3.1.1 #4178

Closed

3 tasks

nobodyiam mentioned this pull request Dec 23, 2022

Allow injecting dynamic config to ApolloConfigChangeListener apolloconfig/apollo-java#7

Closed

nobodyiam mentioned this pull request May 14, 2023

@ApolloConfigChangeListener interestedKeyPrefixes When can dynamic parameterization be supported #4866

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolved embedded value in annotation @ApolloConfigChangeListener #3349

resolved embedded value in annotation @ApolloConfigChangeListener #3349

Anilople commented Oct 23, 2020 •

edited

Loading

nobodyiam commented Oct 30, 2020

nobodyiam commented Nov 1, 2020

Anilople commented Nov 1, 2020

Anilople commented Nov 1, 2020

codecov-io commented Nov 2, 2020 •

edited

Loading

nobodyiam left a comment •

edited

Loading

nobodyiam left a comment

nobodyiam Nov 22, 2020

Anilople Nov 22, 2020

nobodyiam Nov 22, 2020

Anilople Nov 22, 2020

nobodyiam Nov 22, 2020

nobodyiam Nov 22, 2020

nobodyiam Nov 22, 2020

Anilople Nov 22, 2020 •

edited

Loading

Anilople commented Nov 22, 2020

nobodyiam commented Nov 22, 2020

nobodyiam left a comment

resolved embedded value in annotation @ApolloConfigChangeListener #3349

resolved embedded value in annotation @ApolloConfigChangeListener #3349

Conversation

Anilople commented Oct 23, 2020 • edited Loading

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

nobodyiam commented Oct 30, 2020

nobodyiam commented Nov 1, 2020

Anilople commented Nov 1, 2020

Anilople commented Nov 1, 2020

codecov-io commented Nov 2, 2020 • edited Loading

Codecov Report

nobodyiam left a comment • edited Loading

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

nobodyiam Nov 22, 2020

Choose a reason for hiding this comment

Anilople Nov 22, 2020

Choose a reason for hiding this comment

nobodyiam Nov 22, 2020

Choose a reason for hiding this comment

Anilople Nov 22, 2020

Choose a reason for hiding this comment

nobodyiam Nov 22, 2020

Choose a reason for hiding this comment

nobodyiam Nov 22, 2020

Choose a reason for hiding this comment

nobodyiam Nov 22, 2020

Choose a reason for hiding this comment

Anilople Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Anilople commented Nov 22, 2020

nobodyiam commented Nov 22, 2020

nobodyiam left a comment

Choose a reason for hiding this comment

Anilople commented Oct 23, 2020 •

edited

Loading

codecov-io commented Nov 2, 2020 •

edited

Loading

nobodyiam left a comment •

edited

Loading

Anilople Nov 22, 2020 •

edited

Loading