[codex] Adapt portal OpenAPI to v0.2.0#5608
Open
nobodyiam wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughUpgrades Apollo Portal to apollo-openapi v0.2.0 and threads explicit operator/modifiedBy parameters through OpenAPI controllers, portal controllers, and services; updates OpenAPI response types and frontend OpenAPI calls; removes UserInfoHolder wiring; updates converters, tests, and migration docs. ChangesOpenAPI v0.2.0 Migration with Operator Parameter Threading
Estimated code review effort Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
dosubot
Bot
added
the
size:XL
Contributor
There was a problem hiding this comment.
Pull request overview
This PR advances the Apollo Portal OpenAPI migration by bumping the generator input contract to apollo-openapi v0.2.0 and aligning Portal OpenAPI controllers/server-services (plus selected Portal UI reads) with the updated generated interfaces and response shapes. It also continues the migration goal of making the operator explicit below controller boundaries rather than relying on UserInfoHolder inside service-layer logic for OpenAPI requests.
Changes:
- Update
apollo-portalOpenAPI spec URL toapollo-openapiv0.2.0 and adapt OpenAPI App/Cluster contracts to the new paths and DTOs. - Propagate
operatorexplicitly through Portal services/OpenAPI server services; update controllers and tests accordingly. - Keep already-migrated Portal UI App reads on OpenAPI while normalizing new array-style responses back into the legacy
entities/bodystructure expected byAppUtil.collectData.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/zh/contribution/apollo-portal-openapi-migration-tracking.md | Update migration tracking notes for v0.2.0 baseline and path/compat policy. |
| docs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.md | Update inventory entries for the new App OpenAPI paths (env-cluster-info, miss-envs). |
| docs/en/contribution/apollo-portal-openapi-migration-tracking.md | English migration tracking updates for v0.2.0 baseline and exceptions. |
| docs/en/contribution/apollo-portal-openapi-frontend-url-inventory.md | English inventory updates to reflect the new OpenAPI paths. |
| CHANGES.md | Add release-note entry referencing the OpenAPI v0.2.0 adaptation PR. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/ServiceExceptionTest.java | Adjust App service call signature and mock operator source. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java | Update namespace deletion to pass explicit operator. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/FavoriteServiceTest.java | Update FavoriteService calls to include explicit operator/login user id. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java | Update import/export test stubs to include operator parameter propagation. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java | Update ItemService method signatures to accept operator explicitly. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java | Update AppService APIs to accept operator explicitly (removing UserInfoHolder reliance). |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppNamespaceServiceTest.java | Update AppNamespaceService APIs to accept operator explicitly. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ServerConfigControllerTest.java | Update controller tests for new service signatures requiring operator. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.java | Update ConsumerController construction and operator propagation assertions. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportControllerTest.java | Update controller tests for operator being passed into import service calls. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ClusterControllerTest.java | Update cluster controller tests for operator propagation. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java | Update OpenAPI v1 cluster tests for operator propagation to services. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerParamBindLowLevelTest.java | Ensure operator query param binding reaches service layer. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java | Adapt App OpenAPI tests to v0.2.0 paths/DTOs and new return/void semantics. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java | Add/adjust low-level binding tests for create/update/delete operator handling. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.java | Update ConsumerService tests for explicit operator parameters. |
| apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiServiceTest.java | New unit tests for v0.2.0 App server-service DTO shapes and per-env failures. |
| apollo-portal/src/main/resources/static/scripts/services/AppService.js | Update UI OpenAPI paths and normalize new array responses into legacy entities/body shape. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ServerConfigService.java | Remove UserInfoHolder dependency; require modifiedBy explicitly. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java | Remove implicit fallback to UserInfoHolder; rely on releasedBy set by callers. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/NamespaceService.java | Remove UserInfoHolder dependency; pass operator explicitly for create/delete. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/NamespaceBranchService.java | Remove overloads that implicitly read operator from UserInfoHolder. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.java | Remove UserInfoHolder dependency; require operator explicitly for mutations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/FavoriteService.java | Remove UserInfoHolder dependency; require login user id explicitly. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java | Remove UserInfoHolder dependency; pass operator through import workflow. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java | Remove UserInfoHolder dependency; require operator for create/delete. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java | Remove UserInfoHolder dependency; require operator for create/update/delete flows. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppNamespaceService.java | Remove UserInfoHolder dependency; require operator for create/delete flows. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ServerConfigController.java | Pass current user id into ServerConfigService methods. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java | Ensure releasedBy is set from current user before publishing releases. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java | Pass operator explicitly into Namespace/AppNamespace service operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java | Pass operator explicitly into branch operations and merges. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java | Pass operator explicitly into ItemService mutation operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/FavoriteController.java | Pass operator explicitly into FavoriteService operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java | Pass operator explicitly into ConsumerService operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java | Pass operator explicitly into ConfigsImportService operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java | Pass operator explicitly into ClusterService operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java | Pass operator explicitly into AppService operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java | Align with v0.2.0 operator semantics and propagate operator to OpenAPI services. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java | Align App OpenAPI endpoints/DTOs with v0.2.0 and explicit operator resolution. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java | Remove converters for models no longer generated in v0.2.0; adjust rule-items type. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java | Remove UserInfoHolder dependency; require operator explicitly for write operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiService.java | Propagate operator into AppNamespace creation. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java | Propagate operator into cluster create/delete operations. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java | Adapt App server-service to v0.2.0 DTOs/paths and operator propagation. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java | Update OpenAPI server service interface for operator propagation. |
| apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java | Update OpenAPI server service interface for v0.2.0 return types and operator propagation. |
| apollo-portal/pom.xml | Point OpenAPI generator input spec URL to apollo-openapi v0.2.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.warn("Failed to load env {} navigation for app {}", env, appId, e); | ||
| OpenEnvClusterInfo info = new OpenEnvClusterInfo(); | ||
| info.setCode(HttpStatus.INTERNAL_SERVER_ERROR.value()); | ||
| info.setMessage("load env:" + env.getName() + " cluster error." + e.getMessage()); |
Member
Author
There was a problem hiding this comment.
Fixed in abfd47a. The env-cluster-info error path now logs the exception server-side and returns a stable client-facing message without appending e.getMessage().
| "load env:" + env.getName() + " cluster error." + e.getMessage()); | ||
| entity.setCode(HttpStatus.INTERNAL_SERVER_ERROR.value()); | ||
| entity.setMessage( | ||
| String.format("load appId:%s from env %s error.", appId, env) + e.getMessage()); |
Member
Author
There was a problem hiding this comment.
Fixed in abfd47a. The miss-envs error path now formats a stable response message and logs the full exception instead of returning raw exception text.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java (1)
57-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd validation for
releasedByinServerReleaseOpenApiService.publishNamespace()before transforming the model.The OpenAPI v1 controller validates
releasedByis non-empty before callingpublishNamespace, butServerReleaseOpenApiService.publishNamespace()is a public interface method that lacks its own validation. WhenBeanUtils.transform()copies properties fromNamespaceReleaseDTOtoNamespaceReleaseModel, a null or emptyreleasedByfrom the DTO will either not be copied or remain null. This should be validated in the service method rather than relying on callers to ensure the field is populated, especially since the method is publicly exposed asReleaseOpenApiServiceinterface.🤖 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/service/ReleaseService.java` around lines 57 - 72, ServerReleaseOpenApiService.publishNamespace() must validate the releasedBy field before transforming the DTO into a NamespaceReleaseModel; add a null/empty check on NamespaceReleaseDTO.getReleasedBy() (or the incoming parameter) at the start of publishNamespace(), and throw an appropriate exception (e.g., IllegalArgumentException or a BadRequest/BadParam exception used in this codebase) if releasedBy is blank so BeanUtils.transform() cannot produce a model with a null/empty releasedBy; reference the method ServerReleaseOpenApiService.publishNamespace(), the DTO class NamespaceReleaseDTO, the model NamespaceReleaseModel and the BeanUtils.transform() call when applying the change.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
79-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate user existence validation.
The
resolveOperatormethod already validates that the user exists (lines 108-110), so this additional check is redundant.♻️ Proposed fix to remove duplication
- if (userService.findByUserId(operator) == null) { - throw BadRequestException.userNotExists(operator); - } - return ResponseEntity.ok(this.clusterOpenApiService.createCluster(env, cluster, operator));🤖 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/ClusterController.java` around lines 79 - 81, Remove the redundant user existence check in ClusterController: delete the block that calls userService.findByUserId(operator) and throws BadRequestException.userNotExists(operator), since resolveOperator already performs that validation; keep resolveOperator as the single source of truth for operator validation to avoid duplicated checks.
🧹 Nitpick comments (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/FavoriteService.java (1)
73-101: 💤 Low valueOptional: avoid reassigning the
userIdparameter.Line 84 silently reassigns the
userIdmethod parameter tologinUserIdwhen the caller is requesting another user's favorites. Functionally this preserves the previous "user can only search his own favorites" guard, but reassigning a method parameter makes the data flow harder to follow. Consider introducing a local (e.g.,effectiveUserId) and using it consistently below, or — if you'd rather make the policy explicit — throw aBadRequestException/AccessDeniedExceptioninstead of silently rewriting input.♻️ Suggested refactor
- if (!isUserIdEmpty) { - // user can only search his own favorite app - if (!Objects.equals(loginUserId, userId)) { - userId = loginUserId; - } - } + String effectiveUserId = userId; + if (!isUserIdEmpty && !Objects.equals(loginUserId, userId)) { + // user can only search his own favorite app + effectiveUserId = loginUserId; + } @@ - if (isAppIdEmpty) { - return favoriteRepository.findByUserIdOrderByPositionAscDataChangeCreatedTimeAsc(userId, - page); - } + if (isAppIdEmpty) { + return favoriteRepository.findByUserIdOrderByPositionAscDataChangeCreatedTimeAsc( + effectiveUserId, page); + } @@ - return Collections.singletonList(favoriteRepository.findByUserIdAndAppId(userId, appId)); + return Collections.singletonList( + favoriteRepository.findByUserIdAndAppId(effectiveUserId, appId));🤖 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/service/FavoriteService.java` around lines 73 - 101, The search method silently reassigns the parameter userId to loginUserId which obscures data flow; introduce a local variable (e.g., effectiveUserId) and set it to either the provided userId or, if the caller is not permitted, to loginUserId (or alternatively throw an AccessDeniedException) and then use effectiveUserId everywhere instead of mutating the userId parameter; update all branches that call favoriteRepository (findByUserIdOrderByPositionAscDataChangeCreatedTimeAsc, findByUserIdAndAppId) to use effectiveUserId while leaving appId and loginUserId checks intact.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (1)
82-91: 💤 Low valueLGTM — operator threaded through correctly.
Optional nit:
createItemandupdateItem(lines 104, 121) already extractuserInfoHolder.getUser().getUserId()into a localuserId/usernamevariable, whilemodifyItemsByText,update, andrevokeItemsinline the chain. Extracting a local for consistency would slightly improve readability and make it easier to add logging/auditing later, but it's not blocking.Also applies to: 209-232, 250-255
🤖 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/controller/ItemController.java` around lines 82 - 91, Extract userInfoHolder.getUser().getUserId() into a local variable in modifyItemsByText, update, and revokeItems (similar to createItem/updateItem) for consistency and easier future logging; e.g., add a local String userId = userInfoHolder.getUser().getUserId() at the top of each method and use userId when calling configService.updateConfigItemByText or other service methods, and apply the same change in the other occurrences referenced (around the 209-232 and 250-255 blocks).apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
33-35: 💤 Low valueTwo near-identical method names; consider Javadoc to disambiguate.
getEnvClustersandgetEnvClusterInfodiffer only in their return type and a single word, but they correspond to different OpenAPI endpoints/payloads in v0.2.0. Adding a brief Javadoc on each (purpose, endpoint they back, payload shape) would help callers pick the right one and prevent accidental misuse.As per coding guidelines: "New Java classes should include a short Javadoc describing the class purpose" — extending the same care to the public methods of changed/expanded interfaces aligns with that intent.
🤖 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/server/service/AppOpenApiService.java` around lines 33 - 35, The two public methods getEnvClusters and getEnvClusterInfo on AppOpenApiService are easily confused; add concise Javadoc above each method that states its purpose, the OpenAPI endpoint/version (v0.2.0) it implements, and the payload/return shape (e.g., returns List<OpenEnvClusterDTO> vs List<OpenEnvClusterInfo>) so callers can distinguish them; reference the interface name AppOpenApiService and the DTO types OpenEnvClusterDTO and OpenEnvClusterInfo in the comments to make intent and usage explicit.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java (1)
65-86: 💤 Low valueConsider asserting the operator is propagated to the change set.
These tests now pass
"test"as the operator, but neither test verifies that the value actually reacheschangeSets.setDataChangeLastModifiedBy(...)oritemAPI.updateItemsByChangeSet(...). A simpleArgumentCaptor<ItemChangeSets>orverify(itemAPI).updateItemsByChangeSet(...)would lock in the new contract and prevent silent regressions if the operator threading is later broken.Also applies to: 101-123
🤖 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/portal/service/ConfigServiceTest.java` around lines 65 - 86, In testUpdateConfigByText ensure the "test" operator is actually propagated to the ItemChangeSets and passed to the update call: capture the ItemChangeSets passed to itemAPI.updateItemsByChangeSet (use an ArgumentCaptor<ItemChangeSets>), assert that captor.getValue().getDataChangeLastModifiedBy() (or changeSets.getDataChangeLastModifiedBy()) equals "test", and optionally verify itemAPI.updateItemsByChangeSet(...) was invoked with that captured change set; reference methods/configService.updateConfigItemByText, ItemChangeSets, changeSets.setDataChangeLastModifiedBy, and itemAPI.updateItemsByChangeSet when adding the assertions.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/FavoriteController.java (1)
49-49: 💤 Low valueConsider extracting the user once per request.
userInfoHolder.getUser().getUserId()is called in each method. While correct, extracting it to a local variable would improve efficiency and make it easier to add null-safety checks if needed.♻️ Optional refactor to extract user once
`@PostMapping`("/favorites") public Favorite addFavorite(`@RequestBody` Favorite favorite) { - return favoriteService.addFavorite(favorite, userInfoHolder.getUser().getUserId()); + String userId = userInfoHolder.getUser().getUserId(); + return favoriteService.addFavorite(favorite, userId); }Apply similar pattern to other methods if desired.
🤖 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/controller/FavoriteController.java` at line 49, In FavoriteController, avoid calling userInfoHolder.getUser().getUserId() inline for each request; extract the user/id to a local variable at the start of the controller method (e.g., long userId = userInfoHolder.getUser().getUserId() or User user = userInfoHolder.getUser()) and pass that variable into favoriteService.addFavorite(...) and other service calls, adding a null-safety check for user (throw or return an appropriate error if null) so the methods use the cached value instead of repeated calls.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java (1)
97-116: ⚡ Quick winAdd a centralized non-blank operator precondition for write paths.
Now that audit attribution is fully caller-supplied, guarding
operator/createdByat service entry points will prevent null/blank audit values from leaking into persistence calls.Proposed hardening diff
@@ public class ConsumerService { @@ + private void validateOperator(String operator) { + Preconditions.checkArgument(!Strings.isNullOrEmpty(operator), "Operator can not be empty"); + } + public Consumer createConsumer(Consumer consumer, String operator) { + validateOperator(operator); String appId = consumer.getAppId(); @@ public ConsumerToken generateAndSaveConsumerToken(Consumer consumer, Integer rateLimit, Date expires, String operator) { Preconditions.checkArgument(consumer != null, "Consumer can not be null"); + validateOperator(operator); @@ public List<ConsumerRole> assignNamespaceRoleToConsumer(String token, String appId, String namespaceName, String env, String operator) { + validateOperator(operator); @@ public ConsumerRole assignCreateApplicationRoleToConsumer(String token, String operator) { + validateOperator(operator); @@ public ConsumerRole assignAppRoleToConsumer(Long consumerId, String appId, String operator) { + validateOperator(operator); @@ private ConsumerToken generateConsumerToken(Consumer consumer, Integer rateLimit, Date expires, String createdBy) { + validateOperator(createdBy);Also applies to: 118-126, 278-328, 341-357
🤖 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/service/ConsumerService.java` around lines 97 - 116, Add a centralized precondition that the caller-supplied operator is non-null and non-blank at service write entry points: check the operator parameter at the start of methods like ConsumerService.createConsumer (and the other write methods called out: the ranges covering consumer updates and other create/update/delete paths) and throw a BadRequestException (or appropriate validation exception) immediately if operator is null/blank; do this before any audit fields are set (e.g., before setDataChangeCreatedBy/setDataChangeLastModifiedBy) so no null/blank operator values reach persistence, and factor the check into a small private helper (e.g., validateOperatorOrThrow) to reuse across those methods.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
129-130: ⚡ Quick winConsider extracting operator once per method.
Lines 129-130, 143, and 190 call
userInfoHolder.getUser().getUserId()inline, while lines 167-171 extract the operator into a local variable first. The local variable approach (used in thecreate(@PathVariableString env, ...)method) is more efficient and safer if the operator is used multiple times or passed to multiple service calls.♻️ Consistent operator extraction pattern
`@PostMapping` `@ApolloAuditLog`(type = OpType.CREATE, name = "App.create") public App create(`@Valid` `@RequestBody` AppModel appModel) { - + String operator = userInfoHolder.getUser().getUserId(); App app = transformToApp(appModel); - return appService.createAppAndAddRolePermission(app, appModel.getAdmins(), - userInfoHolder.getUser().getUserId()); + return appService.createAppAndAddRolePermission(app, appModel.getAdmins(), operator); }Apply the same pattern to
update()anddeleteApp().Also applies to: 143-143, 190-190
🤖 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/controller/AppController.java` around lines 129 - 130, In update() and deleteApp() replace repeated inline calls to userInfoHolder.getUser().getUserId() with a single local variable (e.g., String operator = userInfoHolder.getUser().getUserId()) at the top of each method and pass that operator to service calls such as appService.createAppAndAddRolePermission(...) and other appService methods; locate usages in update(), deleteApp(), and the call sites to ensure all occurrences use the new operator variable consistently.
🤖 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/ServerAppOpenApiService.java`:
- Around line 125-130: The handler in ServerAppOpenApiService is putting raw
exception text (e.getMessage()) into the OpenEnvClusterInfo response (see
OpenEnvClusterInfo and calls to info.setMessage), which must be removed; instead
log the full exception server-side (logger.warn(..., e)) and set a stable
generic client message such as "Internal server error while loading environment"
(or similar) in info.setMessage; apply the same change to the other occurrence
around the OpenEnvClusterInfo creation at the 233-242 region so no exception
details are returned to callers.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiService.java`:
- Around line 76-80: In createAppNamespace(OpenAppNamespaceDTO), validate
appNamespaceDTO.getDataChangeCreatedBy() is non-null and non-blank before
calling appNamespaceService.createAppNamespaceInLocal: if the creator is
null/blank, throw an appropriate bad-request/illegal-argument exception (e.g.,
IllegalArgumentException or a framework BadRequestException) so the write is
rejected early; perform this check at the top of createAppNamespace, referencing
appNamespaceDTO.getDataChangeCreatedBy() and the createAppNamespaceInLocal call
to ensure no unaudited write is attempted.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java`:
- Around line 83-89: The controller currently only rejects null appId but allows
empty or whitespace strings to reach downstream methods like createApp and
assignAppRoleToConsumer; update the null check in AppController (where
req.getApp() and OpenAppDTO app are validated) to reject blank appId values by
using StringUtils.hasText(app.getAppId()) (or equivalent trim-and-empty check)
and throw BadRequestException("AppId is null or blank") when the check fails so
empty/whitespace appId is rejected at the controller boundary.
- Around line 176-179: createAppInEnv currently resolves the operator but does
not stamp audit fields on the OpenAppDTO; before calling
appOpenApiService.createAppInEnv(env, app, resolvedOperator) set
app.setDataChangeCreatedBy(resolvedOperator) and
app.setDataChangeLastModifiedBy(resolvedOperator) (same pattern used in
createApp and updateApp) so the DTO contains the normalized audit metadata prior
to delegation.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java`:
- Around line 127-138: The createAppInRemote method currently accepts a
blank/null operator and uses it for audit fields and role initialization; add a
guard at the start of createAppInRemote to reject blank operators (e.g.,
StringUtils.isBlank(operator)) and throw an appropriate unchecked exception
(IllegalArgumentException or a custom BadRequestException) so we fail fast
before mutating app or calling appAPI.createApp and
roleInitializationService.initClusterNamespaceRoles; apply the same pattern to
the other affected methods mentioned (the other create/update/delete methods
that set dataChangeCreatedBy/dataChangeLastModifiedBy and call
roleInitializationService) to ensure no write happens with a blank operator.
---
Outside diff comments:
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java`:
- Around line 79-81: Remove the redundant user existence check in
ClusterController: delete the block that calls
userService.findByUserId(operator) and throws
BadRequestException.userNotExists(operator), since resolveOperator already
performs that validation; keep resolveOperator as the single source of truth for
operator validation to avoid duplicated checks.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.java`:
- Around line 57-72: ServerReleaseOpenApiService.publishNamespace() must
validate the releasedBy field before transforming the DTO into a
NamespaceReleaseModel; add a null/empty check on
NamespaceReleaseDTO.getReleasedBy() (or the incoming parameter) at the start of
publishNamespace(), and throw an appropriate exception (e.g.,
IllegalArgumentException or a BadRequest/BadParam exception used in this
codebase) if releasedBy is blank so BeanUtils.transform() cannot produce a model
with a null/empty releasedBy; reference the method
ServerReleaseOpenApiService.publishNamespace(), the DTO class
NamespaceReleaseDTO, the model NamespaceReleaseModel and the
BeanUtils.transform() call when applying the change.
---
Nitpick comments:
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java`:
- Around line 33-35: The two public methods getEnvClusters and getEnvClusterInfo
on AppOpenApiService are easily confused; add concise Javadoc above each method
that states its purpose, the OpenAPI endpoint/version (v0.2.0) it implements,
and the payload/return shape (e.g., returns List<OpenEnvClusterDTO> vs
List<OpenEnvClusterInfo>) so callers can distinguish them; reference the
interface name AppOpenApiService and the DTO types OpenEnvClusterDTO and
OpenEnvClusterInfo in the comments to make intent and usage explicit.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java`:
- Around line 97-116: Add a centralized precondition that the caller-supplied
operator is non-null and non-blank at service write entry points: check the
operator parameter at the start of methods like ConsumerService.createConsumer
(and the other write methods called out: the ranges covering consumer updates
and other create/update/delete paths) and throw a BadRequestException (or
appropriate validation exception) immediately if operator is null/blank; do this
before any audit fields are set (e.g., before
setDataChangeCreatedBy/setDataChangeLastModifiedBy) so no null/blank operator
values reach persistence, and factor the check into a small private helper
(e.g., validateOperatorOrThrow) to reuse across those methods.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java`:
- Around line 129-130: In update() and deleteApp() replace repeated inline calls
to userInfoHolder.getUser().getUserId() with a single local variable (e.g.,
String operator = userInfoHolder.getUser().getUserId()) at the top of each
method and pass that operator to service calls such as
appService.createAppAndAddRolePermission(...) and other appService methods;
locate usages in update(), deleteApp(), and the call sites to ensure all
occurrences use the new operator variable consistently.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/FavoriteController.java`:
- Line 49: In FavoriteController, avoid calling
userInfoHolder.getUser().getUserId() inline for each request; extract the
user/id to a local variable at the start of the controller method (e.g., long
userId = userInfoHolder.getUser().getUserId() or User user =
userInfoHolder.getUser()) and pass that variable into
favoriteService.addFavorite(...) and other service calls, adding a null-safety
check for user (throw or return an appropriate error if null) so the methods use
the cached value instead of repeated calls.
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java`:
- Around line 82-91: Extract userInfoHolder.getUser().getUserId() into a local
variable in modifyItemsByText, update, and revokeItems (similar to
createItem/updateItem) for consistency and easier future logging; e.g., add a
local String userId = userInfoHolder.getUser().getUserId() at the top of each
method and use userId when calling configService.updateConfigItemByText or other
service methods, and apply the same change in the other occurrences referenced
(around the 209-232 and 250-255 blocks).
In
`@apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/FavoriteService.java`:
- Around line 73-101: The search method silently reassigns the parameter userId
to loginUserId which obscures data flow; introduce a local variable (e.g.,
effectiveUserId) and set it to either the provided userId or, if the caller is
not permitted, to loginUserId (or alternatively throw an AccessDeniedException)
and then use effectiveUserId everywhere instead of mutating the userId
parameter; update all branches that call favoriteRepository
(findByUserIdOrderByPositionAscDataChangeCreatedTimeAsc, findByUserIdAndAppId)
to use effectiveUserId while leaving appId and loginUserId checks intact.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.java`:
- Around line 65-86: In testUpdateConfigByText ensure the "test" operator is
actually propagated to the ItemChangeSets and passed to the update call: capture
the ItemChangeSets passed to itemAPI.updateItemsByChangeSet (use an
ArgumentCaptor<ItemChangeSets>), assert that
captor.getValue().getDataChangeLastModifiedBy() (or
changeSets.getDataChangeLastModifiedBy()) equals "test", and optionally verify
itemAPI.updateItemsByChangeSet(...) was invoked with that captured change set;
reference methods/configService.updateConfigItemByText, ItemChangeSets,
changeSets.setDataChangeLastModifiedBy, and itemAPI.updateItemsByChangeSet when
adding the assertions.
🪄 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: ac29553a-320a-482f-bb85-056107a33d14
📒 Files selected for processing (53)
CHANGES.mdapollo-portal/pom.xmlapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/FavoriteController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ServerConfigController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppNamespaceService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/FavoriteService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ItemService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/NamespaceBranchService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/NamespaceService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ReleaseService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ServerConfigService.javaapollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerParamBindLowLevelTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/ServiceExceptionTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ClusterControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ConsumerControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ServerConfigControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppNamespaceServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/FavoriteServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.javadocs/en/contribution/apollo-portal-openapi-frontend-url-inventory.mddocs/en/contribution/apollo-portal-openapi-migration-tracking.mddocs/zh/contribution/apollo-portal-openapi-frontend-url-inventory.mddocs/zh/contribution/apollo-portal-openapi-migration-tracking.md
![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: f5115238b8
ℹ️ 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".
| <name>Apollo Portal</name> | ||
| <properties> | ||
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.1.0/apollo-openapi.yaml</apollo.openapi.spec.url> | ||
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.2.0/apollo-openapi.yaml</apollo.openapi.spec.url> |
There was a problem hiding this comment.
Switching the generated contract to v0.2.0 also changes the item mutation endpoints to accept operator as a query parameter, but the existing /openapi/v1/.../items controller is still the hand-written DTO implementation and is not wired to the generated ItemManagementApi (checked with rg "ItemManagementApi"). In this state, clients generated from v0.2.0 can call update/create/delete with ?operator=... as documented, but the runtime controller ignores it and still rejects requests unless dataChangeCreatedBy/dataChangeLastModifiedBy is duplicated in the body, so item mutations through the new contract fail or audit the wrong field. Please update the item controller/service path to the generated v0.2 interface before advertising this spec.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Good catch. This is a valid Item-domain gap, but we are intentionally deferring it to the next migration slice instead of expanding this App-focused PR. The follow-up should migrate the OpenAPI item controller/service path to the generated v0.2 ItemManagementApi contract, including the operator query-parameter behavior for item mutations.
Comment on lines
+94
to
+96
| if (Boolean.TRUE.equals(req.getAssignAppRoleToSelf())) { | ||
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | ||
| consumerService.assignAppRoleToConsumer(consumerId, app.getAppId()); | ||
| consumerService.assignAppRoleToConsumer(consumerId, app.getAppId(), resolvedOperator); |
There was a problem hiding this comment.
When a Portal USER calls POST /openapi/v1/apps with assignAppRoleToSelf=true, the new USER-aware operator resolution succeeds, but this block still unconditionally reads a consumer id from the request; Portal-session requests do not have the ApolloConsumerId attribute, so retrieveConsumerIdFromCtx() throws and the app creation path returns a server error after creating the app. Please either ignore/reject this flag for USER auth or only retrieve the consumer id when UserIdentityContextHolder is CONSUMER.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in abfd47a. assignAppRoleToSelf now only retrieves and assigns a consumer role when the current auth type is CONSUMER; Portal USER requests no longer depend on ApolloConsumerId. Added coverage in AppControllerParamBindLowLevelTest.
| <name>Apollo Portal</name> | ||
| <properties> | ||
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.1.0/apollo-openapi.yaml</apollo.openapi.spec.url> | ||
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.2.0/apollo-openapi.yaml</apollo.openapi.spec.url> |
There was a problem hiding this comment.
The v0.2 spec now advertises PUT /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/items (batchUpdateItemsByText), but the runtime OpenAPI item controller still only has PUT .../items/{key} and is not implementing the generated item interface (verified with rg "ItemManagementApi" and the mappings under openapi/v1/controller/ItemController.java). Generated v0.2 clients using the text-mode batch update operation will therefore get a 404 even though the POM now publishes that contract; please add the OpenAPI mapping or keep the spec pinned until this operation exists.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Agreed, this endpoint still needs a runtime implementation before Item v0.2 coverage can be considered complete. We are leaving it for the next Item-domain migration PR, where batchUpdateItemsByText should be added through the generated ItemManagementApi path rather than folded into this App compatibility patch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiServiceTest.java (1)
47-56: ⚡ Quick winAdd a test for null
NamespaceReleaseDTOpath
publishNamespacenow guards both blankreleasedByand null DTO, but only the blank branch is asserted here. Add one null-input test to lock in that behavior.Suggested test addition
+ `@Test` + void publishNamespaceShouldRejectNullDto() { + assertThrows(BadRequestException.class, + () -> service.publishNamespace("appId", "DEV", "default", "application", null)); + + verify(releaseService, never()).publish(any(NamespaceReleaseModel.class)); + }🤖 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/ServerReleaseOpenApiServiceTest.java` around lines 47 - 56, Add a new unit test that calls service.publishNamespace("appId", "DEV", "default", "application", null) and asserts it throws BadRequestException; name it e.g. publishNamespaceShouldRejectNullDto. After asserting the exception, verify(releaseService, never()).publish(any(NamespaceReleaseModel.class)) to ensure publish is not invoked. This mirrors the existing publishNamespaceShouldRejectBlankReleasedBy test and locks in the null-DTO guard in publishNamespace.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
66-69: 💤 Low valueConsider migrating to JUnit 5 for new test methods.
While the existing JUnit 4 tests are supported via Vintage, new test methods could use JUnit 5 annotations (
@BeforeEach,@AfterEach,@ExtendWith(SpringExtension.class)) to align with the project's default testing framework guideline. This would be a gradual migration path.Example JUnit 5 migration pattern
For new tests, you could use:
`@ExtendWith`(SpringExtension.class) `@SpringBootTest` `@AutoConfigureMockMvc`(addFilters = false) public class AppControllerParamBindLowLevelTest { `@BeforeEach` // instead of `@Before` public void setUp() { ... } `@BeforeEach` // instead of second `@Before` public void setAuthentication() { ... } `@AfterEach` // instead of `@After` public void clearAuthentication() { ... } }As per coding guidelines,
Use JUnit 5 as the default testing framework with Vintage enabled for legacy JUnit 4 tests.🤖 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/v1/controller/AppControllerParamBindLowLevelTest.java` around lines 66 - 69, The test class AppControllerParamBindLowLevelTest currently uses JUnit 4 (`@RunWith`(SpringRunner.class)) while the project standard is JUnit 5; update the class to use JUnit 5 by replacing `@RunWith`(SpringRunner.class) with `@ExtendWith`(SpringExtension.class), and convert lifecycle methods from `@Before/`@After to `@BeforeEach/`@AfterEach (or `@BeforeAll/`@AfterAll if appropriate) while keeping `@SpringBootTest` and `@AutoConfigureMockMvc`(addFilters = false); ensure imports are updated to JUnit 5 counterparts and that any JUnit 4-specific constructs are adapted or left under Vintage only for legacy tests.
🤖 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/ServerReleaseOpenApiService.java`:
- Around line 46-48: The current single guard in ServerReleaseOpenApiService
conflates a null releaseDTO and a blank releasedBy; change it to two checks:
first if (releaseDTO == null) throw new BadRequestException("Release payload can
not be null."); then if (StringUtils.isBlank(releaseDTO.getReleasedBy())) throw
new BadRequestException("Params(releasedBy) can not be empty."); reference the
releaseDTO variable and its getReleasedBy() call to locate and split the
validation.
---
Nitpick comments:
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiServiceTest.java`:
- Around line 47-56: Add a new unit test that calls
service.publishNamespace("appId", "DEV", "default", "application", null) and
asserts it throws BadRequestException; name it e.g.
publishNamespaceShouldRejectNullDto. After asserting the exception,
verify(releaseService, never()).publish(any(NamespaceReleaseModel.class)) to
ensure publish is not invoked. This mirrors the existing
publishNamespaceShouldRejectBlankReleasedBy test and locks in the null-DTO guard
in publishNamespace.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java`:
- Around line 66-69: The test class AppControllerParamBindLowLevelTest currently
uses JUnit 4 (`@RunWith`(SpringRunner.class)) while the project standard is JUnit
5; update the class to use JUnit 5 by replacing `@RunWith`(SpringRunner.class)
with `@ExtendWith`(SpringExtension.class), and convert lifecycle methods from
`@Before/`@After to `@BeforeEach/`@AfterEach (or `@BeforeAll/`@AfterAll if
appropriate) while keeping `@SpringBootTest` and `@AutoConfigureMockMvc`(addFilters
= false); ensure imports are updated to JUnit 5 counterparts and that any JUnit
4-specific constructs are adapted or left under Vintage only for legacy tests.
🪄 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: 8839a217-4d4f-4286-a87f-ffd755aa8a99
📒 Files selected for processing (14)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerNamespaceOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiServiceTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
Comment on lines
+46
to
+48
| if (releaseDTO == null || StringUtils.isBlank(releaseDTO.getReleasedBy())) { | ||
| throw new BadRequestException("Params(releasedBy) can not be empty."); | ||
| } |
Contributor
There was a problem hiding this comment.
Split validation errors for null payload vs blank releasedBy
The guard is good, but returning the same message for both cases makes client-side troubleshooting harder. Please return a specific message for a null body and another for blank releasedBy.
Proposed patch
- if (releaseDTO == null || StringUtils.isBlank(releaseDTO.getReleasedBy())) {
- throw new BadRequestException("Params(releasedBy) can not be empty.");
- }
+ if (releaseDTO == null) {
+ throw new BadRequestException("Request body can not be empty.");
+ }
+ if (StringUtils.isBlank(releaseDTO.getReleasedBy())) {
+ throw new BadRequestException("Param(releasedBy) can not be empty.");
+ }📝 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.
Suggested change
| if (releaseDTO == null || StringUtils.isBlank(releaseDTO.getReleasedBy())) { | |
| throw new BadRequestException("Params(releasedBy) can not be empty."); | |
| } | |
| if (releaseDTO == null) { | |
| throw new BadRequestException("Request body can not be empty."); | |
| } | |
| if (StringUtils.isBlank(releaseDTO.getReleasedBy())) { | |
| throw new BadRequestException("Param(releasedBy) can not be empty."); | |
| } |
🤖 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/server/service/ServerReleaseOpenApiService.java`
around lines 46 - 48, The current single guard in ServerReleaseOpenApiService
conflates a null releaseDTO and a blank releasedBy; change it to two checks:
first if (releaseDTO == null) throw new BadRequestException("Release payload can
not be null."); then if (StringUtils.isBlank(releaseDTO.getReleasedBy())) throw
new BadRequestException("Params(releasedBy) can not be empty."); reference the
releaseDTO variable and its getReleasedBy() call to locate and split the
validation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the purpose of this PR
Adapt Apollo Portal to the published
apollo-openapiv0.2.0contract and continue the Portal OpenAPI migration work started in #5607.This PR updates the Portal OpenAPI generator input from
v0.1.0tov0.2.0, aligns the App and Cluster OpenAPI controller/service contracts with the generated interfaces, and keeps already-migrated Portal UI App reads on the current OpenAPI paths.It also makes operator propagation explicit below controllers, so service-layer code no longer depends on
UserInfoHolder.getUser()for OpenAPI requests.Which issue(s) this PR fixes:
N/A
Brief changelog
apollo-portalOpenAPI generation toapollo-openapiv0.2.0.v0.2.0.v0.2.0baseline.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.Verification
git ls-remote --tags https://github.com/apolloconfig/apollo-openapi.git refs/tags/v0.2.0python3 scripts/openapi/check_openapi_compatibility_test.pypython3 scripts/openapi/check_openapi_compatibility.py --base https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.1.0/apollo-openapi.yaml --head https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v0.2.0/apollo-openapi.yamlfails as expected with documentedv0.1.0tov0.2.0compatibility differences../mvnw -B -pl apollo-portal -am -DskipTests clean compile./mvnw -B clean test./mvnw -B -pl apollo-assembly -am -DskipTests package.github/workflows/portal-ui-e2e.yml:wait-for-ready.shpassed;npm ci && npx playwright install chromium && BASE_URL=http://127.0.0.1:8070 PLAYWRIGHT_WORKERS=2 npm run test:e2e:cipassed with 16 tests../mvnw -B spotless:applygit diff --checkSummary by CodeRabbit
New Features
Improvements
Documentation
Tests