test: overhaul automated compatibility coverage#123
test: overhaul automated compatibility coverage#123nobodyiam merged 6 commits intoapolloconfig:mainfrom
Conversation
nobodyiam
force-pushed
the
codex/full-automation-test-rework
branch
from
8ad9334 to
20eb72d
Compare
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR adds extensive tests and compatibility modules, restructures the CI workflow to a matrix-driven build, introduces appId-aware APIs to the mock server and EmbeddedApollo, replaces WebClientCustomizer with Consumer<WebClient.Builder>, adapts WebFlux status handling via reflection, and adds related test dependencies and test resources. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Embedded as EmbeddedApollo/MockServer
participant Client as Apollo Client (ConfigService)
participant Listener as App Change Listener
Test->>Embedded: addOrModifyProperty(appId, namespace, key, value)
Embedded->>Embedded: persist per-appId+namespace overrides
Embedded->>Client: notify/long-poll endpoint receives change
Client->>Client: RemoteConfigLongPollService detects update
Client->>Listener: dispatch ConfigChangeEvent
Listener-->>Test: event observed / assertions pass
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
============================================
+ Coverage 68.68% 70.96% +2.28%
- Complexity 1503 1628 +125
============================================
Files 212 224 +12
Lines 6396 6720 +324
Branches 647 678 +31
============================================
+ Hits 4393 4769 +376
+ Misses 1673 1600 -73
- Partials 330 351 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| - name: Upload coverage to Codecov | ||
| if: matrix.jdk == '8' | ||
| uses: codecov/codecov-action@v1 | ||
| uses: codecov/codecov-action@v4 | ||
| with: | ||
| files: ${{ github.workspace }}/apollo-*/target/site/jacoco/jacoco.xml |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
codecov/codecov-action v4 token requirement documentation
💡 Result:
For codecov/codecov-action@v4, tokenless uploads are not supported (i.e., you should provide CODECOV_TOKEN via with: token: or env: CODECOV_TOKEN). The one exception called out in the v4 docs is that PRs from forks into an upstream public repo can upload without the upstream repo’s token. [1]
Codecov’s token docs also explain that private repos always require a token, and that tokenless uploading for public repos depends on org/repo settings—but the docs note that disabling token authentication for public repos is only available on codecov-action > v5.0 (or codecov-cli > 0.9), reinforcing that v4 generally requires tokens. [2]
Primary references: Codecov Action README (v4 release notes) [1], Codecov “Codecov Tokens” documentation [2].
Sources: [1] (github.com) [2] (docs.codecov.com)
Citations:
- 1: https://github.com/codecov/codecov-action?utm_source=openai
- 2: https://docs.codecov.com/docs/codecov-tokens?utm_source=openai
codecov-action@v4 requires a token — upload will fail without it.
Starting with v4, codecov/codecov-action requires CODECOV_TOKEN to be explicitly provided. Without it, the coverage upload step will fail in most scenarios. The only exception is pull requests from forks into public upstream repositories.
Proposed fix
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v4
with:
files: ${{ github.workspace }}/apollo-*/target/site/jacoco/jacoco.xml
+ token: ${{ secrets.CODECOV_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Upload coverage to Codecov | |
| if: matrix.jdk == '8' | |
| uses: codecov/codecov-action@v1 | |
| uses: codecov/codecov-action@v4 | |
| with: | |
| files: ${{ github.workspace }}/apollo-*/target/site/jacoco/jacoco.xml | |
| - name: Upload coverage to Codecov | |
| uses: codecov/codecov-action@v4 | |
| with: | |
| files: ${{ github.workspace }}/apollo-*/target/site/jacoco/jacoco.xml | |
| token: ${{ secrets.CODECOV_TOKEN }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml around lines 70 - 73, The Codecov upload step
"Upload coverage to Codecov" uses codecov/codecov-action@v4 which requires a
token; update that job to pass the token (e.g., add a with: token: ${{
secrets.CODECOV_TOKEN }} or set CODECOV_TOKEN via env/secrets) so the action
receives the required CODECOV_TOKEN for uploads when running in non-fork PR or
CI contexts; ensure the token reference uses the repository secret name you have
configured.
There was a problem hiding this comment.
Thanks for the note. I’m intentionally not forcing a token here: this repo runs as a public upstream + fork PR flow, and codecov-action@v4 is working in this setup (coverage upload passed on the latest run). Adding a required secret here may cause fork-PR visibility issues. If uploads become unstable later, I’ll follow up with a dedicated token/action upgrade change.
There was a problem hiding this comment.
@nobodyiam Thank you for the clarification! Your approach makes perfect sense — since the repository operates as a public upstream with fork PRs, you're correctly leveraging the exception where codecov-action@v4 uploads without a token. The successful coverage uploads in this PR confirm that setup is working as expected, and avoiding secret visibility concerns for fork contributors is a good practice.
I appreciate you explaining the context, and your plan to revisit if needed is reasonable. 👍
🧠 Learnings used
Learnt from: CR
Repo: apolloconfig/apollo-java PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-08T04:55:31.357Z
Learning: Applies to CHANGES.md : Format CHANGES.md entries as Markdown links with link text as change description and link target as PR URL (not issue URL), e.g., [Fix ... detailed summary](https://github.com/apolloconfig/apollo-java/pull/123)
Learnt from: CR
Repo: apolloconfig/apollo-java PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-08T04:55:31.357Z
Learning: Applies to {pom.xml,*/pom.xml} : Multi-module Maven repository with root pom.xml containing core modules: apollo-core, apollo-client, apollo-client-config-data, apollo-mockserver, apollo-openapi, and apollo-plugin
...java/com/ctrip/framework/apollo/config/data/ApolloClientConfigDataAutoConfigurationTest.java
Show resolved
Hide resolved
.../test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java
Show resolved
Hide resolved
...ctrip/framework/apollo/config/data/listener/ApolloDeferredLoggerApplicationListenerTest.java
Outdated
Show resolved
Hide resolved
...ctrip/framework/apollo/config/data/listener/ApolloDeferredLoggerApplicationListenerTest.java
Show resolved
Hide resolved
...est/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java
Outdated
Show resolved
Hide resolved
...t/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java
Outdated
Show resolved
Hide resolved
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!-- | ||
| ~ Copyright 2022 Apollo Authors | ||
| ~ | ||
| ~ Licensed under the Apache License, Version 2.0 (the "License"); | ||
| ~ you may not use this file except in compliance with the License. | ||
| ~ You may obtain a copy of the License at | ||
| ~ | ||
| ~ http://www.apache.org/licenses/LICENSE-2.0 | ||
| ~ | ||
| ~ Unless required by applicable law or agreed to in writing, software | ||
| ~ distributed under the License is distributed on an "AS IS" BASIS, | ||
| ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| ~ See the License for the specific language governing permissions and | ||
| ~ limitations under the License. | ||
| ~ | ||
| --> | ||
| <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> | ||
| <groupId>com.ctrip.framework.apollo</groupId> | ||
| <artifactId>apollo-java</artifactId> | ||
| <version>${revision}</version> | ||
| <relativePath>../pom.xml</relativePath> | ||
| </parent> | ||
| <modelVersion>4.0.0</modelVersion> | ||
|
|
||
| <artifactId>apollo-compat-tests</artifactId> | ||
| <name>Apollo Compatibility Tests</name> | ||
| <packaging>pom</packaging> | ||
|
|
||
| <modules> | ||
| <module>apollo-api-compat-it</module> | ||
| <module>apollo-spring-compat-it</module> | ||
| <module>apollo-spring-boot-compat-it</module> | ||
| </modules> | ||
|
|
||
| <properties> | ||
| <maven.deploy.skip>true</maven.deploy.skip> | ||
| <maven.install.skip>true</maven.install.skip> | ||
| <compat.surefire.version>3.2.5</compat.surefire.version> | ||
| </properties> | ||
|
|
||
| <build> | ||
| <pluginManagement> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <version>${compat.surefire.version}</version> | ||
| <configuration> | ||
| <trimStackTrace>false</trimStackTrace> | ||
| </configuration> | ||
| </plugin> | ||
| </plugins> | ||
| </pluginManagement> | ||
| </build> | ||
| </project> |
There was a problem hiding this comment.
OSV scanner flags three HIGH-severity advisories in the compatibility test dependency tree — review acceptability for test scope.
The static analysis tool reports:
| Advisory | Artifact | Severity |
|---|---|---|
| GHSA-rqfh-9r24-8c9r | assertj-core 3.22.0 — XXE in isXmlEqualTo |
HIGH |
| GHSA-rc42-6c7j-7h5r | spring-boot 2.7.18 — wrong EndpointRequest.to() matcher |
HIGH |
| GHSA-jmp9-x22r-554x | spring-core 5.3.31 — improper authorization via annotation detection |
HIGH |
These are all in test-scope transitive dependencies pulled in by apollo-spring-boot-compat-it, which intentionally targets Spring Boot 2.7.18. Since the test harness operates in a controlled environment and never exposes these to untrusted input or network traffic, the practical risk is low. However, the team should explicitly acknowledge these advisories (e.g., in an OSV suppression file or a project security policy) to prevent CI security gates from failing unexpectedly.
🧰 Tools
🪛 OSV Scanner (2.3.3)
[HIGH] 1-1: org.assertj:assertj-core 3.22.0: AssertJ has XML External Entity (XXE) vulnerability when parsing untrusted XML via isXmlEqualTo assertion
[HIGH] 1-1: org.springframework.boot:spring-boot 2.7.18: Spring Boot EndpointRequest.to() creates wrong matcher if actuator endpoint is not exposed
[HIGH] 1-1: org.springframework:spring-core 5.3.31: Spring Framework annotation detection mechanism may result in improper authorization
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apollo-compat-tests/pom.xml` around lines 1 - 59, OSV reported three HIGH
advisories in test-scope transitive deps (assertj-core 3.22.0, spring-boot
2.7.18, spring-core 5.3.31) pulled in by the apollo-spring-boot-compat-it module
under the apollo-compat-tests aggregator; add an explicit OSSV/OSV suppression
or project security exception that lists the GHSA IDs (GHSA-rqfh-9r24-8c9r,
GHSA-rc42-6c7j-7h5r, GHSA-jmp9-x22r-554x), documents they are test-scope only,
records the controlled-test-harness justification and risk acceptance, and
ensure your CI vulnerability scan reads that suppression (or reference the
exception in the project security policy) so the compat test module
(apollo-spring-boot-compat-it) does not fail CI while retaining current
dependency targets.
There was a problem hiding this comment.
Acknowledged. These are test-scope advisories from compatibility dependencies. I’m keeping this PR focused on functional test automation changes and not introducing suppression/policy changes here. We can handle dependency-security governance in a dedicated follow-up PR.
There was a problem hiding this comment.
Rate Limit Exceeded
@nobodyiam have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 25 seconds before sending another message.
...rc/test/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiMockIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
apollo-client/src/main/java/com/ctrip/framework/apollo/spring/annotation/ApolloAnnotationProcessor.java (1)
274-279: Null-guard fix looks correct — addresses the previously flagged NPE risk.The new
resolveAppIdhelper cleanly short-circuits whenappIdisnull(which can happen when bothannotation.appId()is blank andconfigUtil.getAppId()returnsnull), preventing aNullPointerExceptioninsideEnvironment.resolveRequiredPlaceholders. Both call sites (lines 111 and 135) are updated consistently, andConfigService.getConfigalready handles anullappId downstream.Consider adding
@Nullableto the return type for consistency with the existingresolvePropertyValuemethod at line 245, which already uses the importedjavax.annotation.Nullable:♻️ Optional consistency improvement
+ `@Nullable` private String resolveAppId(String appId) { if (appId == null) { return null; } return this.environment.resolveRequiredPlaceholders(appId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client/src/main/java/com/ctrip/framework/apollo/spring/annotation/ApolloAnnotationProcessor.java` around lines 274 - 279, The new resolveAppId method correctly null-guards but should declare its return as `@Nullable` for consistency with resolvePropertyValue; add the javax.annotation.Nullable annotation to the resolveAppId declaration (ensure the import javax.annotation.Nullable is present or reused) so callers and static analysis know the method can return null.apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/listener/ApolloSpringApplicationRegisterListenerTest.java (1)
47-66: Extract duplicated helper methods into a shared test utility.
newApplicationStartingEventandnewDefaultBootstrapContextare duplicated across at least two test classes:ApolloSpringApplicationRegisterListenerTestandBootstrapRegistryHelperCompatibilityTest. Additionally,newDefaultBootstrapContextappears inApolloConfigDataLoaderInitializerTestandApolloConfigDataLoaderTestwith slight variations. Consider creating a shared test utility (e.g.,BootstrapTestSupportin a common test package) to centralize this reflection logic, making Spring Boot version updates easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/listener/ApolloSpringApplicationRegisterListenerTest.java` around lines 47 - 66, Extract the duplicated reflection helpers into a shared test utility class (e.g., BootstrapTestSupport) and update callers to use it: move the logic in newApplicationStartingEvent (iterating ApplicationStartingEvent constructors and instantiating with bootstrapContext, SpringApplication, new String[0]) into a static method BootstrapTestSupport.newApplicationStartingEvent(Object, SpringApplication) and the logic in newDefaultBootstrapContext (resolving "org.springframework.boot.bootstrap.DefaultBootstrapContext" vs "org.springframework.boot.DefaultBootstrapContext" and instantiating) into BootstrapTestSupport.newDefaultBootstrapContext(); replace the duplicated implementations in ApolloSpringApplicationRegisterListenerTest, BootstrapRegistryHelperCompatibilityTest, ApolloConfigDataLoaderInitializerTest, and ApolloConfigDataLoaderTest with calls to these static helpers and keep the existing fallback/classloader behavior and exception handling unchanged.apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java (3)
196-196:getDeclaredFielddoes not traverse superclasses — fragile if fields move.
target.getClass().getDeclaredField(fieldName)only inspects the immediate concrete class. If any of the target implementations (DefaultConfigManager,DefaultConfigFactoryManager,DefaultConfigRegistry) ever moves a field to an abstract parent, this will throwNoSuchFieldException. A small helper that walks the hierarchy makes the utility more robust.♻️ Proposed helper
private static void clearField(Object target, String fieldName) throws Exception { - Field field = target.getClass().getDeclaredField(fieldName); + Field field = findField(target.getClass(), fieldName); field.setAccessible(true); ... } + +private static Field findField(Class<?> clazz, String fieldName) throws NoSuchFieldException { + for (Class<?> c = clazz; c != null && c != Object.class; c = c.getSuperclass()) { + try { + return c.getDeclaredField(fieldName); + } catch (NoSuchFieldException ignored) { + // continue up the hierarchy + } + } + throw new NoSuchFieldException("Field '" + fieldName + "' not found in class hierarchy of " + clazz.getName()); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java` at line 196, Replace the direct call to target.getClass().getDeclaredField(fieldName) with a small helper that walks the class hierarchy: start from target.getClass() and loop through getSuperclass() trying clazz.getDeclaredField(fieldName) until found (returning the Field) or the root is reached, then throw NoSuchFieldException; update the use site in ApolloApiCompatibilityTest to call this helper so it finds fields declared in superclasses of DefaultConfigManager/DefaultConfigFactoryManager/DefaultConfigRegistry.
186-193: Cache theConfigManagerinstance in a local variable.
ApolloInjector.getInstance(ConfigManager.class)is called four times independently. If the injector isn't bound as a strict singleton, or if its internal binding changes between calls, each invocation could return a different instance and the field-clearing operations would target orphaned objects. Storing the result once is both safer and cleaner.♻️ Proposed refactor
private static void clearApolloClientCaches() throws Exception { - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configs"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configLocks"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFiles"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFileLocks"); + ConfigManager configManager = ApolloInjector.getInstance(ConfigManager.class); + clearField(configManager, "m_configs"); + clearField(configManager, "m_configLocks"); + clearField(configManager, "m_configFiles"); + clearField(configManager, "m_configFileLocks"); clearField(ApolloInjector.getInstance(ConfigFactoryManager.class), "m_factories"); clearField(ApolloInjector.getInstance(ConfigRegistry.class), "m_instances"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java` around lines 186 - 193, In clearApolloClientCaches(), avoid repeatedly calling ApolloInjector.getInstance(ConfigManager.class); instead call it once and store the result in a local variable (e.g., configManager) and use that variable for clearField calls on "m_configs", "m_configLocks", "m_configFiles", "m_configFileLocks"; similarly, obtain a single local reference for ConfigFactoryManager (e.g., configFactoryManager) and for ConfigRegistry (e.g., configRegistry) and use those variables in the clearField calls for "m_factories" and "m_instances" respectively so each clearField targets the same instance returned by the injector.
142-160:resolveValueByFallbackconflates "key absent" with "key has value\"undefined\"".Comparing against
DEFAULT_VALUEas a sentinel means any config key whose actual stored value is the string"undefined"would be silently skipped, and the test would assert on the wrong source. Replacing the sentinel withnullis the idiomatic Apollo pattern and eliminates the ambiguity.♻️ Proposed refactor
private static String resolveValueByFallback(String key, Config appConfig, Config anotherAppConfig, Config publicConfig, Config yamlConfig) { - String value = appConfig.getProperty(key, DEFAULT_VALUE); - if (!DEFAULT_VALUE.equals(value)) { + String value = appConfig.getProperty(key, null); + if (value != null) { return value; } - value = anotherAppConfig.getProperty(key, DEFAULT_VALUE); - if (!DEFAULT_VALUE.equals(value)) { + value = anotherAppConfig.getProperty(key, null); + if (value != null) { return value; } - value = publicConfig.getProperty(key, DEFAULT_VALUE); - if (!DEFAULT_VALUE.equals(value)) { + value = publicConfig.getProperty(key, null); + if (value != null) { return value; } - return yamlConfig.getProperty(key, DEFAULT_VALUE); + String yamlValue = yamlConfig.getProperty(key, null); + return yamlValue != null ? yamlValue : DEFAULT_VALUE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java` around lines 142 - 160, The method resolveValueByFallback currently uses DEFAULT_VALUE as a sentinel which treats a real stored value of "undefined" as missing; change the sentinel to null by calling getProperty(key, null) for appConfig, anotherAppConfig, publicConfig and yamlConfig, check each returned value for null (if not null return it), and finally return DEFAULT_VALUE only if all four calls returned null; update references in resolveValueByFallback accordingly.apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java (1)
105-112: Four redundantApolloInjector.getInstance(ConfigManager.class)lookups.
ApolloInjector.getInstanceis called four separate times for the same type; a single local variable suffices.♻️ Proposed refactor
private static void clearApolloClientCaches() throws Exception { - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configs"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configLocks"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFiles"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFileLocks"); + Object configManager = ApolloInjector.getInstance(ConfigManager.class); + clearField(configManager, "m_configs"); + clearField(configManager, "m_configLocks"); + clearField(configManager, "m_configFiles"); + clearField(configManager, "m_configFileLocks"); clearField(ApolloInjector.getInstance(ConfigFactoryManager.class), "m_factories"); clearField(ApolloInjector.getInstance(ConfigRegistry.class), "m_instances"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java` around lines 105 - 112, Replace the repeated ApolloInjector.getInstance(ConfigManager.class) calls with a single local variable and reuse it: obtain one ConfigManager instance in a local variable (e.g., configManager) and call clearField(configManager, "m_configs"), clearField(configManager, "m_configLocks"), clearField(configManager, "m_configFiles"), clearField(configManager, "m_configFileLocks"); keep the separate calls for ApolloInjector.getInstance(ConfigFactoryManager.class) and ApolloInjector.getInstance(ConfigRegistry.class) as-is (or similarly cache them) and use their instances when calling clearField on "m_factories" and "m_instances" respectively to remove the redundant lookups in clearApolloClientCaches.apollo-compat-tests/apollo-spring-boot-compat-it/src/test/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java (2)
376-396: ParameterizeApplicationListenerto eliminate the raw-type and manualinstanceofguardUsing the raw
ApplicationListenertriggers an unchecked warning and requires the manualinstanceof ApolloConfigChangeEventguard. Binding the generic parameter restricts delivery to the desired event type automatically.♻️ Proposed fix
-static class ApolloApplicationListenerProbe implements ApplicationListener { +static class ApolloApplicationListenerProbe implements ApplicationListener<ApolloConfigChangeEvent> { `@Override` - public void onApplicationEvent(ApplicationEvent event) { - if (event instanceof ApolloConfigChangeEvent) { - ConfigChangeEvent configChangeEvent = ((ApolloConfigChangeEvent) event).getConfigChangeEvent(); - changes.add(configChangeEvent.getAppId() + "#" + configChangeEvent.getNamespace()); - namespaces.add(configChangeEvent.getNamespace()); - } + public void onApplicationEvent(ApolloConfigChangeEvent event) { + ConfigChangeEvent configChangeEvent = event.getConfigChangeEvent(); + changes.add(configChangeEvent.getAppId() + "#" + configChangeEvent.getNamespace()); + namespaces.add(configChangeEvent.getNamespace()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-spring-boot-compat-it/src/test/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java` around lines 376 - 396, Change ApolloApplicationListenerProbe to implement ApplicationListener<ApolloConfigChangeEvent> and update the onApplicationEvent method to accept an ApolloConfigChangeEvent directly (remove the raw ApplicationListener and the instanceof/cast logic); then obtain the ConfigChangeEvent via event.getConfigChangeEvent() and continue adding to the changes and namespaces sets as before (references: ApolloApplicationListenerProbe, ApplicationListener, onApplicationEvent, ApolloConfigChangeEvent, ConfigChangeEvent, changes, namespaces).
325-329:interestedKeyPrefixesis over-broad for an exact-key match — preferinterestedKeys
interestedKeyPrefixes = {"compat.origin"}fires the listener for any key whose name starts with"compat.origin"(e.g.,"compat.originDate","compat.origin.host"). Since only the exact key"compat.origin"is relevant here,interestedKeysis more precise.♻️ Proposed fix
-@ApolloConfigChangeListener(appId = ANOTHER_APP_ID, - interestedKeyPrefixes = {"compat.origin"}) +@ApolloConfigChangeListener(appId = ANOTHER_APP_ID, + interestedKeys = {"compat.origin"}) private void onAnotherAppChange(ConfigChangeEvent event) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-compat-tests/apollo-spring-boot-compat-it/src/test/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java` around lines 325 - 329, The ApolloConfigChangeListener onAnotherAppChange is using interestedKeyPrefixes = {"compat.origin"} which is too broad and will trigger for any key starting with that prefix; replace it with interestedKeys = {"compat.origin"} on the `@ApolloConfigChangeListener` (for the listener method onAnotherAppChange tied to ANOTHER_APP_ID that calls anotherAppEvents.offer(event)) so the listener only fires for the exact "compat.origin" key.apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java (2)
19-22: Pre-existing JUnit 5 usage conflicts with the JUnit 4 coding guidelineThe file already imported
org.junit.jupiter.api.Assertions.assertTrue(line 22, not~) and used JUnit Jupiter@Testbefore this PR. The new assertion imports and@Teston line 48 continue that pattern. This is a pre-existing violation rather than something introduced here, but it's worth flagging for the test suite as a whole.As per coding guidelines, "Use JUnit 4 and JUnit Vintage for testing in this repository" — these imports use JUnit 5 Jupiter APIs instead.
Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java` around lines 19 - 22, The test file is importing and using JUnit 5 (org.junit.jupiter.api.Assertions.* and the JUnit 5 `@Test`) which conflicts with the repo guideline to use JUnit 4/Vintage; replace the JUnit 5 assertion imports and annotation with JUnit 4 equivalents (use org.junit.Assert static imports for assertEquals, assertNotNull, assertNull, assertTrue and org.junit.Test for the `@Test` annotation) wherever Methods/annotations like assertTrue, assertEquals, assertNotNull, assertNull and the `@Test` on ConfigIntegrationTest are used so the class consistently uses JUnit 4.
637-650: Sentinel"undefined"could silently mask a real config value
resolveValueByFallbackOrdertreatsDEFAULT_VALUE = "undefined"as the "not found" sentinel. If any namespace ever stores the literal value"undefined"for a key, the fallback logic will skip it and look at the next config, producing a silently wrong result. The test data is safe today, but consider using a private static finalObjectsentinel (or a dedicated marker string less likely to collide, e.g."__NOT_FOUND__") to make the helper more robust against accidental collisions in future test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java` around lines 637 - 650, The helper resolveValueByFallbackOrder currently treats DEFAULT_VALUE ("undefined") as the sentinel which can collide with real values; change the lookup to use a real "not found" check by calling appConfig.getProperty(key, null) / anotherAppConfig.getProperty(key, null) / publicConfig.getProperty(key, null) and treat null as missing (instead of comparing to DEFAULT_VALUE), or alternatively replace DEFAULT_VALUE with a dedicated unlikely sentinel like "__NOT_FOUND__"; update resolveValueByFallbackOrder to use the new sentinel or null-check and return the final value accordingly, referencing resolveValueByFallbackOrder and DEFAULT_VALUE in your changes.apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java (2)
254-258:getDeclaredFielddoes not traverse the superclass hierarchy.
target.getClass().getDeclaredField(fieldName)will throwNoSuchFieldExceptionif any of the target fields (m_configs,m_configLocks, etc.) are defined in a superclass rather than in the concrete implementation class returned byApolloInjector. This silently breaks if the implementation class is refactored to extend a base class that owns those fields.♻️ Proposed helper for hierarchy traversal
private static void clearField(Object target, String fieldName) throws Exception { - Field field = target.getClass().getDeclaredField(fieldName); + Field field = findField(target.getClass(), fieldName); field.setAccessible(true); ... } + +private static Field findField(Class<?> clazz, String fieldName) throws NoSuchFieldException { + for (Class<?> c = clazz; c != null; c = c.getSuperclass()) { + try { + return c.getDeclaredField(fieldName); + } catch (NoSuchFieldException ignored) { + } + } + throw new NoSuchFieldException(fieldName); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java` around lines 254 - 258, The helper clearField currently uses target.getClass().getDeclaredField(fieldName) which fails if the field lives in a superclass; modify clearField to walk the class hierarchy (start from target.getClass() and repeatedly set clazz = clazz.getSuperclass()) and call getDeclaredField(fieldName) on each until found (or throw NoSuchFieldException if null), then setAccessible(true) and clear the field value as before; reference the clearField method and replace the single getDeclaredField call with a superclass-traversal loop that locates the field before calling field.set(target, null).
245-252: ExtractConfigManagerinstance to a local variable.
ApolloInjector.getInstance(ConfigManager.class)is invoked four times redundantly.♻️ Proposed refactor
private static void clearApolloClientCaches() throws Exception { - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configs"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configLocks"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFiles"); - clearField(ApolloInjector.getInstance(ConfigManager.class), "m_configFileLocks"); + ConfigManager configManager = ApolloInjector.getInstance(ConfigManager.class); + clearField(configManager, "m_configs"); + clearField(configManager, "m_configLocks"); + clearField(configManager, "m_configFiles"); + clearField(configManager, "m_configFileLocks"); clearField(ApolloInjector.getInstance(ConfigFactoryManager.class), "m_factories"); clearField(ApolloInjector.getInstance(ConfigRegistry.class), "m_instances"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java` around lines 245 - 252, The helper clearApolloClientCaches repeatedly invokes ApolloInjector.getInstance(ConfigManager.class) four times; refactor by extracting ApolloInjector.getInstance(ConfigManager.class) into a local variable (e.g., ConfigManager configManager) and use that variable when calling clearField for "m_configs", "m_configLocks", "m_configFiles", and "m_configFileLocks"; leave the existing calls for ConfigFactoryManager and ConfigRegistry as-is and keep using the existing clearField helper and the method name clearApolloClientCaches to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 69-72: Increase the timeout for the Maven integration job by
raising timeout_minutes from 3 to a more realistic value (e.g., 15–20 minutes)
so cold-cache Maven runs and JDK8 test execution won't prematurely abort; update
the timeout_minutes key in the same job that runs the command "mvn -B clean test
-P travis jacoco:report -Dmaven.gitcommitid.skip=true" (the job referenced as
unit-integration-pr in the review) and keep max_attempts/retry_wait_seconds
unchanged.
In
`@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java`:
- Line 73: Remove the class-level `@DirtiesContext`(classMode =
ClassMode.AFTER_EACH_TEST_METHOD) from ConfigDataIntegrationTest and instead
enforce a stable test order by adding
`@FixMethodOrder`(MethodSorters.NAME_ASCENDING) to the class and renaming the
tests so the non-mutating import test
(testImportMultipleNamespacesAndConditionalOnProperty) sorts before the mutating
test (testApolloConfigChangeListenerWithInterestedKeyPrefixes) — alternatively,
if you must recreate contexts per test keep `@DirtiesContext` but ensure
client/server state is reset before context creation by invoking
resetApolloStaticState() and embeddedApollo.resetOverriddenProperties() from a
`@Rule` ExternalResource.before() that runs prior to Spring context
initialization.
In
`@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java`:
- Around line 38-52: The static initializer only sets app.id and env once at
class load, so update the beforeClass(EmbeddedApollo) method to re-establish
those required system properties before resetting Apollo state: explicitly call
System.setProperty("app.id", "someAppId") and System.setProperty("env", "local")
at the start of beforeClass (before calling
embeddedApollo.resetOverriddenProperties() and resetApolloState()) so tests run
with the expected properties even after afterClass/restoreOrClear has cleared
them.
- Around line 71-77: The copyConfigProperties method can pass null into
Properties.setProperty causing an NPE if a key disappears between
config.getPropertyNames() and config.getProperty(); update copyConfigProperties
to call config.getProperty(key, null), check the returned value for null, and
only call properties.setProperty(key, value) when value != null (i.e., skip keys
whose value is null) so Properties.setProperty is never invoked with a null
argument.
- Around line 129-131: The current use of
container.getClass().getDeclaredMethod("clear") fails for inherited public
clear() implementations; replace the lookup with
container.getClass().getMethod("clear") to search the public inheritance
hierarchy, then invoke the resulting Method (remove or omit setAccessible(true)
since getMethod returns public methods) so clearMethod.invoke(container)
succeeds for inherited implementations.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 73-76: The Codecov upload step ("Upload coverage to Codecov")
calling codecov/codecov-action@v4 is missing the required token; add the token
input (or set env var) so the action can authenticate, e.g., include token: ${{
secrets.CODECOV_TOKEN }} under the step's with (or set CODECOV_TOKEN in the
step's env) to supply the secret to codecov/codecov-action@v4.
In
`@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java`:
- Around line 158-160: The negative-assertion uses a long 3000ms wait which is
flaky/duplicate; after calling addOrModifyForAllAppIds("application",
"apollo.unrelated.key", "value") change the listenerProbe.pollEvent timeout to a
much shorter deterministic value (e.g., 100ms) and keep the assertNull check so
the test fails fast and avoids flakiness: update the call to
listenerProbe.pollEvent(100, TimeUnit.MILLISECONDS) while keeping
assertNull(unrelatedEvent) to assert no event was produced.
In
`@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/util/BootstrapRegistryHelperCompatibilityTest.java`:
- Around line 78-97: The two helper methods newApplicationStartingEvent(...) and
newDefaultBootstrapContext() are duplicated in
BootstrapRegistryHelperCompatibilityTest and
ApolloSpringApplicationRegisterListenerTest; extract them into a shared test
utility (e.g., a new TestUtils class or a common base test class) and update
both test classes to call that single implementation instead of duplicating
logic, keeping the same signatures and the constructor-signature loop and
ClassUtils presence check intact.
In
`@apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java`:
- Line 167: The test contains a duplicated/no-op review comment about updating
the negative-assertion timeout; remove the duplicate and ensure there is a
single assertion using the updated timeout by keeping the line
assertNull(pollFuture(defaultAppFuture, 1000)) in ConfigIntegrationTest and
deleting any redundant duplicate assertion or comment referring to the timeout
update so only one authoritative pollFuture(defaultAppFuture, 1000)
negative-assertion remains.
---
Nitpick comments:
In
`@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java`:
- Around line 254-258: The helper clearField currently uses
target.getClass().getDeclaredField(fieldName) which fails if the field lives in
a superclass; modify clearField to walk the class hierarchy (start from
target.getClass() and repeatedly set clazz = clazz.getSuperclass()) and call
getDeclaredField(fieldName) on each until found (or throw NoSuchFieldException
if null), then setAccessible(true) and clear the field value as before;
reference the clearField method and replace the single getDeclaredField call
with a superclass-traversal loop that locates the field before calling
field.set(target, null).
- Around line 245-252: The helper clearApolloClientCaches repeatedly invokes
ApolloInjector.getInstance(ConfigManager.class) four times; refactor by
extracting ApolloInjector.getInstance(ConfigManager.class) into a local variable
(e.g., ConfigManager configManager) and use that variable when calling
clearField for "m_configs", "m_configLocks", "m_configFiles", and
"m_configFileLocks"; leave the existing calls for ConfigFactoryManager and
ConfigRegistry as-is and keep using the existing clearField helper and the
method name clearApolloClientCaches to locate the change.
In
`@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/listener/ApolloSpringApplicationRegisterListenerTest.java`:
- Around line 47-66: Extract the duplicated reflection helpers into a shared
test utility class (e.g., BootstrapTestSupport) and update callers to use it:
move the logic in newApplicationStartingEvent (iterating
ApplicationStartingEvent constructors and instantiating with bootstrapContext,
SpringApplication, new String[0]) into a static method
BootstrapTestSupport.newApplicationStartingEvent(Object, SpringApplication) and
the logic in newDefaultBootstrapContext (resolving
"org.springframework.boot.bootstrap.DefaultBootstrapContext" vs
"org.springframework.boot.DefaultBootstrapContext" and instantiating) into
BootstrapTestSupport.newDefaultBootstrapContext(); replace the duplicated
implementations in ApolloSpringApplicationRegisterListenerTest,
BootstrapRegistryHelperCompatibilityTest, ApolloConfigDataLoaderInitializerTest,
and ApolloConfigDataLoaderTest with calls to these static helpers and keep the
existing fallback/classloader behavior and exception handling unchanged.
In
`@apollo-client/src/main/java/com/ctrip/framework/apollo/spring/annotation/ApolloAnnotationProcessor.java`:
- Around line 274-279: The new resolveAppId method correctly null-guards but
should declare its return as `@Nullable` for consistency with
resolvePropertyValue; add the javax.annotation.Nullable annotation to the
resolveAppId declaration (ensure the import javax.annotation.Nullable is present
or reused) so callers and static analysis know the method can return null.
In
`@apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java`:
- Around line 19-22: The test file is importing and using JUnit 5
(org.junit.jupiter.api.Assertions.* and the JUnit 5 `@Test`) which conflicts with
the repo guideline to use JUnit 4/Vintage; replace the JUnit 5 assertion imports
and annotation with JUnit 4 equivalents (use org.junit.Assert static imports for
assertEquals, assertNotNull, assertNull, assertTrue and org.junit.Test for the
`@Test` annotation) wherever Methods/annotations like assertTrue, assertEquals,
assertNotNull, assertNull and the `@Test` on ConfigIntegrationTest are used so the
class consistently uses JUnit 4.
- Around line 637-650: The helper resolveValueByFallbackOrder currently treats
DEFAULT_VALUE ("undefined") as the sentinel which can collide with real values;
change the lookup to use a real "not found" check by calling
appConfig.getProperty(key, null) / anotherAppConfig.getProperty(key, null) /
publicConfig.getProperty(key, null) and treat null as missing (instead of
comparing to DEFAULT_VALUE), or alternatively replace DEFAULT_VALUE with a
dedicated unlikely sentinel like "__NOT_FOUND__"; update
resolveValueByFallbackOrder to use the new sentinel or null-check and return the
final value accordingly, referencing resolveValueByFallbackOrder and
DEFAULT_VALUE in your changes.
In
`@apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java`:
- Line 196: Replace the direct call to
target.getClass().getDeclaredField(fieldName) with a small helper that walks the
class hierarchy: start from target.getClass() and loop through getSuperclass()
trying clazz.getDeclaredField(fieldName) until found (returning the Field) or
the root is reached, then throw NoSuchFieldException; update the use site in
ApolloApiCompatibilityTest to call this helper so it finds fields declared in
superclasses of
DefaultConfigManager/DefaultConfigFactoryManager/DefaultConfigRegistry.
- Around line 186-193: In clearApolloClientCaches(), avoid repeatedly calling
ApolloInjector.getInstance(ConfigManager.class); instead call it once and store
the result in a local variable (e.g., configManager) and use that variable for
clearField calls on "m_configs", "m_configLocks", "m_configFiles",
"m_configFileLocks"; similarly, obtain a single local reference for
ConfigFactoryManager (e.g., configFactoryManager) and for ConfigRegistry (e.g.,
configRegistry) and use those variables in the clearField calls for
"m_factories" and "m_instances" respectively so each clearField targets the same
instance returned by the injector.
- Around line 142-160: The method resolveValueByFallback currently uses
DEFAULT_VALUE as a sentinel which treats a real stored value of "undefined" as
missing; change the sentinel to null by calling getProperty(key, null) for
appConfig, anotherAppConfig, publicConfig and yamlConfig, check each returned
value for null (if not null return it), and finally return DEFAULT_VALUE only if
all four calls returned null; update references in resolveValueByFallback
accordingly.
In
`@apollo-compat-tests/apollo-spring-boot-compat-it/src/test/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java`:
- Around line 376-396: Change ApolloApplicationListenerProbe to implement
ApplicationListener<ApolloConfigChangeEvent> and update the onApplicationEvent
method to accept an ApolloConfigChangeEvent directly (remove the raw
ApplicationListener and the instanceof/cast logic); then obtain the
ConfigChangeEvent via event.getConfigChangeEvent() and continue adding to the
changes and namespaces sets as before (references:
ApolloApplicationListenerProbe, ApplicationListener, onApplicationEvent,
ApolloConfigChangeEvent, ConfigChangeEvent, changes, namespaces).
- Around line 325-329: The ApolloConfigChangeListener onAnotherAppChange is
using interestedKeyPrefixes = {"compat.origin"} which is too broad and will
trigger for any key starting with that prefix; replace it with interestedKeys =
{"compat.origin"} on the `@ApolloConfigChangeListener` (for the listener method
onAnotherAppChange tied to ANOTHER_APP_ID that calls
anotherAppEvents.offer(event)) so the listener only fires for the exact
"compat.origin" key.
In
`@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java`:
- Around line 105-112: Replace the repeated
ApolloInjector.getInstance(ConfigManager.class) calls with a single local
variable and reuse it: obtain one ConfigManager instance in a local variable
(e.g., configManager) and call clearField(configManager, "m_configs"),
clearField(configManager, "m_configLocks"), clearField(configManager,
"m_configFiles"), clearField(configManager, "m_configFileLocks"); keep the
separate calls for ApolloInjector.getInstance(ConfigFactoryManager.class) and
ApolloInjector.getInstance(ConfigRegistry.class) as-is (or similarly cache them)
and use their instances when calling clearField on "m_factories" and
"m_instances" respectively to remove the redundant lookups in
clearApolloClientCaches.
.../test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java
Show resolved
Hide resolved
...t/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java
Show resolved
Hide resolved
| static Properties copyConfigProperties(Config config) { | ||
| Properties properties = new Properties(); | ||
| for (String key : config.getPropertyNames()) { | ||
| properties.setProperty(key, config.getProperty(key, null)); | ||
| } | ||
| return properties; | ||
| } |
There was a problem hiding this comment.
Properties.setProperty with a null value will throw NPE.
config.getProperty(key, null) uses null as the default — if a key is removed between the getPropertyNames() call and the individual getProperty() call (TOCTOU), the returned null is passed to Properties.setProperty, which delegates to Hashtable.put and throws NullPointerException.
🛡️ Proposed fix
static Properties copyConfigProperties(Config config) {
Properties properties = new Properties();
for (String key : config.getPropertyNames()) {
- properties.setProperty(key, config.getProperty(key, null));
+ String value = config.getProperty(key, null);
+ if (value != null) {
+ properties.setProperty(key, value);
+ }
}
return properties;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java`
around lines 71 - 77, The copyConfigProperties method can pass null into
Properties.setProperty causing an NPE if a key disappears between
config.getPropertyNames() and config.getProperty(); update copyConfigProperties
to call config.getProperty(key, null), check the returned value for null, and
only call properties.setProperty(key, value) when value != null (i.e., skip keys
whose value is null) so Properties.setProperty is never invoked with a null
argument.
There was a problem hiding this comment.
Applied. copyConfigProperties(...) now uses a non-null default (config.getProperty(key, "")) before setProperty. Included in commit 264b7bb.
| Method clearMethod = container.getClass().getDeclaredMethod("clear"); | ||
| clearMethod.setAccessible(true); | ||
| clearMethod.invoke(container); |
There was a problem hiding this comment.
getDeclaredMethod("clear") misses inherited clear() implementations.
Class.getDeclaredMethod only searches methods declared directly on the concrete class; if the container holds an object whose clear() is inherited from a parent class, this throws NoSuchMethodException. Class.getMethod("clear") searches the full public hierarchy and is the safer choice here.
♻️ Proposed fix
- Method clearMethod = container.getClass().getDeclaredMethod("clear");
- clearMethod.setAccessible(true);
+ Method clearMethod = container.getClass().getMethod("clear");
clearMethod.invoke(container);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Method clearMethod = container.getClass().getDeclaredMethod("clear"); | |
| clearMethod.setAccessible(true); | |
| clearMethod.invoke(container); | |
| Method clearMethod = container.getClass().getMethod("clear"); | |
| clearMethod.invoke(container); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java`
around lines 129 - 131, The current use of
container.getClass().getDeclaredMethod("clear") fails for inherited public
clear() implementations; replace the lookup with
container.getClass().getMethod("clear") to search the public inheritance
hierarchy, then invoke the resulting Method (remove or omit setAccessible(true)
since getMethod returns public methods) so clearMethod.invoke(container)
succeeds for inherited implementations.
There was a problem hiding this comment.
Applied. Switched reflective lookup from getDeclaredMethod("clear") to getMethod("clear") in the compatibility helpers so inherited clear() implementations are handled correctly. Included in commit 264b7bb.
There was a problem hiding this comment.
Pull request overview
This PR moves manual compatibility validation into repository-native automated tests and CI workflows, expanding UT/IT/compat coverage across API-only, Spring Framework, and Spring Boot scenarios to improve release confidence.
Changes:
- Introduces new compatibility test modules (
apollo-compat-tests/*) and adds/extends integration tests across client, config-data, OpenAPI, and log4j2 plugin areas. - Enhances the mock server to support multi-AppId overrides and improves client test infrastructure (mock config routing, listener isolation, config file formats).
- Restructures GitHub Actions CI into clearer jobs (compile matrix, unit/integration, and compat matrices for Spring/Spring Boot).
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Adds apollo-mockserver dependency entry for shared build usage. |
| apollo-plugin/apollo-plugin-log4j2/src/test/java/com/ctrip/framework/apollo/plugin/log4j2/ApolloClientConfigurationFactoryTest.java | Adds regression tests for log4j2 plugin enable/disable and XML config loading. |
| apollo-plugin/apollo-plugin-log4j2/pom.xml | Adds apollo-mockserver for plugin testing support. |
| apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/ApolloOpenApiMockIntegrationTest.java | Adds mock HTTP integration tests for OpenAPI client request/response behavior. |
| apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerApiTest.java | Adds multi-AppId override/isolation test coverage. |
| apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/EmbeddedApollo.java | Adds appId-aware property override and delete APIs. |
| apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/ApolloTestingServer.java | Extends mock server to handle app-specific resources and resets Apollo long-poll/client state between runs. |
| apollo-compat-tests/pom.xml | Adds compat test aggregator module and surefire management for compat runs. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/resources/spring/apollo-context.xml | Adds Spring XML context for compat scenarios. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/resources/mockdata-application.yaml.properties | Adds YAML config-file mock resource for Spring compat. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/resources/mockdata-application.properties | Adds properties mock resource for Spring compat. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/resources/mockdata-TEST1.apollo.properties | Adds public namespace mock resource for Spring compat. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/resources/mockdata-100004459-application.properties | Adds another-appId mock resource for Spring compat. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringXmlCompatibilityTest.java | Adds Spring XML wiring + refresh propagation compat test. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringXmlBean.java | Adds bean used by Spring XML compat tests. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringCompatibilityTestSupport.java | Adds shared Apollo reset + refresh helpers for Spring compat tests. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java | Adds event listener probe for Spring event propagation assertions. |
| apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringAnnotationCompatibilityTest.java | Adds annotation/multiple-config compat scenarios with refresh + listener isolation. |
| apollo-compat-tests/apollo-spring-compat-it/pom.xml | Defines Spring Framework compat IT module and Spring version property. |
| apollo-compat-tests/apollo-spring-boot-compat-it/src/test/resources/mockdata-application.yaml.properties | Adds Boot YAML config-file mock resource. |
| apollo-compat-tests/apollo-spring-boot-compat-it/src/test/resources/mockdata-application.properties | Adds Boot properties mock resource. |
| apollo-compat-tests/apollo-spring-boot-compat-it/src/test/resources/mockdata-TEST1.apollo.properties | Adds Boot public namespace mock resource. |
| apollo-compat-tests/apollo-spring-boot-compat-it/src/test/resources/mockdata-100004459-application.properties | Adds Boot another-appId mock resource. |
| apollo-compat-tests/apollo-spring-boot-compat-it/src/test/java/com/ctrip/framework/apollo/compat/springboot/ApolloSpringBootCompatibilityTest.java | Adds Spring Boot demo-style compat IT with refresh + listener coverage. |
| apollo-compat-tests/apollo-spring-boot-compat-it/pom.xml | Defines Boot compat IT module and matrix-tunable deps. |
| apollo-compat-tests/apollo-api-compat-it/src/test/resources/mockdata-datasources.xml.properties | Adds API-only XML config-file mock resource. |
| apollo-compat-tests/apollo-api-compat-it/src/test/resources/mockdata-application.yaml.properties | Adds API-only YAML config-file mock resource. |
| apollo-compat-tests/apollo-api-compat-it/src/test/resources/mockdata-application.properties | Adds API-only default namespace mock resource. |
| apollo-compat-tests/apollo-api-compat-it/src/test/resources/mockdata-TEST1.apollo.properties | Adds API-only public namespace mock resource. |
| apollo-compat-tests/apollo-api-compat-it/src/test/resources/mockdata-100004459-application.properties | Adds API-only another-appId fallback resource. |
| apollo-compat-tests/apollo-api-compat-it/src/test/java/com/ctrip/framework/apollo/compat/api/ApolloApiCompatibilityTest.java | Adds API-only compat tests for fallback order, listener isolation, and config-file formats. |
| apollo-compat-tests/apollo-api-compat-it/pom.xml | Defines API-only compat IT module excluding Spring deps. |
| apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java | Adds appId-aware listener routing tests and placeholder resolution coverage. |
| apollo-client/src/test/java/com/ctrip/framework/apollo/integration/ConfigIntegrationTest.java | Expands integration tests for fallback order, multi-AppId isolation, and config-file refresh. |
| apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java | Enhances config mocking to support appId/cluster/namespace-specific endpoints. |
| apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java | Adds overload to mock configs for specific appId routes. |
| apollo-client/src/main/java/com/ctrip/framework/apollo/spring/annotation/ApolloAnnotationProcessor.java | Fixes listener config retrieval to use placeholder-resolved appId value. |
| apollo-client-config-data/src/test/resources/mockdata-application.yaml.properties | Adds YAML config-file mock resource for config-data tests. |
| apollo-client-config-data/src/test/resources/mockdata-application.properties | Adds properties mock resource for config-data tests. |
| apollo-client-config-data/src/test/resources/mockdata-TEST1.apollo.properties | Adds public namespace mock resource for config-data tests. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/util/BootstrapRegistryHelperCompatibilityTest.java | Adds compat tests for bootstrap context behavior across Boot versions. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/listener/ApolloSpringApplicationRegisterListenerTest.java | Adds test coverage for registering SpringApplication in bootstrap context. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/listener/ApolloDeferredLoggerApplicationListenerTest.java | Adds regression coverage for deferred logger replay hooks. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/integration/ConfigDataIntegrationTest.java | Adds integration tests asserting refresh/listener behavior in config-data mode. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/importer/ApolloConfigDataLocationResolverTest.java | Adds tests for location parsing and ordering. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/importer/ApolloConfigDataLoaderTest.java | Adds tests for property source ordering and initializer reuse. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/importer/ApolloConfigDataLoaderInitializerTest.java | Adds tests for one-time initialization and bootstrap disabling behavior. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/extension/webclient/ApolloWebClientHttpClientTest.java | Adds tests for WebClient-backed HttpClient status/body/header handling. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/extension/initialize/ApolloClientExtensionInitializeFactoryTest.java | Adds tests for extension initialization toggles and unsupported modes. |
| apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/ApolloClientConfigDataAutoConfigurationTest.java | Adds auto-configuration bean presence/override tests. |
| apollo-client-config-data/src/main/java/com/ctrip/framework/apollo/config/data/extension/webclient/customizer/spi/ApolloClientWebClientCustomizerFactory.java | Changes SPI signature to avoid Spring Boot type coupling for compatibility. |
| apollo-client-config-data/src/main/java/com/ctrip/framework/apollo/config/data/extension/webclient/ApolloWebClientHttpClient.java | Adds reflective status code resolution for WebFlux 5/6/7 compatibility. |
| apollo-client-config-data/src/main/java/com/ctrip/framework/apollo/config/data/extension/webclient/ApolloClientLongPollingExtensionInitializer.java | Adapts customizer application to new SPI type. |
| apollo-client-config-data/src/main/java/com/ctrip/framework/apollo/config/data/extension/initialize/ApolloClientPropertiesFactory.java | Removes unused OAuth2 properties binding from factory. |
| apollo-client-config-data/pom.xml | Adds test deps for mockserver and reactor-netty HTTP. |
| CHANGES.md | Adds changelog entry for the compatibility coverage overhaul. |
| .github/workflows/build.yml | Splits CI into compile, unit/integration, and compat matrix jobs across Spring/Spring Boot versions. |
Comments suppressed due to low confidence (11)
apollo-plugin/apollo-plugin-log4j2/src/test/java/com/ctrip/framework/apollo/plugin/log4j2/ApolloClientConfigurationFactoryTest.java:1
- Relying on
@FixMethodOrderplus numbered test names indicates shared global state leaking between tests (System properties, Apollo static caches). This makes the suite more fragile if additional tests are added/renamed. Prefer isolating each test by fully resetting/restoring the modified system properties within each test (or in@After) so tests can run in any order without enforced sorting.
apollo-plugin/apollo-plugin-log4j2/src/test/java/com/ctrip/framework/apollo/plugin/log4j2/ApolloClientConfigurationFactoryTest.java:1 - Relying on
@FixMethodOrderplus numbered test names indicates shared global state leaking between tests (System properties, Apollo static caches). This makes the suite more fragile if additional tests are added/renamed. Prefer isolating each test by fully resetting/restoring the modified system properties within each test (or in@After) so tests can run in any order without enforced sorting.
apollo-plugin/apollo-plugin-log4j2/src/test/java/com/ctrip/framework/apollo/plugin/log4j2/ApolloClientConfigurationFactoryTest.java:1 - Relying on
@FixMethodOrderplus numbered test names indicates shared global state leaking between tests (System properties, Apollo static caches). This makes the suite more fragile if additional tests are added/renamed. Prefer isolating each test by fully resetting/restoring the modified system properties within each test (or in@After) so tests can run in any order without enforced sorting.
apollo-plugin/apollo-plugin-log4j2/src/test/java/com/ctrip/framework/apollo/plugin/log4j2/ApolloClientConfigurationFactoryTest.java:1 - Relying on
@FixMethodOrderplus numbered test names indicates shared global state leaking between tests (System properties, Apollo static caches). This makes the suite more fragile if additional tests are added/renamed. Prefer isolating each test by fully resetting/restoring the modified system properties within each test (or in@After) so tests can run in any order without enforced sorting.
apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerApiTest.java:1 - This polling loop with
Thread.sleepcan be flaky and slow under CI load. Consider extracting a shared 'await until' helper (or using an await utility already used elsewhere in the repo) that provides better diagnostics on timeout and avoids duplicating time/deadline logic across tests.
apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java:1 - Using the raw
ApplicationListenertype loses type-safety and can hide accidental event handling issues. Preferimplements ApplicationListener<ApplicationEvent>(or the specific event type if feasible) to ensure compile-time checking and clearer intent.
apollo-client/src/test/java/com/ctrip/framework/apollo/MockedConfigService.java:262 - Dropping
implements AutoCloseablereduces ergonomics and can break existing try-with-resources usage in tests. Since this class manages an external resource (MockServer), consider keepingimplements AutoCloseableand retaining an@Override close()to ensure consistent lifecycle management patterns.
public void close() {
if (this.server.isRunning()) {
this.server.stop();
}
}
apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/ApolloTestingServer.java:1
- This assumes the reflected fields are always non-null and of the expected concrete types; if any field is null (or changes shape across versions), the code will throw (e.g., NPE/ClassCastException) and the catch-all in
resetApolloClientStatewill silently skip the reset, potentially causing cascading test failures. Consider null-checking each retrieved field before clearing and handling unexpected field types with a targeted/loggable fallback to keep the reset reliable across versions.
apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/EmbeddedApollo.java:1 - In Javadoc, 'existed' is grammatically incorrect here; use 'existing' (or 'already existing') to read naturally.
apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/EmbeddedApollo.java:1 - In Javadoc, 'existed' is grammatically incorrect here; use 'existing' (or 'already existing') to read naturally.
apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/EmbeddedApollo.java:1 - In Javadoc, 'existed' is grammatically incorrect here; use 'existing' (or 'already existing') to read naturally.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @author vdisk <vdisk@foxmail.com> | ||
| */ | ||
| public interface ApolloClientWebClientCustomizerFactory extends Ordered { |
There was a problem hiding this comment.
Changing this SPI method signature (from WebClientCustomizer to Consumer<WebClient.Builder>) is a breaking change for external implementers and will also break binary compatibility for existing service providers. Consider keeping backward compatibility by either (a) adding a new method with the Consumer<WebClient.Builder> return type while retaining the old method as a default/deprecated bridge, or (b) introducing a new SPI interface name and supporting both via ServiceBootstrap.loadAllOrdered(...).
There was a problem hiding this comment.
Addressed with a compatibility bridge. ApolloClientWebClientCustomizerFactory now keeps the new Consumer-based API while providing a default reflective adapter path for legacy implementations compiled against the old WebClientCustomizer signature. Included in commit 264b7bb.
There was a problem hiding this comment.
Follow-up update: I removed the interface-level compatibility bridge and restored a simple SPI method signature. We re-checked current usage and there is no in-repo legacy implementation depending on the old return type, so keeping the interface straightforward is preferred here.
| Consumer<WebClient.Builder> createWebClientCustomizer(ApolloClientProperties apolloClientProperties, | ||
| Binder binder, BindHandler bindHandler, Log log, | ||
| Object bootstrapContext); |
There was a problem hiding this comment.
Changing this SPI method signature (from WebClientCustomizer to Consumer<WebClient.Builder>) is a breaking change for external implementers and will also break binary compatibility for existing service providers. Consider keeping backward compatibility by either (a) adding a new method with the Consumer<WebClient.Builder> return type while retaining the old method as a default/deprecated bridge, or (b) introducing a new SPI interface name and supporting both via ServiceBootstrap.loadAllOrdered(...).
There was a problem hiding this comment.
Addressed with the same compatibility bridge as above: default method + reflective adaptation for legacy return type, while preserving the new Boot-4-compatible consumer model. Included in commit 264b7bb.
There was a problem hiding this comment.
Addressed in the same way as above: the reflective adapter/bridge has been removed, and the SPI is now kept minimal to reduce complexity and maintenance cost.
| private int resolveStatusCode(Object clientResponse) { | ||
| try { | ||
| Method statusCodeMethod = ClientResponse.class.getMethod("statusCode"); | ||
| Object statusCode = statusCodeMethod.invoke(clientResponse); |
There was a problem hiding this comment.
This does reflective method lookups (getMethod) on every request, which can be expensive on hot paths. Cache the resolved Method instances (or use MethodHandles) in static final fields and only do the reflective lookup once, while keeping the same cross-version compatibility behavior.
There was a problem hiding this comment.
Applied. Reflection method lookups are now cached (statusCode() method and per-status-class value() methods) to avoid repeated lookup on every request. Included in commit 264b7bb.
| Method valueMethod = statusCode.getClass().getMethod("value"); | ||
| Object value = valueMethod.invoke(statusCode); | ||
| return ((Number) value).intValue(); | ||
| } catch (Exception ex) { | ||
| throw new ApolloConfigException("Failed to resolve response status code", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
This does reflective method lookups (getMethod) on every request, which can be expensive on hot paths. Cache the resolved Method instances (or use MethodHandles) in static final fields and only do the reflective lookup once, while keeping the same cross-version compatibility behavior.
There was a problem hiding this comment.
Applied. Same fix as above: reflective method resolution in resolveStatusCode is now cached instead of looked up per call. Included in commit 264b7bb.
| String appId = StringUtils.defaultIfBlank(annotation.appId(), configUtil.getAppId()); | ||
| String resolvedAppId = resolveAppId(appId); |
There was a problem hiding this comment.
resolveAppId can return null, and this code will then call ConfigService.getConfig(null, namespace), which can lead to incorrect behavior or a downstream failure that’s harder to diagnose. Consider making resolveAppId either: (1) always return a non-null appId by falling back to configUtil.getAppId() and validating it, or (2) throw a clear exception when the resolved appId is null/blank so the failure is immediate and actionable.
There was a problem hiding this comment.
Keeping this behavior intentionally. resolveAppId was introduced to avoid placeholder-resolution failure on null input; passing a null appId through ConfigService is existing behavior when app.id is absent and is used in current non-app-id runtime paths. I did not change this semantic in this PR.
| for (String namespace : resolvedNamespaces) { | ||
| Config config = ConfigService.getConfig(appId, namespace); | ||
| Config config = ConfigService.getConfig(resolvedAppId, namespace); |
There was a problem hiding this comment.
resolveAppId can return null, and this code will then call ConfigService.getConfig(null, namespace), which can lead to incorrect behavior or a downstream failure that’s harder to diagnose. Consider making resolveAppId either: (1) always return a non-null appId by falling back to configUtil.getAppId() and validating it, or (2) throw a clear exception when the resolved appId is null/blank so the failure is immediate and actionable.
There was a problem hiding this comment.
Same rationale as above: this PR keeps existing null-appId runtime semantics and only hardens placeholder resolution. No functional change to null-appId config lookup behavior was made intentionally.
| * @author wxq | ||
| */ | ||
| public class MockedConfigService implements AutoCloseable { | ||
| public class MockedConfigService { |
There was a problem hiding this comment.
Dropping implements AutoCloseable reduces ergonomics and can break existing try-with-resources usage in tests. Since this class manages an external resource (MockServer), consider keeping implements AutoCloseable and retaining an @Override close() to ensure consistent lifecycle management patterns.
There was a problem hiding this comment.
Applied. MockedConfigService now implements AutoCloseable again and close() is annotated with @Override, so try-with-resources style remains supported. Included in commit 264b7bb.
|
Note: this PR was completed end-to-end by Codex, including implementation, test updates/execution, and PR preparation/iteration. |
1 participant
What's the purpose of this PR
This PR migrates the manual
apollo-demo-javarelease validation path intoapollo-javaautomated coverage, and consolidates the new compatibility testing strategy into repository-native CI.The goal is to make release confidence rely on automated UT/IT/compat checks instead of manual demo regression.
Which issue(s) this PR fixes:
Fixes #N/A (test and CI coverage enhancement)
Brief changelog
apollo-clientandapollo-client-config-datafor:properties/yaml/xml)apollo-compat-tests:apollo-api-compat-itapollo-spring-compat-it(Spring3.1.1.RELEASEand6.1.18via matrix)apollo-spring-boot-compat-it(Spring Boot2.7.18/3.3.10/4.0.0via matrix)Run unit and integration testsin CICHANGES.mdwith PR link entry as requiredFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Tests
Chores