feat: modification of App and Instance#5499
feat: modification of App and Instance#5499tacklequestions wants to merge 1 commit intoapolloconfig:masterfrom
Conversation
dosubot
bot
added
the
size:XXL
📝 WalkthroughWalkthroughUpdated OpenAPI surfaces to use explicit OpenDTOs, added instance APIs, introduced a ThreadLocal operator via UserIdentityContextHolder (set/cleared by PortalUserSessionFilter), updated services/controllers/frontend and tests, and migrated frontend API paths to /openapi/v1/. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Filter as PortalUserSessionFilter
participant Ctx as UserIdentityContextHolder
participant Controller as AppController
participant Service as ServerAppOpenApiService
participant Remote as RemoteAppAPI
Client->>Filter: HTTP request (may include operator param)
Filter->>Filter: authenticate & extract operator
Filter->>Ctx: setOperator(userInfo)
Filter->>Controller: forward request
Controller->>Service: call OpenAPI method (no operator param)
Service->>Ctx: getOperator()
Ctx-->>Service: operator UserInfo
Service->>Remote: call remote API with operator.userId
Remote-->>Service: returns DTO/result
Service-->>Controller: return OpenAppDTO / lists
Controller-->>Client: ResponseEntity with DTO(s)
Filter->>Ctx: clear() (finally)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tacklequestions
force-pushed
the
master
branch
from
ab018f2 to
352ab72
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
145-145: Inconsistent data extraction method - potential runtime error.Line 145 still uses
collectDatawhile similar operations at lines 125 and 195 now usegatherData. If the backend API forfind_missing_namespaceshas been updated to match the new response format (flat array instead ofresponse.entities), this will cause a runtime error.Apply this diff to maintain consistency:
- $scope.missingNamespaces = AppUtil.collectData(result); + $scope.missingNamespaces = AppUtil.gatherData(result);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
173-189: Add null checks forappparameter and operator context to prevent NPETwo potential pitfalls confirmed:
appis dereferenced byconvert(app)without a null check. If the controller passes a nullOpenAppDTO, this method will throw aNullPointerExceptioninstead of a controlled 400-style error. The controller has no@Validannotation on the parameter. Add a guard here:@Override public void createAppInEnv(String env, OpenAppDTO app) { if (env == null) { throw BadRequestException.invalidEnvFormat("null"); } + if (app == null) { + throw new BadRequestException("App is null"); + } Env envEnum;
UserIdentityContextHolder.getOperator().getUserId()at line 188 assumes the operator has already been set. While the controller handles the CONSUMER auth path (lines 210-214 in AppController), the USER auth path delegates operator setup toPortalUserSessionFilter, which sets the operator only if an "operator" parameter is present (line 84-88). If that condition is not met, the operator remains null and this call will NPE. Add a defensive null check:roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), ConfigConsts.NAMESPACE_APPLICATION, env, + Optional.ofNullable(UserIdentityContextHolder.getOperator()) + .map(UserInfo::getUserId) + .orElseThrow(() -> new BadRequestException("Operator context not set"))); - UserIdentityContextHolder.getOperator().getUserId());
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
209-219: createAppInEnv: behavior is correct; app-null path already flagged at service levelThe conditional operator validation for CONSUMER auth (and reliance on session-based operator for USER auth) is consistent with the established pattern and with how
deleteAppandupdateAppbehave.Note that the only potential issue here—the risk of
appbeing null and causing an NPE in the service—has already been flagged and addressed in theServerAppOpenApiService.createAppInEnvcomment. From the controller side, mirroring thecreateAppnull-check (e.g.,if (app == null) throw new BadRequestException("App is null");) would further harden the API but is not strictly required if the service-level guard is added.
🧹 Nitpick comments (10)
apollo-portal/src/main/resources/static/scripts/services/AppService.js (1)
127-132: Reconsider passing hard‑codedoperator: ''for update/create_remote/delete_appFor
update_app,create_app_remote, anddelete_appyou now always sendoperator: ''as a query param. Given the ongoing OpenAPI refactor where USER‑auth portal requests typically derive the operator fromUserIdentityContextHolderand only CONSUMER calls rely on an explicit operator, this empty value is likely redundant and may be misleading for future readers.Consider either:
- Dropping
operatorentirely for these portal flows, letting the backend infer the operator from the logged‑in user, or- Populating it with the actual username only where the API truly requires an explicit operator.
If portal is the only caller of these specific paths, the first option is usually cleaner.
Example (if you choose to drop the param):
- app_resource.update_app({ - appId: app.appId, - operator: '' - }, app, function (result) { + app_resource.update_app({ + appId: app.appId + }, app, function (result) {…and similarly for
create_app_remoteanddelete_app.Based on learnings, this aligns portal-initiated operations with other OpenAPI flows that rely on context-backed operators for USER auth.
Also applies to: 139-142, 196-201
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (2)
55-80: Remove commented-out mock calls.Lines 58 and 77 contain commented-out calls to
mockPortalPageDTO. These debug/testing stubs should be removed from production code.Apply this diff:
public OpenInstancePageDTO getByRelease(String env, long releaseId, int page, int size) { PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(Env.valueOf(env), releaseId, page, size); - // PageDTO<InstanceDTO> portalPageDTO = mockPortalPageDTO(page, size); return transformToOpenPageDTO(portalPageDTO); }public OpenInstancePageDTO getByNamespace(String env, String appId, String clusterName, String namespaceName, String instanceAppId, Integer page, Integer size) { - // return transformToOpenPageDTO((mockPortalPageDTO(page,size))); return transformToOpenPageDTO(instanceService.getByNamespace(Env.valueOf(env), appId, clusterName, namespaceName, instanceAppId, page, size)); }
96-112: Move mock helper to test sources.The
mockPortalPageDTOmethod is a test utility that should reside in test sources rather than production code. Additionally, the inline comment contains Chinese text ("mock 总数") and unusual formatting.Consider moving this method to a test utility class, or remove it if it's no longer needed (given the commented-out references).
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (1)
192-195: Avoid shadowing theuserInfoHolderfield with local variablesIn the OIDC branches you create a local
UserInfoHolder userInfoHolder = mock(UserInfoHolder.class);which shadows the@MockBeanfield of the same name. It works, but it’s slightly confusing when reading the class.Consider renaming the local variable (e.g.,
localUserInfoHolder) to make it explicit that this is an ad‑hoc mock, independent of the injected bean.Also applies to: 233-235
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
61-83: Good high-level coverage of InstanceController; consider aligningreleaseIdtypeThese tests exercise the main happy paths and error handling for the instance endpoints and verify that the controller delegates correctly to
InstanceOpenApiService, which is exactly what you want at this level.One minor consistency tweak:
releaseIdis declared asinthere, while the service and low‑level param‑binding tests uselong. Changing it tolong(e.g.,long releaseId = 100L;) would better match the API and avoid any future confusion, without changing behavior.Also applies to: 85-111, 113-136, 138-155
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
132-147: Make the operator contract ofcreateAppInRemoteNewexplicit and null-safe
createAppInRemoteNewassumesUserIdentityContextHolder.getOperator()is non‑null and has a non‑blankuserId. If this method were ever called outside of a properly prepared request thread (e.g., missingPortalUserSessionFilteror equivalent), it would fail with an NPE atgetOperator().getUserId().To make the expectations clearer and failure mode more controlled, consider extracting the operator lookup into a small helper that asserts presence:
private String currentOperatorUserId() { UserInfo operator = UserIdentityContextHolder.getOperator(); if (operator == null || StringUtils.isBlank(operator.getUserId())) { throw new IllegalStateException("Operator must be set in UserIdentityContextHolder before calling AppService methods that mutate state"); } return operator.getUserId(); }Then use it here:
if (StringUtils.isBlank(app.getDataChangeCreatedBy())) { String username = currentOperatorUserId(); app.setDataChangeCreatedBy(username); app.setDataChangeLastModifiedBy(username); } // ... roleInitializationService.initClusterNamespaceRoles( app.getAppId(), env.getName(), ConfigConsts.CLUSTER_NAME_DEFAULT, currentOperatorUserId());This keeps the behavior the same when context is correctly initialized, but produces a clearer error if the contract is violated.
Please double‑check that all call sites of
createAppInRemoteNewexecute in a context whereUserIdentityContextHolderhas a non‑null operator set (e.g., via filters or controller advice). If there are any non‑HTTP or batch callers, they may need to be updated to establish this context first.
177-189: Centralized operator sourcing is good; consider a shared helper to avoid NPEs and duplicationSwitching
createAppInLocal,createAppAndAddRolePermission,updateAppInLocal, anddeleteAppInLocalto source the operator fromUserIdentityContextHoldermakes operator handling consistent with the new OpenAPI/portal model, and aligns with the pattern used elsewhere in the PR (operator from per‑thread context rather than directly fromUserInfoHolder). This is good from a design standpoint. Based on learnings, this matches the move to centralized operator handling.Given that all these methods now call
UserIdentityContextHolder.getOperator().getUserId()directly, I’d strongly suggest:
- Introduce a shared helper (like
currentOperatorUserId()illustrated in the previous comment) and use it everywhere instead of repeatingUserIdentityContextHolder.getOperator().getUserId(); and- Enforce presence of the operator in that helper (throwing a clear
IllegalStateExceptionif missing) rather than risking a bare NPE.Example usage in these sites:
String operator = currentOperatorUserId(); app.setDataChangeCreatedBy(operator); app.setDataChangeLastModifiedBy(operator); // ... roleInitializationService.initClusterNamespaceRoles(appId, env.getName(), ConfigConsts.CLUSTER_NAME_DEFAULT, operator); // ... managedApp.setDataChangeLastModifiedBy(operator); // ... appRepository.deleteApp(appId, operator);This reduces duplication, tightens the contract around
UserIdentityContextHolder, and makes any misconfigured call‑site fail fast with a clear message instead of a generic 500.Please verify that all entry points that can reach these methods (portal controllers, OpenAPI controllers, any background jobs) correctly initialize
UserIdentityContextHolderbefore invoking AppService so that the helper above will always see a valid operator in normal flows.Also applies to: 203-207, 253-254, 272-292
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
153-164: Clamp negative page indexes to avoidPageableerrors
pageIndexis taken directly from thepageargument, so a negative value will be passed intoPageable.withPage, which is likely to throw anIllegalArgumentExceptionand surface as a 500.You can defensively clamp negatives to zero:
- int pageIndex = page == null ? 0 : page; + int pageIndex = (page == null || page < 0) ? 0 : page;The existing defaulting for
pageSizeis fine; if you expect very largesizevalues from clients, you might also consider an upper bound at the controller/API level.
210-231: findMissEnvs correctly maps per-env failures into OpenMissEnvDTO listThe new
findMissEnvsimplementation iterates over active envs, attemptsappService.load, and on failure buildsOpenMissEnvDTOentries with:
code = HttpStatus.OKandmessage = env.toString()when the remote returns 404 (env considered “missing”),code = 500and a detailed message including the env and exception message for other failures.This mirrors the intent of the old multi-response wrapper while keeping the contract simple. If debugging cross-env issues becomes tricky, you might additionally log the caught exception inside the
catchblock, but functionally this looks fine.
240-258: getEnvClusterInfos provides per-env success/error details as intended
getEnvClusterInfosbuilds oneOpenEnvClusterInfoper active env, marking successful conversions withcode = 200and, on failure, creating a DTO withcode = 500,envset, and an errormessage. This is a clear and client-friendly representation of partial failures across environments.Similar to
findMissEnvs, you may optionally log the caught exception to ease troubleshooting, but the current behavior is coherent and non-breaking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(10 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
🧬 Code graph analysis (8)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(52-182)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
RunWith(45-157)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (4)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/InstanceConfigDTO.java (1)
InstanceConfigDTO(24-52)apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (38)
apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (1)
19-37: OpenAPI v1 endpoint migration looks consistent; double‑check backend contract, especially for the count endpointThe four
$resourceURLs now consistently route through/openapi/v1/envs/:env/..., and the path variables (env,appId,clusterName,namespaceName) match howInstanceServiceis calling them. This aligns with the broader OpenAPI v1 migration pattern and keeps the client code simple. Based on learnings, this also preserves the existing frontend service pattern instead of introducing new conventions.The only thing to verify is that the backend OpenAPI surface exposes exactly these paths and expected response shapes:
GET /openapi/v1/envs/:env/instances/by-releaseGET /openapi/v1/envs/:env/instances/by-namespaceGET /openapi/v1/envs/:env/instances/by-namespace-and-releases-not-inGET /openapi/v1/envs/:env/apps/:appId/clusters/:clusterName/namespaces/:namespaceName/instancesforget_instance_count_by_namespace(ensure this still returns the count payload that callers expect, or that callers were updated if it now returns a list/DTO).If helpful, you can run your existing FE+BE integration tests or hit these endpoints manually (via Postman/curl) to confirm status codes and response formats still match what the UI expects.
apollo-portal/src/main/resources/static/scripts/services/AppService.js (1)
18-38: Validate new /openapi/v1 app endpoints andisArraycontractsThe move to
/openapi/v1/apps...looks consistent, but please double‑check that:
/openapi/v1/apps/:appId/env-cluster-infoand/openapi/v1/apps/:appId/miss-envsactually return bare JSON arrays, sinceisArray: truewill make$resourcetreat the response as such./openapi/v1/apps/:appIdandDELETE /openapi/v1/apps/:appIdindeed return single objects / non‑array payloads, matchingisArray: false.Verifying these shapes via the OpenAPI spec or a quick call will avoid subtle client‑side parsing issues.
Also applies to: 51-55, 64-68
apollo-portal/src/main/resources/static/scripts/controller/AppController.js (1)
111-132: Good defensive programming and proper error handling.The code demonstrates good practices:
- The
|| []fallback for admins ensures a valid array is always passed- Proper success/error handling with toast notifications
- Submit button is correctly re-enabled on error
The DTO structure correctly maps all required fields from the scope to the new OpenAPI format.
apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (2)
195-195: Verify that the backend API response format matches the newgatherDataexpectation.The change from
collectDatatogatherDataassumes the backend now returns a flat array of entities. Ensure theload_nav_treeAPI has been updated accordingly, and verify that entities haveenvandclustersproperties at the top level (not nested in abodyfield).
125-125: Verify backend API response format before merging.The change from
collectDatatogatherDataat line 125 represents a critical assumption: that thefind_miss_envsbackend endpoint now returns a flat array instead of the previously wrapped{entities: [...]}structure.
gatherDataexpects:[{code: 200, ...}, ...](flat array with.forEach())collectDataexpected:{entities: [{code: 200, body: ...}, ...]}(wrapped object with.entitiesproperty)Without access to the backend implementation or API specification in the repository, this assumption cannot be verified. If the backend still returns the wrapped format, line 125 will fail at runtime with "response.forEach is not a function."
Confirm the backend response format matches
gatherData's expectation before deploying.apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js (1)
30-30: LGTM - verify backend API compatibility.The change from
collectDatatogatherDatais consistent with other usages ofload_nav_treein the codebase. Ensure the backend returns a flat array where entities haveenvandclustersproperties at the top level.apollo-portal/src/main/resources/static/scripts/directive/directive.js (1)
168-168: LGTM - verify backend API compatibility.The change from
collectDatatogatherDatais consistent with other usages ofload_nav_treein the codebase. This aligns with the broader refactoring to use the new data extraction pattern.apollo-portal/src/main/resources/static/scripts/AppUtils.js (1)
97-107: The review comment is incorrect.The backend code confirms that the
codefield is declared asprivate int codeinRichResponseEntityand all response DTOs. When serialized to JSON via standard Spring serialization, Javaintvalues become JSON numbers (e.g.,200), not strings. The frontend JavaScript will receiveentity.codeas a number type, making the strict equality checkentity.code === 200safe and correct.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (2)
59-59: LGTM: Import addition aligns with manual conversion logic.The explicit Env import supports the manual conversion in
fromEnvClusterInfowhereEnv.toString()is called.
399-412: LGTM: Manual conversion improves explicitness and type safety.The refactor from
BeanUtils.transformto manual construction makes the Env-to-String conversion explicit and provides better type safety for the cluster mapping. Null handling is appropriate.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
85-94: LGTM: Conversion helper is correct and follows DTO transformation patterns.The
transformToOpenPageDTOhelper correctly mapsPageDTO<InstanceDTO>toOpenInstancePageDTOusing the converter utility.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (2)
25-57: LGTM: Operator context follows established ThreadLocal patterns.The new
OPERATOR_HOLDERThreadLocal and associatedgetOperator()/setOperator()methods mirror the existingAUTH_TYPE_HOLDERpattern and provide proper per-thread operator context management.
62-65: LGTM: Cleanup includes operator context.The
clear()method correctly removes theOPERATOR_HOLDERThreadLocal to prevent memory leaks, maintaining consistency with the existing cleanup pattern.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
86-100: LGTM: Test lifecycle correctly manages operator context.The test setup now uses
UserIdentityContextHolder.setOperator()to establish per-thread operator context, and the new@AfterEachensures proper cleanup viaclear(), maintaining test isolation.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (2)
61-66: LGTM: Filter constructor updated to accept UserInfoHolder.The constructor signature correctly adds the
UserInfoHolderdependency needed to populate the operator context for authenticated portal users.
84-96: LGTM: Operator context properly managed with cleanup.The filter correctly sets operator context when an
operatorparameter/attribute is present and ensures cleanup via thefinallyblock to prevent ThreadLocal memory leaks.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
120-127: LGTM: Test correctly verifies operator context flow.The test now verifies that
createAppInEnvis called without an operator parameter (line 120), and confirms that the operator is retrieved from context viauserService.findByUserId(lines 125-126), aligning with the new operator context pattern.
146-147: LGTM: Test updated for renamed method.The verification correctly uses the renamed method
getAppsWithPageAndSizeinstead ofgetAppsBySelf.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
67-77: LGTM: Filter bean configuration updated for new constructor.The
portalUserSessionFilterbean method correctly accepts and passes theUserInfoHolderdependency to thePortalUserSessionFilterconstructor, aligning with the filter implementation changes.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
25-37: LGTM: Interface follows established patterns and conventions.The
InstanceOpenApiServiceinterface defines clean method signatures with consistent parameter ordering (env early, pagination parameters last) and appropriate return types (OpenAPI DTOs). This aligns with existing patterns in the codebase.Based on learnings, the interface maintains consistency with established patterns in Apollo OpenAPI services.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (1)
153-160: User identity context lifecycle is well covered in testsThe additional
@MockBean UserInfoHolder, teardown reset/clears, and the newopenApiPortalRequestWithOperatorParam_shouldPopulateUserIdentityContexttest together give good protection against leaks and regressions aroundUserIdentityContextHolder. The test nicely asserts that the operator is populated only within the filter chain and cleared afterward, while teardown ensures a cleanSecurityContextHolderand thread-local state between tests.Also applies to: 271-297
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (5)
133-155:testGetEnvClusterscorrectly tracks the renamed service and endpoint contractThe updated
testGetEnvClustersusesappOpenApiService.getEnvClusters(appId)and asserts the expected env/cluster structure from/openapi/v1/apps/{appId}/envclusters, which matches the new API surface. The verification of both env names and cluster lists gives good confidence that mapping and response formatting stayed intact.
204-221: App retrieval tests now cover authorized, not-found, and unauthorized consumer scenariosThe trio of tests (
testGetApp,testGetAppNotFound,testGetAppUnauthorizedForConsumer) nicely separates:
- authorized consumer with data present (200 and body with
appIdandownerDisplayName),- authorized consumer with no app data (400), and
- consumer not authorized for the app (400, and no call to
getAppsInfo).This is a solid coverage pattern for the controller’s authorization and lookup branching, and the verification on
consumerServiceandappOpenApiServiceinteractions ensures the service is only hit when appropriate.Also applies to: 225-240, 242-256
258-288: Rename togetAppsWithPageAndSizeis reflected cleanly in tests
testGetAppsBySelfnow stubs and verifiesappOpenApiService.getAppsWithPageAndSize(authorizedAppIds, page, size), while still asserting the same observable behavior at the HTTP level. This keeps the test aligned with the refactored service API without overfitting to implementation details.
290-305: New miss-env and env-cluster-info tests validate additional OpenAPI surfaceThe new
testFindMissEnvsandtestGetEnvClusterInfoscover the/miss-envsand/env-cluster-infoendpoints by:
- stubbing
OpenMissEnvDTOandOpenEnvClusterInforesponses onappOpenApiService, and- asserting key fields (
code,message, andenv) on the JSON payload.This adds useful coverage for the new DTO types and endpoints without tying tests to unnecessary fields.
Also applies to: 307-323
343-346: Update/delete app tests track the new response contractsThe adjustments in
testUpdateAppandtestDeleteApp—expecting 200 for update and{}JSON body for delete—match the more explicit response behavior, and the verify calls ensureappOpenApiService.updateApp/deleteAppare invoked exactly once.No issues from a test-behavior perspective.
Also applies to: 384-387
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (1)
1-140: Param-binding tests for InstanceController are focused and robustThis class cleanly exercises:
- correct path-variable binding for
getInstanceCountByNamespace,- binding of env +
releaseId/page/sizeforgetByRelease,- rejection of empty or non-numeric
releaseIds, and- proper parsing of comma-separated
releaseIds(including extra spaces) into aSet<Long>viaArgumentCaptor.Together with the higher-level
InstanceControllerTest, this gives solid coverage of both binding and behavior. The use ofcontainsExactlyInAnyOrderon the capturedSet<Long>is a nice touch to ensure parsing isn’t order-dependent.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
31-51: AppOpenApiService surface matches implementation and OpenAPI refactorThe updated signatures (returning
OpenAppDTO,List<OpenEnvClusterDTO>,List<OpenMissEnvDTO>,List<OpenEnvClusterInfo>, andgetAppsWithPageAndSize) look consistent with the server implementation and the new OpenAPI DTO model. No functional issues from the interface changes.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
42-66: Delegating methods correctly adapt API to InstanceOpenApiService
getInstanceCountByNamespace,getByRelease, andgetByNamespacecleanly forward parameters toInstanceOpenApiServiceand wrap results inResponseEntity, matching the new OpenAPI-style contract. The env/appId ordering in the service call is consistent with the service signature.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
86-93: createApp implementation is straightforward and consistentThe new
createApp(OpenCreateAppDTO req)correctly converts the incoming DTO, defaultsadminsto an empty set, and returns the createdOpenAppDTOviaOpenApiModelConverters. This aligns with the updated OpenAPI surface and keeps the responsibility of role assignment inAppService.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (8)
83-105: createApp: operator validation and consumer role assignment look soundThis flow validates the request body (
req.getApp(),app.getAppId()), derives the operator fromdataChangeCreatedByorownerName, checks that the user exists, and sets the operator intoUserIdentityContextHolder. The optionalassignAppRoleToSelfhandling for CONSUMER auth, usingConsumerAuthUtilandConsumerService, is consistent with the OpenAPI consumer model and keeps side effects explicit.
108-110: getEnvClusters wrapper is correctly aligned with service contract
getEnvClusterssimply forwardsappIdtoappOpenApiService.getEnvClusters(appId)and returns the resultingList<OpenEnvClusterDTO>in aResponseEntity, which matches the updated service/API surface.
139-155: getApp correctly enforces consumer authorization and enriches owner display nameFor CONSUMER auth, you first verify that the target
appIdis in the consumer’s authorized app set before delegating togetAppsInfo, which aligns with the authorization patterns from earlier OpenAPI work. The post-processing step to setownerDisplayNamefromownerNamekeeps the DTO consumer-friendly. Behavior for non-consumer contexts remains unchanged aside from the DTO return type.
163-176: updateApp follows the conditional operator validation patternThe method validates
appIdvs.dto.getAppId()and, for CONSUMER auth only, checks that theoperatorexists and sets it intoUserIdentityContextHolderbefore callingappOpenApiService.updateApp(dto). This matches the pattern established in the earlier ClusterController refactor where USER auth relies on session/user filters and CONSUMER auth relies on an explicit operator parameter.
182-200: getAppsBySelf correctly supports both consumer and portal-user flowsFor CONSUMER auth,
appIdscome fromConsumerService.findAppIdsAuthorizedByConsumerId, and for non-consumer flows you deriveappIdsfrom the current user’s roles viaRolePermissionServiceandRoleUtils.extractAppIdFromRoleName, deduplicated through aSet. Delegating toappOpenApiService.getAppsWithPageAndSizecentralizes pagination logic and reuses the new service API.
226-236: deleteApp operator handling is consistent with auth-type rules
deleteAppenforces app-admin permissions via@PreAuthorize, and only for CONSUMER auth validates theoperatorand sets it intoUserIdentityContextHolderbefore delegating toappOpenApiService.deleteApp(appId). Returning an empty map inResponseEntityis consistent with other mutation endpoints in this controller that don’t need to expose the deleted resource.
242-244: findMissEnvs wrapper correctly exposes new DTO shapeThis method now returns
ResponseEntity<List<OpenMissEnvDTO>>, delegating fully toappOpenApiService.findMissEnvs(appId). That matches the new service contract and simplifies the controller.
250-252: getEnvClusterInfo now exposes detailed env/cluster info listThe controller method bridges the generated API name (
getEnvClusterInfo) to the new service method (getEnvClusterInfos) and returns aList<OpenEnvClusterInfo>in the body. This matches the refactored DTO model and keeps the controller thin.
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/AppController.js
Show resolved
Hide resolved
| return openInstancePageDTO; | ||
| } | ||
|
|
||
| private PageDTO<InstanceDTO> mockPortalPageDTO(int page, int size) { |
There was a problem hiding this comment.
Can the mock method be migrated to the test class
There was a problem hiding this comment.
Yeah, actually it's a deprecated method defined in testing period, it should be removed in the formal submit
| @Override | ||
| public OpenInstancePageDTO getByNamespace(String env, String appId, String clusterName, | ||
| String namespaceName, String instanceAppId, Integer page, Integer size) { | ||
| // return transformToOpenPageDTO((mockPortalPageDTO(page,size))); |
There was a problem hiding this comment.
Abandoned code should be deleted
There was a problem hiding this comment.
Yeah, already removed, thanks for reviewing!
| public OpenInstancePageDTO getByRelease(String env, long releaseId, int page, int size) { | ||
| PageDTO<InstanceDTO> portalPageDTO = | ||
| instanceService.getByRelease(Env.valueOf(env), releaseId, page, size); | ||
| // PageDTO<InstanceDTO> portalPageDTO = mockPortalPageDTO(page, size); |
There was a problem hiding this comment.
Abandoned code should be deleted
There was a problem hiding this comment.
Yeah, already removed, thanks for reviewing!
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | ||
| Set<String> appIds = this.consumerService.findAppIdsAuthorizedByConsumerId(consumerId); | ||
| if (!appIds.contains(appId)) { | ||
| throw new BadRequestException("Trying to access unauthorized app: " + appId); |
There was a problem hiding this comment.
Check if it is necessary to verify permissions for the app. The current portal user can view the basic information of unauthorized apps
There was a problem hiding this comment.
Yeah, this verification process was removed after double checking
| @PreAuthorize(value = "@unifiedPermissionValidator.isAppAdmin(#appId)") | ||
| @ApolloAuditLog(type = OpType.UPDATE, name = "App.update") | ||
| public ResponseEntity<OpenAppDTO> updateApp(String appId, String operator, OpenAppDTO dto) { | ||
| public ResponseEntity<Object> updateApp(String appId, OpenAppDTO dto, String operator) { |
There was a problem hiding this comment.
Modifying the interface to return the object type may not be appropriate
There was a problem hiding this comment.
Thanks for pointing this out. The current return type issue is caused by the
use of SuccessEmptyResponse, which is defined as an empty object and thus
leads to generating Object as the return type.
I will adjust this in the next commit by updating the response definition to
properly reflect a void-style success response.
| @Override | ||
| @PreAuthorize(value = "@unifiedPermissionValidator.hasCreateApplicationPermission()") | ||
| @ApolloAuditLog(type = OpType.CREATE, name = "App.create.forEnv") | ||
| public ResponseEntity<Object> createAppInEnv(String env, String operator, OpenAppDTO app) { |
There was a problem hiding this comment.
Creating an interface that returns the object type may not be appropriate
There was a problem hiding this comment.
Thanks for pointing this out. The current return type issue is caused by the
use of SuccessEmptyResponse, which is defined as an empty object and thus
leads to generating Object as the return type.
I will adjust this in the next commit by updating the response definition to
properly reflect a void-style success response.
| @Override | ||
| @PreAuthorize(value = "@unifiedPermissionValidator.isAppAdmin(#appId)") | ||
| @ApolloAuditLog(type = OpType.DELETE, name = "App.delete") | ||
| @ApolloAuditLog(type = OpType.RPC, name = "App.delete") |
There was a problem hiding this comment.
What is the reason for changing to RPC type
There was a problem hiding this comment.
It has been changed back, thanks for reviewing
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| @RestController("openapiInstanceController") |
There was a problem hiding this comment.
Is it more reasonable to modify it to a service annotation
There was a problem hiding this comment.
At the moment InstanceManagementApi does not define any request mappings,
so this implementation is the actual web controller. That’s why it is
annotated with @RestController instead of @Service.
| public ResponseEntity<List<OpenInstanceDTO>> getByReleasesAndNamespaceNotIn(String env, | ||
| String appId, String clusterName, String namespaceName, String releaseIds) { | ||
| List<String> rawReleaseIds = | ||
| Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds); |
There was a problem hiding this comment.
ReleaseIds is not mandatory, null pointers need to be considered
There was a problem hiding this comment.
I believe that releaseIds is essential. The business semantics are "to find instances in the specified namespace that do not belong to these releases," and a list of releases to exclude must be provided. instanceService.getByReleasesNotIn(...) requires this collection to generate the query, hence its design requirement. However, it's true that releaseIds lacked explicit null pointer checks, which have now been added.
Again, thanks for your reviewing!
tacklequestions
force-pushed
the
master
branch
2 times, most recently
from
1ec4fbd to
904d039
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
122-133: Bug: missEnvs is now an array of DTOs, but the rest of the code still treats it as stringsAfter switching to
AppUtil.gatherData(result),$scope.missEnvsholds DTO objects (e.g., withcodeandmessage), while:
createAppInMissEnvpasses each element asenvintoAppService.create_remote(env, ...)and intotoastr.success(env, ...), expecting a string.findMissingNamespacesuses$scope.missEnvs.indexOf($rootScope.pageContext.env), which only works whenmissEnvsis astring[].This will cause incorrect behavior (e.g., wrong env checks, malformed create-remote calls).
You can preserve the existing string-based semantics by mapping DTOs to env identifiers (assuming
messageholds the env, as perOpenMissEnvDTOtests):function findMissEnvs() { $scope.missEnvs = []; AppService.find_miss_envs($rootScope.pageContext.appId).then(function (result) { - $scope.missEnvs = AppUtil.gatherData(result); + var missEnvDtos = AppUtil.gatherData(result) || []; + // Convert DTOs to plain env strings for downstream logic + $scope.missEnvs = missEnvDtos.map(function (dto) { + return dto.message; + }); @@ if ($scope.missEnvs.length > 0) { toastr.warning($translate.instant('Config.ProjectMissEnvInfos')); }This keeps
missEnvsasstring[]and avoids breaking existing logic.
🧹 Nitpick comments (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
186-189: Duplicate call to UserIdentityContextHolder.getOperator().getUserId()The operator is already retrieved at line 177. Reuse the
operatorvariable instead of making another call to avoid redundancy and potential NPE:for (Env env : envs) { roleInitializationService.initClusterNamespaceRoles(appId, env.getName(), - ConfigConsts.CLUSTER_NAME_DEFAULT, UserIdentityContextHolder.getOperator().getUserId()); + ConfigConsts.CLUSTER_NAME_DEFAULT, operator); }
132-134: TODO comment indicates migration debtThe TODO comment mentions this should be removed after migration. Consider creating a tracking issue to ensure this technical debt is addressed.
Would you like me to help open an issue to track the removal of the legacy
createAppInRemotemethod after migration is complete?apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
86-111: Type mismatch:releaseIdshould belongto match service signatureThe
releaseIdis declared asint(line 88), but the mock setup at line 98 passes it directly togetByRelease(env, releaseId, page, size). Looking atInstanceControllerParamBindLowLevelTest(line 87), the service expects aLongparameter. While autoboxing will handle this, using the correct type improves clarity and consistency:- int releaseId = 100; + long releaseId = 100L;- Mockito.when(instanceOpenApiService.getByRelease(env, releaseId, page, size)) + Mockito.when(instanceOpenApiService.getByRelease(env, releaseId, page, size))apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
80-82: Redundant empty check after SplitterThe
CollectionUtils.isEmpty(rawReleaseIds)check on line 80 is redundant. Since line 74 already rejectsnulland emptyreleaseIds, andSplitter.omitEmptyStrings()only produces an empty list if the input contained only delimiters (e.g.,",,,"), this check handles a valid but edge-case scenario.However, the error messages are inconsistent ("release ids can not be empty" vs "excludeReleases parameter cannot be empty"). Consider consolidating:
- if(null == releaseIds || releaseIds.isEmpty()) { + if (null == releaseIds || releaseIds.isEmpty()) { throw new BadRequestException("release ids can not be empty"); } List<String> rawReleaseIds = Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds); if (CollectionUtils.isEmpty(rawReleaseIds)) { - throw new BadRequestException("excludeReleases parameter cannot be empty"); + throw new BadRequestException("release ids can not be empty"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(10 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/resources/static/scripts/services/AppService.js
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
🧬 Code graph analysis (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(52-182)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(66-390)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (1)
RunWith(48-141)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-93)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
🪛 GitHub Actions: code style check
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
[error] 71-71: Spotless formatting violation: whitespace/formatting issue in the if condition. Run 'mvn spotless:apply' to fix this violation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (45)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (2)
61-66: LGTM! Clean constructor injection ofUserInfoHolderdependency.
84-95: Verify the intended operator context behavior.The code sets the operator to
userInfoHolder.getUser()(the current authenticated user) when an "operator" parameter/attribute is present. However, the actual value of the "operator" parameter is ignored.Two concerns:
Intended behavior: If the "operator" parameter is meant to specify who the operator is (e.g., an admin acting on behalf of another user), this logic would be incorrect. If it's just a flag to enable operator tracking for the current user, this is fine.
Potential null value:
userInfoHolder.getUser()could return null. While this path only executes for authenticated users, adding a defensive null check would be safer:if (request.getParameterMap().containsKey("operator") || request.getAttribute("operator") != null) { - UserIdentityContextHolder.setOperator(userInfoHolder.getUser()); + UserInfo currentUser = userInfoHolder.getUser(); + if (currentUser != null) { + UserIdentityContextHolder.setOperator(currentUser); + } }The try/finally cleanup pattern is correct for preventing ThreadLocal memory leaks.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (7)
132-155: getEnvClusters test wiring looks consistent with new service APIMocking
appOpenApiService.getEnvClusters(appId)and asserting array JSON structure (env,clusters) matches the new contract and verifies the controller delegates correctly.
242-256: Good negative coverage for unauthorized consumer
testGetAppUnauthorizedForConsumercorrectly verifies that when the consumer is not authorized for the requested app, the endpoint returns400andappOpenApiService.getAppsInfois never invoked.
258-288: getAppsWithPageAndSize pagination binding is exercised wellThe test validates that
pageandsizequery params are bound and passed togetAppsWithPageAndSize, and that authorized app IDs fromConsumerServiceare forwarded. Argument capture on IDs/page/size makes the contract explicit.
291-305: miss-envs JSON contract now explicitly uses OpenMissEnvDTOThe test correctly asserts
codeandmessageon the array items returned from/openapi/v1/apps/{appId}/miss-envs, matching the newOpenMissEnvDTOshape. This will be important to keep in sync with front-end helpers that consume these fields.
307-322: Confirm OpenEnvClusterInfo shape stays compatible with front-end expectationsThe test for
/env-cluster-infoonly assertsenvandmessageonOpenEnvClusterInfo. If the front end (viaAppUtil.gatherData) relies on additional fields likecodeto filter entries, ensure the real DTO includes them and that tests cover such fields where relevant.
324-346: updateApp test focuses on status and delegation, which is appropriateAsserting only
200 OKand verifyingappOpenApiService.updateAppinvocation keeps the test focused on binding and delegation. Permission mocking (isAppAdmin) and security context setup look sufficient for this path.
371-387: deleteApp now returning{}JSON is reflected correctly in the testThe test adapts to the new behavior of returning an empty JSON object while still verifying
appOpenApiService.deleteApp(appId)is invoked. This matches the updated API surface.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
31-51: Service interface changes are consistent with OpenAPI patternsReturning
OpenAppDTOfromcreateApp, renaming togetEnvClusters/getAppsWithPageAndSize, dropping the explicitoperatorincreateAppInEnv, and introducing DTO-typed lists for miss-envs and env-cluster-infos align the service contract with the broader OpenAPI refactor. The interface matches how tests and controllers are calling it.apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js (1)
28-39: Migration from collectData to gatherData matches new response shapeSwitching to
AppUtil.gatherData(result)forload_nav_treeis appropriate with array-based OpenAPI responses and keeps the rest of the logic (env + clusters extraction) intact.apollo-portal/src/main/resources/static/scripts/directive/directive.js (1)
165-186: env cluster selector correctly switched to gatherData for OpenAPI arraysUsing
AppUtil.gatherData(result)here matches the new array-style OpenAPI response for env-cluster info and keeps subsequent processing ofenvandclustersunchanged.apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
191-201: loadNavTree’s switch to gatherData is consistent with OpenAPI nav-tree arraysFor the nav-tree,
nodes = AppUtil.gatherData(result)matches the new array-based OpenAPI response and the subsequent logic operates directly onenvandclustersof each node, which is compatible with gatherData’s return type.apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (1)
19-37: InstanceService correctly migrated to /openapi/v1 instance endpointsUpdating the
$resourceURLs to/openapi/v1/envs/:env/...for all four instance-related calls keeps the client in sync with the new OpenAPI routes while preserving existing parameter binding.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (3)
101-127: Param binding and operator handling for createAppInEnv are validated correctlyThe test now:
- Confirms
envand body binding viacreateAppInEnv(String env, OpenAppDTO app), and- Verifies operator propagation by asserting
userService.findByUserId("bob")was called.This matches the new service signature and the ThreadLocal-based operator handling.
129-151: getAppsBySelf low-level binding test matches new getAppsWithPageAndSize APIThe test ensures that:
pageandsizequery params are bound as integers, and- Authorized app IDs from
ConsumerServiceare passed intogetAppsWithPageAndSize.The argument captors and assertions accurately exercise the updated method.
171-181: deleteApp binding test now includes operator query paramPassing
operatorwhile still verifyingdeleteApp("app-1")invocation ensures that both path and query parameters are bound as expected under the updated operator-handling flow.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (3)
61-83: LGTM!The test correctly verifies the instance count endpoint, including proper path variable ordering and response content validation.
113-136: LGTM!The test properly verifies the exclusion endpoint with valid release IDs and uses explicit set matching for verification.
138-155: LGTM!Good negative test case for invalid input validation—verifies that non-numeric release IDs result in a 400 Bad Request and no service interaction.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (4)
62-77: LGTM!The test correctly verifies path variable binding order (appId, env, clusterName, namespaceName) for the namespace instance count endpoint.
79-94: LGTM!Good coverage of path and query parameter binding for the release-based instance lookup.
96-116: LGTM!Good negative test cases for empty and non-numeric
releaseIdsparameter validation.
118-140: LGTM!Excellent test for verifying whitespace trimming in
releaseIdsparameter parsing ("10, 11 ,12"→{10L, 11L, 12L}). TheArgumentCaptorusage correctly validates the parsedSet<Long>.apollo-portal/src/main/resources/static/scripts/services/AppService.js (3)
129-132: Verify intent of hardcoded emptyoperatorparameterThe
operatorparameter is hardcoded as an empty string. Based on the server-side changes, it appears the operator is now derived fromUserIdentityContextHolderon the server. If the server ignores this parameter when empty, this is fine. However, if the server requires a valid operator value, this could cause issues.Please confirm that the server-side implementation correctly handles or ignores the empty
operatorparameter in favor of the context-based operator fromUserIdentityContextHolder.
22-68: LGTM!The endpoint migrations to
/openapi/v1/paths are consistent and align with the server-side OpenAPI refactoring. TheisArrayflags are correctly set for endpoints returning collections.
196-206: Same pattern: verify emptyoperatorfor delete operationSame verification needed—ensure the server correctly derives the operator from context when the client sends an empty string.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (4)
42-47: LGTM!Clean implementation of the instance count endpoint with proper delegation to the service layer.
52-56: LGTM!Release-based instance query with pagination support is correctly implemented.
61-66: LGTM!Namespace-based instance query correctly delegates to the service with all required parameters including optional
instanceAppId.
84-93: LGTM!The release ID parsing with proper error handling for non-numeric values is well implemented. The
NumberFormatExceptionis correctly caught and transformed into a user-friendlyBadRequestException.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (6)
86-93: LGTM! Clean return type migration.The method now returns
OpenAppDTOdirectly instead of void, properly handling null admins with an empty set and converting the result throughOpenApiModelConverters.fromApp.
96-113: LGTM! Method renamed with consistent implementation.The rename from
getEnvClusterInfotogetEnvClustersaligns with the return type and provides a clearer API surface.
154-164: LGTM! Method renamed for clarity.The rename from
getAppsBySelftogetAppsWithPageAndSizebetter describes the method's purpose and parameters.
210-231: LGTM! Clean migration to concrete DTO list.The method now returns
List<OpenMissEnvDTO>directly instead of a wrapper, with explicit code/message fields properly populated for both success (NOT_FOUND) and error cases.
240-258: LGTM! Consistent error handling with explicit status codes.The method now returns
List<OpenEnvClusterInfo>with explicitcodefield set for both success (HTTP 200) and failure (HTTP 500) paths, providing clear status information in each DTO.
173-189: The review comment references code that does not exist in the actual file.The actual
ServerAppOpenApiService.createAppInEnv()method (lines 171-186) takesoperatoras a String parameter and passes it directly toroleInitializationService.initNamespaceSpecificEnvRoles()at line 185. There is no call toUserIdentityContextHolder.getOperator()in this method, and therefore no NPE risk.The operator is validated in the controller before the service is invoked (see
AppController.createAppInEnvline 162, which checksuserService.findByUserId(operator)). The suggested diff and defensive null check are unnecessary because the code implementation differs from what the review comment describes.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (8)
62-75: LGTM! New dependencies properly wired.The addition of
RolePermissionServiceandUserInfoHoldersupports the new operator handling and role-based app filtering logic introduced in this PR.
83-105: LGTM! Comprehensive operator handling and app creation.The method properly validates the operator, sets it in
UserIdentityContextHolderfor all auth types, creates the app, and conditionally assigns the app role to the consumer. The return type change toOpenAppDTOprovides better type safety.
155-168: LGTM! Proper operator validation for CONSUMER auth.The operator validation is conditional based on auth type, consistent with the pattern from previous PRs where USER auth operators are handled by the filter layer and CONSUMER auth operators are validated explicitly.
Based on learnings, operator validation is conditional by auth type in Apollo OpenAPI.
174-192: LGTM! Well-structured handling of both auth types.The method properly handles both CONSUMER (via consumer service) and USER (via role-based filtering) auth types. The non-CONSUMER path correctly extracts app IDs from user roles using
RoleUtils.extractAppIdFromRoleName.
201-211: LGTM! Consistent operator handling pattern.The operator validation follows the established pattern for CONSUMER auth. Note that the underlying service call
appOpenApiService.createAppInEnv(env, app)depends onUserIdentityContextHolder.getOperator()being properly set, which is addressed in the ServerAppOpenApiService review.Based on learnings, operator validation is conditional by auth type.
219-228: LGTM! Consistent operator validation.The operator validation for CONSUMER auth is consistent with other operation methods in this controller.
Based on learnings, operator validation is conditional by auth type.
108-110: LGTM! Clean delegation to service layer.The methods
getEnvClusters,findMissEnvs, andgetEnvClusterInfoproperly delegate to the service layer with appropriate return types.Also applies to: 234-236, 242-244
143-146: The review comment references code that does not exist in the specified location.Lines 143-146 in
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javacontain the method signaturepublic ResponseEntity<List<OpenAppDTO>> getAppsBySelf(Integer page, Integer size), not the code snippet shown in the review. The problematic patternresult.setOwnerDisplayName(result.getOwnerName())does not appear anywhere in the file or the openapi v1 controller package. Additionally, no occurrences of assigningownerNamedirectly toownerDisplayNameexist in the codebase.Likely an incorrect or invalid review comment.
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Show resolved
Hide resolved
...portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java
Show resolved
Hide resolved
tacklequestions
force-pushed
the
master
branch
from
904d039 to
42ef76d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/pom.xml (1)
1-277: Update OpenAPI spec URL to use stable tag instead of unstable branch reference.The
apollo.openapi.spec.urlproperty should reference a stable release tag (e.g.,refs/tags/v0.1.0) rather than a branch reference likerefs/heads/main. Using branch references introduces brittleness—the spec could change unexpectedly if the branch is updated, causing inconsistent builds.
♻️ Duplicate comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
177-189: Centralize operator retrieval to avoid repeated NPE risks across local app operationsAll these sites call
UserIdentityContextHolder.getOperator().getUserId()without a null-check. If the operator context is missing, you’ll get NPEs during create/update/delete flows. Since you already need a helper forcreateAppInRemoteNew, reuse it here to make the failure mode explicit and consistent.Example refactor using
getOperatorUserIdOrThrow():@@ private App createAppInLocal(App app) { - String operator = UserIdentityContextHolder.getOperator().getUserId(); - app.setDataChangeCreatedBy(operator); - app.setDataChangeLastModifiedBy(operator); + String operatorUserId = getOperatorUserIdOrThrow(); + app.setDataChangeCreatedBy(operatorUserId); + app.setDataChangeLastModifiedBy(operatorUserId); @@ - for (Env env : envs) { - roleInitializationService.initClusterNamespaceRoles(appId, env.getName(), - ConfigConsts.CLUSTER_NAME_DEFAULT, UserIdentityContextHolder.getOperator().getUserId()); - } + for (Env env : envs) { + roleInitializationService.initClusterNamespaceRoles(appId, env.getName(), + ConfigConsts.CLUSTER_NAME_DEFAULT, operatorUserId); + } @@ public App createAppAndAddRolePermission(App app, Set<String> admins) { - rolePermissionService.assignRoleToUsers( - RoleUtils.buildAppMasterRoleName(createdApp.getAppId()), admins, - UserIdentityContextHolder.getOperator().getUserId()); + rolePermissionService.assignRoleToUsers( + RoleUtils.buildAppMasterRoleName(createdApp.getAppId()), admins, + getOperatorUserIdOrThrow()); @@ public App updateAppInLocal(App app) { - String operator = UserIdentityContextHolder.getOperator().getUserId(); - managedApp.setDataChangeLastModifiedBy(operator); + String operatorUserId = getOperatorUserIdOrThrow(); + managedApp.setDataChangeLastModifiedBy(operatorUserId); @@ public App deleteAppInLocal(String appId) { - String operator = UserIdentityContextHolder.getOperator().getUserId(); + String operator = getOperatorUserIdOrThrow(); @@ - managedApp.setDataChangeLastModifiedBy(operator); + managedApp.setDataChangeLastModifiedBy(operator); @@ - appRepository.deleteApp(appId, operator); + appRepository.deleteApp(appId, operator); @@ - appNamespaceService.batchDeleteByAppId(appId, operator); + appNamespaceService.batchDeleteByAppId(appId, operator); @@ - favoriteService.batchDeleteByAppId(appId, operator); + favoriteService.batchDeleteByAppId(appId, operator); @@ - rolePermissionService.deleteRolePermissionsByAppId(appId, operator); + rolePermissionService.deleteRolePermissionsByAppId(appId, operator);This keeps the existing operator string plumbing but removes the risk of unexpected NPEs by failing fast with a clear message when the context is absent. Based on learnings, ...
Also applies to: 203-207, 253-255, 272-292
132-147: Guard against null operator and avoid duplicate lookups increateAppInRemoteNew
UserIdentityContextHolder.getOperator()may benull(e.g., tests or non-filtered flows), so calling.getUserId()directly can NPE. You also call it twice in the same method. Retrieve the operator userId once via a small helper that null-checks and use it for both the audit fields and role initialization.Suggested refactor:
@@ - public void createAppInRemoteNew(Env env, App app) { - if (StringUtils.isBlank(app.getDataChangeCreatedBy())) { - String username = UserIdentityContextHolder.getOperator().getUserId(); - app.setDataChangeCreatedBy(username); - app.setDataChangeLastModifiedBy(username); - } + public void createAppInRemoteNew(Env env, App app) { + String operatorUserId = getOperatorUserIdOrThrow(); + if (StringUtils.isBlank(app.getDataChangeCreatedBy())) { + app.setDataChangeCreatedBy(operatorUserId); + app.setDataChangeLastModifiedBy(operatorUserId); + } @@ - roleInitializationService.initClusterNamespaceRoles(app.getAppId(), env.getName(), - ConfigConsts.CLUSTER_NAME_DEFAULT, UserIdentityContextHolder.getOperator().getUserId()); + roleInitializationService.initClusterNamespaceRoles(app.getAppId(), env.getName(), + ConfigConsts.CLUSTER_NAME_DEFAULT, operatorUserId); } + + private String getOperatorUserIdOrThrow() { + UserInfo operator = UserIdentityContextHolder.getOperator(); + if (operator == null || StringUtils.isBlank(operator.getUserId())) { + throw new IllegalStateException("Operator context is not set"); + } + return operator.getUserId(); + }(You can place
getOperatorUserIdOrThrownear other private helpers.) Also consider tying the TODO to a tracking issue or ticket so this transitional method is cleaned up after migration.
Based on learnings, ...apollo-portal/src/main/resources/static/scripts/AppUtils.js (1)
97-107: gatherData’s contract is incompatible with callers; make it shape‑aware and backward‑compatibleRight now
gatherData:
- Assumes
responseis an array.- Requires each element to have
code === 200to be included.- Returns the entity itself (not
body), and warns on non‑200.This breaks several realistic cases:
- If the new OpenAPI endpoints return plain DTO arrays (e.g.,
{env, clusters}or string envs),entity.codeisundefined, nothing is pushed, andnodes/missEnvsend up empty with spurious warnings.- If services still return wrapper entities like
{code, message, body}, callers that expect DTOs (node.env,node.clusters, simple env strings) now receive the wrapper object instead of thebody.To keep both old and new shapes working,
gatherDatashould:
- Accept either
responseorresponse.entities.- If an element has a numeric
code:
- On
code === 200, pushentity.bodywhen available, otherwise the entity itself.- On non‑200, show
entity.message(if present) and skip.- If an element has no
code, treat it as an already‑unwrapped DTO/primitive and push as‑is.A possible implementation:
- gatherData: function (response) { - var data = []; - response.forEach(function (entity) { - if (entity.code === 200) { - data.push(entity); - } else { - toastr.warning(entity.message); - } - }); - return data; - }, + gatherData: function (response) { + if (!response) { + return []; + } + + // Support both old `{ entities: [...] }` and new plain array responses + var entities; + if (Array.isArray(response)) { + entities = response; + } else if (Array.isArray(response.entities)) { + entities = response.entities; + } else { + return []; + } + + var data = []; + entities.forEach(function (entity) { + if (!entity) { + return; + } + + // Wrapper shape: { code, message, body } + if (typeof entity.code === 'number') { + if (entity.code === 200) { + if (entity.body !== undefined && entity.body !== null) { + data.push(entity.body); + } else { + data.push(entity); + } + } else if (entity.message) { + toastr.warning(entity.message); + } + return; + } + + // Plain DTO / primitive (new endpoints) + data.push(entity); + }); + return data; + },This should keep existing UIs working while supporting the new OpenAPI DTO responses.
🧹 Nitpick comments (17)
apollo-portal/pom.xml (1)
154-167: ReconsiderskipValidateSpec=true— validation suppression may hide schema issues.Setting
skipValidateSpec=truedisables OpenAPI specification validation during code generation. This could mask problems with the OpenAPI schema (malformed, invalid constraints, inconsistent types, etc.) that would only surface at runtime or integration time.Consider enabling validation (
skipValidateSpec=falseor remove the flag) to catch schema issues early during the build.apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js (1)
28-37: Ensure gatherData returns env-cluster DTOs, not wrapper entities
nodesis used as an array of objects withenvandclusters(node.env,node.clusters). This assumesAppUtil.gatherData(result)returns those DTOs directly, not wrapper objects (e.g.,{code,message,body}) or an empty array when items lackcode. Please confirm the server response shape and gatherData implementation line up with this expectation (see AppUtils.js comment).apollo-portal/src/main/resources/static/scripts/directive/directive.js (1)
165-186: Cluster selector also assumes gatherData returns{env, clusters}DTOs
envClusterInfois treated as an array of nodes with.envand.clusters. This depends onAppUtil.gatherData(result)returning those DTOs directly and not wrapper objects or an empty list. Please verify the response shape and gatherData logic so this selector stays compatible.apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
122-151: Confirm gatherData contract for miss envs/nav tree and consistency with collectData
findMissEnvsnow sets$scope.missEnvs = AppUtil.gatherData(result);but later treatsmissEnvsas a list of plain env strings (indexOf($rootScope.pageContext.env)).loadNavTreenow usesnodes = AppUtil.gatherData(result);and expects each node to expose.envand.clusters.findMissingNamespacesstill usesAppUtil.collectData(result);.These three helpers likely see different response shapes after the OpenAPI changes. Please verify:
- That
gatherDatareturns primitive env strings forfind_miss_envsand proper{env, clusters}DTOs forload_nav_tree, not wrapper entities.- That keeping
collectDataonly forfind_missing_namespacesis intentional (i.e., that endpoint still returns the oldentities-wrapper shape).If not, consider aligning these usages after adjusting
gatherData(see AppUtils.js).Also applies to: 191-200
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
59-60: Explicit EnvClusterInfo → OpenEnvClusterInfo mapping looks correctThe new
fromEnvClusterInfocorrectly:
- Extracts
Envand writes its string form intoOpenEnvClusterInfo.env.- Maps
List<ClusterDTO>toList<OpenClusterDTO>using the existingfromClusterDTO.- Handles null env and empty cluster lists safely.
If not already covered elsewhere, consider adding a small unit test for this converter to lock in the mapping semantics.
Also applies to: 399-412
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
27-37: Tests correctly adopt UserIdentityContextHolder for operator contextUsing
UserIdentityContextHolder.setOperator(...)in@BeforeEachand clearing in@AfterEachmatches the new production behavior and keeps the operator ID assertions (OPERATOR_USER_ID) meaningful.Optionally, you could defensively call
UserIdentityContextHolder.clear()at the start ofbeforeEach()as well, but the current setup is already sufficient.Also applies to: 56-100
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
19-21: Operator context handling in PortalUserSessionFilter looks sound; consider more defensive clearingThe filter now:
- Injects
UserInfoHolderand, for authenticated portal users, setsUserIdentityContextHolder.operatorwhen anoperatorparameter/attribute is present.- Wraps
chain.doFilterin a try/finally and callsUserIdentityContextHolder.clear()in the finally block, avoiding leaks for this branch.That’s a good pairing of set/clear on the ThreadLocal. As a minor defensive enhancement, you might also clear the context at the very beginning of
doFilterto guard against any future code paths that might set the operator without a corresponding clear.Also applies to: 60-66, 79-96
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (1)
153-161: Good operator context coverage; consider avoidinguserInfoHoldershadowingThe new
@MockBean UserInfoHolder, enhancedtearDown()(resetting mocks, clearingSecurityContextHolderandUserIdentityContextHolder), and theopenApiPortalRequestWithOperatorParam_shouldPopulateUserIdentityContexttest together give solid coverage thatPortalUserSessionFiltersets and clears the per‑thread operator correctly for portal OpenAPI requests.Minor readability nit: in the expired‑session OIDC tests you create a local
userInfoHoldermock that shadows the@MockBeanfield, which is easy to confuse when reading the file. Renaming the local variable would clarify intent without changing behavior, for example:- UserInfoHolder userInfoHolder = mock(UserInfoHolder.class); - PortalUserSessionFilter oidcFilter = - new PortalUserSessionFilter(oidcEnvironment, userInfoHolder); + UserInfoHolder localUserInfoHolder = mock(UserInfoHolder.class); + PortalUserSessionFilter oidcFilter = + new PortalUserSessionFilter(oidcEnvironment, localUserInfoHolder);(and likewise in the OpenAPI OIDC test).
Also applies to: 184-205, 217-244, 271-297
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
61-155: InstanceController integration tests exercise key OpenAPI v1 flowsThe tests validate:
- Numeric instance count responses for
/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/instances.- JSON page structure for
getByRelease.- Happy‑path and invalid‑input behaviors for
by-namespace-and-releases-not-in, includingverifyNoInteractionson badreleaseIds.If
InstanceControlleralso exposes agetByNamespace(/instances/by-namespace) endpoint, consider adding a small test for that path as well to keep coverage symmetric with the service interface.apollo-portal/src/main/resources/static/scripts/services/AppService.js (1)
127-132: Double‑check operator query value used by the portal UI
update,create_remote, anddelete_appnow always sendoperator: ''in the query parameters when calling the OpenAPI v1 endpoints. InPortalOpenApiAuthenticationScenariosTest, the portal‑side OpenAPI request explicitly usesoperator=keep-current-userwhen asserting thatPortalUserSessionFilterpopulatesUserIdentityContextHolderfor the current user.If the filter logic depends on the presence of a non‑empty sentinel value (like
"keep-current-user") rather than just a non‑null parameter, sending an empty string here might skip the operator‑population path for portal‑initiated calls.Consider aligning the frontend with the sentinel used in the filter tests, for example:
- app_resource.update_app({ - appId: app.appId, - operator: '' - }, app, function (result) { + app_resource.update_app({ + appId: app.appId, + operator: 'keep-current-user' + }, app, function (result) { @@ - app_resource.create_app_remote({env: env, operator: ''}, app, function (result) { + app_resource.create_app_remote({env: env, operator: 'keep-current-user'}, app, function (result) { @@ - app_resource.delete_app({ - appId: appId, - operator: '' - }, function (result) { + app_resource.delete_app({ + appId: appId, + operator: 'keep-current-user' + }, function (result) {(or whatever constant the backend expects), so portal UI behavior and filter expectations cannot drift apart. Please verify this against the actual
PortalUserSessionFilterimplementation.Also applies to: 139-147, 196-201
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
51-78: Align pagination null-handling and env parsing with other OpenAPI servicesThe delegation logic here is straightforward, but two aspects are worth tightening:
getByReleaseuses primitiveint page, int sizewhile the controller/HTTP layer exposesIntegerquery params. Ifpageorsizeare omitted and map tonull, auto-unboxing will throw an NPE before you can apply any defaults. Consider usingIntegerhere as well and deriving safe primitives with defaults (similar togetAppsWithPageAndSizeinServerAppOpenApiService), e.g. default page to 0 and size to a sane value.- All three methods use
Env.valueOf(env). If the env string is invalid,IllegalArgumentExceptionwill bubble up. For consistency withServerAppOpenApiService.createAppInEnv, you may want to catch this and throwBadRequestException.invalidEnvFormat(env)instead so callers always see a structured 400 for bad env values.This keeps the OpenAPI server surface resilient to missing pagination parameters and malformed env strings, and aligns behaviour with the other OpenAPI services.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
71-93: Exclude‑releases validation is solid; consider minor message alignmentThe null/empty checks, trimming/splitting, and numeric parsing for
releaseIdsare thorough and should prevent bad input from slipping through or causing 500s. The only small nit is that the first error mentions “release ids” while the others mention “excludeReleases parameter”; you may want to standardize the parameter name in messages for easier client debugging, but behaviour is otherwise fine.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
154-164: Pagination defaults are sensible; optionally guard against negative pages
getAppsWithPageAndSizenicely handles nullpage/sizeand uses a default size of 20 before building aPageable. One small robustness tweak would be to clamp negativepagevalues to 0 so that accidental negatives from callers don’t bubble intoPageableand cause exceptions, e.g.:int pageIndex = (page == null || page < 0) ? 0 : page;The rest of the method (null/empty appIds → empty list, conversion via
OpenApiModelConverters.fromApps) looks good.
173-189: Ensure operator is always set before using UserIdentityContextHolder in createAppInEnv
createAppInEnvultimately calls:roleInitializationService.initNamespaceSpecificEnvRoles( appEntity.getAppId(), ConfigConsts.NAMESPACE_APPLICATION, env, UserIdentityContextHolder.getOperator().getUserId());This assumes
UserIdentityContextHolder.getOperator()is non‑null for every call path (both CONSUMER and USER flows). The controller sets it for CONSUMER auth, and the portal filter is expected to set it for USER auth, but if either is misconfigured or if someone calls this service directly in the future, you’ll get an NPE here.Consider adding a defensive null check with a clear exception (e.g., a
BadRequestException.userNotExists(...)or a dedicated message) or at least asserting the invariant before dereferencing.
210-231: Confirm “OK” code semantics for missing envs in findMissEnvsIn
findMissEnvs, whenappService.load(env, appId)throws aHttpClientErrorExceptionwithNOT_FOUND, you set:missEnv.setCode(HttpStatus.OK.value()); missEnv.setMessage(env.toString());and only use
INTERNAL_SERVER_ERRORfor other exceptions. If this mirrors the legacyMultiResponseEntitysemantics, that’s fine, but the “200 OK” code for a missing app/env is a bit counter‑intuitive on first read.It may be worth double‑checking that clients indeed expect
code == 200to signal a “normal missing env” case here, and consider a more explicit domain code if not.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
83-105: Tighten operator derivation/validation in createApp for clearer failuresThe operator logic here:
String operator = app.getDataChangeCreatedBy() == null ? app.getOwnerName() : app.getDataChangeCreatedBy(); if (userService.findByUserId(operator) == null) { throw new BadRequestException("operator missing or not exist: " + operator); } UserIdentityContextHolder.setOperator(new UserInfo(operator));works assuming at least one of
dataChangeCreatedByorownerNameis non‑blank and maps to a real user. To make failures more explicit and avoid passingnullor empty values intouserService, you might:
- Guard against blank
operator(and possibly use existing helpers likeBadRequestException.ownerNameIsBlank()/userNotExists(...)for more specific messages).- Optionally normalize whitespace before lookup.
Behaviour is otherwise consistent with the new operator‑context model.
155-168: Consider extracting CONSUMER operator validation into a helperThe pattern:
if (UserIdentityConstants.CONSUMER.equals(UserIdentityContextHolder.getAuthType())) { if (userService.findByUserId(operator) == null) { throw BadRequestException.userNotExists(operator); } UserIdentityContextHolder.setOperator(new UserInfo(operator)); }is repeated across
updateApp,createAppInEnv, anddeleteApp. Extracting this into a private helper (e.g.,validateAndSetConsumerOperator(String operator)) would:
- Reduce duplication and the chance of future divergence between endpoints.
- Make it easier to adjust behaviour (logging, extra checks) in one place later.
Return types and overall flow of these methods look fine otherwise.
Also applies to: 201-211, 219-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apollo-portal/pom.xml(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(10 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.javaapollo-portal/pom.xmlapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
🧬 Code graph analysis (9)
apollo-portal/src/main/resources/static/scripts/directive/directive.js (1)
apollo-portal/src/main/resources/static/scripts/controller/config/DiffConfigController.js (1)
result(206-206)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(52-182)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(66-390)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
RunWith(45-157)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-93)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
RestController(55-245)
🪛 GitHub Actions: code style check
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
[error] 1-1: Spotless formatting violations detected in file. The following changes are required: remove/adjust imports and formatting to match Spotless rules. Run 'mvn spotless:apply' to fix these violations.
apollo-portal/pom.xml
[error] 1-1: Spotless check failed for apollo-portal module during mvn spotless:check. Build will fail until formatting is corrected.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (27)
apollo-portal/pom.xml (2)
31-33: Verify OpenAPI spec URL stability:refs/heads/mainis fragile.The property uses
refs/heads/mainwhich is a mutable branch reference. If branch naming or structure changes, this URL will break. Consider using a stable reference such as a release tag, version number, or pinned commit.Current: https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/heads/main/apollo-openapi.yaml Consider: https://raw.githubusercontent.com/apolloconfig/apollo-openapi/refs/tags/v<version>/apollo-openapi.yaml or a pinned commit hash
139-277: Plugin architecture and dependency management look sound.The use of
pluginManagementto declare centralized OpenAPI generator configuration, coupled with explicitpluginssection to invoke it, follows Maven best practices. Dependency scoping (test, runtime, optional) is correct. The build-helper-maven-plugin is properly configured to add OpenAPI-generated sources to the compile path.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
31-31: Import aligns with new operator context usageThe
UserIdentityContextHolderimport matches its usages below; no issues here.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
24-75: PortalUserSessionFilter bean wiring looks consistent with new constructorInjecting
UserInfoHolderintoportalUserSessionFilterand passing it tonew PortalUserSessionFilter(environment, userInfoHolder)matches the updated filter signature and keeps filter ordering/URL patterns intact.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (3)
101-127: Operator binding and updatedcreateAppInEnvsignature look correctThe test now verifies that only
envand body DTO reachAppOpenApiService.createAppInEnv(...)and that theoperatorquery param is used solely to resolve a user viauserService.findByUserId("bob"), which matches the new operator-resolution design.
129-151: RenamedgetAppsWithPageAndSizeservice call is wired as expectedThe by-self endpoint test correctly captures authorized app IDs plus
page/sizeand verifies delegation toappOpenApiService.getAppsWithPageAndSize(...), keeping behavior identical to the priorgetAppsBySelf(...)contract.
171-181:deleteApppath/query binding still verified end‑to‑endThe delete flow continues to assert correct binding of
appIdpath variable andoperatorquery parameter, while delegating onlyappIdtoappOpenApiService.deleteApp, which is consistent with the updated OpenAPI surface.apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (1)
19-37: InstanceService endpoints correctly migrated to/openapi/v1All four resource URLs (
by-release,by-namespace,by-namespace-and-releases-not-in, and instance count by namespace) now match the new OpenAPI v1 paths used in the server and tests, so the Angular service should remain in sync withInstanceController.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (1)
62-140: Low‑level parameter binding tests for InstanceController v1 are comprehensiveThis suite cleanly verifies:
- Correct path‑variable ordering for
getInstanceCountByNamespace.- Binding of
env,releaseId,page, andsizeforgetByRelease.- Rejection of empty and non‑numeric
releaseIdsbefore hitting the service.- Proper parsing and trimming of
releaseIdsinto aSet<Long>forgetByReleasesNotIn.It nicely complements the higher‑level
InstanceControllerTestassertions on response shapes.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
19-52: AppOpenApiService interface evolution aligns with new DTO‑centric APIReturning
OpenAppDTOfromcreateApp, renaminggetEnvClusterInfo→getEnvClusters, introducinggetAppsWithPageAndSize, and switchingfindMissEnvs/getEnvClusterInfostoOpenMissEnvDTO/OpenEnvClusterInfolists make the service contract much clearer and easier for controllers to consume. Keeping validation annotations out of this interface (aside from@NonNullon the create request) also matches the pattern of doing bean validation at the API/controller boundary rather than in service interfaces. Based on learnings, this is consistent with the existing OpenAPI service design.apollo-portal/src/main/resources/static/scripts/services/AppService.js (1)
19-68: AppService now targets the OpenAPI v1 app endpoints consistentlyThe Angular
$resourcemappings for listing, self apps, navtree/env‑cluster‑info, single app, create, remote create, miss‑envs, and delete are all updated to/openapi/v1/apps[...], withisArrayflags adjusted for collection vs single responses. This should keep the portal UI aligned with the newAppControllerv1 contract.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (6)
132-155:testGetEnvClusterscorrectly follows the renamed service methodStubbing and verifying
appOpenApiService.getEnvClusters(appId)and asserting on the JSONenvandclustersarray align with the new service name and DTO shape; this keeps coverage for the env‑cluster listing behavior intact.
203-240: Consumer‑awaregetApptests cover success, not‑found, and unauthorized casesThe trio of tests now:
- Derive
consumerIdviaConsumerAuthUtil.- Use
ConsumerService.findAppIdsAuthorizedByConsumerIdto gate access.- Exercise both the happy path and the two failure modes (not found vs unauthorized consumer) with appropriate expectations and service verifications.
That gives good confidence in the authorization flow around
/openapi/v1/apps/{appId}without over‑mocking internals.
259-288:testGetAppsBySelfcorrectly transitions togetAppsWithPageAndSizeThe test now passes the authorized app ID set plus
pageandsizeintoappOpenApiService.getAppsWithPageAndSize(...)and verifies both the JSON result and the exact service invocation, matching the new service surface.
291-305:testFindMissEnvsupdated to the newOpenMissEnvDTOcontractUsing
OpenMissEnvDTOwithcodeandmessagefields and verifying the JSON array response from/openapi/v1/apps/{appId}/miss-envsaccurately reflects the migrated API shape (plain DTO list instead of wrapped response).
307-322: NewtestGetEnvClusterInfosmatches the env‑cluster‑info DTO semanticsStubbing
appOpenApiService.getEnvClusterInfos(appId)and asserting onenvandmessagein the JSON array returned from/openapi/v1/apps/{appId}/env-cluster-infonicely covers the new env‑cluster‑info endpoint.
324-346: Update and delete app tests are aligned with the simplified response contractsFor update:
- The test sets up a
UserInfoprincipal, ensures the caller is an app admin, and only asserts 200 OK plus a singleupdateAppinvocation, which is appropriate now that the endpoint no longer returns a rich body.For delete:
- Expecting 200 OK with an empty JSON object
{}and verifying a singledeleteApp(appId)call matches the new delete‑semantics while still checking the authorization gate.Both tests exercise the critical behavior without over‑specifying response payload details.
Also applies to: 371-387
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
80-92: DTO page conversion looks correct and consistentThe
transformToOpenPageDTOhelper cleanly mapsPageDTO<InstanceDTO>toOpenInstancePageDTO(page, size, total, and converted instances), matching the pattern used elsewhere in the OpenAPI layer. No issues from a correctness perspective.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
42-66: Controller → service wiring for instance queries looks correctThe three methods (
getInstanceCountByNamespace,getByRelease,getByNamespace) correctly reorder parameters where needed and delegate straight toInstanceOpenApiService, returningResponseEntitywrappers as expected. No functional issues spotted here.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
86-93: createApp OpenAPI DTO conversion and admin handling look goodConverting the created
ApptoOpenAppDTOviaOpenApiModelConverters.fromAppand normalizingadminsto an empty set when null keeps the OpenAPI surface consistent and avoids null handling downstream. This method looks correct.
95-113: getEnvClusters implementation matches existing portal patternsIterating over
portalSettings.getActiveEnvs(), collecting cluster names fromclusterService.findClusters, and returningOpenEnvClusterDTOinstances is in line with existing portal logic. The null‑safe handling ofclusterDTOsviaCollections.emptySet()is also appropriate.
240-258: getEnvClusterInfos error/success mapping looks consistentWrapping successful
createEnvNavNoderesults inOpenEnvClusterInfowithcodeset toHttpStatus.OK.value()and populating a fallbackOpenEnvClusterInfowithINTERNAL_SERVER_ERROR, env, and message on exceptions is a clear and backward‑compatible pattern. No issues from a correctness standpoint.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (5)
108-110: getEnvClusters delegation is straightforwardThe controller simply wraps
appOpenApiService.getEnvClusters(appId)in a 200ResponseEntity, which matches the new service API and is easy for clients to consume. No issues here.
143-147: Good: enrich getApp response with ownerDisplayNamePopulating
ownerDisplayNamefromownerNamebefore returning the singleOpenAppDTOpreserves existing UI expectations while keeping the underlying service API unchanged. This is a clean place to apply that presentation tweak.
174-193: getAppsBySelf correctly differentiates CONSUMER and USER flowsThe CONSUMER path using
ConsumerServiceand the USER path usingRolePermissionService+RoleUtils.extractAppIdFromRoleNamegive a coherent view of “apps I can operate,” and delegation toappOpenApiService.getAppsWithPageAndSizecentralizes pagination logic. The control flow and set handling look correct.
234-236: findMissEnvs now cleanly surfaces DTO list from serviceReturning
ResponseEntity<List<OpenMissEnvDTO>>directly fromappOpenApiService.findMissEnvs(appId)simplifies the controller and aligns it with the new DTO‑based service contract. No problems here.
242-243: getEnvClusterInfo delegation to DTO‑based service looks goodDelegating to
appOpenApiService.getEnvClusterInfos(appId)and returningResponseEntity<List<OpenEnvClusterInfo>>matches the refactored service surface and removes the need for controller‑side wrapping. Looks correct.
...ain/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
apollo-portal/src/main/resources/static/scripts/AppUtils.js (1)
97-107: ClarifygatherDatacontract and make it shape‑aware for backward compatibility
gatherDatacurrently assumesresponseis an array of{code,message,...}and returns the full entity objects on success, whereascollectDataworks onresponse.entitiesand returnsentity.body. Any caller migrated fromcollectData(result)togatherData(result)but still expecting an array of primitive/body values will now receive a different shape and may break.To keep this utility robust and backward‑compatible, consider making it tolerant of both the old and new response shapes and preserving the primitive/body contract when available:
- gatherData: function (response) { - var data = []; - response.forEach(function (entity) { - if (entity.code === 200) { - data.push(entity); - } else { - toastr.warning(entity.message); - } - }); - return data; - }, + gatherData: function (response) { + var data = []; + // Support both array responses and { entities: [...] } wrappers + var items = Array.isArray(response) + ? response + : (response && Array.isArray(response.entities) ? response.entities : []); + + items.forEach(function (item) { + // New-style entity: { code, message, body? } + if (item && typeof item === 'object' && typeof item.code !== 'undefined') { + if (item.code === 200) { + // Preserve old contract: prefer body when present + if (Object.prototype.hasOwnProperty.call(item, 'body')) { + data.push(item.body); + } else { + data.push(item); + } + } else if (item.message) { + toastr.warning(item.message); + } + } else { + // Backward compat for primitive arrays (e.g. env strings) + data.push(item); + } + }); + return data; + },apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (6)
177-179: Same NPE risk pattern in createAppInLocal.Line 177 follows the same unsafe pattern. Apply the null-safe helper method suggested in the previous comment.
188-188: Same NPE risk pattern in createAppInLocal (role initialization).Line 188 follows the same unsafe pattern. Apply the null-safe helper method suggested above.
206-206: Same NPE risk pattern in createAppAndAddRolePermission.Line 206 follows the same unsafe pattern. Apply the null-safe helper method suggested above.
253-254: Same NPE risk pattern in updateAppInLocal.Line 253 follows the same unsafe pattern. Apply the null-safe helper method suggested above.
272-272: Same NPE risk pattern in deleteAppInLocal.Line 272 follows the same unsafe pattern. Apply the null-safe helper method suggested above.
135-147: Critical NPE risk: getOperator() can return null.
UserIdentityContextHolder.getOperator()returns null if the context hasn't been populated. Directly calling.getUserId()(lines 137, 146) will throw NPE in scenarios where:
- The filter doesn't set operator context (e.g., non-OpenAPI flows)
- Testing without proper context setup
- Unexpected request paths
Consider adding a null-safe helper method or explicit null checks:
+ private String getOperatorUserId() { + UserInfo operator = UserIdentityContextHolder.getOperator(); + if (operator == null) { + throw new BadRequestException("Operator context is not set"); + } + return operator.getUserId(); + } + public void createAppInRemoteNew(Env env, App app) { if (StringUtils.isBlank(app.getDataChangeCreatedBy())) { - String username = UserIdentityContextHolder.getOperator().getUserId(); + String username = getOperatorUserId(); app.setDataChangeCreatedBy(username); app.setDataChangeLastModifiedBy(username); } AppDTO appDTO = BeanUtils.transform(AppDTO.class, app); appAPI.createApp(env, appDTO); roleInitializationService.initClusterNamespaceRoles(app.getAppId(), env.getName(), - ConfigConsts.CLUSTER_NAME_DEFAULT, UserIdentityContextHolder.getOperator().getUserId()); + ConfigConsts.CLUSTER_NAME_DEFAULT, getOperatorUserId()); }Apply the same helper method to all other usages in this file (lines 177, 188, 206, 253, 272).
🧹 Nitpick comments (6)
apollo-portal/pom.xml (2)
31-33: Network dependency on external OpenAPI spec during build.The build depends on fetching the OpenAPI spec from GitHub at build time. If the URL is unreachable, changes, or the file is deleted, builds will fail. The
<skipValidateSpec>true</skipValidateSpec>(line 166) suppresses validation, which may mask spec format issues.Consider whether this is acceptable for your build pipeline, or if the spec should be vendored into the repository.
162-166: Consider the implications ofskipValidateSpec>true.Skipping OpenAPI spec validation may allow invalid or incomplete specs to be used for code generation, potentially resulting in broken generated code. If this is necessary due to spec issues, document the reason and plan to address the underlying spec problems.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
45-50: Minor Javadoc typo in comment.Line 46 has "current_thread" which should be "current thread" (with a space) for consistency with the other Javadoc comments in this file (e.g., line 32 "current thread").
Apply this diff:
/** - * Read operator for current_thread + * Read operator for current thread */ public static UserInfo getOperator() { return OPERATOR_HOLDER.get(); }apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
85-111: Type mismatch:releaseIdshould belongto match service signature.The test declares
releaseIdasinton line 88, butInstanceOpenApiService.getByReleaseexpects alongparameter. While Java will auto-wideninttolong, using the correct type improves clarity and consistency with the API contract.public void testGetInstancesByRelease() throws Exception { String env = "DEV"; - int releaseId = 100; + long releaseId = 100L; int page = 0; int size = 10;apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
167-168: Use parameterizedHashMap<>()orCollections.emptyMap()to avoid raw type warning.Raw types are used when returning empty maps. Consider using diamond operator or
Collections.emptyMap()for type safety.- return ResponseEntity.ok(new HashMap<>()); + return ResponseEntity.ok(Collections.emptyMap());Apply similar fix to lines 210 and 227.
159-164: Consider extracting duplicated operator validation logic.The same pattern for consumer operator validation appears in
updateApp,createAppInEnv, anddeleteApp:if (UserIdentityConstants.CONSUMER.equals(UserIdentityContextHolder.getAuthType())) { if (userService.findByUserId(operator) == null) { throw BadRequestException.userNotExists(operator); } UserIdentityContextHolder.setOperator(new UserInfo(operator)); }Consider extracting this to a private helper method to improve maintainability.
private void validateAndSetOperatorForConsumer(String operator) { if (UserIdentityConstants.CONSUMER.equals(UserIdentityContextHolder.getAuthType())) { if (userService.findByUserId(operator) == null) { throw BadRequestException.userNotExists(operator); } UserIdentityContextHolder.setOperator(new UserInfo(operator)); } }Also applies to: 201-207, 220-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apollo-portal/pom.xml(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(10 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apollo-portal/src/main/resources/static/scripts/directive/directive.js
- apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java
- apollo-portal/src/main/resources/static/scripts/services/InstanceService.js
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java
- apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/resources/static/scripts/services/AppService.js
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
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/ServerInstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
🧬 Code graph analysis (8)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-93)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
🔇 Additional comments (19)
apollo-portal/pom.xml (2)
139-276: Build structure and plugin configuration look sound.The restructuring correctly separates plugin version/configuration management (pluginManagement block) from plugin invocation (plugins block). Versions are pinned, execution phases are appropriate, and the approach of generating code during generate-sources phase is standard practice. Verify only that the generated sources path aligns with actual generator output.
155-160: The generated sources path configuration is correct; no adjustment needed.The openapi-generator-maven-plugin with the Spring generator does create a nested
src/main/javasubdirectory within the configured output directory. The configuration correctly specifies${project.build.directory}/generated-sources/openapias the output directory, and the Spring generator places generated source files under${project.build.directory}/generated-sources/openapi/src/main/java, which matches the path the build-helper-maven-plugin references. This configuration pattern is standard for the openapi-generator Spring generator and will not cause build failures.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
399-412: LGTM! Explicit conversion is safer for OpenAPI boundaries.The manual construction of
OpenEnvClusterInfowith explicit null/empty checks and direct field mapping is more reliable than generic reflection-based transformation when crossing API boundaries.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
84-96: LGTM! Proper thread-local lifecycle management.The try/finally block ensures
UserIdentityContextHolder.clear()is always called, which is critical for preventing memory leaks in thread-pooled servlet containers. The condition correctly checks both parameter and attribute for the operator.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
67-77: LGTM! Straightforward dependency injection update.The addition of
UserInfoHolderparameter aligns with the updatedPortalUserSessionFilterconstructor requirements.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (1)
271-297: LGTM! Comprehensive operator context lifecycle test.The new test validates:
- Operator context is correctly populated during filter chain execution (line 292)
- Context is properly cleared after the chain completes (line 296)
This ensures the thread-local lifecycle is correctly managed.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
29-52: Interface signature changes align with OpenAPI DTO migration.The updated method signatures reflect the broader refactoring to use explicit OpenAPI DTOs (
OpenEnvClusterInfo,OpenMissEnvDTO) and remove wrapper responses. The removal of theoperatorparameter fromcreateAppInEnvsuggests operator is now sourced from context (e.g.,UserIdentityContextHolder).apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
47-88: LGTM! New instance query methods follow consistent patterns.The new methods correctly:
- Delegate to
instanceServicefor data retrieval- Convert portal DTOs to OpenAPI DTOs via
OpenApiModelConverters- Use the
transformToOpenPageDTOhelper for pagination mappingThe helper correctly maps all pagination fields (page, size, total, instances).
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
61-155: Good test coverage for the new InstanceController endpoints.The test class provides solid coverage for the new OpenAPI Instance endpoints:
- Count by namespace
- Instances by release with pagination
- Instances excluding releases
- Invalid release ID validation
Consider adding tests for edge cases like empty result sets or boundary pagination values in follow-up work.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (3)
203-223: TheownerDisplayNameassertion is now valid.The past review flagged a potential issue with asserting
ownerDisplayNamewhen onlyownerNamewas set. However, the updatedAppController.getApp()method (lines 143-144 in AppController.java) now explicitly setsownerDisplayNamefromownerName:result.setOwnerDisplayName(result.getOwnerName());This makes the test assertion correct.
242-256: Good addition of unauthorized consumer test.This test properly verifies that consumers cannot access apps they're not authorized for, and that the service is not invoked when authorization fails.
307-322: NewgetEnvClusterInfostest looks good.The test properly sets up the mock, performs the request, and verifies both the response content and service invocation.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
71-94: Input validation is thorough and robust.The validation logic correctly handles:
- Null/empty string input (lines 74-76)
- Whitespace-only or comma-separated empty values via
omitEmptyStrings().trimResults()(lines 77-82)- Non-numeric release IDs with proper exception wrapping (lines 84-90)
The error messages are clear and consistent with
BadRequestExceptionpatterns used elsewhere in the codebase.
42-66: Controller implementation looks clean.The controller properly delegates to
InstanceOpenApiServiceand wraps responses inResponseEntity. The method signatures align with theInstanceManagementApiinterface contract.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
210-231:findMissEnvsimplementation is correct.The method properly:
- Iterates through active environments
- Returns HTTP 200 status for genuine "not found" cases (app missing in env)
- Returns HTTP 500 for unexpected errors with descriptive messages
240-258:getEnvClusterInfosproperly handles per-environment errors.Good resilience pattern: each environment is processed independently, and failures in one environment don't prevent returning results for others. Error details are captured in the DTO for client-side handling.
184-189: The review comment is incorrect. The code at lines 184-189 does not callUserIdentityContextHolder.getOperator(). Instead, it receivesoperatoras a validated String parameter from the controller method signature, which is already validated for non-null and user existence inAppController.createAppInEnv(lines 162-164) before being passed to the service.The actual code passes the operator parameter directly to
roleInitializationService.initNamespaceSpecificEnvRoles()without any risk of NPE.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
174-193:getAppsBySelfimplementation handles both auth contexts well.The logic correctly differentiates between consumer and user contexts:
- Consumer: uses
ConsumerServiceto find authorized apps- User: extracts app IDs from user roles via
RolePermissionServiceThe null check on line 185 properly handles cases where
extractAppIdFromRoleNamereturns null.
83-105:createAppoperator derivation and context setup looks correct.The logic properly:
- Falls back to
ownerNameifdataChangeCreatedByis not set- Validates operator existence before proceeding
- Sets the operator context for downstream services
- Conditionally assigns app role to consumer when requested
tacklequestions
force-pushed
the
master
branch
2 times, most recently
from
4e43264 to
f6a016e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
122-132: gatherData shape mismatch breaksmissEnvsand nav tree consumersBoth usages:
- Line 125:
$scope.missEnvs = AppUtil.gatherData(result);- Line 195:
var nodes = AppUtil.gatherData(result);assume
gatherDatareturns the underlying payload (env strings / nav-tree DTOs), but the current implementation returns the wrapper entities themselves (withcode/message/body).This causes multiple issues:
createAppInMissEnviterates$scope.missEnvsand passes eachenvintoAppService.create_remote(env, ...). With the currentgatherData,envwill be an entity object, not the env identifier the service likely expects.- The condition
if ($rootScope.pageContext.env && $scope.missEnvs.indexOf($rootScope.pageContext.env) === -1)compares a string to an array of objects, so it will always treat the env as “not missing”.loadNavTreetreats each element ofnodesas{ env, clusters }, but with the currentgatherDatait will instead receive{ code, message, body }and attempts likeenv.envorenv.clusterswill fail or produce undefined.These call sites either need to consume
entity.body(e.g.AppUtil.gatherData(result).map(e => e.body)) or, preferably,gatherDataitself should be adjusted to return the body/primitive for successful items so the external contract matches previouscollectDatabehavior and keeps these flows working.Also applies to: 191-201
♻️ Duplicate comments (3)
apollo-portal/src/main/resources/static/scripts/AppUtils.js (1)
97-107: MakegatherDatabackward‑compatible and payload‑oriented (return body/primitive, not wrapper entities)Current
gatherData:
- Assumes
responseis already an array.- Returns the wrapper entities with
{code,message,body}instead of the actual payload (body/primitive).- Misaligns with callers that expect plain env strings or DTOs (see
ConfigBaseInfoController.js) and with the existingcollectDatacontract.To avoid breaking existing and new callers, consider normalizing both old and new response shapes and returning the underlying payload:
- Support:
- Old
{ entities: [...] }shape.- New plain arrays of
{code,message,body}.- Arrays of primitives (no
code/body).A possible implementation:
- gatherData: function (response) { - var data = []; - response.forEach(function (entity) { - if (entity.code === 200) { - data.push(entity); - } else { - toastr.warning(entity.message); - } - }); - return data; - }, + gatherData: function (response) { + var data = []; + if (!response) { + return data; + } + + // Support both { entities: [...] } and plain arrays + var entities = Array.isArray(response.entities) ? response.entities : response; + + entities.forEach(function (entity) { + // Primitive values: keep as-is + if (entity === null || typeof entity !== 'object') { + data.push(entity); + return; + } + + // Wrapper with code/message/body (OpenAPI-style) + if (typeof entity.code !== 'undefined') { + if (entity.code === 200) { + if (Object.prototype.hasOwnProperty.call(entity, 'body')) { + data.push(entity.body); + } else { + data.push(entity); + } + } else if (entity.message) { + toastr.warning(entity.message); + } + return; + } + + // Fallback: object without code property + data.push(entity); + }); + + return data; + },This keeps
gatherDatausable for both legacy and new endpoints while ensuring callers receive the actual payloads they operate on (env strings, DTOs, etc.).apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
135-147: Null-safety issue already flagged in previous review.Past review comments have identified the NPE risk from calling
.getUserId()on a potentially null operator returned byUserIdentityContextHolder.getOperator(). The same pattern appears throughout this file at lines 177, 188, 206, 253, and 272. Please address the null-safety concerns raised in the previous review.apollo-portal/src/main/resources/static/scripts/controller/AppController.js (1)
111-121: HardcodedassignAppRoleToSelf: falseissue already flagged.A previous review comment identified that
assignAppRoleToSelfis hardcoded tofalseeven when the current user is added to theadminsarray (lines 102-103). The suggested fix is to compute this boolean dynamically based on whether the current user is present in the admins list.
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
83-105: Consider clearer error messaging for operator validation.The operator derivation logic at lines 91-92 falls back from
dataChangeCreatedBytoownerName, but if both are null or invalid, the error message "operator missing or not exist: null" doesn't clearly indicate which field needs to be provided.Consider improving the error message to guide API consumers:
- if (userService.findByUserId(operator) == null) { - throw new BadRequestException("operator missing or not exist: " + operator); + if (operator == null) { + throw new BadRequestException("operator must be specified via dataChangeCreatedBy or ownerName"); + } + if (userService.findByUserId(operator) == null) { + throw BadRequestException.userNotExists(operator); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apollo-portal/pom.xml(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(9 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java
- apollo-portal/src/main/resources/static/scripts/directive/directive.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/pom.xmlapollo-portal/src/main/resources/static/scripts/services/InstanceService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/resources/static/scripts/services/AppService.js
🧬 Code graph analysis (8)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(52-182)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
RunWith(45-157)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(79-465)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-93)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (28)
apollo-portal/pom.xml (4)
31-33: The OpenAPI spec URL is already pinned to a stable version.The code uses
refs/tags/v0.1.0rather thanrefs/heads/main, which ensures reproducible builds and prevents unexpected breakage from upstream spec changes. No action required.Likely an incorrect or invalid review comment.
257-275: build-helper-maven-plugin path is correctly configured.The build-helper-maven-plugin source path
${project.build.directory}/generated-sources/openapi/src/main/javacorrectly aligns with the openapi-generator-maven-plugin's output directory structure. The spring generator creates generated Java sources undersrc/main/javaby default within the specified output directory, so this configuration is correct.
139-277: Plugin execution order is correct; no phase ordering issues.The build configuration properly handles OpenAPI code generation and integration. The openapi-generator-maven-plugin is declared in the
<plugins>section before build-helper-maven-plugin, ensuring openapi-generator executes first and generates sources before build-helper adds them to the compile path. Both plugins execute in the generate-sources phase, and Maven guarantees execution order based on declaration order within the same phase.
38-138: Dependency version management from parent POM is properly configured.All dependencies in this section correctly omit explicit
<version>tags and inherit versions from the parent POM, which is a best practice for multi-module projects. Verification confirms:
- Apollo internal dependencies (apollo-common, apollo-openapi, apollo-audit-spring-boot-starter) are explicitly versioned in the parent's
<dependencyManagement>section.- OpenAPI-related libraries (jackson-databind-nullable, swagger-annotations, swagger-models) are versioned in the parent's
<dependencyManagement>via version properties.- OAuth2 and Spring Session dependencies are managed through the Spring Boot 2.7.11 BOM imported in the parent POM, which ensures alignment across the Spring ecosystem.
- No explicit version conflicts are present in the dependency hierarchy.
The structure properly leverages multi-level dependency management: parent POM's explicit declarations for Apollo artifacts and OpenAPI libraries, combined with Spring Boot BOM for Spring ecosystem libraries. This approach ensures reproducibility and maintainability.
apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (2)
19-32: LGTM! Consistent API namespace migration.The first three endpoint URLs have been correctly updated to use the
/openapi/v1/namespace while preserving the existing URL structure and parameters.
33-37: The endpoint implementation is correct and matches the frontend code. The backendInstanceControllerclass has a class-level@RequestMapping("/openapi/v1/envs/{env}")annotation that, when combined with the method-level@GetMapping("/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/instances"), produces the exact URL path used in the frontend. The method correctly returnsint(a count value), not a collection, which aligns with both the function namegetInstanceCountByNamespaceand the expected behavior. No breaking changes or semantic mismatches exist.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
19-65: LGTM! Clean implementation of per-thread operator context.The new operator holder follows the established pattern for AUTH_TYPE_HOLDER and properly cleans up in the
clear()method to prevent memory leaks. The implementation enables thread-safe operator propagation across the request lifecycle.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
47-88: LGTM! Clean DTO conversion layer for instance queries.The new methods properly delegate to the underlying
instanceServiceand consistently convert between internal DTOs and OpenAPI DTOs usingOpenApiModelConverters. ThetransformToOpenPageDTOhelper eliminates duplication and maintains a clear separation of concerns.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
210-258: Good error handling with explicit DTO-based responses.The updated
findMissEnvsandgetEnvClusterInfosmethods properly populate error codes and messages in DTOs, replacing the previous wrapper-based approach. This provides clearer API responses and better error context for clients.apollo-portal/src/main/resources/static/scripts/services/AppService.js (2)
22-68: LGTM! Consistent migration to OpenAPI v1 endpoints.All endpoint URLs have been consistently updated to use the
/openapi/v1prefix, aligning with the backend OpenAPI refactoring. The payload structures and return types match the new DTO-based API surface.
130-131: Pass the current user ID for theoperatorparameter instead of an empty string.The backend methods
updateApp(line 134-135),createAppInEnv(line 162-163), anddeleteApp(line 177-178) all validate the operator parameter by checking if the user exists viauserService.findByUserId(operator). Passingoperator: ''will cause aBadRequestExceptionfor all three operations since an empty string is not a valid user ID. Theoperatorparameter must be populated with the actual current user's ID.⛔ Skipped due to learnings
Learnt from: tacklequestions Repo: apolloconfig/apollo PR: 5475 File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105 Timestamp: 2025-10-27T04:54:47.833Z Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type: - USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated - CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists) This pattern applies to delete operations where the operator source differs by auth type.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
271-297: LGTM! Comprehensive test for operator context lifecycle.The new test properly verifies that:
UserIdentityContextHolder.getOperator()is populated during the filter chain when an authenticated portal user is present- The context is cleared after the filter completes
This ensures the operator context follows the expected request lifecycle without leaking across threads.
153-161: Good addition of userInfoHolder cleanup.The
tearDownmethod now properly resets theuserInfoHoldermock and clears bothSecurityContextHolderandUserIdentityContextHolder, preventing test state leakage.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (1)
1-141: LGTM! Thorough parameter binding tests for InstanceController.This test class comprehensively validates:
- Path variable binding for instance count endpoint
- Query parameter conversion (string to long for releaseId)
- Input validation for empty and non-numeric release IDs
- Complex parameter binding including Set extraction from comma-separated values with whitespace handling
The use of
ArgumentCaptorat lines 134-139 to verify exact Set contents is particularly good practice.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (5)
204-223: Past review concern aboutownerDisplayNamehas been resolved.The previous review flagged that the test sets
app.setOwnerName("apollo-owner")but asserts on$.ownerDisplayName. This is now correct becauseAppController.getApp()(lines 143-144 in AppController.java) explicitly copiesownerNametoownerDisplayName:OpenAppDTO result = apps.get(0); result.setOwnerDisplayName(result.getOwnerName());The test should pass as expected.
226-257: LGTM! Authorization test coverage is appropriate.The new tests
testGetAppNotFoundandtestGetAppUnauthorizedForConsumerproperly verify the authorization flow for consumer-based access, ensuring that:
- Empty results trigger a bad request
- Unauthorized consumers cannot access apps outside their authorization scope
260-289: LGTM! Test updated correctly for pagination.The test properly verifies the new
getAppsWithPageAndSizemethod signature and behavior, with correct mock setup and verification.
308-323: LGTM! New test coverage for env cluster info.The test properly validates the new
getEnvClusterInfosendpoint with correct assertions on theOpenEnvClusterInfostructure.
342-390: LGTM! Test expectations aligned with new response format.The tests correctly expect empty JSON maps (
{}) forupdateAppanddeleteAppoperations, aligning with the controller's updated return types.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
33-66: LGTM! Controller structure and simple delegations are correct.The controller properly implements
InstanceManagementApiwith clean delegation toInstanceOpenApiServicefor the simple query methods.
71-94: LGTM! Input validation is comprehensive and defensive.The method properly validates the
releaseIdsparameter with:
- Null/empty string check
- Post-split empty collection check
- Numeric parsing with clear error messages
The validation addresses past review concerns about NPE and provides clear feedback for invalid inputs.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (7)
62-75: LGTM! Dependencies properly injected.The constructor correctly adds
RolePermissionServiceandUserInfoHolderto support the new operator validation and role management features.
108-120: LGTM! Methods correctly delegate to service layer.The
getEnvClustersrename andfindAppsconditional logic are both implemented correctly.
138-147: LGTM! Explicit ownerDisplayName population is correct.The method properly sets
ownerDisplayNamefromownerName, ensuring the DTO is complete for API consumers. This aligns with the test expectations inAppControllerTest.
155-168: LGTM! Conditional operator validation follows established pattern.The method correctly validates the operator for CONSUMER auth type while allowing USER auth type to derive the operator from the session context. This aligns with the learning that operator validation is conditional based on auth type.
Based on learnings, this pattern is correct for update operations.
201-211: LGTM! Operator validation consistent with other methods.The method follows the established pattern for consumer operator validation and delegates to the service layer. The service can retrieve the operator from
UserIdentityContextHolderas needed.
219-244: LGTM! Remaining methods correctly implement the updated API surface.The
deleteApp,findMissEnvs, andgetEnvClusterInfomethods properly:
- Follow the conditional operator validation pattern for consumer auth
- Return the correct DTO types aligned with the OpenAPI specification
- Delegate appropriately to the service layer
174-193: This review comment references code that no longer exists in the current version of this file.The code snippet provided in the review (lines 174-193 showing conditional CONSUMER/USER authentication paths with role iteration) does not match the current implementation in
AppController.java. The current code at lines 146-152 contains only the consumer authentication path and retrieves authorized apps viaconsumerService.findAppIdsAuthorizedByConsumerId(), without the role-based iteration logic the review addresses.The concerns about including namespace-level or cluster-level roles in the app listing no longer apply to this implementation.
...src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
Show resolved
Hide resolved
tacklequestions
force-pushed
the
master
branch
from
f6a016e to
2c8c5ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/resources/static/scripts/controller/config/ConfigBaseInfoController.js (1)
145-145: Consolidate to usegatherDatafor consistency with other methods.Line 145 still uses
AppUtil.collectData(result), while lines 125 and 195 have been updated to useAppUtil.gatherData(result). Both functions are identical in implementation—they iterate throughresult.dataand return an array. This inconsistency should be resolved by updating line 145 to usegatherDatato maintain consistency across all three methods.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
438-467: OpenItemDiffDTO class is missing from the codebase. The code imports and instantiatesOpenItemDiffDTOat line 440, but this class does not exist in the repository. This will cause a compilation error. You must create theOpenItemDiffDTOclass with the required fields (namespace,createItems,updateItems,deleteItems, andmessage) before this converter method can function.
🧹 Nitpick comments (6)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (1)
271-297: Optional: add an exception-path assertion to prove ThreadLocal cleanup is in afinally.Right now you validate “set during chain” and “cleared after” on the happy path. Consider also having the chain throw and then asserting
UserIdentityContextHolder.getOperator()is cleared, to lock in leak-proof behavior.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
27-36: Align nullability/types with controller/API to avoid unboxing NPEs
getByRelease(String env, long releaseId, int page, int size)will NPE if the OpenAPI layer ever passes nulls (boxed types auto-unboxed). Consider using boxed types in the service interface as well, consistent withgetByNamespace(..., Integer page, Integer size).- OpenInstancePageDTO getByRelease(String env, long releaseId, int page, int size); + OpenInstancePageDTO getByRelease(String env, Long releaseId, Integer page, Integer size);apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (3)
73-83: AvoidMockMvcResultHandlers.print()in committed tests (too noisy)
Consider removing.andDo(MockMvcResultHandlers.print())unless diagnosing a specific flaky test.Also applies to: 101-111, 125-136, 146-155
85-111: Uselong/LongforreleaseIdin tests to match the API contract
Right nowint releaseId = 100;doesn’t reflect real IDs and can mask long-specific binding/overflow edges.- int releaseId = 100; + long releaseId = 100L;
73-77: Prefer MockMvc path templates overString.formatURLs
This tends to be less error-prone and reads closer to how routes are defined.Also applies to: 101-105, 126-131, 147-152
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
142-145: Clarify theownerDisplayNamelogic.The code sets
ownerDisplayNameto the same value asownerName. If this is intentional (e.g., as a placeholder for future enhancement where display name differs from username), consider adding a comment. Otherwise, verify ifownerDisplayNameshould be populated from a different source (e.g., user's full name fromUserService).OpenAppDTO result = apps.get(0); // TODO: Enhance to fetch actual display name from UserService if available result.setOwnerDisplayName(result.getOwnerName());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apollo-portal/pom.xml(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(10 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(9 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java
- apollo-portal/src/main/resources/static/scripts/AppUtils.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
- apollo-portal/src/main/resources/static/scripts/directive/directive.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-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/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.javaapollo-portal/pom.xmlapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.javaapollo-portal/src/main/resources/static/scripts/services/InstanceService.js
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/main/resources/static/scripts/services/AppService.js
📚 Learning: 2025-12-07T07:28:11.982Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
🧬 Code graph analysis (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (1)
RunWith(48-141)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-112)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/ItemBO.java (1)
ItemBO(21-76)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/NamespaceBO.java (1)
NamespaceBO(23-103)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceSyncModel.java (1)
NamespaceSyncModel(26-70)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/EnvClusterInfo.java (1)
EnvClusterInfo(24-48)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/NamespaceIdentifier.java (1)
NamespaceIdentifier(23-72)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (33)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
97-97: LGTM! Return type now accurately reflects empty response.The change from
ResponseEntity<Object>toResponseEntity<Void>is appropriate because the method returns an empty response body viaResponseEntity.ok().build(). This makes the API contract more explicit and aligns with REST best practices for DELETE operations.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
153-161: Good test isolation: reset mocks + clearSecurityContextHolderandUserIdentityContextHolder.This is the right place to prevent ThreadLocal/security context leakage across tests.
31-37: No actionable issues found. The test correctly usescom.ctrip.framework.apollo.portal.entity.bo.UserInfoconsistently acrossUserIdentityContextHolder,UserInfoHolder, and test assertions. There is no type mismatch.apollo-portal/pom.xml (1)
52-62: Confirm OpenAPI and Swagger dependency versions are locked or managed.New OpenAPI and Swagger dependencies (
jackson-databind-nullable,swagger-annotations,swagger-models) have been added. Ensure their versions are either explicitly pinned in<dependencyManagement>in the parent POM or managed consistently. This prevents version drift and reduces supply-chain risk.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
27-36: Parameter ordering is inconsistent across methods—confirm this matches established patternYou have
getInstanceCountByNamespace(String appId, String env, ...)butgetByNamespace(String env, String appId, ...). If this is intentional to match existing OpenAPI/service conventions, OK—otherwise consider standardizing. (Based on learnings, consistency with existing patterns is preferred.)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
42-66: Delegation + ResponseEntity wrapping looks consistent with other v1 controllers
TheResponseEntity.ok(...)pattern and env/appId parameter adaptation are clear and align with the v1 controller style shown in similar controllers.apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js (1)
30-30: LGTM! Data collection method updated.The migration from
collectDatatogatherDataaligns with the PR's objective to handle new OpenAPI DTO responses.apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (2)
125-125: LGTM! Data collection method updated.The migration from
collectDatatogatherDatain thefindMissEnvsfunction aligns with the PR's objective to handle new OpenAPI DTO responses.
195-195: LGTM! Data collection method updated.The migration from
collectDatatogatherDatain theloadNavTreefunction aligns with the PR's objective to handle new OpenAPI DTO responses.apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (2)
19-32: API endpoint migration looks consistent.The migration of these three endpoints to
/openapi/v1namespace follows a consistent pattern, maintaining the original path structure while adding the new prefix. All endpoints are properly implemented in the backend OpenAPI controller.
33-37: Response format remains compatible despite URL path change.The endpoint path changed from
/envs/:env/instances/by-namespace/countto/openapi/v1/envs/:env/apps/:appId/clusters/:clusterName/namespaces/:namespaceName/instances, but the response format is still a count integer (not an instance collection). The test confirms the endpoint returns a serialized count value, and theisArray: falseconfiguration is correct. The URL semantics are misleading (ending in/instancesrather than/count), but this is an API design inconsistency, not a breaking change.Likely an incorrect or invalid review comment.
apollo-portal/src/main/resources/static/scripts/services/AppService.js (2)
22-67: LGTM! Endpoint migration to/openapi/v1/is consistent.The URL updates correctly migrate all app-related endpoints to the new OpenAPI v1 namespace. The
isArrayflags have been appropriately adjusted to match the expected response types from the backend.
130-131: The empty operator parameter is correctly handled by the backend filter—no action needed.Based on the Apollo portal architecture,
PortalUserSessionFilterpopulates the operator context from the request parameters before service methods are invoked, even when theoperatorparameter is an empty string. This allows the frontend to passoperator: ''without issue, and the filter ensures the operator context is properly set for downstream processing.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
132-147: LGTM! The new method aligns with the operator context migration.The
createAppInRemoteNewmethod properly usesUserIdentityContextHolder.getOperator()to retrieve operator information. The TODO comment appropriately documents this as a temporary migration method to avoid conflicts during the transition period.Based on learnings,
UserIdentityContextHolderis populated byPortalUserSessionFilterbefore service methods are invoked.
177-179: LGTM! Consistent operator context usage across all service methods.All methods have been updated to use
UserIdentityContextHolder.getOperator().getUserId()consistently. This aligns with the architectural pattern where the operator context is set by the filter layer before service methods are called.Also applies to: 188-188, 206-206, 253-254, 272-272
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
120-127: LGTM! Test correctly verifies operator resolution pattern.The test now properly verifies that the operator parameter is passed to
userService.findByUserId()for validation, and thatcreateAppInEnvis called without the operator parameter (since it's now retrieved from context). This aligns with the new operator context handling pattern.
146-147: LGTM! Method rename reflects service interface update.The test correctly uses the renamed method
getAppsWithPageAndSize, which better describes the pagination parameters.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
31-31: LGTM! Interface updates support DTO-based OpenAPI design.The method signature changes are well-aligned:
- Returning
OpenAppDTOfromcreateAppprovides better feedback to callers- Method renames (
getEnvClusters,getAppsWithPageAndSize,getEnvClusterInfos) improve clarity- Using explicit DTO types (
OpenEnvClusterInfo,OpenMissEnvDTO) instead of generic response wrappers provides better type safety- Removing the
operatorparameter fromcreateAppInEnvaligns with the context-based operator handling patternAlso applies to: 33-33, 43-43, 45-45, 49-49, 51-51
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (4)
90-103: LGTM! Operator validation and context handling are properly implemented.The
createAppmethod correctly:
- Derives the operator from either
dataChangeCreatedByorownerName- Validates the operator exists via
userService.findByUserId()- Sets the operator context before service invocation
- Returns the created
OpenAppDTO- Preserves conditional consumer role assignment
158-166: LGTM! Consistent operator validation for consumer authentication.The
updateAppmethod properly validates the operator when using consumer authentication, following the established pattern from learnings.
174-191: LGTM! Well-designed enhancement to support both consumer and user contexts.The
getAppsBySelfmethod now intelligently handles both authentication types:
- Consumer auth: Uses
ConsumerServiceto find authorized apps (existing behavior)- User auth: Derives app IDs from user's roles via
RolePermissionServiceandRoleUtils(new behavior)This provides a more complete implementation that works across different authentication contexts.
200-210: LGTM! Consistent operator validation pattern applied.Both
createAppInEnvanddeleteAppcorrectly implement the conditional operator validation pattern for consumer authentication.Also applies to: 218-227
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (4)
133-155: LGTM! Tests comprehensively updated for API changes.The tests correctly verify:
- Renamed methods (
getEnvClusters,getAppsWithPageAndSize)- New DTO-based responses (
OpenEnvClusterInfo)- Proper service method invocations
Also applies to: 278-289, 308-323
204-257: Excellent test coverage for authorization scenarios.The tests now properly cover:
- Happy path: Authorized consumer accessing their app
- Not found: App doesn't exist
- Unauthorized: Consumer accessing app they don't have permission for
This comprehensive coverage ensures the authorization logic works correctly.
295-306: LGTM! Test correctly uses newOpenMissEnvDTOresponse type.The test properly verifies the DTO fields (
codeandmessage) and aligns with the updated API response structure.
387-387: LGTM! Assertion updated for empty JSON object response.The test now expects
{}instead of an empty string, which aligns with theVoidreturn type andResponseEntity.ok().build()pattern.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (3)
155-191: LGTM! Well-designed conditional extended info population.The
isExtendparameter allows callers to control whether extended information (modification flags, old/new values) is populated in the DTO. This provides flexibility for different use cases (e.g., detailed editing vs. simple viewing).
224-227: LGTM! Bidirectional conversion support added.The new
toNamespaceDTOmethod complements the existingfromNamespaceDTO, enabling round-trip conversions between OpenAPI and internal DTOs.
394-417: LGTM! Proper handling ofEnvtype conversion.The
fromEnvClusterInfoconverter correctly:
- Null-checks the
Envobject before callingtoString()- Transforms
ClusterDTOlist using the existing converter- Handles empty cluster lists appropriately
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (4)
86-92: LGTM! Improved return value and admin collection type.The changes correctly:
- Return the created
OpenAppDTOto provide feedback to callers- Use
Set<String>for admins (prevents duplicates, more semantically appropriate)- Default to an empty set instead of empty list
173-189: LGTM! Proper operator context usage in environment-specific app creation.The method correctly:
- Removes the operator parameter from the signature (now retrieved from context)
- Calls
createAppInRemoteNew(the new migration method)- Uses
UserIdentityContextHolder.getOperator().getUserId()for role initializationBased on learnings, the operator context is set by
PortalUserSessionFilterbefore this method is called.
210-230: LGTM! Structured error handling withOpenMissEnvDTO.The method now returns explicit DTOs for each missing/error environment instead of using generic response wrappers. This provides:
- Better type safety
- Clearer API contracts
- Structured error information (code + message)
240-257: LGTM! Consistent DTO-based response structure.The
getEnvClusterInfosmethod follows the same pattern asfindMissEnvs, providing structured responses with status codes and messages for each environment. This improves API consistency and error handling.
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
Show resolved
Hide resolved
| if (null == releaseIds || releaseIds.isEmpty()) { | ||
| throw new BadRequestException("release ids can not be empty"); | ||
| } | ||
| List<String> rawReleaseIds = | ||
| Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds); | ||
|
|
||
| if (CollectionUtils.isEmpty(rawReleaseIds)) { | ||
| throw new BadRequestException("excludeReleases parameter cannot be empty"); | ||
| } | ||
|
|
||
| final Set<Long> releaseIdSet; | ||
| try { | ||
| releaseIdSet = rawReleaseIds.stream().map(Long::parseLong).collect(Collectors.toSet()); | ||
| } catch (NumberFormatException ex) { | ||
| throw new BadRequestException( | ||
| "excludeReleases parameter must contain only numeric release ids", ex); | ||
| } |
There was a problem hiding this comment.
Unify validation messages + preserve parse failure context
- Messages alternate between
releaseIdsandexcludeReleases—pick one (prefer the actual parameter name). new BadRequestException("...", ex)likely doesn’t setcause(depends on base exception implementation). If you want the cause retained, set it explicitly.
- if (null == releaseIds || releaseIds.isEmpty()) {
- throw new BadRequestException("release ids can not be empty");
- }
+ if (releaseIds == null || releaseIds.isEmpty()) {
+ throw new BadRequestException("releaseIds parameter cannot be empty");
+ }
@@
- if (CollectionUtils.isEmpty(rawReleaseIds)) {
- throw new BadRequestException("excludeReleases parameter cannot be empty");
- }
+ if (CollectionUtils.isEmpty(rawReleaseIds)) {
+ throw new BadRequestException("releaseIds parameter cannot be empty");
+ }
@@
- } catch (NumberFormatException ex) {
- throw new BadRequestException(
- "excludeReleases parameter must contain only numeric release ids", ex);
- }
+ } catch (NumberFormatException ex) {
+ BadRequestException bre =
+ new BadRequestException("releaseIds parameter must contain only numeric release ids");
+ bre.initCause(ex);
+ throw bre;
+ }🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
around lines 74-90, unify the validation messages to use the actual request
parameter name consistently (use "releaseIds" instead of "excludeReleases"), and
ensure the NumberFormatException is preserved as the cause when throwing
BadRequestException — either use a BadRequestException constructor that accepts
a cause or create the BadRequestException and call initCause(ex) before throwing
so the original parse failure context is retained.
81f3c67 to
0af0bc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
269-277: Guard against null env in fromNamespaceIdentifier
IfnamespaceIdentifier.getEnv()can be null for invalid/partial identifiers, this will NPE.- openNamespaceIdentifier.setEnv(namespaceIdentifier.getEnv().toString()); + Env env = namespaceIdentifier.getEnv(); + if (env != null) { + openNamespaceIdentifier.setEnv(env.toString()); + }
♻️ Duplicate comments (4)
apollo-portal/pom.xml (2)
31-33: Pin${apollo.openapi.spec.url}to an immutable reference (tag/commit) for reproducible builds.
Usingrefs/heads/mainmakes codegen non-deterministic and can break builds if the upstream spec changes or is temporarily unavailable. Consider pinning to a release tag/commit SHA, or vendoring the spec in-repo (or as an internal artifact).apolloconfig/apollo-openapi releases tag list raw.githubusercontent.com refs/heads/main caching intermittent 404
141-172: Document the Java 8 constraint onopenapi-generator-maven-pluginand justifyskipValidateSpec=true.
Given the earlier note that 6.6.0 is the highest feasible version for Java 8, please add an inline XML comment explaining that constraint so future readers don’t “upgrade” blindly. Separately,skipValidateSpec=trueshould either be removed (preferred) or have a short reason + tracking issue link to avoid silently accepting spec regressions.<plugin> <groupId>org.openapitools</groupId> <artifactId>openapi-generator-maven-plugin</artifactId> - <version>6.6.0</version> + <!-- Pinned to 6.6.0 due to Java 8 runtime/tooling constraints; verify before upgrading. --> + <version>6.6.0</version> <executions> <execution> @@ - <skipValidateSpec>true</skipValidateSpec> + <!-- TODO(ISSUE-XXXX): enable spec validation once upstream spec is compliant/stable. --> + <skipValidateSpec>true</skipValidateSpec> </configuration> </execution> </executions> </plugin>openapi-generator-maven-plugin 6.6.0 java 8 support openapi-generator-maven-plugin latest version supporting java 8apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
173-189: Potential NPE: operator context may be unset for USER path after signature change
UserIdentityContextHolder.getOperator().getUserId()assumes operator is always present; withcreateAppInEnv(String env, OpenAppDTO app)no longer taking an operator param, please ensure USER-auth requests still populate operator context (e.g., via controller usingUserInfoHolder, as suggested). Based on learnings, relying on the filter only works when the request includes an operator parameter.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
72-93: Fix inconsistent validation messages + preserve parse error cause
You throw forreleaseIdsemptiness but then mentionexcludeReleases, andnew BadRequestException("...", ex)won’t retainexas the cause with the currentBadRequestExceptionconstructor. citeturn0- if (null == releaseIds || releaseIds.isEmpty()) { - throw new BadRequestException("release ids can not be empty"); - } + if (releaseIds == null || releaseIds.isEmpty()) { + throw new BadRequestException("releaseIds parameter cannot be empty"); + } @@ - if (CollectionUtils.isEmpty(rawReleaseIds)) { - throw new BadRequestException("excludeReleases parameter cannot be empty"); - } + if (CollectionUtils.isEmpty(rawReleaseIds)) { + throw new BadRequestException("releaseIds parameter cannot be empty"); + } @@ - } catch (NumberFormatException ex) { - throw new BadRequestException( - "excludeReleases parameter must contain only numeric release ids", ex); - } + } catch (NumberFormatException ex) { + BadRequestException bre = + new BadRequestException("releaseIds parameter must contain only numeric release ids"); + bre.initCause(ex); + throw bre; + }
🧹 Nitpick comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
154-227: Align consumer operator context setting with “set+restore” pattern
updateApp,createAppInEnv, anddeleteAppset the operator for CONSUMER but never restore it locally; even if a filter clears later, restoring locally reduces leak risk in tests/alternate entrypoints.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
154-164: Clamp negative page to 0
pageIndexaccepts negative values today; considerMath.max(0, page)to avoid surprising paging behavior.
210-258: Avoid returning raw exception messages in OpenAPI DTOs; log instead
BothfindMissEnvsandgetEnvClusterInfosappende.getMessage()into client-facing responses; preferlogger.warn/error(..., e)and return a stable message/code to reduce info leakage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apollo-portal/pom.xml(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(10 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/resources/static/scripts/AppUtils.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/AppController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js(2 hunks)apollo-portal/src/main/resources/static/scripts/directive/directive.js(1 hunks)apollo-portal/src/main/resources/static/scripts/services/AppService.js(5 hunks)apollo-portal/src/main/resources/static/scripts/services/InstanceService.js(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(9 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/resources/static/scripts/directive/directive.js
- apollo-portal/src/main/resources/static/scripts/AppUtils.js
- apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js
- apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/main/resources/static/scripts/services/InstanceService.js
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
📚 Learning: 2025-12-07T07:28:11.982Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/pom.xmlapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
📚 Learning: 2024-11-06T12:12:14.419Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo PR: 5267
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java:70-72
Timestamp: 2024-11-06T12:12:14.419Z
Learning: In `apollo-portal`, prefer using `PositiveOrZero` from `javax.validation.constraints` to validate that integer fields are non-negative.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
🧬 Code graph analysis (7)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-112)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/ItemBO.java (1)
ItemBO(21-76)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/NamespaceBO.java (1)
NamespaceBO(23-103)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceSyncModel.java (1)
NamespaceSyncModel(26-70)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceTextModel.java (1)
NamespaceTextModel(24-104)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/EnvClusterInfo.java (1)
EnvClusterInfo(24-48)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/NamespaceIdentifier.java (1)
NamespaceIdentifier(23-72)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-469)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-26)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-93)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-133)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (27)
apollo-portal/pom.xml (1)
251-275: Verify generated-sources path matches generator output under all build modes.
Thebuild-helper-maven-pluginsource path assumes OpenAPI generator outputs to.../openapi/src/main/java(default). If you later change generatoroutputlayout (or enable multi-module generation), this can silently stop compiling generated sources—worth a quick CI check with a clean workspace.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
97-97: LGTM! Explicit Void return type for delete operation.The change from
ResponseEntity<Object>toResponseEntity<Void>makes the API contract clearer by explicitly indicating that the delete operation returns no body content. This aligns with REST conventions for delete operations and matches the broader PR pattern of using explicit DTO types rather than generic Object types.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
31-51: LGTM! Interface updates align with DTO-centric API design.The method signature changes improve the API surface:
createAppnow returnsOpenAppDTOinstead of void, enabling clients to receive confirmation of the created resource.- Method renames (
getEnvClusterInfo→getEnvClusters,getAppsBySelf→getAppsWithPageAndSize,getAppNavTree→getEnvClusterInfos) improve naming clarity.- Return types now use explicit DTOs (
List<OpenEnvClusterDTO>,List<OpenMissEnvDTO>,List<OpenEnvClusterInfo>) instead of generic wrappers.createAppInEnvoperator parameter removal aligns with the operator context pattern viaUserIdentityContextHolder.Based on learnings, operator context is now managed centrally through the filter chain.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
67-71: LGTM! Filter wiring updated to support operator context.The addition of
UserInfoHolderas a constructor parameter toPortalUserSessionFilterenables the filter to populate operator context from the authenticated user. This change is part of the broader operator context handling pattern introduced viaUserIdentityContextHolder.Based on learnings, the filter now sets operator context when an operator parameter is present in the request.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
120-126: LGTM! Test updated for operator context handling.The verification correctly reflects the updated
createAppInEnvsignature (two parameters instead of three) and validates that the operator is resolved viauserService.findByUserId("bob")rather than passed directly. This aligns with the operator context pattern.
146-146: LGTM! Test updated for renamed service method.The verification correctly uses the renamed method
getAppsWithPageAndSizeinstead of the previousgetAppsBySelf.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
84-95: LGTM! Operator context handling with proper cleanup.The filter now populates
UserIdentityContextHolderwith the operator when an operator parameter is present in the request. The try/finally block ensures thatUserIdentityContextHolder.clear()is called after request processing, preventing ThreadLocal memory leaks.This implementation aligns with the broader operator context pattern documented in the learnings, where the filter sets operator context before service methods are invoked.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
153-160: LGTM! Test wiring updated for operator context.The addition of
UserInfoHoldermock and its reset intearDown()ensures proper test isolation for operator context handling.
271-297: LGTM! Excellent test coverage for operator context lifecycle.This test validates the critical operator context behavior:
- Operator context is populated from
UserInfoHolderwhen an operator parameter is present- The operator is available within the filter chain execution
- The operator context is properly cleared after the request completes
The use of
AtomicBooleanto track chain execution and assertions both during and after filter execution provide thorough verification of the context lifecycle.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (4)
135-147: LGTM! New method uses centralized operator context.The new
createAppInRemoteNewmethod correctly retrieves the operator fromUserIdentityContextHolder.getOperator().getUserId()for both app creation metadata and role initialization. Based on learnings, the operator context is populated byPortalUserSessionFilterbefore service methods are invoked, making the direct.getUserId()call safe in the normal request flow.The TODO comment appropriately indicates this is a transitional method during migration.
177-179: LGTM! Consistent operator context usage.The update to use
UserIdentityContextHolder.getOperator().getUserId()aligns with the centralized operator context pattern. Based on learnings, the filter populates this context before service methods are called.
253-254: LGTM! Operator context usage in update flow.The change from
userInfoHolder.getUser().getUserId()toUserIdentityContextHolder.getOperator().getUserId()maintains consistency with the operator context pattern established across the service layer.
272-272: LGTM! Operator context usage in delete flow.Consistent with other methods, the delete operation now retrieves the operator from
UserIdentityContextHolder, aligning with the centralized operator context pattern.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (5)
133-154: LGTM! Test updated for renamed method.The test correctly validates the
getEnvClustersmethod (renamed fromgetEnvClusterInfo) and verifies the response structure with proper environment and cluster data.
225-257: LGTM! Enhanced test coverage for authorization scenarios.The new test cases (
testGetAppNotFoundandtestGetAppUnauthorizedForConsumer) provide valuable coverage for error paths when:
- The requested app doesn't exist in the service response
- The consumer is not authorized to access the requested app
These scenarios ensure proper error handling at the controller level.
278-288: LGTM! Test updated for renamed method.The verification correctly uses
getAppsWithPageAndSizeinstead of the previousgetAppsBySelf, aligning with the service interface changes.
292-306: LGTM! Test updated to use explicit DTO type.The test now works with
OpenMissEnvDTOinstead of a generic wrapper, properly validating thecodeandmessagefields in the response.
308-323: LGTM! New test for env cluster info endpoint.This test validates the new
/env-cluster-infoendpoint, ensuring it returnsOpenEnvClusterInfoobjects with properenvandmessagefields.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
42-66: ResponseEntity wrappers + delegation look consistent
The controller stays thin and delegates toInstanceOpenApiService, matching the OpenAPI-controller pattern.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
61-74: Constructor injection expansion is fine
No concerns with addingRolePermissionServiceandUserInfoHolderfor the new behaviors.
173-192: Role-to-appId extraction looks OK; ensure null user handling matches auth model
The non-consumer path assumesuserInfoHolder.getUser()is always present; that’s consistent with portal-auth, but worth double-checking for anonymous/internal calls.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
86-93: Admins defaulting to empty Set is a good fix
Avoids null handling downstream and matches “no admins specified” semantics.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
155-201: Extended namespace/item mapping looks reasonable
TheisExtendflag keeps the non-extended payload minimal while still enabling richer responses when needed.apollo-portal/src/main/resources/static/scripts/services/AppService.js (4)
19-27: LGTM! URL migrations to OpenAPI v1 are correct.The endpoint URLs have been properly migrated to the
/openapi/v1/namespace for bothfind_appsandfind_app_by_selfoperations, maintaining the correctisArray: trueflags.
29-37: LGTM! Correct isArray flags and URL migrations.The
isArrayflag changes are appropriate:
load_navtreecorrectly set totruefor the env-cluster-info array responseload_appexplicitly set tofalsefor single app responseBoth URLs properly migrated to
/openapi/v1/endpoints.
39-49: LGTM! URL migrations are consistent.All three app creation/update operations correctly migrated to
/openapi/v1/endpoints with appropriate resource paths.
51-67: LGTM! Correct isArray flags and URL migrations.The changes properly reflect the response types:
find_miss_envscorrectly set toisArray: truefor missing environments arraydelete_appexplicitly set toisArray: falsefor delete operationBoth URLs properly migrated to
/openapi/v1/endpoints.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
Show resolved
Hide resolved
tacklequestions
force-pushed
the
master
branch
from
d89a5a2 to
dd12f72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
74-90: Inconsistent validation message terminology.The error messages alternate between "release ids" (line 75) and "excludeReleases" (lines 81, 89). For consistency, use the actual parameter name
releaseIdsthroughout all validation messages.
🧹 Nitpick comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (1)
76-88: Fix comment typo.The comment on line 77 has a minor typo: "OpenPageDTOOpenInstanceDTO" should be "OpenPageDTO".
🔎 Proposed fix
/** - * Convert PageDTO<InstanceDTO> to OpenPageDTOOpenInstanceDTO + * Convert PageDTO<InstanceDTO> to OpenPageDTO<OpenInstanceDTO> */ private OpenInstancePageDTO transformToOpenPageDTO(PageDTO<InstanceDTO> pageDTO) {apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
90-105: Inconsistent context cleanup pattern across methods.
createAppexplicitly callsUserIdentityContextHolder.clear()at line 103, but other methods that set the operator context (updateApp,createAppInEnv,deleteApp) do not. WhilePortalUserSessionFilterhandles cleanup in itsfinallyblock, the explicit clear increateAppcreates an inconsistency.Consider either:
- Removing line 103 and relying entirely on the filter's cleanup, or
- Adding similar cleanup to other methods that set the operator context
Based on the past review discussion where you indicated cleanup would be addressed, this appears to be an incomplete fix.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
210-231: Consider improving the miss-env DTO semantics.The method uses
HttpStatus.OK(line 220) when an environment returns NOT_FOUND, which represents "successfully identified this env is missing." While functionally correct, this could be confusing. Consider:
- Using a more descriptive code like
HttpStatus.NOT_FOUND.value()to indicate the environment's state directly, or- Adding documentation explaining that
code=200means "missing env detected successfully" vscode=500means "error checking env"The current implementation works but the semantics may not be immediately intuitive to API consumers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apollo-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/InstanceOpenApiService.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/ServerInstanceOpenApiService.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/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.javaapollo-portal/src/main/resources/static/scripts/AppUtils.jsapollo-portal/src/main/resources/static/scripts/controller/AppController.jsapollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.jsapollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.jsapollo-portal/src/main/resources/static/scripts/directive/directive.jsapollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/resources/static/scripts/services/InstanceService.jsapollo-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/InstanceControllerParamBindLowLevelTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/resources/static/scripts/controller/AppController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java
- apollo-portal/src/main/resources/static/scripts/controller/ManageClusterController.js
- apollo-portal/src/main/resources/static/scripts/AppUtils.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
📚 Learning: 2025-12-29T08:03:20.356Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/resources/static/scripts/AppUtils.js:97-107
Timestamp: 2025-12-29T08:03:20.356Z
Learning: In apollo-portal/src/main/resources/static/scripts/AppUtils.js, gatherData and collectData serve different contracts: gatherData returns full entity objects for new OpenAPI endpoints (entities with code === 200), while collectData extracts entity.body for legacy response formats.
Applied to files:
apollo-portal/src/main/resources/static/scripts/directive/directive.jsapollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js
📚 Learning: 2025-12-29T07:37:52.105Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java:82-104
Timestamp: 2025-12-29T07:37:52.105Z
Learning: In apollo-portal AppController.createApp (Java), the operator is intentionally derived from the request body fields (dataChangeCreatedBy or ownerName) for all authentication types, unlike other methods in the same controller (updateApp, createAppInEnv, deleteApp) which use conditional operator handling based on auth type. This design maintains historical consistency in the user experience.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-12-29T08:02:15.307Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/resources/static/scripts/services/AppService.js:130-131
Timestamp: 2025-12-29T08:02:15.307Z
Learning: In apollo-portal AppService.js, the `operator: ''` parameter passed in update_app, create_app_remote, and delete_app calls is intentionally empty. Its presence in the request parameter map signals PortalUserSessionFilter to populate UserIdentityContextHolder with the operator from userInfoHolder.getUser(). The parameter acts as a flag (checked via containsKey("operator")), not as a value source.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-12-07T07:28:11.982Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5499
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java:135-147
Timestamp: 2025-12-07T07:28:11.982Z
Learning: In the Apollo portal, `UserIdentityContextHolder.getOperator()` is populated by `PortalUserSessionFilter` before service layer methods like `AppService.createAppInRemoteNew()` are invoked. The filter sets the operator context when an operator parameter is present in the request, making explicit null checks unnecessary in service methods that expect to be called through the normal filter chain.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2025-10-18T13:00:25.360Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java:37-37
Timestamp: 2025-10-18T13:00:25.360Z
Learning: In the Apollo OpenAPI refactoring (PR #5475), validation annotations like Valid are defined in the generated NamespaceManagementApi interface, not in the service layer interface (NamespaceOpenApiService). Validation occurs at the API/controller boundary, so service interfaces do not need to redeclare Valid annotations.
Applied to files:
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.javaapollo-portal/pom.xmlapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/resources/static/scripts/services/InstanceService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
📚 Learning: 2025-10-17T07:29:06.231Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5473
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java:25-33
Timestamp: 2025-10-17T07:29:06.231Z
Learning: In the Apollo OpenAPI services, when designing new service interfaces, prefer maintaining consistency with existing established patterns in the codebase over creating new conventions, even if it means some methods within an interface have different parameter ordering.
Applied to files:
apollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
📚 Learning: 2025-10-27T04:54:47.833Z
Learnt from: tacklequestions
Repo: apolloconfig/apollo PR: 5475
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java:98-105
Timestamp: 2025-10-27T04:54:47.833Z
Learning: In Apollo OpenAPI ClusterController.deleteCluster (PR #5475), operator validation is conditional based on auth type:
- USER auth: operator comes from UserInfoHolder in portal requests and is already trusted, so the operator parameter is optional and not validated
- CONSUMER auth: operator is passed as a parameter and must be validated (check non-empty and user exists)
This pattern applies to delete operations where the operator source differs by auth type.
Applied to files:
apollo-portal/src/main/resources/static/scripts/services/AppService.jsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.javaapollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
📚 Learning: 2024-11-06T12:12:14.419Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo PR: 5267
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/entity/ConsumerToken.java:70-72
Timestamp: 2024-11-06T12:12:14.419Z
Learning: In `apollo-portal`, prefer using `PositiveOrZero` from `javax.validation.constraints` to validate that integer fields are non-negative.
Applied to files:
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
🧬 Code graph analysis (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-80)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-469)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(21-66)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (8)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/ItemBO.java (1)
ItemBO(21-76)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/NamespaceBO.java (1)
NamespaceBO(23-103)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceSyncModel.java (1)
NamespaceSyncModel(26-70)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceTextModel.java (1)
NamespaceTextModel(24-104)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/EnvClusterInfo.java (1)
EnvClusterInfo(24-48)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/NamespaceIdentifier.java (1)
NamespaceIdentifier(23-72)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (8)
- GitHub Check: build (17)
🔇 Additional comments (51)
apollo-portal/src/main/resources/static/scripts/directive/directive.js (1)
168-168: Change correctly aligns with OpenAPI migration to new response format.The switch from
collectDatatogatherDatais correct and consistent with the codebase. Multiple controllers (ManageClusterController, ConfigBaseInfoController) already usegatherDatawithload_nav_treein the same pattern, confirming that the backend endpoint has been updated to return the new OpenAPI format where entities have{code, env, clusters}directly accessible. The downstream code (accessingnode.envandnode.clusters) is compatible withgatherData's output.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/PortalOpenApiAuthenticationScenariosTest.java (5)
153-154: LGTM!The
@MockBeanannotation correctly injects theUserInfoHoldermock into the Spring context, ensuring it's available for both the autowiredPortalUserSessionFilterbean and the test class.
156-161: Good test hygiene with proper context cleanup.Proper management of ThreadLocal states in unit tests is essential for ensuring test isolation and accuracy. It's a good practice to reset the ThreadLocal state before or after each test to prevent one test from influencing another. The cleanup of both
SecurityContextHolderandUserIdentityContextHolderprevents state leakage between tests.
192-194: LGTM!The filter construction correctly adapts to the new two-argument constructor. The local mock for
UserInfoHolderis appropriate since this test scenario focuses on expired session handling and doesn't require the mock to return user data.
233-235: LGTM!Consistent adaptation to the new constructor signature.
271-297: Well-structured test for the operator context lifecycle.This test effectively validates both the population and cleanup of the
UserIdentityContextHolder:
- Verifies that
getOperator()returns the authenticated user during filter chain execution (line 292)- Confirms the context is properly cleared after filter completion (line 296)
The use of
AtomicBooleanto track chain invocation is a clean pattern for verifying the filter didn't short-circuit.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/PortalUserSessionFilter.java (2)
61-66: LGTM!Constructor injection of
UserInfoHolderis the right approach for testability and aligns with dependency injection best practices.
84-96: Good implementation of thread-safe context management.The try-finally pattern with ThreadLocal cleanup ensures cleanup even if exceptions occur. The
UserIdentityContextHolder.clear()in thefinallyblock prevents memory leaks in pooled thread environments.Based on learnings, the
containsKey("operator")check is intentional—the parameter's presence signals the filter to populate context fromuserInfoHolder.getUser(), not the parameter's value.One minor observation: the condition also checks
request.getAttribute("operator") != null. Could you clarify where this attribute would be set from? If it's for internal forward/include scenarios, a brief comment would help future maintainers understand this path.apollo-portal/pom.xml (3)
38-138: Dependencies look appropriate.The dependency set correctly includes OpenAPI tooling (jackson-databind-nullable, swagger-annotations, swagger-models) alongside Spring Boot and portal requirements. No concerns.
141-172: pluginManagement configuration is correct.The pluginManagement pattern is proper Maven practice—defining full configuration centrally and inheriting it in the plugins section. Configuration options are appropriate:
interfaceOnly=truealigns with generating API interfaces (per PR objectives)useTags=truegroups operations by OpenAPI tagsdateLibrary=java8matches the Java 8 target- Package structure follows Apollo conventions
252-275: Plugin integration is correct.The openapi-generator plugin correctly inherits configuration from pluginManagement (lines 252-255), and build-helper-maven-plugin properly adds the generated sources to the compile path (lines 257-275).
Path alignment is correct:
- Generator outputs to
target/generated-sources/openapi- Generator creates
src/main/javasubdirectory by default- build-helper adds
target/generated-sources/openapi/src/main/java✓Execution order within the
generate-sourcesphase is determined by plugin declaration order, so sources are generated before being added to the compile path.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java (2)
47-56: LGTM!The method correctly delegates to the portal InstanceService and transforms the PageDTO response to OpenInstancePageDTO. The use of
Env.valueOf(env)is consistent with the codebase pattern for environment handling.
58-74: LGTM!Both methods correctly delegate to the underlying InstanceService and apply appropriate DTO conversions. The getByReleasesNotIn method converts the list directly, while getByNamespace uses the helper method for page transformation.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (6)
155-191: LGTM!The updated fromNamespaceBO method correctly handles the new isExtend parameter, conditionally populating OpenItemExtendDTO and OpenNamespaceExtendDTO with metadata when extended information is requested. The field mappings are complete and appropriate.
194-201: LGTM!The method correctly propagates the isExtend parameter to the individual fromNamespaceBO conversion calls.
204-227: LGTM!The fromNamespaceLockDTO method correctly includes the new isEmergencyPublishAllowed flag, and the new toNamespaceDTO method provides proper bidirectional conversion using BeanUtils.
281-302: LGTM!The conversion methods correctly transform OpenNamespaceSyncDTO to NamespaceSyncModel, properly handling the nested lists for syncToNamespaces and syncItems.
394-407: LGTM!The fromEnvClusterInfo method correctly converts EnvClusterInfo to OpenEnvClusterInfo, safely handling null env and properly transforming the clusters list.
438-458: LGTM!The fromItemDiffs method correctly converts ItemDiffs to OpenItemDiffDTO, properly handling the namespace identifier and all three types of item changes (create, update, delete). The variable naming follows conventions.
apollo-portal/src/main/resources/static/scripts/services/AppService.js (2)
18-68: LGTM!All endpoint URLs have been correctly updated to use the /openapi/v1/ namespace, maintaining consistency with the backend OpenAPI refactoring. The isArray configuration changes (load_navtree, find_miss_envs, load_app) properly align with the expected response types from the updated endpoints.
127-147: LGTM!The operator: '' parameters in update_app, create_app_remote, and delete_app are intentionally included. As confirmed in the retrieved learning, these empty operator parameters serve as flags to signal PortalUserSessionFilter to populate UserIdentityContextHolder with the operator from userInfoHolder.getUser(). The parameter presence (checked via containsKey("operator")) triggers the context initialization, not the value itself.
Based on learnings: operator parameter acts as a flag for filter-based context initialization.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (3)
102-127: LGTM!The test correctly verifies the updated createAppInEnv signature (now taking only env and OpenAppDTO) and adds proper verification that the operator is resolved via userService.findByUserId. The test appropriately captures and asserts the operator value as "bob".
129-151: LGTM!The test correctly reflects the renamed service method from getAppsBySelf to getAppsWithPageAndSize, maintaining proper verification of parameter binding for page, size, and authorized app IDs.
171-181: LGTM!The formatting adjustment for the MockMvc assertion improves readability. The test correctly verifies the deleteApp operation with path and query parameter binding.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
29-52: LGTM!The interface updates correctly reflect the DTO-centric OpenAPI refactoring:
- createApp now returns OpenAppDTO for better response handling
- Method renamings (getEnvClusters, getAppsWithPageAndSize, getEnvClusterInfos) improve clarity
- createAppInEnv signature simplified by removing explicit operator parameter (now handled via UserIdentityContextHolder)
- Return types updated to use OpenAPI DTOs (OpenEnvClusterDTO, OpenMissEnvDTO, OpenEnvClusterInfo)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (5)
132-147: LGTM!The new createAppInRemoteNew method correctly uses UserIdentityContextHolder.getOperator() to populate operator fields. As confirmed in the retrieved learning, PortalUserSessionFilter populates UserIdentityContextHolder before service methods are invoked when an operator parameter is present in the request, making the context access safe in the normal request flow.
Based on learnings: UserIdentityContextHolder is populated by PortalUserSessionFilter before service layer execution.
163-194: LGTM!The createAppInLocal method correctly uses UserIdentityContextHolder.getOperator().getUserId() for operator context. The filter-based context initialization pattern ensures the operator is available when this method executes through the normal request flow.
Based on learnings: Operator context is set by filter before service methods are called.
196-210: LGTM!The createAppAndAddRolePermission method appropriately uses UserIdentityContextHolder for role assignment operations, consistent with the established filter-based context initialization pattern.
Based on learnings: UserIdentityContextHolder provides operator context set by PortalUserSessionFilter.
232-257: LGTM!The updateAppInLocal method correctly uses UserIdentityContextHolder.getOperator().getUserId() to set the dataChangeLastModifiedBy field, consistent with the operator context pattern established across the service layer.
Based on learnings: Operator context is populated by filter before service execution.
265-294: LGTM!The deleteAppInLocal method appropriately uses UserIdentityContextHolder for tracking the deletion operator, maintaining consistency with the filter-based operator context initialization pattern used throughout the service layer.
Based on learnings: PortalUserSessionFilter ensures operator context is available in service methods.
apollo-portal/src/main/resources/static/scripts/services/InstanceService.js (1)
17-38: LGTM!All instance-related endpoint URLs have been correctly updated to use the /openapi/v1/ namespace, maintaining consistency with the broader OpenAPI refactoring. The updated get_instance_count_by_namespace endpoint now uses a more explicit path that includes appId, clusterName, and namespaceName parameters, improving resource addressing clarity.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerParamBindLowLevelTest.java (4)
48-77: LGTM!The test properly verifies path variable binding for getInstanceCountByNamespace, correctly capturing all four path parameters (env, appId, clusterName, namespaceName) and verifying the service is invoked with the expected arguments. The response content type assertion is appropriate.
79-94: LGTM!The test correctly verifies that getInstancesByRelease properly binds both path variables (env) and query parameters (releaseId, page, size) to the service method. The mock setup with OpenInstancePageDTO is appropriate for validating the complete request-response flow.
96-116: LGTM!These tests properly validate input validation for the getInstancesExcludingReleases endpoint, ensuring that empty releaseIds and non-numeric releaseIds are rejected with 400 Bad Request. The verifyNoInteractions assertions confirm no service calls are made on invalid input, preventing potential downstream errors.
118-140: LGTM!The test thoroughly validates full parameter binding for getInstancesExcludingReleases, including proper parsing of comma-separated releaseIds into a Set. The use of ArgumentCaptor to verify the set contents (10L, 11L, 12L) is appropriate, and the test correctly handles whitespace in the input string.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (4)
99-105: LGTM! Proper setup and teardown for UserIdentityContextHolder.The test correctly initializes the auth type context in
@Beforeand clears it in@Afterto prevent ThreadLocal leaks between tests.
203-223: Test correctly validates the ownerDisplayName population logic.The test sets
ownerNameon the mock DTO, and the controller'sgetAppmethod (lines 143-146 in AppController.java) correctly copiesownerNametoownerDisplayNamebefore returning. The assertion at line 220 will pass. The past review comment about this is now outdated since the controller logic handles this population.
225-257: Good coverage of error scenarios for getApp endpoint.The tests properly verify:
testGetAppNotFound: Returns 400 when app doesn't existtestGetAppUnauthorizedForConsumer: Returns 400 when consumer isn't authorized for the requested appBoth tests correctly mock the consumer authorization flow and verify the expected HTTP status codes.
291-323: Tests correctly validate the new DTO-based responses.Both
testFindMissEnvsandtestGetEnvClusterInfosproperly:
- Use the new
OpenMissEnvDTOandOpenEnvClusterInfoDTOs- Mock the service layer with appropriate responses
- Assert the expected JSON structure with
code,message, andenvfieldsapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (2)
42-47: LGTM! Clean delegation to service layer.The method correctly wraps the service response in a
ResponseEntityand delegates all business logic toinstanceOpenApiService.
49-66: Clean implementation following the API delegation pattern.Both
getByReleaseandgetByNamespaceproperly delegate to the service layer. Per the retrieved learning, validation annotations are defined in the generatedInstanceManagementApiinterface, so parameter validation occurs at the API boundary.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (4)
61-74: LGTM! Clean dependency injection for new features.The constructor properly injects
RolePermissionServiceandUserInfoHolderto support the new role-based app discovery and operator context handling.
174-193: Good implementation of dual-path app discovery.The method correctly:
- Uses consumer authorization for CONSUMER auth type
- Extracts app IDs from user roles for USER auth type via
RoleUtils.extractAppIdFromRoleName- Handles null appId values safely before adding to the set
This aligns with the pattern of conditional handling based on auth type used elsewhere in the controller.
152-168: LGTM! Consistent operator validation pattern for consumer auth.The method correctly validates the operator exists and sets the context only for CONSUMER auth type, aligning with the pattern in
createAppInEnvanddeleteApp. Per the retrieved learnings, USER auth derives the operator fromUserInfoHolderwhich is already trusted.
233-244: Clean delegation with new DTO return types.Both
findMissEnvsandgetEnvClusterInfoproperly delegate to the service layer and return the newOpenMissEnvDTOandOpenEnvClusterInfoDTOs respectively.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (4)
86-93: LGTM! Good defensive coding with admins defaulting.The change from
List<String>toSet<String>for admins is appropriate, and defaulting toCollections.emptySet()when null is good defensive coding. The method now returns the created app asOpenAppDTO, providing useful feedback to callers.
95-113: LGTM! Proper null handling in cluster DTO construction.The method correctly handles potential null
clusterDTOsby falling back to an empty set. The DTO construction is clean and follows the project's conversion patterns.
172-189: Operator context usage is safe per established architecture.Based on the retrieved learnings,
UserIdentityContextHolder.getOperator()is populated byPortalUserSessionFilterbefore service methods are invoked. Additionally,AppController.createAppInEnvvalidates and sets the operator context for CONSUMER auth before calling this method, ensuring the context is always populated.
239-258: LGTM! Consistent error handling pattern.The method follows the same pattern as
findMissEnvs, providing per-environment status with appropriate error messages. The error details include both the environment name and exception message, which aids debugging.apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js (2)
139-151: The API format is correctly differentiated based on the endpoint's response type. Thefind_missing_namespacesendpoint inNamespaceController.java(line 283) still returnsMultiResponseEntity<String>in the legacy format, which is whyAppUtil.collectData(result)is appropriate on line 145. This differs fromfind_miss_envsand other endpoints that were migrated to OpenAPI format and now return structured DTO objects. The code is consistent with the backend implementation—no change needed.Likely an incorrect or invalid review comment.
195-195: The change fromcollectDatatogatherDatais correct and compatible with the backend API.The endpoint
/openapi/v1/apps/:appId/env-cluster-inforeturns aList<OpenEnvClusterInfo>where each object contains theenv,clusters, andcodeproperties. ThegatherDatafunction properly filters entities withcode === 200and returns them with the required structure intact. The downstream code accessesenv,clusters,cluster.name, andcluster.comment—all properties present in theOpenEnvClusterInfoandClusterDTOobjects. No data structure incompatibility exists.
| $scope.missEnvs = []; | ||
| AppService.find_miss_envs($rootScope.pageContext.appId).then(function (result) { | ||
| $scope.missEnvs = AppUtil.collectData(result); | ||
| $scope.missEnvs = AppUtil.gatherData(result); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find AppService implementation (likely Java backend)
find . -name "AppService*.java" | head -20Repository: apolloconfig/apollo
Length of output: 324
🏁 Script executed:
# Search for find_miss_envs in the codebase to understand the API contract
rg -nP "find_miss_envs" --type=js --type=java -B2 -A5Repository: apolloconfig/apollo
Length of output: 3210
🏁 Script executed:
# Check ConfigBaseInfoController.js lines around 125 and 195 to see actual usage
cat -n apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js | sed -n '120,130p;190,200p'Repository: apolloconfig/apollo
Length of output: 934
🏁 Script executed:
# Verify AppUtils.js gatherData and collectData implementation to confirm the learning
cat -n apollo-portal/src/main/resources/static/scripts/AppUtils.js | sed -n '97,115p'Repository: apolloconfig/apollo
Length of output: 916
🏁 Script executed:
# Search for the backend implementation of the miss-envs endpoint
rg -nP "miss-envs" --type=java -B5 -A10Repository: apolloconfig/apollo
Length of output: 2450
🏁 Script executed:
# Check the Java AppService for the API implementation
cat -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java | head -50Repository: apolloconfig/apollo
Length of output: 2879
🏁 Script executed:
# Search for REST controllers that handle the miss-envs endpoint
rg -nP "miss-envs|missEnvs" --type=java -B3 -A10Repository: apolloconfig/apollo
Length of output: 6553
🏁 Script executed:
# Search for createAppInMissEnv function to see how missEnvs data is consumed
rg -nP "createAppInMissEnv" --type=js -B3 -A15Repository: apolloconfig/apollo
Length of output: 2656
🏁 Script executed:
# Check if there are any other references to missEnvs in the controller
rg -nP "missEnvs" apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js -B2 -A2Repository: apolloconfig/apollo
Length of output: 1604
🏁 Script executed:
# Look for the actual implementation of the data mutation - check what properties the elements should have
rg -nP "newEnv|missEnv" apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js -B2 -A5 | head -60Repository: apolloconfig/apollo
Length of output: 2003
🏁 Script executed:
# Verify OpenMissEnvDTO structure - check what properties it has
rg -nP "class OpenMissEnvDTO" --type=java -A20Repository: apolloconfig/apollo
Length of output: 45
🏁 Script executed:
# Search for OpenMissEnvDTO definition
rg -nP "OpenMissEnvDTO" --type=java -A10 | head -50Repository: apolloconfig/apollo
Length of output: 7281
🏁 Script executed:
# Check AppService.create_remote to see what it expects
rg -nP "create_remote" --type=js -B3 -A10Repository: apolloconfig/apollo
Length of output: 3623
Extract the environment name from each DTO object returned by gatherData.
The change to gatherData returns full OpenMissEnvDTO objects ({code: 200, message: "DEV"}), but the downstream code expects simple string environment names. This breaks:
create_remote(env)receives an object instead of a string (line 95)toastrmessages display[object Object]instead of env name (lines 96, 102)indexOflookup fails because it compares string against objects (line 142)
Extract the message property from each entity: AppUtil.gatherData(result).map(function(entity) { return entity.message; })
🤖 Prompt for AI Agents
In
apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.js
around line 125, AppUtil.gatherData(result) now returns OpenMissEnvDTO objects
but the rest of the code expects plain env name strings; change the assignment
so you map each DTO to its message property (i.e., replace the direct gatherData
result with a mapped array extracting entity.message) so create_remote, toastr
messages, and indexOf comparisons receive simple strings.
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
这版还有 2 个阻塞问题，需要修复后再继续。
请修复以下阻塞项后再更新：
-
miss-envs返回 DTO 后，ConfigBaseInfoController仍把结果当字符串 env 使用，导致创建缺失环境和缺失命名空间判断异常。apollo-portal/src/main/resources/static/scripts/controller/config/ConfigBaseInfoController.jsline 94, 125, 142apollo-portal/src/main/resources/static/scripts/AppUtils.jsline 102
建议：在findMissEnvs处把AppUtil.gatherData(result)映射为 env 字符串（例如entity.message），或统一改全链路消费 DTO 对象。
-
AppService现在依赖UserIdentityContextHolder.getOperator()，但 operator 注入仅在/openapi/*过滤器。旧/apps写接口调用链仍可能触发 NPE。apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.javaline 137, 177, 253, 272apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.javaline 72apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.javaline 126, 142, 188
建议：- 要么在
AppService做 operator 安全获取（必要时 fallback 到userInfoHolder）； - 要么明确
/apps写接口已下线并同步移除/防护兼容路径，避免线上 500。
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 14 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@tacklequestions This pull request has conflicts with the target branch. Please resolve them and update the branch before merging. |
4 participants
Summary by CodeRabbit
New Features
API Changes
Frontend
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.