fix: ConfigService.getConfig(appId, namespace) returns wrong app's config#140
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a new public ChangesAppId-aware ConfigFile retrieval
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
============================================
+ Coverage 68.68% 70.94% +2.26%
- Complexity 1503 1635 +132
============================================
Files 212 224 +12
Lines 6396 6733 +337
Branches 647 680 +33
============================================
+ Hits 4393 4777 +384
+ Misses 1673 1603 -70
- Partials 330 353 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java`:
- Around line 104-115: Update the Javadoc for ConfigService.getConfigFile:
clarify that the namespace parameter should NOT include a file extension (e.g.,
use "application" not "application.yml") because the extension is derived from
the configFileFormat and appended internally by DefaultConfigManager; reference
ConfigService.getConfigFile, the namespace parameter, and ConfigFileFormat (and
DefaultConfigManager behavior) in the comment so callers know to pass just the
base namespace name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af07b96b-693d-4836-af0e-931a7384e731
📒 Files selected for processing (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.javaapollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.javaapollo-client/src/test/java/com/ctrip/framework/apollo/ConfigServiceTest.java
Shawyeok
force-pushed
the
fix/appid-leaked-in-properties-compatible-config
branch
from
2a1096e to
c7d2dac
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java (1)
41-55: Discarded namespaces aren’t a reliability problem in the current test
waitForNamespace(...)consumes thenamespacesqueue viapoll(...)and discards non-matching values, but in this module theSpringApolloEventListenerProbeis only consumed once (waitForNamespace("application", ...)inSpringAnnotationCompatibilityTest), and there are no other usages of the probe (no otherwaitForNamespace/pollNamespacecalls). Discarding won’t affect later assertions here.
If you later reuse the same probe instance to wait for multiple different namespaces, consider a non-destructive approach (e.g., retain unmatched values).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java` around lines 41 - 55, waitForNamespace in SpringApolloEventListenerProbe currently polls and discards non-matching entries from the namespaces queue, which may lose events if the probe is reused; change the logic in waitForNamespace (and any helper like namespaces queue handling) to be non-destructive: peek the head and only poll when it matches expectedNamespace (or temporarily buffer unmatched values and requeue them if needed) so unmatched namespace values are retained for subsequent waits (reference waitForNamespace, namespaces, and SpringApolloEventListenerProbe to locate the code; consider SpringAnnotationCompatibilityTest which calls waitForNamespace("application", ...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java`:
- Around line 41-55: waitForNamespace in SpringApolloEventListenerProbe
currently polls and discards non-matching entries from the namespaces queue,
which may lose events if the probe is reused; change the logic in
waitForNamespace (and any helper like namespaces queue handling) to be
non-destructive: peek the head and only poll when it matches expectedNamespace
(or temporarily buffer unmatched values and requeue them if needed) so unmatched
namespace values are retained for subsequent waits (reference waitForNamespace,
namespaces, and SpringApolloEventListenerProbe to locate the code; consider
SpringAnnotationCompatibilityTest which calls waitForNamespace("application",
...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85763920-8d5b-434f-9070-533887030288
📒 Files selected for processing (2)
apollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringAnnotationCompatibilityTest.javaapollo-compat-tests/apollo-spring-compat-it/src/test/java/com/ctrip/framework/apollo/compat/spring/SpringApolloEventListenerProbe.java
|
Thanks for the fix. I checked the latest head ( I do not see a blocking issue from this pass. A maintainer can do the final approval/merge decision. Note: this reply was generated and posted automatically by AI for initial triage; a maintainer will follow up if needed. |
@nobodyiam Do you time to take a look about this issue? |
…tory for non-default appId DefaultConfigFactory.createPropertiesCompatibleFileConfigRepository() received an appId parameter but called ConfigService.getConfigFile(namespace, format) — the two-arg overload that ignores appId and resolves against the default app.id from app.properties. Fixes the bug by: 1. Adding ConfigService.getConfigFile(appId, namespace, format) that delegates to the already-correct ConfigManager.getConfigFile(appId, namespace, format). 2. Updating DefaultConfigFactory to call the new three-arg overload so the caller-specified appId is preserved. Adds tests: - DefaultConfigFactoryTest.testCreatePropertiesCompatibleFileConfigRepositoryForwardsCustomAppId: verifies ConfigManager is invoked with the supplied appId, never the default. - ConfigServiceTest.testGetConfigFileWithCustomAppId: verifies the new ConfigService.getConfigFile(appId, ns, format) overload returns a ConfigFile whose getAppId() equals the requested appId. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nobodyiam
force-pushed
the
fix/appid-leaked-in-properties-compatible-config
branch
from
f6110d1 to
81cab3c
Compare
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
Thanks for the fix. The implementation direction looks right, but please add one regression test for the actual non-properties path before approval.
ConfigServiceTest.testGetConfigWithCustomAppId currently uses a properties namespace ("mock"), and testGetConfigFileWithCustomAppId calls the new getConfigFile(appId, namespace, format) overload directly. Please add a test for ConfigService.getConfig(customAppId, "mock.yml") or an equivalent DefaultConfigFactory path, so the test would fail if DefaultConfigFactory.createPropertiesCompatibleFileConfigRepository(...) drops the custom appId again.
|
Thanks for the review @nobodyiam. Added |
…amespace Add ConfigServiceTest.testGetConfigWithCustomAppIdForPropertiesCompatibleNamespace, which drives the real DefaultConfigFactory path (create -> createPropertiesCompatibleFileConfigRepository -> ConfigService.getConfigFile(appId, namespace, format)) for a .yml namespace. The existing custom-appId tests either used a properties namespace or called the new getConfigFile overload directly, so neither would catch DefaultConfigFactory.createPropertiesCompatibleFileConfigRepository dropping the custom appId again. The new test stubs only createConfigFile and echoes the received appId into the resulting Config, so it fails if the appId is dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Shawyeok
force-pushed
the
fix/appid-leaked-in-properties-compatible-config
branch
from
f8722b2 to
25b8bde
Compare
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
Thanks for the update. The new regression test covers the requested non-properties path by calling ConfigService.getConfig(customAppId, "mock.yml") and exercising the real DefaultConfigFactory path while only stubbing createConfigFile.
The current checks are green, and I do not see any remaining blocking issues from this pass.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 3 minutes 18 seconds in the queue, including 3 minutes 3 seconds running CI. Required conditions to merge
|
2 participants
Problem
ConfigService.getConfig(appId, "some-namespace.yml")silently returns config belonging to the default application (app.idinapp.properties) instead of the requestedappId, with no error or warning.The same bug affects any non-properties namespace format (YAML, YML, JSON, XML, TXT).
Root cause
DefaultConfigFactory.create(appId, namespace)correctly dispatches tocreatePropertiesCompatibleFileConfigRepository(appId, namespace, format)for non-properties namespaces, but that method then calls the two-argConfigService.getConfigFile(namespace, format)— silently droppingappId:ConfigManageralready had a correct three-arggetConfigFile(appId, namespace, format)overload, but it was not exposed onConfigService.Fix
ConfigService.java— add the missing overload that mirrors the existinggetConfig(appId, namespace)pattern:DefaultConfigFactory.java— use the new overload soappIdis preserved:Tests
| Test | What it verifies |
|------|-----------------||
ConfigServiceTest.testGetConfigWithCustomAppId|ConfigService.getConfig(appId, namespace)returns aConfigwhose property values reflect the requested appId, not the default ||
ConfigServiceTest.testGetConfigFileWithCustomAppId|ConfigService.getConfigFile(appId, ns, format)returns aConfigFilewhosegetAppId()matches the requested appId |Affected versions
Reproducible on
apollo-client 2.4.0and2.5.0.ConfigFileFormat.Propertiesnamespaces are unaffected (they go throughcreateConfigRepository, notcreatePropertiesCompatibleFileConfigRepository).Summary by CodeRabbit
New Features
Tests