[codex] Migrate portal item config UI to OpenAPI#5610
Conversation
📝 WalkthroughWalkthroughMigrates Apollo Portal item UI flows to OpenAPI v1: adds ChangesItem Management OpenAPI Layer
Sequence DiagramsequenceDiagram
participant ItemController
participant ServerItemOpenApiService
participant ItemService
participant Database
ItemController->>ServerItemOpenApiService: create/update/remove/compare/sync request (Open DTO + operator)
ServerItemOpenApiService->>ItemService: convert and delegate (ItemDTO / Namespace models)
ItemService->>Database: persist/query/delete items
ServerItemOpenApiService->>ItemController: return Open DTO / diffs / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
Pull request overview
This PR continues the Apollo Portal OpenAPI migration by moving Config Item (Item domain) backend endpoints and Portal UI item operations onto the generated OpenAPI v1 contract, while preserving compatibility behaviors (operator resolution, legacy DTO shapes, diff/sync/revoke semantics) for existing clients and UI consumers.
Changes:
- Portal backend:
ItemControllernow implements the generatedItemManagementApi, delegates to a portal-localItemOpenApiService, and adds generated/legacy DTO conversion + compatibility logic (encoded keys, diff/sync/revoke, hidden-config behavior, syntax check). - Portal UI + e2e: Config item CRUD/diff/sync/validation/revocation calls are switched to
/openapi/v1/..., and Playwright helpers/smoke coverage is extended to ensure item delete/revoke flows hit OpenAPI endpoints. - Tests/docs: Adds backend tests around parameter binding/operator resolution/encoded keys/hidden-config handling; updates OpenAPI migration tracking and URL inventory docs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/portal-e2e/tests/portal-core.spec.js | Adds smoke coverage for item delete + revoke flows using OpenAPI endpoints. |
| e2e/portal-e2e/tests/helpers/portal-helpers.js | Updates e2e wait-for-response helpers to match new /openapi/v1/... item endpoints and adds delete/revoke UI helpers. |
| docs/zh/contribution/apollo-portal-openapi-migration-tracking.md | Updates migration status narrative and Item domain progress notes. |
| docs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.md | Updates URL inventory counts and marks ConfigService item URLs as migrated to OpenAPI. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerParamBindLowLevelTest.java | Adds MockMvc tests for OpenAPI v1 item controller parameter binding and operator rules. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java | Adds unit tests for the portal-local Item OpenAPI service conversions/delegation behavior. |
| apollo-portal/src/main/resources/static/scripts/services/ConfigService.js | Switches item operations to OpenAPI v1 endpoints and adapts paging/diff shapes for legacy UI expectations. |
| apollo-portal/src/main/resources/static/scripts/controller/config/SyncConfigController.js | Updates diff/sync calls to pass the now-required env/cluster/appId parameters for OpenAPI routes. |
| apollo-portal/src/main/resources/static/scripts/controller/config/ConfigNamespaceController.js | Updates delete-item flow to delete by key (encoded-key endpoint) instead of legacy itemId endpoint. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java | Implements generated ItemManagementApi and centralizes compatibility logic (operator resolution, encoded keys, sync/diff/revoke, syntax check). |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java | Adds converters between legacy OpenAPI DTOs / portal VO diffs and generated OpenAPI v1 DTOs. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java | Implements portal-local Item OpenAPI service using generated models while delegating to existing portal services. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java | Introduces portal-local Item OpenAPI service interface based on generated model contracts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
apollo-portal/src/main/resources/static/scripts/services/ConfigService.js (1)
155-169: ⚡ Quick winAdd an empty-page stop condition in paged fetch.
At Line 165, pagination continues only by comparing fetched count vs
total. If a page returns emptycontentwhiletotalis still larger, this can recurse indefinitely and spam requests.Proposed patch
var fetchPage = function (page) { config_source.find_items({ appId: appId, env: env, clusterName: clusterName, namespaceName: namespaceName, page: page, size: OPENAPI_ITEM_PAGE_SIZE }, function (result) { - items = items.concat(result.content || []); - if (items.length < (result.total || 0)) { + var pageItems = result.content || []; + var total = result.total || 0; + items = items.concat(pageItems); + if (pageItems.length > 0 && items.length < total) { fetchPage(page + 1); return; } d.resolve(sortItems(items, orderBy)); }, function (result) {🤖 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-portal/src/main/resources/static/scripts/services/ConfigService.js` around lines 155 - 169, The paged fetch in fetchPage keeps recursing if a page returns empty content while result.total remains larger; modify the callback for config_source.find_items inside fetchPage to treat an empty or missing result.content as a terminal page (e.g., if (!result.content || result.content.length === 0) then stop further fetches and proceed to d.resolve(sortItems(items, orderBy))). Keep the existing items.concat and total check but add this empty-page guard before recursing so you don't call fetchPage(page + 1) infinitely; reference fetchPage, config_source.find_items, items, result.content, result.total, OPENAPI_ITEM_PAGE_SIZE and sortItems when applying the change.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java (1)
187-197: ⚡ Quick winAdd test coverage for the update path of
createOrUpdateItem.The existing test only verifies the create path (when
loadItemreturns null). Consider adding a test case that verifies the update path when an existing item is found, ensuring it delegates toupdateItemwith the correct parameters and preserves identity fields.🧪 Suggested test case
`@Test` void createOrUpdateItemShouldUpdateWhenExistingItemIsPresent() { ItemDTO existing = item("timeout", "100"); existing.setId(99); existing.setNamespaceId(10); when(itemService.loadItem(Env.valueOf(ENV), APP_ID, CLUSTER, NAMESPACE, "timeout")) .thenReturn(existing); OpenItemDTO request = openItem("timeout", "200"); service.createOrUpdateItem(APP_ID, ENV, CLUSTER, NAMESPACE, request, "operator"); ArgumentCaptor<ItemDTO> captor = ArgumentCaptor.forClass(ItemDTO.class); verify(itemService).updateItem(eq(APP_ID), eq(Env.valueOf(ENV)), eq(CLUSTER), eq(NAMESPACE), captor.capture()); assertThat(captor.getValue().getId()).isEqualTo(99); assertThat(captor.getValue().getValue()).isEqualTo("200"); }🤖 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-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java` around lines 187 - 197, Add a new unit test in ServerItemOpenApiServiceTest to cover the update path of createOrUpdateItem: arrange itemService.loadItem(...) to return an existing ItemDTO with id and namespaceId set, call service.createOrUpdateItem(...) with an OpenItemDTO that changes the value, then verify itemService.updateItem(...) was invoked (use ArgumentCaptor<ItemDTO> to capture the argument) and assert the captured ItemDTO preserves the original id and namespaceId while its value equals the updated value; reference createOrUpdateItem, itemService.updateItem, ItemDTO, OpenItemDTO and the test class ServerItemOpenApiServiceTest when adding the test.
🤖 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-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java`:
- Around line 93-101: The current createOrUpdateItem uses a check-then-act via
itemService.loadItem and races on concurrent creates; make it atomic by removing
the pre-check and instead try creating first (call createItem) and on a
duplicate-key/conflict exception fall back to updateItem, or alternatively
add/use an atomic upsert method on itemService (e.g.,
itemService.createOrUpdateAtomic) that encapsulates the create-or-update logic.
Update createOrUpdateItem to catch the specific duplicate/conflict exception
thrown by your persistence layer (or
DataIntegrityViolation/DuplicateKeyException) and call updateItem when that
occurs; do not rely on loadItem for the decision. Ensure createItem and
updateItem signatures remain unchanged so callers still work.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java`:
- Around line 80-90: getItem and getItemByEncodedKey currently return raw items
and bypass the hidden-config suppression that
findItemsByNamespace/findBranchItems apply; update getItem (and thus
getItemByEncodedKey which delegates to it) to invoke the same hidden-config
guard/masking logic used for list reads (reuse the suppression helper or the
service method that masks USER identity values) so single-item reads also have
sensitive values suppressed for USER identities — locate getItem,
getItemByEncodedKey and the masking/suppression logic in itemOpenApiService (or
the helper used by findItemsByNamespace/findBranchItems) and apply it to the
returned OpenItemDTO.
- Around line 327-330: The validation in isInvalid(OpenNamespaceSyncDTO model)
incorrectly treats an empty syncItems list as invalid; update the check so that
model.getSyncItems() may be an empty list (but still reject null), i.e. keep the
null check for model and syncToNamespaces and the emptiness check for
syncToNamespaces, but remove the .isEmpty() check for model.getSyncItems() so
empty lists are accepted for delete-all diffs/propagation.
- Around line 372-374: The decodeBase64 method currently lets
Base64.getDecoder().decode(...) throw IllegalArgumentException which bubbles up
and becomes a 500; catch IllegalArgumentException inside decodeBase64 (or in the
calling path within ItemController) and translate it into a 400 client error by
throwing a request-validation exception (e.g.,
ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid base64 key") or a
custom BadRequestException) so malformed encoded keys produce HTTP 400 instead
of 500; update the method signature/usage of decodeBase64 accordingly to
propagate the new exception type.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java`:
- Around line 49-50: Add a concise class-level Javadoc to
ServerItemOpenApiServiceTest that summarizes the test class purpose (e.g., what
service it tests and the scope of tests), placed immediately above the class
declaration for ServerItemOpenApiServiceTest; ensure it follows project Javadoc
style (one-sentence description, optional `@see` or `@author` if required by
conventions).
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerParamBindLowLevelTest.java`:
- Around line 45-48: This test class (ItemControllerParamBindLowLevelTest) is
written with JUnit4 imports/annotations; replace org.junit.Before,
org.junit.After, org.junit.Test and org.junit.runner.RunWith usage with JUnit5
equivalents: import org.junit.jupiter.api.BeforeEach,
org.junit.jupiter.api.AfterEach, org.junit.jupiter.api.Test and remove
`@RunWith`(SpringRunner.class); add
org.springframework.test.context.junit.jupiter.SpringExtension and annotate the
class with `@ExtendWith`(SpringExtension.class). Update any lifecycle method
names/annotations (e.g., methods annotated with `@Before/`@After →
`@BeforeEach/`@AfterEach) and ensure imports reference junit.jupiter packages so
the class uses JUnit5 while allowing Vintage for legacy tests.
- Around line 61-64: Add a short class-level Javadoc above the
ItemControllerParamBindLowLevelTest class that briefly describes the test
class's purpose (e.g., what aspect of ItemController parameter binding it
verifies and why it's "low level"), mentioning any important context if needed;
place this Javadoc immediately above the `public class
ItemControllerParamBindLowLevelTest` declaration and keep it concise following
project guidelines.
---
Nitpick comments:
In `@apollo-portal/src/main/resources/static/scripts/services/ConfigService.js`:
- Around line 155-169: The paged fetch in fetchPage keeps recursing if a page
returns empty content while result.total remains larger; modify the callback for
config_source.find_items inside fetchPage to treat an empty or missing
result.content as a terminal page (e.g., if (!result.content ||
result.content.length === 0) then stop further fetches and proceed to
d.resolve(sortItems(items, orderBy))). Keep the existing items.concat and total
check but add this empty-page guard before recursing so you don't call
fetchPage(page + 1) infinitely; reference fetchPage, config_source.find_items,
items, result.content, result.total, OPENAPI_ITEM_PAGE_SIZE and sortItems when
applying the change.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java`:
- Around line 187-197: Add a new unit test in ServerItemOpenApiServiceTest to
cover the update path of createOrUpdateItem: arrange itemService.loadItem(...)
to return an existing ItemDTO with id and namespaceId set, call
service.createOrUpdateItem(...) with an OpenItemDTO that changes the value, then
verify itemService.updateItem(...) was invoked (use ArgumentCaptor<ItemDTO> to
capture the argument) and assert the captured ItemDTO preserves the original id
and namespaceId while its value equals the updated value; reference
createOrUpdateItem, itemService.updateItem, ItemDTO, OpenItemDTO and the test
class ServerItemOpenApiServiceTest when adding the test.
🪄 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: ce705ac0-4343-4c1e-9e85-7e5178b52697
📒 Files selected for processing (13)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.javaapollo-portal/src/main/resources/static/scripts/controller/config/ConfigNamespaceController.jsapollo-portal/src/main/resources/static/scripts/controller/config/SyncConfigController.jsapollo-portal/src/main/resources/static/scripts/services/ConfigService.jsapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerParamBindLowLevelTest.javadocs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.mddocs/zh/contribution/apollo-portal-openapi-migration-tracking.mde2e/portal-e2e/tests/helpers/portal-helpers.jse2e/portal-e2e/tests/portal-core.spec.js
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b7dc6fcfb
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java (1)
59-68: ⚡ Quick winAdd Javadoc for the nested YAML factory helper.
TypeLimitedYamlPropertiesFactoryBeanis a new class but has no class-level Javadoc. Please add a brief description of why this helper exists.As per coding guidelines, "New Java classes should include a short Javadoc describing the class purpose".
🤖 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-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java` around lines 59 - 68, Add a class-level Javadoc for the nested helper TypeLimitedYamlPropertiesFactoryBean explaining its purpose: that it customizes YAML parsing by creating a Yaml instance with strict LoaderOptions (disallowing duplicate keys) and specific DumperOptions to enforce type-limited parsing for namespace text validation; mention that it overrides createYaml() to supply the SafeConstructor/Representer with those options so future maintainers understand why this factory exists and how it affects YAML loading.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java (1)
164-172: 💤 Low valueThe unused
clusterNameparameter appears intentional but warrants clarification.In this branch-items endpoint,
clusterNameis part of the method signature (required by theItemManagementApiinterface) but is not used. The permission check and service call both usebranchNameinstead:// Line 167: uses branchName, not clusterName if (shouldHideConfigToCurrentUser(appId, env, branchName, namespaceName)) { // ... } // Line 171: doesn't pass clusterName to service return ResponseEntity.ok(this.itemOpenApiService.findBranchItems(appId, env, branchName, namespaceName));The underlying
ItemOpenApiService.findBranchItemsmethod (lines 49-50) only accepts four parameters and does not includeclusterName. This design assumes that for branch-specific operations, the parent cluster is implicit in the branch itself.While this appears to match the pattern used elsewhere (e.g.,
ReleaseControlleralso treatsbranchNameas the effective cluster), the unused parameter should be either documented with a comment explaining why it's not validated against the branch's parent cluster, or removed from the signature if the OpenAPI spec allows it.🤖 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-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java` around lines 164 - 172, The method findBranchItems has an unused clusterName parameter (required by the ItemManagementApi) but neither the permission check shouldHideConfigToCurrentUser(...) nor the service call itemOpenApiService.findBranchItems(...) reference it; either annotate/document this choice or remove the parameter if the API contract permits: update findBranchItems to add a clear comment explaining that branchName implicitly determines its parent cluster and clusterName is intentionally ignored (referencing findBranchItems, shouldHideConfigToCurrentUser, and itemOpenApiService.findBranchItems), or if the OpenAPI/interface allows, remove clusterName from the method signature and adjust the interface and callers accordingly so the code no longer contains an unused parameter.
🤖 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-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java`:
- Around line 164-172: The method findBranchItems has an unused clusterName
parameter (required by the ItemManagementApi) but neither the permission check
shouldHideConfigToCurrentUser(...) nor the service call
itemOpenApiService.findBranchItems(...) reference it; either annotate/document
this choice or remove the parameter if the API contract permits: update
findBranchItems to add a clear comment explaining that branchName implicitly
determines its parent cluster and clusterName is intentionally ignored
(referencing findBranchItems, shouldHideConfigToCurrentUser, and
itemOpenApiService.findBranchItems), or if the OpenAPI/interface allows, remove
clusterName from the method signature and adjust the interface and callers
accordingly so the code no longer contains an unused parameter.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java`:
- Around line 59-68: Add a class-level Javadoc for the nested helper
TypeLimitedYamlPropertiesFactoryBean explaining its purpose: that it customizes
YAML parsing by creating a Yaml instance with strict LoaderOptions (disallowing
duplicate keys) and specific DumperOptions to enforce type-limited parsing for
namespace text validation; mention that it overrides createYaml() to supply the
SafeConstructor/Representer with those options so future maintainers understand
why this factory exists and how it affects YAML loading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fed3080-97ab-4a7a-a282-79816ba432e6
📒 Files selected for processing (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.javaapollo-portal/src/main/resources/static/scripts/services/ConfigService.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/resources/static/scripts/services/ConfigService.js
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ 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". |
What's the purpose of this PR
Continue the Apollo Portal OpenAPI migration by moving the Config Item backend and Portal UI item operations onto the generated OpenAPI v1 contract while preserving compatibility for existing OpenAPI clients.
Which issue(s) this PR fixes:
N/A
Brief changelog
ItemControllerimplement the generatedItemManagementApiand delegate item operations through a portal-local OpenAPI service./openapi/v1/..., with adapters for list, diff, delete, sync, and revoke behavior.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.Compatibility
This PR does not change the published
apollo-openapispec. The implementation targetsapollo-openapiv0.2.0 generated interfaces and preserves the existing apollo-java OpenAPI client item paths and payload/query operator behavior.Validation
python3 scripts/openapi/check_openapi_compatibility_test.pypython3 scripts/openapi/check_openapi_compatibility.py --base https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.2.0/apollo-openapi.yaml --head /Users/jason/git/mine/apollo-openapi/apollo-openapi.yaml./mvnw -B -pl apollo-portal -am -DskipTests -Dapollo.openapi.spec.url=/Users/jason/git/mine/apollo-openapi/apollo-openapi.yaml compile./mvnw -Dapollo.openapi.spec.url=/Users/jason/git/mine/apollo-openapi/apollo-openapi.yaml clean test./mvnw spotless:apply && git diff --check.github/workflows/portal-ui-e2e.yml: buildapollo-assembly, start the jar with H2/auth env, wait for readiness, then run the portal Playwright CI suite.BASE_URL=http://127.0.0.1:8070 PLAYWRIGHT_WORKERS=1 npx playwright test tests/portal-core.spec.js --project=chromium --reporter=list(7 passed).Summary by CodeRabbit
New Features
Improvements
Tests
Docs