feat: modification of App and Instance #5499
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughThis pull request refactors Apollo's OpenAPI portal services and controllers. It updates method signatures across multiple service layers (renaming getEnvClusterInfo to getEnvClusters, getAppsBySelf to getAppsWithPageAndSize), changes return types from wrapper entities to concrete DTOs, removes operator parameters in favor of ThreadLocal context management via UserIdentityContextHolder, and updates frontend JavaScript to use new /openapi/v1/ API paths and helper utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Filter as PortalUserSessionFilter
participant CtxHolder as UserIdentityContextHolder
participant Controller as AppController
participant Service as ServerAppOpenApiService
participant Remote as Remote Portal API
Client->>Filter: Request with operator param
Filter->>Filter: Extract operator parameter
Filter->>CtxHolder: setOperator(userInfo)
CtxHolder->>CtxHolder: Store in ThreadLocal
Filter->>Controller: chain.doFilter
Controller->>Controller: Validate consumer/operator
Controller->>Service: Call service method
Service->>CtxHolder: getOperator()
CtxHolder-->>Service: Return operator from ThreadLocal
Service->>Remote: Create/Update/Delete with operator
Remote-->>Service: Return DTO
Service-->>Controller: Return OpenAppDTO/List
Controller-->>Client: ResponseEntity<DTO>
Filter->>CtxHolder: clear() in finally
CtxHolder->>CtxHolder: Remove OPERATOR_HOLDER
CtxHolder->>CtxHolder: Remove other contexts
sequenceDiagram
participant Browser as Frontend Browser
participant AppSvc as AppService.js
participant API as /openapi/v1/apps Endpoint
participant Controller as AppController
participant Svc as ServerAppOpenApiService
participant Util as AppUtil.gatherData
Browser->>AppSvc: create(openCreateAppDTO)
AppSvc->>API: POST /openapi/v1/apps + operator: ''
API->>Controller: createApp(openCreateAppDTO)
Controller->>Svc: createApp(dto)
Svc-->>Controller: OpenAppDTO
Controller-->>API: ResponseEntity<OpenAppDTO>
API-->>AppSvc: {code: 200, data: OpenAppDTO}
AppSvc->>Util: gatherData(response)
Util->>Util: Filter code===200, warn others
Util-->>AppSvc: filtered data array
AppSvc->>Browser: Redirect to app details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention:
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.
| public ResponseEntity<List<OpenInstanceDTO>> getByReleasesAndNamespaceNotIn(String env, | ||
| String appId, String clusterName, String namespaceName, String releaseIds) { | ||
| 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); | ||
| } | ||
|
|
||
| return ResponseEntity.ok(this.instanceOpenApiService.getByReleasesNotIn(env, appId, clusterName, | ||
| namespaceName, releaseIdSet)); |
There was a problem hiding this comment.
Guard against null releaseIds to avoid NPE
If releaseIds is null, Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds) will throw a NullPointerException, returning a 500 instead of the intended 400 for bad input.
Consider adding an explicit null check before splitting:
- List<String> rawReleaseIds =
- Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds);
+ if (releaseIds == null) {
+ throw new BadRequestException("excludeReleases parameter cannot be null");
+ }
+
+ List<String> rawReleaseIds =
+ Splitter.on(",").omitEmptyStrings().trimResults().splitToList(releaseIds);The rest of the validation (CollectionUtils.isEmpty and numeric parsing) can remain unchanged.
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
around lines 72 to 90, guard against a null releaseIds input before calling
Splitter to avoid an NPE: add an explicit null check for releaseIds and throw a
BadRequestException (with a clear message like "excludeReleases parameter cannot
be null") if it is null, then proceed with the existing Splitter,
CollectionUtils.isEmpty check, and numeric parsing logic unchanged.
| var openCreateAppDTO = { | ||
| assignAppRoleToSelf: false, | ||
| admins: $scope.app.admins || [], | ||
| app: { | ||
| name: $scope.app.name, | ||
| appId: $scope.app.appId, | ||
| orgId: $scope.app.orgId, | ||
| orgName: $scope.app.orgName, | ||
| ownerName: $scope.app.ownerName | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the hardcoded assignAppRoleToSelf: false value.
The assignAppRoleToSelf field is hardcoded to false, but when isOpenManageAppMasterRoleLimit is true (lines 89-90, 102-103), the current user is automatically assigned as both owner and admin. This appears to be a self-assignment scenario where assignAppRoleToSelf should potentially be true.
Please verify whether this field should be dynamic based on isOpenManageAppMasterRoleLimit or other conditions.
🏁 Script executed:
#!/bin/bash
# Description: Check how assignAppRoleToSelf is used on the backend to understand expected behavior
# Search for assignAppRoleToSelf usage in Java backend files
rg -n --type=java -C5 'assignAppRoleToSelf'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for assignAppRoleToSelf usage across all file types in the repository
rg -n 'assignAppRoleToSelf' -C3
# Also search for the app service being called with this DTO
rg -n 'AppService\.create' -C5
# Check the full context of the create function
rg -n -A30 'this\.create\s*=\s*function' apollo-portal/src/main/resources/static/scripts/controller/AppController.jsLength of output: 7566
🏁 Script executed:
#!/bin/bash
# Get the full context of the create function including logic around owner/admin assignment
rg -n -B10 -A40 'var openCreateAppDTO' apollo-portal/src/main/resources/static/scripts/controller/AppController.js
# Also check if there's any conditional assignment of assignAppRoleToSelf elsewhere
rg -n 'isOpenManageAppMasterRoleLimit' apollo-portal/src/main/resources/static/scripts/controller/AppController.jsLength of output: 2345
Fix the hardcoded assignAppRoleToSelf: false value to match actual admin assignment logic.
The API documentation defines assignAppRoleToSelf as a required boolean indicating whether admin privileges are granted to the current user. However, the code hardcodes this to false while simultaneously adding the current user to the admins array when isOpenManageAppMasterRoleLimit is true (lines 102-103).
Set assignAppRoleToSelf: true when the current user is included in the admins array; otherwise false. For example:
var openCreateAppDTO = {
assignAppRoleToSelf: $scope.app.admins.indexOf($scope.currentUser.userId) !== -1,
admins: $scope.app.admins || [],
app: {
name: $scope.app.name,
appId: $scope.app.appId,
orgId: $scope.app.orgId,
orgName: $scope.app.orgName,
ownerName: $scope.app.ownerName
}
};🤖 Prompt for AI Agents
In apollo-portal/src/main/resources/static/scripts/controller/AppController.js
around lines 111 to 121, the openCreateAppDTO currently hardcodes
assignAppRoleToSelf to false; change it to reflect whether the current user is
actually included in the admins array by computing a boolean from the admins
list (use ($scope.app.admins || []) and check for $scope.currentUser.userId via
indexOf/includes) so assignAppRoleToSelf is true when the current user is
present in admins and false otherwise, while leaving the admins and app
properties as-is.
1 participant
Summary by CodeRabbit
Release Notes
New Features
API Changes
/openapi/v1/path prefix for improved API organization.Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.