Skip to content

[codex] Migrate portal item config UI to OpenAPI#5610

Open
nobodyiam wants to merge 4 commits into
apolloconfig:masterfrom
nobodyiam:codex/openapi-migration-item-ui
Open

[codex] Migrate portal item config UI to OpenAPI#5610
nobodyiam wants to merge 4 commits into
apolloconfig:masterfrom
nobodyiam:codex/openapi-migration-item-ui

Conversation

@nobodyiam
Copy link
Copy Markdown
Member

@nobodyiam nobodyiam commented May 16, 2026

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

  • Make the portal OpenAPI ItemController implement the generated ItemManagementApi and delegate item operations through a portal-local OpenAPI service.
  • Add generated DTO conversion and compatibility handling for item create, update, delete, encoded keys, list, branch reads, text update, diff, sync, syntax check, and revocation.
  • Move ConfigService item operations in the Portal UI to /openapi/v1/..., with adapters for list, diff, delete, sync, and revoke behavior.
  • Add backend coverage for parameter binding, USER vs CONSUMER operator resolution, encoded keys, hidden config reads, generated DTO conversion, diff/sync/revoke, and protection behavior.
  • Extend portal e2e helpers/smoke coverage so create/update/text-modify/delete/revoke item paths wait for OpenAPI item URLs.
  • Refresh the Portal OpenAPI frontend URL inventory and migration tracking docs for Config Item/UI operations.

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

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

Compatibility

This PR does not change the published apollo-openapi spec. The implementation targets apollo-openapi v0.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.py
  • python3 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
  • Portal UI e2e per .github/workflows/portal-ui-e2e.yml: build apollo-assembly, start the jar with H2/auth env, wait for readiness, then run the portal Playwright CI suite.
  • After adding delete/revoke e2e coverage: 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

    • Item management via OpenAPI v1: create, update, delete, retrieve, paged & branch listing; batch text updates, diff/compare, sync, syntax check, and revert.
  • Improvements

    • Explicit operator handling, config-hiding for portal users, encoded-key support, unified request/response shapes, frontend switched to OpenAPI endpoints and robust client-side paging/sorting.
  • Tests

    • Added/expanded unit, controller, and E2E tests covering bindings, conversions, operator rules, Base64 key handling, and OpenAPI flows.
  • Docs

    • Updated OpenAPI migration tracking, URL inventory, and release notes.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

📝 Walkthrough

Walkthrough

Migrates Apollo Portal item UI flows to OpenAPI v1: adds ItemOpenApiService and converters, implements operator-aware server service, refactors ItemController for validation/authorization and base64 keys, updates frontend OpenAPI wiring, and adds unit and E2E tests plus docs updates.

Changes

Item Management OpenAPI Layer

Layer / File(s) Summary
OpenAPI Service Contract & DTO Converters
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java, apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
New ItemOpenApiService interface defines CRUD, paging, and sync contracts; OpenApiModelConverters adds legacy item/page/diff DTO-to-model conversion utilities.
Service Implementation: Item Lifecycle & Sync Operations
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java
ServerItemOpenApiService implements operator-aware create/update/delete with existence checks (NotFoundException), paged listing, branch queries, batch text updates, compare/diff, namespace sync, and revoke delegating to itemService.
Controller Refactor: API Adapter with Authorization & Validation
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java
Controller implements ItemManagementApi, resolves effective operator (USER vs CONSUMER), enforces payload validation and comment length, handles base64 encoded keys, hides config for USERs, performs YAML syntax checks, and routes through ItemOpenApiService.
Frontend Service & Controller Wiring to OpenAPI v1 Endpoints
apollo-portal/src/main/resources/static/scripts/services/ConfigService.js, apollo-portal/src/main/resources/static/scripts/controller/config/ConfigNamespaceController.js, apollo-portal/src/main/resources/static/scripts/controller/config/SyncConfigController.js
Frontend controllers and ConfigService now target /openapi/v1 item endpoints, add key encoding/decoding, implement paginated item fetching, map OpenAPI diffs to legacy shapes, and expand diff/sync call contexts.
Service Layer Unit Tests
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java
Unit tests verify legacy DTO/page/diff conversions, batch text updates, compare/sync/revoke delegation, update/create-or-update identity preservation, and duplicate-create fallback behavior.
Controller Parameter Binding & Authorization Tests
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerParamBindLowLevelTest.java
Tests cover operator binding for consumer/user identities, attribution overrides, key/operator validation, config-hiding behavior, base64 decoding, and compare endpoint parameter handling.
E2E Test Helpers & Smoke Test
e2e/portal-e2e/tests/helpers/portal-helpers.js, e2e/portal-e2e/tests/portal-core.spec.js
E2E helpers updated to await /openapi/v1 item endpoints; added deleteNamespaceItem and revokeNamespaceItemsViaUi helpers and a smoke test exercising create/delete/revoke via OpenAPI.
Migration Tracking & URL Inventory Documentation
docs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.md, docs/zh/contribution/apollo-portal-openapi-migration-tracking.md
Docs updated to reflect increased OpenAPI coverage and Item-domain migration to /openapi/v1, including updated URL listings and counts.
Namespace Text Syntax Checker
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java
New shared syntax checker validates YAML namespace text with duplicate-key protection and translates parse errors into BadRequestException.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • apolloconfig/apollo#5470: Introduces OpenApiModelConverters legacy item/page/diff conversion utilities that this PR extends for item DTO and page conversions.

