新建应用时按环境自动创建并启用 AccessKey（Auto-provision）#5589
新建应用时按环境自动创建并启用 AccessKey（Auto-provision）#5589youngzil wants to merge 3 commits intoapolloconfig:masterfrom
Conversation
dosubot
bot
added
the
size:L
📝 WalkthroughWalkthroughAdds an optional per-environment config Changes
Sequence Diagram(s)sequenceDiagram
participant Portal
participant AdminService
participant BizConfig
participant AccessKeyService
rect rgba(0,128,0,0.5)
Portal->>AdminService: POST /apps (createNewApp)
end
AdminService->>BizConfig: isAccessKeyAutoProvisionEnabled()
BizConfig-->>AdminService: true
rect rgba(0,0,255,0.5)
AdminService->>AccessKeyService: createAccessKey(appId, AccessKey{secret=generateId(), mode=FILTER, enabled=true, createdBy=operator})
AccessKeyService-->>AdminService: AccessKey(created) / throws
end
AdminService-->>Portal: respond with created App (success regardless of key creation failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/UniqueKeyGenerator.java (1)
62-64: Consider usingreplaceinstead ofreplaceAllfor better performance.Since the replacement pattern is a literal string (not a regex),
replace("-", "")is slightly more efficient thanreplaceAll("-", "")as it avoids regex compilation overhead.Additionally, consider adding a brief Javadoc to describe the method's purpose and the format of the returned ID (32-character hex string).
♻️ Suggested improvement
+ /** + * Generates a unique 32-character hexadecimal ID based on UUID. + * + * `@return` a hyphen-less UUID string + */ public static String generateId() { - return UUID.randomUUID().toString().replaceAll("-", ""); + return UUID.randomUUID().toString().replace("-", ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/UniqueKeyGenerator.java` around lines 62 - 64, Change UniqueKeyGenerator.generateId to use String.replace instead of replaceAll to avoid regex overhead, i.e., replace("-", "") rather than replaceAll("-", ""); also add a brief Javadoc on the generateId method that states it returns a 32-character hex-like UUID string with hyphens removed. Ensure you update the method's comment only and keep the return behavior unchanged.apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.java (1)
91-91: Minor style suggestion: consider usingassertTruefor boolean assertions.Using
assertTrue(created.isEnabled())is slightly more idiomatic thanassertEquals(true, created.isEnabled()).♻️ Suggested change
- assertEquals(true, created.isEnabled()); + assertTrue(created.isEnabled());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.java` at line 91, In AdminServiceUnitTest replace the boolean equality assertion assertEquals(true, created.isEnabled()) with the idiomatic assertTrue(created.isEnabled()) to improve readability; update imports if needed to use org.junit.Assert.assertTrue (or the corresponding assertTrue from your test framework) and run the test to confirm no behavior change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.java`:
- Line 91: In AdminServiceUnitTest replace the boolean equality assertion
assertEquals(true, created.isEnabled()) with the idiomatic
assertTrue(created.isEnabled()) to improve readability; update imports if needed
to use org.junit.Assert.assertTrue (or the corresponding assertTrue from your
test framework) and run the test to confirm no behavior change.
In
`@apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/UniqueKeyGenerator.java`:
- Around line 62-64: Change UniqueKeyGenerator.generateId to use String.replace
instead of replaceAll to avoid regex overhead, i.e., replace("-", "") rather
than replaceAll("-", ""); also add a brief Javadoc on the generateId method that
states it returns a 32-character hex-like UUID string with hyphens removed.
Ensure you update the method's comment only and keep the return behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec2f2b00-aeee-4cba-8f65-975703819804
📒 Files selected for processing (10)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.javaapollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/AdminService.javaapollo-biz/src/test/java/com/ctrip/framework/apollo/biz/config/BizConfigTest.javaapollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.javaapollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/UniqueKeyGenerator.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.javadocs/en/deployment/distributed-deployment-guide.mddocs/en/portal/apollo-user-guide.mddocs/zh/deployment/distributed-deployment-guide.mddocs/zh/portal/apollo-user-guide.md
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c6a523158
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accessKey.setMode(AccessKeyMode.FILTER); | ||
| accessKey.setEnabled(true); | ||
| accessKey.setDataChangeCreatedBy(operator); | ||
| accessKeyService.create(appId, accessKey); |
There was a problem hiding this comment.
Calling accessKeyService.create inside AdminService#createNewApp's @Transactional block means any runtime failure during provisioning (for example BadRequestException from AccessKeyService#create when the key limit is hit) can mark the shared transaction rollback-only even though the exception is caught here. At commit time this can surface as UnexpectedRollbackException, so app creation is rolled back despite the intended "log only, do not rollback" behavior described in this change.
Useful? React with 👍 / 👎.
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
这次改动的主要阻塞点是 auto-provision 仍然在 createNewApp 的事务内执行。accessKeyService.create(...) 一旦抛出运行时异常，会把当前事务标成 rollback-only；即使这里 catch 住，最终提交时也可能把应用创建一起回滚，这和 PR/文档里“仅打日志、不回滚应用创建”的预期不一致。
请修复以下阻塞项后再更新：
- 将 AccessKey 自动创建移到应用创建成功提交之后再执行，而不是放在
AdminService#createNewApp的同一事务里；同时避免应用创建失败时留下孤儿 AccessKey。 - 增加失败分支回归测试，覆盖 auto-provision 失败时“应用仍创建成功、只记录告警”的行为。
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ControllerExceptionTest.java (1)
110-143: Consider adding a test for when auto-provisioning is disabled.The tests cover the enabled scenarios well (success and failure), but there's no test verifying that
accessKeyService.createis not called whenbizConfig.isAccessKeyAutoProvisionEnabled()returnsfalse. This would ensure the feature flag works correctly in both directions.📝 Suggested test to add
`@Test` public void createShouldNotProvisionAccessKeyWhenDisabled() { AppDTO dto = generateSampleDTOData(); App app = new App(); app.setAppId(dto.getAppId()); app.setDataChangeCreatedBy(dto.getDataChangeCreatedBy()); when(appService.findOne(any(String.class))).thenReturn(null); when(adminService.createNewApp(any(App.class))).thenReturn(app); when(bizConfig.isAccessKeyAutoProvisionEnabled()).thenReturn(false); appController.create(dto); verify(accessKeyService, never()).create(any(String.class), any(AccessKey.class)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ControllerExceptionTest.java` around lines 110 - 143, Add a new unit test in ControllerExceptionTest named createShouldNotProvisionAccessKeyWhenDisabled that mirrors the existing create tests but sets when(bizConfig.isAccessKeyAutoProvisionEnabled()).thenReturn(false); arrange AppDTO and App, stub appService.findOne(...) to return null and adminService.createNewApp(...) to return the App, call appController.create(dto), and assert that accessKeyService.create(...) was never invoked by using verify(accessKeyService, never()).create(any(String.class), any(AccessKey.class)); this verifies the feature-flag off path for auto-provisioning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ControllerExceptionTest.java`:
- Around line 110-143: Add a new unit test in ControllerExceptionTest named
createShouldNotProvisionAccessKeyWhenDisabled that mirrors the existing create
tests but sets
when(bizConfig.isAccessKeyAutoProvisionEnabled()).thenReturn(false); arrange
AppDTO and App, stub appService.findOne(...) to return null and
adminService.createNewApp(...) to return the App, call
appController.create(dto), and assert that accessKeyService.create(...) was
never invoked by using verify(accessKeyService,
never()).create(any(String.class), any(AccessKey.class)); this verifies the
feature-flag off path for auto-provisioning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84728699-f353-47c9-8fdf-efa254bd1c8f
📒 Files selected for processing (4)
apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/AppController.javaapollo-adminservice/src/test/java/com/ctrip/framework/apollo/adminservice/controller/ControllerExceptionTest.javaapollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/AdminService.javaapollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.java
✅ Files skipped from review due to trivial changes (2)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/AdminService.java
- apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/AdminServiceUnitTest.java
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
上次阻塞点里的代码修复方向已经对了，auto-provision 现在确实已经移出了 AdminService#createNewApp 的事务。
不过当前新增的失败分支测试还没有真正覆盖之前那个事务回滚风险：测试里把 adminService.createNewApp(...) 和 accessKeyService.create(...) 都 mock 掉了，所以只能证明 controller 会 catch 异常，不能证明在真实 Spring 事务边界下，access key 创建失败不会把 app 创建一起回滚。请补一个集成/事务测试：开启 apollo.access-key.auto-provision.enabled，让 accessKeyService.create(...) 抛异常，然后断言应用仍然成功创建且不会被回滚。
2 participants
What's the purpose of this PR
在 Portal 新建应用 且同步到某环境的 apollo-adminservice 时，若该环境在 ApolloConfigDB.ServerConfig 中开启 apollo.access-key.auto-provision.enabled，则为该应用在该环境 自动创建一条 AccessKey（随机 Secret、FILTER 模式、已启用）。失败仅打日志，不回滚应用创建。
Which issue(s) this PR fixes:
Fixes #5588
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.mvn spotless:applyto format your code.CHANGESlog.Summary by CodeRabbit
New Features
Bug Fixes
Docs
Tests