Suggested labels

size:XXL, lgtm

Suggested reviewers

  • arrow1991
  • hezhangjian

Poem

🐰 I nibble keys and hop through code,

Encoded paths and APIs bold,
Controllers check and services sing,
Tests applaud each migrated thing.
Hop—OpenAPI—hop: the portal's told!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.95% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the primary change: migrating portal item config UI operations to OpenAPI. It is concise, specific, and clearly indicates the main objective of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nobodyiam nobodyiam marked this pull request as ready for review May 16, 2026 05:59
Copilot AI review requested due to automatic review settings May 16, 2026 05:59
@dosubot dosubot Bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label May 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ItemController now implements the generated ItemManagementApi, delegates to a portal-local ItemOpenApiService, 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.

Comment thread apollo-portal/src/main/resources/static/scripts/services/ConfigService.js Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (2)
apollo-portal/src/main/resources/static/scripts/services/ConfigService.js (1)

155-169: ⚡ Quick win

Add an empty-page stop condition in paged fetch.

At Line 165, pagination continues only by comparing fetched count vs total. If a page returns empty content while total is 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 win

Add test coverage for the update path of createOrUpdateItem.

The existing test only verifies the create path (when loadItem returns null). Consider adding a test case that verifies the update path when an existing item is found, ensuring it delegates to updateItem with 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

📥 Commits

Reviewing files that changed from the base of the PR and between eaf86be and 1b7dc6f.

📒 Files selected for processing (13)
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java
  • apollo-portal/src/main/resources/static/scripts/controller/config/ConfigNamespaceController.js
  • apollo-portal/src/main/resources/static/scripts/controller/config/SyncConfigController.js
  • apollo-portal/src/main/resources/static/scripts/services/ConfigService.js
  • apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiServiceTest.java
  • apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerParamBindLowLevelTest.java
  • docs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.md
  • docs/zh/contribution/apollo-portal-openapi-migration-tracking.md
  • e2e/portal-e2e/tests/helpers/portal-helpers.js
  • e2e/portal-e2e/tests/portal-core.spec.js

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apollo-portal/src/main/resources/static/scripts/services/ConfigService.js Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java (1)

59-68: ⚡ Quick win

Add Javadoc for the nested YAML factory helper.

TypeLimitedYamlPropertiesFactoryBean is 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 value

The unused clusterName parameter appears intentional but warrants clarification.

In this branch-items endpoint, clusterName is part of the method signature (required by the ItemManagementApi interface) but is not used. The permission check and service call both use branchName instead:

// 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.findBranchItems method (lines 49-50) only accepts four parameters and does not include clusterName. 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., ReleaseController also treats branchName as 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9777cc9 and be1ccdf.

📒 Files selected for processing (4)
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java
  • apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/NamespaceTextSyntaxChecker.java
  • apollo-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

@nobodyiam
Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants