feat(openapi): app domain service impl, controller migration and tests#5471
Conversation
WalkthroughAdds a new AppOpenApiService interface; expands AppController to implement AppManagementApi with ResponseEntity endpoints, operator validation and auditing; extends ServerAppOpenApiService with app lifecycle, env operations, event publishing, and DTO mapping changes; and adds comprehensive controller tests including low-level binding checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as AppController
participant UserSvc as UserService
participant Service as AppOpenApiService
participant Events as ApplicationEventPublisher
rect rgb(235,245,255)
note over Client,Controller: Update App (PUT /openapi/v1/apps/{appId}?operator=...)
Client->>Controller: PUT /apps/{appId} (OpenAppDTO, operator)
Controller->>UserSvc: validate(operator)
UserSvc-->>Controller: operator exists / not found
alt operator valid
Controller->>Service: updateApp(OpenAppDTO)
Service->>Events: publish(AppInfoChangedEvent)
Service-->>Controller: OpenAppDTO
Controller-->>Client: 200 OK (OpenAppDTO)
else invalid operator
Controller-->>Client: 400/404 error
end
end
sequenceDiagram
autonumber
actor Client
participant Controller as AppController
participant UserSvc as UserService
participant Service as AppOpenApiService
participant RoleInit as RoleInitializationService
rect rgb(245,255,235)
note over Client,Controller: Create App in Env (POST /openapi/v1/envs/{env}/apps?operator=...)
Client->>Controller: POST /envs/{env}/apps (OpenAppDTO, operator)
Controller->>UserSvc: validate(operator)
UserSvc-->>Controller: operator exists / not found
alt operator valid
Controller->>Service: createAppInEnv(env, OpenAppDTO, operator)
Service->>RoleInit: init roles for env
Service-->>Controller: success
Controller-->>Client: 200 OK
else invalid operator
Controller-->>Client: 400/404 error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
dosubot
bot
added
the
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (26)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (2)
110-114: Wiring ConsumerService lazily: OK, but guard against missing bean at runtimePassing a lazily-initialized ConsumerService into SpringSecurityUserInfoHolder is fine to break cycles. Note that @lazy still requires a resolvable bean; if ConsumerService is absent under some profiles, startup will fail. Consider ObjectProvider (or Optional) to make it optional and avoid hard dependency.
Apply this diff to make injection optional (and adjust holder accordingly):
- public UserInfoHolder springSecurityUserInfoHolder(UserService userService, @Lazy ConsumerService consumerService) { - return new SpringSecurityUserInfoHolder(userService, consumerService); + public UserInfoHolder springSecurityUserInfoHolder( + UserService userService, + org.springframework.beans.factory.ObjectProvider<ConsumerService> consumerService) { + return new SpringSecurityUserInfoHolder(userService, consumerService.getIfAvailable()); }
214-218: Duplicate change for LDAP profile: apply the same optional injection patternSame consideration as the auth profile bean: prefer ObjectProvider to avoid hard dependency on ConsumerService across profiles.
- public UserInfoHolder springSecurityUserInfoHolder(UserService userService, @Lazy ConsumerService consumerService) { - return new SpringSecurityUserInfoHolder(userService, consumerService); + public UserInfoHolder springSecurityUserInfoHolder( + UserService userService, + org.springframework.beans.factory.ObjectProvider<ConsumerService> consumerService) { + return new SpringSecurityUserInfoHolder(userService, consumerService.getIfAvailable()); }apollo-portal/pom.xml (1)
30-33: Avoid network flakiness: pin or vendor the OpenAPI specBuilding from a mutable GitHub raw URL is brittle and non-hermetic. Prefer:
- Vendoring the spec in-repo and referencing a file path; or
- Pinning the URL to a specific commit in the official repo.
Based on learnings, also consider relying on the published apollo-openapi artifact for model stability.
- <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url> + <!-- Option A: vendor locally --> + <apollo.openapi.spec.url>${project.basedir}/src/main/openapi/apollo-openapi.yaml</apollo.openapi.spec.url> + <!-- Option B (if you must fetch remotely): pin to a commit in the official repo --> + <!-- <apollo.openapi.spec.url>https://raw.githubusercontent.com/apolloconfig/apollo-openapi/<commit-sha>/apollo-openapi.yaml</apollo.openapi.spec.url> -->apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java (1)
49-51: Endpoint migration looks good; add a status assertionThe new RESTful path is correct. Consider asserting HTTP 200 to tighten the test.
- ResponseEntity<OpenAppDTO[]> responseEntity = + ResponseEntity<OpenAppDTO[]> responseEntity = restTemplate.exchange(this.url("/openapi/v1/apps?authorized=true"), HttpMethod.GET, new HttpEntity<>(httpHeaders), OpenAppDTO[].class); + assertEquals(200, responseEntity.getStatusCodeValue());apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (2)
75-82: Path check may miss context-path deploymentsrequest.getRequestURI().startsWith("/openapi/") fails if the app runs under a context path (e.g., "/portal/openapi/..."). Use getServletPath() or strip the context path before checking.
- String requestURI = request.getRequestURI(); + String requestURI = request.getServletPath(); // 只对OpenAPI请求处理Consumer用户信息 if (!requestURI.startsWith("/openapi/")) { return null; }
101-105: Swallowing exceptions hides auth bugsAt least log at debug to aid diagnosis when token-to-consumer resolution fails.
- } catch (Exception e) { - // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 - return null; - } + } catch (Exception e) { + // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 + org.slf4j.LoggerFactory.getLogger(SpringSecurityUserInfoHolder.class) + .debug("Failed to resolve OpenAPI Consumer user from request", e); + return null; + }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (2)
30-33: Don’t ship a default method that throws UnsupportedOperationExceptionThis is a new interface; prefer a proper abstract contract (and return value) over a default throwing method. Consider returning the created app DTO for symmetry with update.
- default void createApp(OpenCreateAppDTO req) { - throw new UnsupportedOperationException(); - } + OpenAppDTO createApp(OpenCreateAppDTO req);
44-46: Pagination surface is asymmetric (Pageable in, List out)Method takes Pageable but returns a List, losing page metadata. Consider returning a Page or a wrapper carrying total/next cursor (align with MultiResponseEntity if that’s your standard).
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (3)
76-102: Add explicit coverage for /apps/authorized endpointThis test hits GET /openapi/v1/apps with an unused "authorized" param; controller also exposes GET /openapi/v1/apps/authorized. Add a dedicated test to cover that route to avoid accidental regressions.
150-158: Prefer 404 Not Found for missing appThe controller currently maps “not found” to 400. Consider updating the controller to return 404 and adjust this test accordingly for REST semantics.
183-210: Add a test for blank appIdController only checks null appId. Add a test that sends appId="" (or whitespace) and expect 400 to ensure validation is enforced.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (7)
72-81: Add null precondition for convert(OpenAppDTO)Guard against null DTO to fail fast with a clear message.
Apply this diff:
private App convert(OpenAppDTO dto) { + com.google.common.base.Preconditions.checkArgument(dto != null, "OpenAppDTO must not be null"); return App.builder() .appId(dto.getAppId()) .name(dto.getName()) .ownerName(dto.getOwnerName()) .orgId(dto.getOrgId()) .orgName(dto.getOrgName()) .ownerEmail(dto.getOwnerEmail()) .build(); }
119-123: Guard against null/empty appIdsPrevent NPE and treat null/empty as empty result.
Apply this diff:
@Override public List<OpenAppDTO> getAppsInfo(List<String> appIds) { - final List<App> apps = this.appService.findByAppIds(new HashSet<>(appIds)); + if (appIds == null || appIds.isEmpty()) { + return java.util.Collections.emptyList(); + } + final List<App> apps = this.appService.findByAppIds(new HashSet<>(appIds)); return OpenApiModelConverters.fromApps(apps); }
125-128: Either implement or remove getAuthorizedApps from the interfaceThrowing UnsupportedOperationException in an interface method implementation is brittle. If the controller won’t use this, consider removing it from the interface; otherwise, implement it by accepting a principal/context and delegating accordingly.
162-174: Don’t leak internal exception messages in APIAppending e.getMessage() can expose internals. Prefer a generic message and log the exception.
Apply this diff:
- } catch (Exception e) { - response.addResponseEntity(RichResponseEntity.error(HttpStatus.INTERNAL_SERVER_ERROR, - "load env:" + env.getName() + " cluster error." + e.getMessage())); + } catch (Exception e) { + response.addResponseEntity(RichResponseEntity.error(HttpStatus.INTERNAL_SERVER_ERROR, + "load env:" + env.getName() + " cluster error.")); + // consider logging e at WARN/ERROR level }
182-188: Validate env and handle invalid input gracefullyEnv.valueOf throws IllegalArgumentException for invalid envs, surfacing as 500. Validate and return a client error (or document exception mapping).
Apply this diff:
- public void createAppInEnv(String env, OpenAppDTO app, String operator) { - App appEntity = convert(app); - appService.createAppInRemote(Env.valueOf(env), appEntity); + public void createAppInEnv(String env, OpenAppDTO app, String operator) { + com.google.common.base.Preconditions.checkArgument(app != null, "App must not be null"); + final Env targetEnv = Env.exists(env) ? Env.valueOf(env) : Env.UNKNOWN; + com.google.common.base.Preconditions.checkArgument(targetEnv != Env.UNKNOWN, "Invalid env: %s", env); + App appEntity = convert(app); + appService.createAppInRemote(targetEnv, appEntity); roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), ConfigConsts.NAMESPACE_APPLICATION, env, operator); }
206-222: Avoid echoing exception messages in miss-envs responseSame concern as above; prefer generic error text and log details.
Apply this diff:
- } else { - response.addResponseEntity(RichResponseEntity.error(HttpStatus.INTERNAL_SERVER_ERROR, - String.format("load appId:%s from env %s error.", appId, env) + e.getMessage())); - } + } else { + response.addResponseEntity(RichResponseEntity.error(HttpStatus.INTERNAL_SERVER_ERROR, + String.format("load appId:%s from env %s error.", appId, env))); + // consider logging e + }
135-139: Return the updated entity (optional)updateApp returns void; consider returning the persisted OpenAppDTO to avoid the controller reflecting stale input if the service applies changes (normalization, enrichment).
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (2)
102-110: Handle null/empty configurations in fromReleaseDTOReleaseDTO.getConfigurations() can be null/empty; guard to avoid JSON parse issues.
Apply this diff:
public static OpenReleaseDTO fromReleaseDTO(ReleaseDTO release) { Preconditions.checkArgument(release != null); OpenReleaseDTO openReleaseDTO = BeanUtils.transform(OpenReleaseDTO.class, release); - Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE); + String raw = release.getConfigurations(); + Map<String, String> configs = (raw == null || raw.isEmpty()) ? java.util.Collections.emptyMap() + : GSON.fromJson(raw, TYPE); openReleaseDTO.setConfigurations(configs); return openReleaseDTO; }
89-100: Consider adding toApp(OpenAppDTO) hereService currently has a private convert(OpenAppDTO). For consistency and reuse, add a symmetrical converter:
Proposed addition:
public static App toApp(final OpenAppDTO dto) { Preconditions.checkArgument(dto != null); return App.builder() .appId(dto.getAppId()) .name(dto.getName()) .ownerName(dto.getOwnerName()) .orgId(dto.getOrgId()) .orgName(dto.getOrgName()) .ownerEmail(dto.getOwnerEmail()) .build(); }Then ServerAppOpenApiService can reuse it.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (6)
46-55: Depend on the interface, not the implInject AppOpenApiService instead of ServerAppOpenApiService to decouple the controller from a concrete class and simplify testing/mocking.
Apply this diff:
-import com.ctrip.framework.apollo.openapi.server.service.impl.ServerAppOpenApiService; +import com.ctrip.framework.apollo.openapi.server.service.AppOpenApiService; @@ - private final ServerAppOpenApiService serverAppOpenApiService; + private final AppOpenApiService appOpenApiService; @@ - final ServerAppOpenApiService serverAppOpenApiService) { + final AppOpenApiService appOpenApiService) { @@ - this.serverAppOpenApiService = serverAppOpenApiService; + this.appOpenApiService = appOpenApiService;Follow through replacing usages of serverAppOpenApiService with appOpenApiService.
65-84: Strengthen createApp validation and return semantics
- Also reject blank appId (not just null).
- Optionally return the created entity from the service (if available).
Apply this diff:
public ResponseEntity<OpenAppDTO> createApp(@Valid @RequestBody OpenCreateAppDTO req) { if (null == req.getApp()) { throw new BadRequestException("App is null"); } final OpenAppDTO appDto = req.getApp(); - if (null == appDto.getAppId()) { - throw new BadRequestException("AppId is null"); - } + if (!StringUtils.hasText(appDto.getAppId())) { + throw new BadRequestException("AppId is blank"); + } - // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 - serverAppOpenApiService.createApp(req); + // Delegate to service + appOpenApiService.createApp(req); @@ - return ResponseEntity.ok(appDto); + return ResponseEntity.ok(appDto);
101-116: Default /apps behavior overlaps with /apps/authorizedBoth endpoints return authorized apps. Consider consolidating to one (e.g., keep /apps with appIds optional) or leave both but document behavior. Add a test for /apps/authorized (not currently covered).
122-128: Test coverage for /apps/authorizedAdd a unit test similar to testGetAuthorizedApps that hits GET /openapi/v1/apps/authorized to ensure wiring doesn’t regress.
134-141: Use 404 Not Found for missing appReturning 400 for a missing resource is non-RESTful. Prefer 404.
Apply this diff:
public ResponseEntity<OpenAppDTO> getApp(@PathVariable String appId) { List<OpenAppDTO> apps = serverAppOpenApiService.getAppsInfo(Collections.singletonList(appId)); if (apps.isEmpty()) { - throw new BadRequestException("App not found: " + appId); + return ResponseEntity.notFound().build(); } return ResponseEntity.ok(apps.get(0)); }
187-197: Guard operator lookup in createAppInEnvIf the consumer is missing or has a blank name, this will NPE or send an empty operator. Validate and fail fast with 400.
Apply this diff:
public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); - String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); + var consumer = this.consumerService.getConsumerByConsumerId(consumerId); + if (consumer == null || !StringUtils.hasText(consumer.getName())) { + throw new BadRequestException("Invalid consumer/operator"); + } + String operator = consumer.getName(); - serverAppOpenApiService.createAppInEnv(env, app, operator); + serverAppOpenApiService.createAppInEnv(env, app, operator); return ResponseEntity.ok().build(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apollo-portal/pom.xml(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (8)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)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/bo/ReleaseBO.java (1)
ReleaseBO(24-46)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-76)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (1)
RestController(42-177)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (7)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java (1)
ClusterDTO(23-80)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(45-351)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/AppModel.java (1)
AppModel(25-95)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
SpringSecurityUserInfoHolder(33-121)
⏰ 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 (14)
apollo-portal/pom.xml (4)
257-277: Generated sources path: OKbuild-helper marks the generator output as sources; path matches OpenAPI generator layout.
281-319: Version gating by JDK: goodProfiles to pin generator versions for Java 8 vs 11+ are appropriate.
48-62: Verify no javax-based Swagger artifacts transitively included
Runmvn dependency:tree -Dincludes=io.swagger.core.v3in the apollo-portal module and confirm onlyswagger-annotations-jakartaandswagger-models-jakartaappear on the classpath—no plainswagger-annotations/swagger-models.
141-173: pluginManagement executions apply to the declared plugin; no duplication neededMaven will merge the
<executions>and version from the<pluginManagement>block into the matching<plugin>under<plugins>, so the OpenAPI generator will run duringgenerate-sourceswithout copying the<executions>block.Likely an incorrect or invalid review comment.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (5)
104-119: LGTM: IDs path exercises the intended branchVerifies that authorized lookup is skipped when appIds are provided. Assertions and mocks look correct.
161-181: Create flow assertions are solidCovers permission, service invocation, and role assignment to self. This is a good end-to-end assertion of the new flow.
253-266: LGTM: env-clusters flowHappy path is asserted with size and field checks; mock wiring is appropriate.
268-281: LGTM: by-self pagination pathCovers authorized app resolution and paging call; assertions look good.
314-321: Nice: capture and assert DTO sent to serviceGood use of ArgumentCaptor to verify DTO contents for create-in-env.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (5)
91-94: LGTM: env-clusters endpointStraight delegation and ResponseEntity wrapping look good.
147-159: LGTM: update path validates ID consistencyPath/body mismatch check and delegation are correct.
165-171: LGTM: by-self endpointAuthorization resolution and pagination delegation look good.
203-209: Confirm intended permission for deleteOpenAPI delete uses @consumerPermissionValidator.isAppAdmin(#appId) while the portal controller requires super admin for deletion. If parity is desired, switch to super admin; if not, document the difference and justify the relaxation.
Reference: portal AppController delete requires super admin (see apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java).
61-64: Transactional annotation namespaceYou’re importing javax.transaction.Transactional. If the project is on Spring 6 / Boot 3 (Jakarta), prefer jakarta.transaction.Transactional or Spring’s @org.springframework.transaction.annotation.Transactional. Please verify baseline and align.
Based on learnings
...ain/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
.../java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
Show resolved
Hide resolved
.../java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
Outdated
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
79827a5 to
5054e69
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/server/service/ServerAppOpenApiService.java (1)
71-75: Null-safety in createApp (req/app/admins) to prevent NPEscreateApp dereferences req.getApp() and passes req.getAdmins() as-is; both can be null. Make it defensive.
@Override public void createApp(OpenCreateAppDTO req) { - App app = convert(req.getApp()); - appService.createAppAndAddRolePermission(app, req.getAdmins()); + com.google.common.base.Preconditions.checkArgument(req != null, "request must not be null"); + com.google.common.base.Preconditions.checkArgument(req.getApp() != null, "app must not be null"); + App app = convert(req.getApp()); + java.util.Set<String> admins = + java.util.Optional.ofNullable(req.getAdmins()).orElse(java.util.Collections.emptySet()); + appService.createAppAndAddRolePermission(app, new java.util.HashSet<>(admins)); }
🧹 Nitpick comments (8)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
41-42: Legacy bean can cause confusion/duplication; deprecate or gate with a profileThis old implementation uses legacy DTOs and OpenApiBeanUtils while a new impl exists. Keeping both active can confuse autowiring and maintenance. Mark it @deprecated and consider guarding with @Profile("legacy-openapi") or @ConditionalOnProperty disabled by default. Also rename the class to ServerAppOpenApiServiceOld to avoid type-name clashes.
-@Service("ServerAppOpenApiServiceOld") +@Deprecated +@Service("ServerAppOpenApiServiceOld") +@org.springframework.context.annotation.Profile("legacy-openapi") +// Consider renaming the class to ServerAppOpenApiServiceOld for clarity.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (4)
160-181: Consider returning 201 Created for create operationsFor POST /openapi/v1/apps, HTTP 201 with Location header is more RESTful than 200. If adopted, update expectations in this test accordingly.
212-227: Update flow verification looks goodAdmin check + body echo and service invocation verified. Optionally assert that the controller rejects requests when isAppAdmin is false (403).
268-281: Also verify pageable forwardingTo ensure pagination params are passed correctly, capture the Pageable and assert page/size.
- Mockito.verify(serverAppOpenApiService, times(1)).getAppsBySelf(any(Set.class), any()); + org.mockito.ArgumentCaptor<org.springframework.data.domain.Pageable> pageCaptor = + org.mockito.ArgumentCaptor.forClass(org.springframework.data.domain.Pageable.class); + Mockito.verify(serverAppOpenApiService, times(1)).getAppsBySelf(any(Set.class), pageCaptor.capture()); + org.junit.Assert.assertEquals(0, pageCaptor.getValue().getPageNumber()); + org.junit.Assert.assertEquals(10, pageCaptor.getValue().getPageSize());
294-321: Add a negative test for invalid env namesEnv.valueOf will throw on invalid env; add a test that POST /openapi/v1/apps/envs/INVALID returns 400 with a clear message.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (3)
125-128: Unimplemented getAuthorizedApps() can surprise callersEither implement or remove from the interface if the controller no longer uses it. Leaving it throwing will cause runtime errors if called.
182-188: Validate env input and return a clear 4xx on invalid envEnv.valueOf throws IllegalArgumentException which will bubble up as 500 if uncaught. Validate/normalize env or catch and rethrow a domain exception the controller can map to 400.
- public void createAppInEnv(String env, OpenAppDTO app, String operator) { - App appEntity = convert(app); - appService.createAppInRemote(Env.valueOf(env), appEntity); + public void createAppInEnv(String env, OpenAppDTO app, String operator) { + App appEntity = convert(app); + try { + appService.createAppInRemote(Env.valueOf(env), appEntity); + } catch (IllegalArgumentException ex) { + throw new IllegalArgumentException("Invalid env: " + env, ex); + } roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), ConfigConsts.NAMESPACE_APPLICATION, env, operator); }
206-222: Avoid leaking internal exception messages in API responses; add loggingReturning e.getMessage() may expose internals. Log the exception and return a concise error message.
- } else { - response.addResponseEntity(RichResponseEntity.error(HttpStatus.INTERNAL_SERVER_ERROR, - String.format("load appId:%s from env %s error.", appId, env) + e.getMessage())); + } else { + // TODO: inject a Logger if not present and log the exception with context + response.addResponseEntity(RichResponseEntity.error( + HttpStatus.INTERNAL_SERVER_ERROR, + String.format("load appId:%s from env %s error.", appId, env) + )); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
Service(50-223)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (1)
RestController(42-177)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (6)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java (1)
ClusterDTO(23-80)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(45-351)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)
⏰ 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 (6)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java (1)
49-50: RESTful path change looks good; confirm backward-compat/deprecation handlingSwitching to /openapi/v1/apps?authorized=true aligns with the issue goals. Please confirm the old /apps/authorized path is either removed with clear release notes or returns a deprecation response/redirect to avoid breaking existing clients.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
76-103: Authorized apps flow: solid coverageGood use of ConsumerAuthUtil + ConsumerService to resolve authorized appIds and delegating to serverAppOpenApiService.getAppsInfo; set-based verification avoids ordering flakiness.
110-119: Skip unnecessary auth lookup when appIds are providedThe negative verification against ConsumerService is good; maintains performance and avoids unnecessary calls.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (2)
95-111: Cluster names collection logic is correct and null-safeConverting Set to ArrayList from BeanUtils.toPropertySet ensures deterministic JSON array; BeanUtils handles null lists safely.
87-93: Fix NPE risk when admins is null in createApp (and when app is null)Same NPE risks: req.getApp() may be null and new HashSet<>(req.getAdmins()) throws on null.
@Override public void createApp(OpenCreateAppDTO req) { - App app = null; - if (req.getApp() != null) { - app = convert(req.getApp()); - } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); + com.google.common.base.Preconditions.checkArgument(req != null, "request must not be null"); + com.google.common.base.Preconditions.checkArgument(req.getApp() != null, "app must not be null"); + App app = convert(req.getApp()); + java.util.Set<String> admins = + java.util.Optional.ofNullable(req.getAdmins()).orElse(java.util.Collections.emptySet()); + appService.createAppAndAddRolePermission(app, new java.util.HashSet<>(admins)); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
28-47: Make createApp non-default and add missing operations used by the controller
- Default-throwing createApp adds surprise; make it abstract so impls must implement.
- Controller/impl expose createAppInEnv, deleteApp, findMissEnvs; include them in the interface to keep the controller depending on the interface rather than the concrete class.
public interface AppOpenApiService { - default void createApp(OpenCreateAppDTO req) { - throw new UnsupportedOperationException(); - } + void createApp(OpenCreateAppDTO req); List<OpenEnvClusterDTO> getEnvClusterInfo(String appId); List<OpenAppDTO> getAllApps(); List<OpenAppDTO> getAppsInfo(List<String> appIds); List<OpenAppDTO> getAuthorizedApps(); void updateApp(OpenAppDTO openAppDTO); List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Pageable page); MultiResponseEntity<OpenEnvClusterInfo> getAppNavTree(String appId); + + // Environment-aware operations exposed by the controller + void createAppInEnv(String env, OpenAppDTO app, String operator); + OpenAppDTO deleteApp(String appId); + MultiResponseEntity<String> findMissEnvs(String appId); }Run this to check if the controller currently injects the concrete class instead of the interface and to locate call sites of the extra methods:
...ain/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
Outdated
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
5054e69 to
ddfda8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (2)
46-63: Guard against NPE when Authentication is null.The method delegates to getCurrentUsername() at line 54, which can throw NPE if
SecurityContextHolder.getContext().getAuthentication()returns null (e.g., during OpenAPI calls that may not populate Spring Security context). Add null-safety to getCurrentUsername() as shown below.Apply this diff to harden getCurrentUsername():
private String getCurrentUsername() { + org.springframework.security.core.Authentication authentication = + SecurityContextHolder.getContext().getAuthentication(); + if (authentication == null) { + return null; + } - Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); + Object principal = authentication.getPrincipal(); + if (principal == null) { + return null; + } if (principal instanceof UserDetails) { return ((UserDetails) principal).getUsername(); } if (principal instanceof Principal) { return ((Principal) principal).getName(); } return String.valueOf(principal); }And update getUser() to handle null username:
// 回退到Spring Security上下文 String userId = getCurrentUsername(); + if (userId == null || userId.isEmpty()) { + // No authentication available; return minimal user + return new UserInfo(); + } UserInfo userInfoFound = userService.findByUserId(userId);
68-106: Retrieve consumer ID from the correct request attribute.Line 84 incorrectly reads
request.getAttribute("Authorization")and parses it as a long. The OpenAPI authentication filter stores the numeric consumer ID under theConsumerAuthUtil.CONSUMER_IDattribute, not "Authorization". Update the code to use the correct attribute name.Apply this diff:
- // 获取Consumer ID - Object consumerIdObj = request.getAttribute("Authorization"); - if (consumerIdObj == null) { + // 获取Consumer ID - use the correct attribute name + Object consumerIdObj = request.getAttribute(com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil.CONSUMER_ID); + if (consumerIdObj == null) { return null; } - long consumerId = Long.parseLong(consumerIdObj.toString()); + // ConsumerAuthUtil stores it as Long, but verify type safety + if (!(consumerIdObj instanceof Long)) { + return null; + } + long consumerId = (Long) consumerIdObj; Consumer consumer = consumerService.getConsumerByConsumerId(consumerId);Alternatively, use the helper method if available:
- // 获取Consumer ID - Object consumerIdObj = request.getAttribute("Authorization"); - if (consumerIdObj == null) { - return null; - } - - long consumerId = Long.parseLong(consumerIdObj.toString()); + // 获取Consumer ID using helper + Long consumerId = com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil.retrieveConsumerIdFromRequest(request); + if (consumerId == null) { + return null; + } Consumer consumer = consumerService.getConsumerByConsumerId(consumerId);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (2)
72-81: Add null validation to convert() method.The method does not guard against a null dto parameter, which will cause NPE when accessing dto getters. Add explicit validation to fail fast with a clear message.
Apply this diff:
private App convert(OpenAppDTO dto) { + com.google.common.base.Preconditions.checkArgument(dto != null, "OpenAppDTO must not be null"); + com.google.common.base.Preconditions.checkArgument(dto.getAppId() != null, "AppId must not be null"); return App.builder() .appId(dto.getAppId()) .name(dto.getName()) .ownerName(dto.getOwnerName()) .orgId(dto.getOrgId()) .orgName(dto.getOrgName()) .ownerEmail(dto.getOwnerEmail()) .build(); }
87-93: Fix NPE risk with null admins list.Line 92 calls
new HashSet<>(req.getAdmins())which throws NPE if admins is null. Additionally, ensure req.getApp() is validated since passing a null app to appService may cause issues.Apply this diff:
@Override public void createApp(OpenCreateAppDTO req) { - App app = null; - if (req.getApp() != null) { - app = convert(req.getApp()); - } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); + com.google.common.base.Preconditions.checkArgument(req != null, "Request must not be null"); + com.google.common.base.Preconditions.checkArgument(req.getApp() != null, "App must not be null"); + App app = convert(req.getApp()); + Set<String> admins = req.getAdmins() != null ? new HashSet<>(req.getAdmins()) : Collections.emptySet(); + appService.createAppAndAddRolePermission(app, admins); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
65-84: Use the service's returned DTO in the response.Line 76 calls
serverAppOpenApiService.createApp(req)but ignores any return value, and line 83 responds with the original request DTO. If the service returns the persisted entity (with server-side defaults, generated IDs, or normalized fields), the controller should propagate that. Otherwise, the API response may not reflect the actual stored state.However, checking the interface (AppOpenApiService),
createAppcurrently returns void. If the service is meant to return the created entity, first update the interface and implementation to returnOpenAppDTO, then update this controller method to use it.Apply this diff (after updating the service interface to return OpenAppDTO):
// 直接调用ServerAppOpenApiService,让Service层处理DTO转换 - serverAppOpenApiService.createApp(req); + OpenAppDTO created = serverAppOpenApiService.createApp(req); if (null != req.getAssignAppRoleToSelf() && req.getAssignAppRoleToSelf()) { long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); - consumerService.assignAppRoleToConsumer(consumerId, appDto.getAppId()); + consumerService.assignAppRoleToConsumer(consumerId, created.getAppId()); } - return ResponseEntity.ok(appDto); + return ResponseEntity.ok(created);
150-159: Return the service's updated DTO.Line 156 calls
serverAppOpenApiService.updateApp(dto)and line 158 echoes the request DTO back. If the service performs normalization or updates (e.g., timestamps, owner info), the response should reflect the persisted state, not the request.First update the AppOpenApiService interface to return
OpenAppDTOfromupdateApp, then capture and return the service result here.Apply this diff (after updating the service interface):
// 直接调用ServerAppOpenApiService,让Service层处理DTO转换 - serverAppOpenApiService.updateApp(dto); + OpenAppDTO updated = serverAppOpenApiService.updateApp(dto); - return ResponseEntity.ok(dto); + return ResponseEntity.ok(updated);
🧹 Nitpick comments (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (2)
30-32: Clarify the UnsupportedOperationException message.The default implementation throws a generic exception. Consider providing a descriptive message to guide implementers or callers who encounter this, e.g., "createApp must be implemented by the service implementation".
Apply this diff:
default void createApp(OpenCreateAppDTO req) { - throw new UnsupportedOperationException(); + throw new UnsupportedOperationException("createApp must be implemented by the service implementation"); }
42-42: Consider returning the updated entity from updateApp.The method currently returns void, but best practice for update operations is to return the canonical state after persistence. This allows callers to see server-side changes (e.g., updated timestamps, normalized fields). Consider changing the signature to
OpenAppDTO updateApp(OpenAppDTO openAppDTO)to align with REST conventions and enable the controller to return the authoritative updated state.Apply this diff:
- void updateApp(OpenAppDTO openAppDTO); + OpenAppDTO updateApp(OpenAppDTO openAppDTO);Note: This change will require updating the implementation in ServerAppOpenApiService and the controller's updateApp method to use the returned value.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
146-152: Handle malformed JSON in configurations parsing
Wrap theGSON.fromJson(release.getConfigurations(), TYPE)call inOpenApiModelConverters.fromReleaseDTO(around line 149) in atry/catchforJsonSyntaxException, defaulting to an empty map or rethrowing a descriptiveIllegalArgumentException.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
182-222: Validateenvpath parameter in AppController#createAppInEnv
Add validation (e.g.,@Patternor manual check) to return HTTP 400 for invalid environment names instead of lettingEnv.valueOf()throw IllegalArgumentException and result in a 500.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java
🧰 Additional context used
🧬 Code graph analysis (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
Service(50-223)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
SpringSecurityUserInfoHolder(33-121)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (6)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
Service(41-111)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (13)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java (1)
ClusterDTO(23-80)apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/GrayReleaseRuleDTO.java (1)
GrayReleaseRuleDTO(24-81)apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/GrayReleaseRuleItemDTO.java (1)
GrayReleaseRuleItemDTO(28-85)apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ItemDTO.java (1)
ItemDTO(20-103)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/KVEntity.java (1)
KVEntity(19-44)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/bo/ReleaseBO.java (1)
ReleaseBO(24-46)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-76)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (1)
RestController(42-177)
⏰ 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 (15)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (4)
22-22: LGTM! Import updates align with the new lazy ConsumerService injection.The addition of
@LazyandConsumerServiceimports, along with the import reorganization, correctly supports the bean definition changes below.Also applies to: 35-35, 58-82
112-113: LGTM! Auth profile bean correctly wired with lazy ConsumerService.The updated bean signature and constructor call align with the
SpringSecurityUserInfoHolderconstructor that now supports OpenAPI consumer-based user resolution.
216-217: LGTM! LDAP profile bean correctly wired with lazy ConsumerService.The updated bean signature and constructor call match the auth profile and correctly support OpenAPI consumer context resolution.
343-346: Support OpenAPI consumer resolution in OIDC and default profiles
NeitherOidcUserInfoHoldernorDefaultUserInfoHolderinjectsConsumerServicefor OpenAPI consumer‐based user resolution. If these profiles should support OpenAPI flows, update both to accept and delegate toConsumerService; otherwise confirm that fallback to standard authentication is acceptable.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
39-43: LGTM! Lazy injection appropriately used.The lazy injection of ConsumerService prevents potential circular dependency issues while enabling consumer-based user resolution for OpenAPI contexts.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (4)
73-84: LGTM! Well-structured utility class.The private constructor prevents instantiation, and the shared Gson instance is thread-safe for concurrent use. The Type token pattern for Map deserialization is appropriate.
88-113: LGTM! Proper null-safety and collection handling.The Item conversion methods correctly validate inputs with Preconditions and handle empty collections gracefully by returning empty lists rather than null.
294-425: LGTM! Comprehensive and consistent conversion implementations.The remaining conversion methods demonstrate consistent patterns:
- Input validation with Preconditions
- Null-safe collection handling (empty lists/sets instead of null)
- Appropriate use of BeanUtils for simple transformations
- Manual construction where type differences require it (e.g., GrayReleaseRule items)
The gray release rule conversion (lines 307-320) correctly creates defensive copies of collections and handles null lists gracefully.
260-268: Remove environment validation note —fromNamespaceIdentifieronly callsgetEnv().toString()and can’t throw.Likely an incorrect or invalid review comment.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (3)
58-74: LGTM! Appropriate test configuration.The test setup correctly uses @WebMvcTest to focus on the controller layer and @MockBean for dependencies. The inclusion of ConsumerPermissionValidator as a named bean ensures authorization aspects are testable.
161-181: LGTM! Comprehensive test for role assignment flow.The test properly verifies the complete flow: permission check, app creation, and conditional role assignment. The use of ArgumentCaptor would further strengthen verification (not critical here since the mock is configured to accept any()).
293-321: LGTM! Excellent use of ArgumentCaptor for DTO verification.The test demonstrates a best practice by using ArgumentCaptor to verify the DTO fields rather than exact object equality (which would be fragile). This approach is resilient to DTO implementation changes and clearly documents the expected transformations.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
96-111: LGTM! Proper environment iteration and cluster extraction.The method correctly iterates over active environments and extracts cluster names using BeanUtils.toPropertySet, then wraps the Set in an ArrayList for the DTO. The empty-collection handling is sound.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
90-141: LGTM! Proper service delegation and response handling.These endpoint methods correctly:
- Delegate to the service layer without business logic in the controller
- Return service results directly via ResponseEntity
- Use appropriate HTTP status codes (200 OK)
- Handle optional parameters (appIds) with conditional logic
The getApp method (lines 135-141) appropriately throws BadRequestException when the app is not found, providing clear error feedback.
184-219: LGTM! Well-structured lifecycle and environment-scoped endpoints.The implementation demonstrates good practices:
- createAppInEnv correctly resolves the operator from the consumer context for audit purposes
- deleteApp uses appropriate authorization check and audit annotation
- findMissEnvs returns MultiResponseEntity to aggregate per-environment results/errors
- All methods use ResponseEntity for consistent response wrapping
The ApolloAuditLog annotations ensure proper audit trail for create/update/delete operations.
...src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
ddfda8b to
87ab98e
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/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
87-93: Construct message:Subject line bold.
Maybe: "Handle null admin lists in
createApp"Explain quickly.
Add diff with fix.
Diff:
+ Preconditions.checkArgument(req != null, "request must not be null"); + Preconditions.checkArgument(req.getApp() != null, "App must not be null"); + App app = convert(req.getApp()); + Set<String> admins = Optional.ofNullable(req.getAdmins()).orElse(Collections.emptySet()); + appService.createAppAndAddRolePermission(app, new HashSet<>(admins));But would require Guava import. Maybe they don't want to add. Could just use
Objects.requireNonNullandCollections.emptySet(). Provide diff using java.util.Objects.+ Objects.requireNonNull(req, "request must not be null"); + OpenAppDTO appDto = Objects.requireNonNull(req.getApp(), "App must not be null"); + App app = convert(appDto); + Set<String> admins = req.getAdmins() == null ? Collections.emptySet() : req.getAdmins(); + appService.createAppAndAddRolePermission(app, new HashSet<>(admins));Need to ensure
Collectionsimport available? Alreadyimport java.util.*;includes.Also need to import Objects? with
java.util.*includes?Objectsin java.util.* yes.But we changed earlier
App app = null;etc. We'll propose diff.Let's craft comment.
Also note we convert to HashSet maybe to copy.
Let's produce comment.
Also note that returning void remains. Not part of fix.
After comment done.
Now review AppController file for issues.
Potential bug: For createApp, they rely on service to throw when
req.getAdmins()null? But we already flagged.Another potential bug:
@GetMapping(value = "/apps/authorized")new endpoint returns list but tests changed to query parameter, so new endpoint untested? not necessary.Focus on updateApp: they don't call
serverAppOpenApiServiceto return updated value. At least there is risk the service modifies e.g. owner details (maybe autopopulated). Without returning, API echoes request body even if underlying service normalized. That may be considered major because clients expect canonical data. Should we raise? I'd mention major.Given previous review comment (from past review) insisted to return service result. New code still ignoring. Should we raise? yes. We'll comment on AppController lines 156-158, referencing that service returns canonical representation (should adjust to return). But do we know service returns something? Implementation
updateAppreturns void. Wait we looked at new service: method returns void. So there is no canonical to return. But we could adjust service to return? ImplementationupdateAppcurrently returns void but maybe we can change toreturn OpenApiModelConverters.fromApp(updatedApp);. InterfaceAppOpenApiService.updateAppcurrently void (line 42). For change they'd need to update interface to returnOpenAppDTO. That's bigger change but maybe necessary? But spec? Without verifying, but restful: They currently send request to service and return request. Could be acceptable but worthless.However due to service not returning, restful. Should we still comment? Could propose to adjust service signature to return
OpenAppDTO. But that's bigger change maybe out-of-s scope? But apparently previous reviewer flagged same issue but referencing "service returns canonical DTO" but that's not accurate given code—they might expect to adjust service to return but not implemented.Maybe we should examine
AppService.updateAppInLocal(app)returnsAppupdated. They convert to DTO? They publish event. Could convert to DTO and return. But they'd need to convert to open API model? We can propose—they already haveOpenApiModelConverters.fromApp. So service could returnOpenAppDTO(converted fromupdatedApp). They just need to return.Given restful design, returning canonical is better. I'd treat as major but arguable? Without restful spec, but open API maybe previously responded with DTO? Need to check old controller (before refactor). We don't know.
But objective said: standardize responses using DTOs - likely they expect restful. Without canonical we degrade. I'd highlight at least for update? For create, same issue. Might treat as recommended? But to avoid speculation, maybe skip.
Focus on actual bug:
createAppreturns ResponseEntity.ok(appDto); but they do not assign roles when assignAppRoleToSelf? They do. But there is no success? But due to earlier bug, createApp may throw.Also
createAppInEnv: obtains operator from consumer butconsumerService.getConsumerByConsumerIdmight return null; they'd get NullPointer. Maybe but not new.In
getAppsmethod: When authorized set empty and they callserverAppOpenApiService.getAppsInfo(new ArrayList<>(authorizedAppIds));-> new HashSet of empty -> ok.Check
getAppsBySelfmethod: they passauthorizedAppIdsset maybe empty -> servicefindByAppIdsexpects set but not.Focus on comment to add.
Additionally, check AppOpenApiService interface
getAuthorizedApps()still in interface but new service returns UnsupportedOperationException. But there is new controller methodgetAuthorizedAppsusingserverAppOpenApiService.getAppsInfo. So maybegetAuthorizedAppsmethod now unused. Should we remove? Not necessary.Alright need to produce review comment for service file lines 88-93.
Also maybe comment on convert? maybe not.
Now maybe there is bug in AppController createApp: they return ResponseEntity.ok even when new resource created -> but restful they'd expect 201? But previously maybe same.
Focus on comment.
Construct comment now.
Need to ensure comment includes tag ``.
Add diff.
After comment, check rest.
Also maybe highlight that
createAppshould not acceptreqwith nullassignAppRoleToSelf? nice.Let's craft comment.
Need to include in comment that this will produce NullPointer when request JSON omits admins (common). Provide example.
Ok.
But before writing comment, we should also ensure we mention in comment to import
java.util.Objects? Already usingjava.util.*, includes.However we used
Objects.requireNonNull? We'll useObjects.requireNonNull. So we need to ensurejava.util.Objectsis available via wildcard? yesjava.util.*includesObjects. Good.The diff we propose:
- App app = null; - if (req.getApp() != null) { - app = convert(req.getApp()); - } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); + Objects.requireNonNull(req, "request must not be null"); + OpenAppDTO appDto = Objects.requireNonNull(req.getApp(), "App must not be null"); + App app = convert(appDto); + Set<String> admins = req.getAdmins() == null ? Collections.emptySet() : req.getAdmins(); + appService.createAppAndAddRolePermission(app, new HashSet<>(admins));Need to ensure indentation consistent (two spaces). We'll adjust to two spaces indentation to match style (two spaces). existing indentation uses two spaces. We'll adjust.
But we also use
OpenAppDTOin diff; ensure accessible? Already imported.Ok.
Now continue.
After comment, go to interface? maybe comment? No.
What about AppController createApp? They rely on service returning nothing. But we already flagged bug on service. Without fix, admin role assign uses request app id but service might canonical? No.
anything else? Another bug: In AppController createApp, they call
assignAppRoleToConsumereven if service fails? but done after service call; if service throws exception they'd not get there.Now final.
Need to add comment.
Also ensure we include
Based on learnings? Not necessary.Let's produce comment.
Need to place within file.
In file
ServerAppOpenApiServicewe already<!-- file_start ...with scratchpad maybe we need to update to include scratch.Wait we previously closed file with no comment. But now we need to reopen? For instructions, we must produce comments inside file markers. But we already done file_end. Can't re-open? maybe we can re-open by reprinting? We can start new
file_startwith same file? Probably not? But we can re-run? Maybe best to restructure: we should revert earlier (lack comment) but once we produce final output we can restructure. Since final answer not yet provided we can adjust: We'll reintroduce file_start for same file? The instructions? Probably we should restructure final message to include comment. We can re-write entire output? Since we haven't provided final message yet, we can restructure final message to include comment. So we need to plan final output accordingly: For each file we should only include final once. So we need to create final message with file_start including comment. We'll rewrite final message from scratch? But we already wrote sections above; in final message we can restructure.Given we haven't final message yet, we can produce final message with proper structure. We'll include comment there. Already wrote earlier sections but we can restructure? Eh we already output some block for first file (without comment). That's part of final message? Yes we already produced for first file. For second file we left blank. But we now need to provide comment for third file? Wait we already output final message? We already responded with sections? Yes earlier we output for first two files (the final output). But after hooking comment we can't change? We already produced partial final output as ChatGPT message. That is posted? yes we already responded with start, file_end etc? we already done first two files? I think yes we wrote final message? The system message? It's recorded. Hmm can't change.
But we still can continue to produce more output in same message? Wait we already responded? Usually we craft final message in single response. However I already output some text (like file_start etc). That's our final response? Maybe not? In conversation, we as assistant wrote message with first two file sections? yes we responded earlier. But we can't send second message? We can continue writing? But we already wrote in final channel? yes the earlier output in final channel (since instructions to respond once). We can't add more? However we can continue in same message, append further sections. Yet we already closed file_end for second file. But we can still comment on same file? Not easily.
But the final message may include multiple file sections. We already have for first two, but we can still append comment referencing second file by re-opening? Not allowed? Hmm.
♻️ Duplicate comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
86-139: Return canonical DTO and avoid NPE when admins absent.
new HashSet<>(req.getAdmins())throws when callers omit admins (the controller tests already build such a request). At the same time, we need to surface the persisted app so controllers can return authoritative data. Guard the inputs, default admins to an empty set, and convert the saved entity back toOpenAppDTO.- public void createApp(OpenCreateAppDTO req) { - App app = null; - if (req.getApp() != null) { - app = convert(req.getApp()); - } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); - } + public OpenAppDTO createApp(OpenCreateAppDTO req) { + Objects.requireNonNull(req, "request must not be null"); + OpenAppDTO appDto = Objects.requireNonNull(req.getApp(), "App must not be null"); + App app = convert(appDto); + Set<String> admins = + req.getAdmins() == null ? Collections.emptySet() : new HashSet<>(req.getAdmins()); + App created = appService.createAppAndAddRolePermission(app, admins); + return OpenApiModelConverters.fromApp(created); + } @@ - public void updateApp(OpenAppDTO openAppDTO) { - App app = convert(openAppDTO); - App updatedApp = appService.updateAppInLocal(app); - publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); - } + public OpenAppDTO updateApp(OpenAppDTO openAppDTO) { + Objects.requireNonNull(openAppDTO, "OpenAppDTO must not be null"); + App updatedApp = appService.updateAppInLocal(convert(openAppDTO)); + publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); + return OpenApiModelConverters.fromApp(updatedApp); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
75-83: Use the persisted DTO in the response.After the service returns the canonical
OpenAppDTO, respond with that object (and use itsappIdwhen self-assigning roles) so clients receive the authoritative state instead of the raw request.- // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 - serverAppOpenApiService.createApp(req); - - if (null != req.getAssignAppRoleToSelf() && req.getAssignAppRoleToSelf()) { + // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 + OpenAppDTO created = serverAppOpenApiService.createApp(req); + + if (null != req.getAssignAppRoleToSelf() && req.getAssignAppRoleToSelf()) { long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); - consumerService.assignAppRoleToConsumer(consumerId, appDto.getAppId()); + consumerService.assignAppRoleToConsumer(consumerId, created.getAppId()); } - return ResponseEntity.ok(appDto); + return ResponseEntity.ok(created);
156-159: Return the updated DTO supplied by the service.Echoing the request body hides any server-side normalization. Capture the
OpenAppDTOproduced byupdateAppand return it.- // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 - serverAppOpenApiService.updateApp(dto); - - return ResponseEntity.ok(dto); + // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 + OpenAppDTO updated = serverAppOpenApiService.updateApp(dto); + + return ResponseEntity.ok(updated);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
Service(50-223)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (7)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java (1)
ClusterDTO(23-80)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
Service(41-111)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)
⏰ 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 (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
294-321: Solid coverage for env-scoped creation.Nice job capturing the operator via
ArgumentCaptorand asserting the DTO fields going intocreateAppInEnv; this gives confidence the controller wiring around env-specific creation is correct.
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
Outdated
Show resolved
Hide resolved
dosubot
bot
added
size:XL
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
58b2775 to
8396bb8
Compare
dosubot
bot
added
size:XXL
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
8396bb8 to
a9b88aa
Compare
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
41-41: Remove unused legacy bean 'ServerAppOpenApiServiceOld'.The renamed bean has no active references in the codebase (verified by prior review); this class or its annotation should be deleted to eliminate dead code.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (2)
76-85: Guard against null DTO in convert method.The method does not validate that
dtois non-null, which will cause NPE when accessing its fields.private App convert(OpenAppDTO dto) { + if (dto == null) { + throw new IllegalArgumentException("OpenAppDTO must not be null"); + } return App.builder() .appId(dto.getAppId()) .name(dto.getName()) .ownerName(dto.getOwnerName()) .orgId(dto.getOrgId()) .orgName(dto.getOrgName()) .ownerEmail(dto.getOwnerEmail()) .build(); }
92-98: Fix NPE risk when admins is null in createApp.
new HashSet<>(req.getAdmins())will throw NPE ifadminsis null, and the method proceeds even ifreq.getApp()is null.@Override public void createApp(OpenCreateAppDTO req) { - App app = null; - if (req.getApp() != null) { - app = convert(req.getApp()); - } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); + if (req == null || req.getApp() == null) { + throw new BadRequestException("Request and app must not be null"); + } + App app = convert(req.getApp()); + Set<String> admins = req.getAdmins() != null ? new HashSet<>(req.getAdmins()) : Collections.emptySet(); + appService.createAppAndAddRolePermission(app, admins); }
🧹 Nitpick comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
246-272: Refactor to eliminate code duplication in operator-aware methods.
updateAppInLocalWithOperatorduplicates the entire logic ofupdateAppInLocal(lines 221-245), differing only in how the operator is obtained (parameter vsuserInfoHolder.getUser().getUserId()). This violates DRY and doubles maintenance cost.Consolidate by having the original method delegate to the operator-aware variant:
@Transactional @ApolloAuditLog(type = OpType.UPDATE, name = "App.update") public App updateAppInLocal(App app) { - String appId = app.getAppId(); - - App managedApp = appRepository.findByAppId(appId); - if (managedApp == null) { - throw BadRequestException.appNotExists(appId); - } - - managedApp.setName(app.getName()); - managedApp.setOrgId(app.getOrgId()); - managedApp.setOrgName(app.getOrgName()); - - String ownerName = app.getOwnerName(); - UserInfo owner = userService.findByUserId(ownerName); - if (owner == null) { - throw new BadRequestException("App's owner not exists. owner = %s", ownerName); - } - managedApp.setOwnerName(owner.getUserId()); - managedApp.setOwnerEmail(owner.getEmail()); - String operator = userInfoHolder.getUser().getUserId(); - managedApp.setDataChangeLastModifiedBy(operator); - - return appRepository.save(managedApp); + return updateAppInLocalWithOperator(app, operator); }
309-332: Refactor to eliminate code duplication in operator-aware deletion.
deleteAppInLocalWithOperatorduplicatesdeleteAppInLocal(lines 282-308), differing only in how the operator is sourced. Apply the same DRY consolidation as recommended for the update methods above.@Transactional @ApolloAuditLog(type = OpType.DELETE, name = "App.delete") public App deleteAppInLocal(String appId) { - App managedApp = appRepository.findByAppId(appId); - if (managedApp == null) { - throw BadRequestException.appNotExists(appId); - } String operator = userInfoHolder.getUser().getUserId(); - - //this operator is passed to com.ctrip.framework.apollo.portal.listener.DeletionListener.onAppDeletionEvent - managedApp.setDataChangeLastModifiedBy(operator); - - //删除portal数据库中的app - appRepository.deleteApp(appId, operator); - - // append a deleted data influence should be bounded - apolloAuditLogApi.appendDataInfluences(Collections.singletonList(managedApp), App.class); - - //删除portal数据库中的appNamespace - appNamespaceService.batchDeleteByAppId(appId, operator); - - //删除portal数据库中的收藏表 - favoriteService.batchDeleteByAppId(appId, operator); - - //删除portal数据库中Permission、Role相关数据 - rolePermissionService.deleteRolePermissionsByAppId(appId, operator); - - return managedApp; + return deleteAppInLocalWithOperator(appId, operator); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
208-208: Use proper log format parameters instead of string concatenation.The error log uses awkward string concatenation. Use SLF4J's parameterized logging for better performance and readability.
- logger.error("{}{}", String.format("load appId:%s from env %s error.", appId, env), e.getMessage()); + logger.error("load appId:{} from env {} error: {}", appId, env, e.getMessage(), e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(4 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)
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/server/service/impl/ServerAppOpenApiService.java (1)
Service(52-214)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
Service(54-333)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (6)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
Service(54-333)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (4)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-84)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (1)
RestController(40-103)
⏰ 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 (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
131-369: LGTM! Comprehensive test coverage for OpenAPI endpoints.The test suite properly validates all new OpenAPI flows (authorized apps, env-cluster info, paginated queries, update/delete operations, and ID-mismatch validation) using appropriate mocks and argument captors.
...ain/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
Outdated
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
3 times, most recently
from
b501cec to
c535d83
Compare
...ain/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
26-26: Consider returning persisted DTOs from create/update operations.Past reviews noted that
createAppandupdateAppshould returnOpenAppDTOto allow controllers to provide authoritative responses reflecting server-side normalization (e.g., trimmed names, owner email population). Without this, controllers must echo the request payload, which may diverge from the persisted state.Note:
deleteApp(line 42) already returnsOpenAppDTO, establishing a precedent for returning DTOs from mutation operations.Also applies to: 36-36
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
74-91: Consider returning the created app in the response.Past reviews noted that
createAppshould return the persistedOpenAppDTOto reflect server-side defaults and normalization. Currently, line 90 returns an empty body. If the service is updated to return the created DTO, update this endpoint to returnResponseEntity.ok(createdApp)for a more informative response.
139-150: Consider returning the updated app from the service.The operator validation (lines 143-145) was properly added per past reviews. However, past reviews also noted that the controller should return the service's updated DTO (line 147) rather than echoing the request DTO (line 149). If the service is updated to return the persisted
OpenAppDTO, change line 149 to return that result for an authoritative response.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
41-41: Remove unused legacy bean or clarify its purpose.The
@Service("ServerAppOpenApiServiceOld")annotation suggests this is a legacy implementation. The past review indicated no references exist, but the code remains unchanged. If this class is no longer needed, remove it entirely. If it must coexist with the new impl/ServerAppOpenApiService, add a clear comment explaining why both are required.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
94-100: Guard against null admins to prevent NPE.Past reviews flagged the NPE risk at line 99:
new HashSet<>(req.getAdmins())throws ifadminsis null. While the controller validatesreq.getApp()(lines 77-79), it does not validatereq.getAdmins(). The service should handle null defensively:public void createApp(OpenCreateAppDTO req) { App app = null; if (req.getApp() != null) { app = convert(req.getApp()); } - appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); + Set<String> admins = req.getAdmins() != null ? new HashSet<>(req.getAdmins()) : Collections.emptySet(); + appService.createAppAndAddRolePermission(app, admins); }This aligns with the null-safe handling in
AppService.createAppAndAddRolePermission(line 191).
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
190-196: Return the deleted app DTO for consistency.The
deleteAppservice method returns anOpenAppDTO(line 194), but the controller discards it and returns an empty body (line 195). For consistency with RESTful practices and to provide confirmation of what was deleted, consider returning the DTO:public ResponseEntity<Object> deleteApp(@PathVariable String appId, @RequestParam String operator) { if (!StringUtils.hasText(operator)) { throw new BadRequestException("operator cannot be null or empty"); } - appOpenApiService.deleteApp(appId, operator); - return ResponseEntity.ok().build(); + OpenAppDTO deleted = appOpenApiService.deleteApp(appId, operator); + return ResponseEntity.ok(deleted); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
77-87: Add validation for required App fields.Past reviews noted the need to validate
dtofor null (now present at line 78), but also suggested validating required fields likeappId. Consider adding:private App convert(OpenAppDTO dto) { Preconditions.checkArgument(dto != null, "OpenAppDTO must not be null"); + Preconditions.checkArgument(dto.getAppId() != null && !dto.getAppId().trim().isEmpty(), + "AppId must not be null or empty"); return App.builder() .appId(dto.getAppId()) ... }This prevents invalid App entities from being constructed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (9)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/ClusterDTO.java (1)
ClusterDTO(23-80)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)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/service/AppService.java (1)
Service(54-333)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
Service(53-223)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-84)
⏰ 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 (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (3)
78-136: LGTM! Well-structured test setup.The test configuration properly mocks service-level authorization through
ConsumerPermissionValidatorandAppOpenApiService, aligning with the production code's architecture. The@Beforemethod establishes sensible permission defaults that individual tests can override as needed.
138-307: LGTM! Comprehensive test coverage.The test methods thoroughly exercise the OpenAPI endpoints with proper mocking, verification, and assertions. Each test follows the Arrange-Act-Assert pattern and validates both service interactions and response payloads.
308-366: LGTM! Thorough mutation endpoint testing.The update and delete tests properly validate operator handling, permission checks, and input validation (e.g., appId mismatch). The use of
ArgumentCaptorensures precise verification of service method arguments.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (2)
143-147: LGTM! Update implementation is correct.The method properly converts the DTO, delegates to the operator-aware service method, and publishes the change event for downstream listeners.
194-222: LGTM! Deletion and environment discovery implementations are correct.
deleteAppproperly uses the operator-aware service method, publishes the deletion event, and returns the deleted app DTO for confirmation.findMissEnvscorrectly identifies missing environments by catchingNOT_FOUNDexceptions while logging other errors appropriately.
...ain/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
Outdated
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
6f22b4f to
50243a8
Compare
dosubot
bot
added
size:XXL
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
3 times, most recently
from
f32d917 to
fb41d52
Compare
dosubot
bot
removed
the
size:XXL
dosubot
bot
added
the
size:XL
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
fb41d52 to
1439b3c
Compare
dosubot
bot
added
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
390-471: Remove duplicate test methods.The test methods
createAppInEnv_shouldBind_env_query_body,getAppsBySelf_shouldBind_page_size_and_ids,updateApp_shouldBind_path_query_body, anddeleteApp_shouldBind_path_and_queryare duplicated inAppControllerParamBindLowLevelTest.java.Remove these duplicate methods from either this file or
AppControllerParamBindLowLevelTest.javato maintain a single source of truth for these test cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(78-473)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(63-199)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (1)
RestController(48-162)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java (1)
RestController(36-74)
🪛 GitHub Actions: build
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
[error] 89-89: Compilation error in apollo-portal: incompatible types: List cannot be converted to Set at ServerAppOpenApiService.java:89. While running 'mvn -B clean compile -Dmaven.gitcommitid.skip=true', the build failed due to a type mismatch.
🔇 Additional comments (15)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (5)
67-78: LGTM! Proper null validation added.The null check for
OpenAppDTOpreventsNullPointerExceptionand provides clear error messaging.
106-112: LGTM! Improved cluster name extraction.The stream-based approach with null filtering is cleaner and more defensive than the previous implementation.
145-149: LGTM! Event publishing implemented.The
updateAppmethod correctly publishesAppInfoChangedEventafter persistence, maintaining event-driven consistency.
157-167: LGTM! Pagination defaults applied correctly.The method handles null/invalid page parameters safely with sensible defaults (page=0, size=20).
199-203: LGTM! Event publishing on deletion.The method correctly publishes
AppDeletionEventafter deletion, maintaining event-driven consistency withupdateApp.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
143-153: LGTM! Security mocks properly configured.The
@Beforemethod correctly sets up permission validator mocks and user service, ensuring tests can run without full authentication infrastructure.
79-87: Evaluate full@SpringBootTestusage for controller testsLoading the entire Spring context for every controller test increases execution time and reduces isolation. Convert tests that only exercise MVC endpoints to
@WebMvcTestwith targeted@Importof required beans. Reserve@SpringBootTestfor tests needing the full application context, and document the rationale when used.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (8)
135-137: LGTM! Operator validation implemented.The method correctly validates that the operator user exists before proceeding with the update operation.
162-164: LGTM! Operator validation in createAppInEnv.Consistent operator validation pattern applied across all mutating endpoints.
177-179: LGTM! Operator validation in deleteApp.Proper validation ensures audit trail integrity.
87-90: LGTM! Clean ResponseEntity wrapper.The method properly wraps the service response in
ResponseEntity.
116-123: LGTM! Proper error handling for missing app.The method throws
BadRequestExceptionwhen the app is not found, providing clear feedback to the client.
146-152: LGTM! Authorization-aware pagination.The method correctly retrieves authorized app IDs for the consumer before applying pagination.
70-84: AppOpenApiService.createApp returns void: there’s no created‐app object to return. Either keep the empty 200 response or update the service to return a DTO if you need to return it.Likely an incorrect or invalid review comment.
131-141: AppOpenApiService.updateApp returns void; the controller cannot forward a value it doesn’t receive—if you need to return the persisted DTO, change the service method to return OpenAppDTO.Likely an incorrect or invalid review comment.
...src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
Outdated
Show resolved
Hide resolved
...ava/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java
Show resolved
Hide resolved
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
1439b3c to
179df5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
30-31: Return canonical DTOs from create/update.Void signatures prevent controllers from returning authoritative, persisted data. Have these return OpenAppDTO.
- void createApp(OpenCreateAppDTO req); + OpenAppDTO createApp(OpenCreateAppDTO req); @@ - void updateApp(OpenAppDTO openAppDTO); + OpenAppDTO updateApp(OpenAppDTO openAppDTO);Also applies to: 40-41
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
63-90: Remove duplicated tests or clarify distinct purpose.This class duplicates binding tests already covered in AppControllerTest. Either delete this file or document/rename to justify separate low-level coverage.
Also applies to: 116-199
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
192-207: Validate operator in service to protect role initialization.Service is callable outside this controller; add a non-blank operator check to avoid attributing roles to empty operators.
public void createAppInEnv(String env, OpenAppDTO app, String operator) { + if (operator == null || operator.trim().isEmpty()) { + throw new BadRequestException("operator cannot be null or empty"); + } if (env == null) { throw BadRequestException.invalidEnvFormat("null"); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
70-85: Return created app instead of empty body.Propagate the persisted DTO from the service so clients receive authoritative data; also use it for role assignment.
- public ResponseEntity<Object> createApp(OpenCreateAppDTO req) { + public ResponseEntity<OpenAppDTO> createApp(OpenCreateAppDTO req) { @@ - // create app - this.appOpenApiService.createApp(req); + // create app + OpenAppDTO created = this.appOpenApiService.createApp(req); @@ - consumerService.assignAppRoleToConsumer(consumerId, app.getAppId()); + consumerService.assignAppRoleToConsumer(consumerId, created.getAppId()); } - return ResponseEntity.ok().build(); + return ResponseEntity.ok(created);
131-141: Use the service’s updated DTO in response.Echoing the request hides server-side normalization. Return the value from service.
- public ResponseEntity<OpenAppDTO> updateApp(String appId, String operator, OpenAppDTO dto) { + public ResponseEntity<OpenAppDTO> updateApp(String appId, String operator, OpenAppDTO dto) { @@ - appOpenApiService.updateApp(dto); - - return ResponseEntity.ok(dto); + OpenAppDTO updated = appOpenApiService.updateApp(dto); + return ResponseEntity.ok(updated); }
🧹 Nitpick comments (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
100-110: createApp should return the persisted DTO.Return the saved app to let callers propagate canonical data (normalized fields, IDs).
- public void createApp(OpenCreateAppDTO req) { + public OpenAppDTO createApp(OpenCreateAppDTO req) { if (req == null || req.getApp() == null) { throw new BadRequestException("App is null"); } App app = convert(req.getApp()); List<String> admins = req.getAdmins(); if (admins == null) { throw new BadRequestException("Admins field in the request is null"); } - appService.createAppAndAddRolePermission(app, new HashSet<>(admins)); + App created = appService.createAppAndAddRolePermission(app, new HashSet<>(admins)); + return OpenApiModelConverters.fromApp(created); }
160-165: updateApp should return the updated DTO.Return the canonical DTO after persistence so controllers don’t echo stale request bodies.
- public void updateApp(OpenAppDTO openAppDTO) { + public OpenAppDTO updateApp(OpenAppDTO openAppDTO) { App app = convert(openAppDTO); App updatedApp = appService.updateAppInLocal(app); publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); + return OpenApiModelConverters.fromApp(updatedApp); }
226-246: Add @OverRide for interface method.Keep annotations consistent; helps catch signature drift at compile time.
- public MultiResponseEntity findMissEnvs(String appId) { + @Override + public MultiResponseEntity findMissEnvs(String appId) {apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
389-471: Avoid duplicating binding tests across classes.This block repeats the same binding tests present in AppControllerParamBindLowLevelTest. Keep one source of truth to reduce maintenance.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
93-99: Trim and filter appIds from query.Handle spaces/empty tokens to avoid lookup misses when clients send “app1, app2”.
- if (StringUtils.hasText(appIds)) { - return ResponseEntity.ok(this.appOpenApiService.getAppsInfo(Arrays.asList(appIds.split(",")))); + if (StringUtils.hasText(appIds)) { + List<String> ids = Arrays.stream(appIds.split(",")) + .map(String::trim) + .filter(StringUtils::hasText) + .toList(); + return ResponseEntity.ok(this.appOpenApiService.getAppsInfo(ids)); } else {
176-182: Consider returning the deleted DTO.Service returns the deleted OpenAppDTO but controller drops it. Returning it improves client feedback and parity with other endpoints.
- public ResponseEntity<Object> deleteApp(String appId, String operator) { + public ResponseEntity<OpenAppDTO> deleteApp(String appId, String operator) { @@ - appOpenApiService.deleteApp(appId); - return ResponseEntity.ok().build(); + OpenAppDTO deleted = appOpenApiService.deleteApp(appId); + return ResponseEntity.ok(deleted); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(78-473)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
RestController(69-236)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-84)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(63-199)
⏰ 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)
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
179df5c to
af54232
Compare
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
187-202: Validate operator parameter before initializing roles.The
operatorparameter is passed directly toinitNamespaceSpecificEnvRoleswithout validation. A null or blank operator could cause issues in role initialization or audit logging.Based on past review comments, add validation before the role initialization call:
@Override public void createAppInEnv(String env, OpenAppDTO app, String operator) { if (env == null) { throw BadRequestException.invalidEnvFormat("null"); } + if (operator == null || operator.trim().isEmpty()) { + throw new BadRequestException("operator is required"); + } Env envEnum; try { envEnum = Env.valueOf(env); } catch (IllegalArgumentException e) { throw BadRequestException.invalidEnvFormat(env); } App appEntity = convert(app); appService.createAppInRemote(envEnum, appEntity); roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), ConfigConsts.NAMESPACE_APPLICATION, env, operator); }
🧹 Nitpick comments (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
1-199: Clarify the distinct purpose of this test class.While past reviews flagged this file as duplicating tests in
AppControllerTest.java, the current version provides complementary coverage focused on low-level parameter binding verification usingArgumentCaptor. However, the tests forgetAppsBySelf,updateApp, anddeleteAppdo overlap with similarly named tests inAppControllerTest.java(lines 282-309, 323-345, 370-387).To improve clarity and reduce confusion:
- Add a class-level Javadoc comment explaining that this test suite focuses specifically on verifying parameter binding (path variables, query params, request body) at the MockMvc layer, complementing the end-to-end behavior tests in
AppControllerTest.- Consider whether both test classes need to remain in the same package, or if parameter binding tests could be consolidated.
- The
createAppInEnv_shouldBind_env_query_bodytest appears to provide unique coverage not present inAppControllerTestand should be retained.Example Javadoc:
/** * Low-level parameter binding tests for AppController OpenAPI endpoints. * * <p>This test suite verifies that HTTP request parameters (path variables, query params, * and request bodies) are correctly bound and passed to the service layer. It complements * the end-to-end behavior tests in {@link AppControllerTest} by focusing on binding * correctness rather than business logic or response validation.</p> * * @author wxq */ @RunWith(SpringRunner.class) @SpringBootTest @AutoConfigureMockMvc(addFilters = false) public class AppControllerParamBindLowLevelTest {
94-110: Consolidate duplicate @before methods.You have two
@Beforemethods (setUpat lines 94-102 andsetAuthenticationat lines 103-110). While JUnit 4 allows multiple@Beforemethods and executes them in declaration order, consolidating them into a single method improves readability and makes the setup sequence explicit.Apply this diff:
- @Before - public void setUp() { + @Before + public void setUpTestContext() { + // Set up security mocks when(consumerPermissionValidator.hasCreateApplicationPermission()).thenReturn(true); when(consumerPermissionValidator.isAppAdmin(anyString())).thenReturn(true); UserInfo user = new UserInfo(); user.setUserId("tester"); when(userService.findByUserId(anyString())).thenReturn(user); - } - @Before - public void setAuthentication() { - // put a dummy Authentication into SecurityContext so @PreAuthorize won't fail + + // Set up security context to prevent @PreAuthorize failures SecurityContextHolder.clearContext(); SecurityContextHolder.getContext().setAuthentication( new UsernamePasswordAuthenticationToken( "tester", "N/A", AuthorityUtils.NO_AUTHORITIES)); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (1)
221-241: Consider adding null check for appId parameter.The
appIdparameter is not validated before being passed toappService.load(env, appId). While the service layer might handle null values, adding an explicit check here would make the contract clearer and provide better error messages.Add validation at the beginning of the method:
public MultiResponseEntity findMissEnvs(String appId) { + if (appId == null || appId.trim().isEmpty()) { + throw BadRequestException.appIdIsBlank(); + } List<RichResponseEntity> entities = new ArrayList<>(); MultiResponseEntity response = new MultiResponseEntity(HttpStatus.OK.value(), entities);apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
322-345: Consider extracting common security context setup.The security context setup pattern
SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken(userInfo, null, Collections.emptyList()))is repeated in multiple tests (lines 332, 356, 376). Consider extracting this into a helper method to reduce duplication and improve maintainability.Add a helper method:
private void setSecurityContext(String userId) { UserInfo userInfo = new UserInfo(); userInfo.setUserId(userId); SecurityContextHolder.getContext().setAuthentication( new UsernamePasswordAuthenticationToken(userInfo, null, Collections.emptyList()) ); }Then use it in tests:
@Test public void testUpdateApp() throws Exception { String appId = "app1"; String operator = "operatorUser"; OpenAppDTO requestDto = new OpenAppDTO(); requestDto.setAppId(appId); requestDto.setName("App One"); - UserInfo userInfo = new UserInfo(); - userInfo.setUserId("test"); - SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken(userInfo, null, Collections.emptyList())); + setSecurityContext("test");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(78-388)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(63-199)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (6)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
Service(54-282)
⏰ 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 (8)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (5)
83-85: LGTM!The null check for the DTO parameter is a good defensive programming practice that prevents NPEs downstream and provides a clear error message.
101-109: LGTM!The type mismatch compilation error from the previous review has been correctly fixed by converting the
List<String>toHashSet<>when callingcreateAppAndAddRolePermission.
156-160: LGTM!Publishing
AppInfoChangedEventafter updating the app follows a proper event-driven pattern that allows other components to react to app changes without tight coupling.
210-214: LGTM!Publishing
AppDeletionEventafter deleting the app is consistent with the event-driven pattern used inupdateAppand enables proper cleanup of dependent resources.
249-267: LGTM!The error handling in
getAppNavTreeproperly logs failures and returns error entries in the response rather than failing completely, which provides better resilience and debugging information.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (3)
79-87: LGTM!The migration from
@WebMvcTestto@SpringBootTestwith@AutoConfigureMockMvcand the addition of HTTP client pool configuration via@TestPropertySourceproperly supports the full integration testing approach needed for the OpenAPI controller layer.
143-153: LGTM!The security setup in
setUpSecurityMocksproperly configures permission validators and user service mocks, ensuring tests can execute without authentication/authorization failures while still validating security-related behavior.
181-204: LGTM!The
testGetEnvClusterInfotest properly validates the JSON response structure for environment clusters, including both the environment names and nested cluster arrays. The use ofjsonPathassertions provides clear verification of the expected response format.
| @@ -0,0 +1,51 @@ | |||
| /* | |||
| * Copyright 2024 Apollo Authors | |||
There was a problem hiding this comment.
Already fixed that, thanks for reviewing!
| } | ||
|
|
||
| private App convert(OpenAppDTO dto) { | ||
| if (dto == null) { |
There was a problem hiding this comment.
After screening the call chain, it was found that the answer was no, so the redundant judgment process was deleted.
| */ | ||
| @Override | ||
| public void createApp(OpenCreateAppDTO req) { | ||
| if (req == null || req.getApp() == null) { |
There was a problem hiding this comment.
really needed? I think we can add jspecify annotation
There was a problem hiding this comment.
Although it may seem non-null now, the jspecify(NonNull) annotation is added to the interface for safety design.
There was a problem hiding this comment.
Thanks for the suggestion, so I learned about the contract guarantee of jspecify compilation period.
Co-authored-by: arrow2020 <316166287@qq.com>
tacklequestions
force-pushed
the
pr/a04-app-domain
branch
from
af54232 to
dcb633b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
1-199: Reconsider the need for this separate test class.While the duplication issue has improved since the earlier review (the tests are no longer identical to those in
AppControllerTest.java), this class still overlaps significantly with existing test coverage:
getAppsBySelf_shouldBind_page_size_and_ids(lines 144-167) overlaps withAppControllerTest.testGetAppsBySelf(lines 281-309)updateApp_shouldBind_path_query_body(lines 169-187) overlaps withAppControllerTest.testUpdateApp(lines 322-345)deleteApp_shouldBind_path_and_query(lines 189-198) overlaps withAppControllerTest.testDeleteApp(lines 369-387)The comprehensive integration tests in
AppControllerTestalready verify parameter binding implicitly. Unless there's a specific low-level binding concern that integration tests cannot catch, this class adds maintenance overhead without clear benefit.Consider either:
- Removing this file and relying on the comprehensive tests in
AppControllerTest, or- Adding a clear comment explaining why separate low-level binding tests are needed and ensuring the tests focus on unique concerns not covered by integration tests
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
69-85: Return the created object from the service call.The service method
appOpenApiService.createApp(req)returns anOpenAppDTOwith persisted/normalized values, but the controller ignores this return value and responds with an empty body. Callers need to see the created object including any server-generated defaults, normalized org/app names, or computed fields.This issue was previously flagged in past review comments but remains unaddressed.
Capture and return the service result:
// create app - this.appOpenApiService.createApp(req); + OpenAppDTO created = this.appOpenApiService.createApp(req); if (Boolean.TRUE.equals(req.getAssignAppRoleToSelf())) { long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); - consumerService.assignAppRoleToConsumer(consumerId, app.getAppId()); + consumerService.assignAppRoleToConsumer(consumerId, created.getAppId()); } - return ResponseEntity.ok().build(); + return ResponseEntity.ok(created);
128-141: Use the service's updated DTO when responding.The service method
appOpenApiService.updateApp(dto)returns the canonicalOpenAppDTOafter persistence, but the controller ignores this return value and echoes the request body. Callers won't see server-side changes (e.g., normalized owner names, org info, or other computed fields).This issue was previously flagged in past review comments but remains unaddressed.
Capture and return the service result:
if (userService.findByUserId(operator) == null) { throw BadRequestException.userNotExists(operator); } - appOpenApiService.updateApp(dto); + OpenAppDTO updated = appOpenApiService.updateApp(dto); - return ResponseEntity.ok(dto); + return ResponseEntity.ok(updated);
🧹 Nitpick comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (3)
202-206: Add input validation for consistency.While the underlying
appService.deleteAppInLocallikely validates theappId, explicit validation at the API service boundary improves consistency and provides clearer error messages.Apply this diff:
@Override public OpenAppDTO deleteApp(String appId) { + if (appId == null || appId.trim().isEmpty()) { + throw BadRequestException.appIdIsBlank(); + } App app = appService.deleteAppInLocal(appId); publisher.publishEvent(new AppDeletionEvent(app)); return OpenApiModelConverters.fromApp(app); }
213-233: Simplify error handling and add input validation.Two improvements needed:
- Missing input validation: Add a null/blank check for
appId.- Complex nesting: Lines 220-229 have nested conditionals that reduce readability.
Apply this refactor:
public MultiResponseEntity findMissEnvs(String appId) { + if (appId == null || appId.trim().isEmpty()) { + throw BadRequestException.appIdIsBlank(); + } List<RichResponseEntity> entities = new ArrayList<>(); MultiResponseEntity response = new MultiResponseEntity(HttpStatus.OK.value(), entities); for (Env env : portalSettings.getActiveEnvs()) { try { appService.load(env, appId); } catch (Exception e) { - RichResponseEntity entity; - if (e instanceof HttpClientErrorException && - ((HttpClientErrorException) e).getStatusCode() == HttpStatus.NOT_FOUND) { - entity = new RichResponseEntity(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase()); + RichResponseEntity entity = createErrorEntity(e, env); + response.addEntitiesItem(entity); + } + } + return response; +} + +private RichResponseEntity createErrorEntity(Exception e, Env env) { + if (e instanceof HttpClientErrorException && + ((HttpClientErrorException) e).getStatusCode() == HttpStatus.NOT_FOUND) { + RichResponseEntity entity = new RichResponseEntity(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase()); - entity.setBody(env.toString()); - } else { - entity = new RichResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR.value(), - "load env:" + env.getName() + " cluster error." + e.getMessage()); - } - response.addEntitiesItem(entity); - } + entity.setBody(env.toString()); + return entity; } - return response; + return new RichResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR.value(), + "load env:" + env.getName() + " cluster error." + e.getMessage()); }
241-259: Add input validation for consistency.The method lacks validation for
appId. For consistency with other methods in the service, add a null/blank check.Apply this diff:
@Override public MultiResponseEntity getAppNavTree(String appId) { + if (appId == null || appId.trim().isEmpty()) { + throw BadRequestException.appIdIsBlank(); + } List<RichResponseEntity> entities = new ArrayList<>(); MultiResponseEntity response = new MultiResponseEntity(HttpStatus.OK.value(),entities); List<Env> envs = portalSettings.getActiveEnvs(); for (Env env : envs) { try { OpenEnvClusterInfo openEnvClusterInfo = OpenApiModelConverters.fromEnvClusterInfo(appService.createEnvNavNode(env, appId)); RichResponseEntity entity = new RichResponseEntity(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase()); entity.setBody(openEnvClusterInfo); response.addEntitiesItem(entity); } catch (Exception e) { logger.warn("Failed to load env {} navigation for app {}", env, appId, e); RichResponseEntity entity = new RichResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR.value(), "load env:" + env.getName() + " cluster error." + e.getMessage()); response.addEntitiesItem(entity); } } return response; }Optional readability improvement: Line 247's chained call could use an intermediate variable:
- OpenEnvClusterInfo openEnvClusterInfo = OpenApiModelConverters.fromEnvClusterInfo(appService.createEnvNavNode(env, appId)); + EnvClusterInfo envClusterInfo = appService.createEnvNavNode(env, appId); + OpenEnvClusterInfo openEnvClusterInfo = OpenApiModelConverters.fromEnvClusterInfo(envClusterInfo);apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
81-87: Consider removing unused test properties.The HTTP client pool properties (
api.pool.max.total,api.pool.max.per.route, etc.) are typically used for integration tests that make actual HTTP calls to external services. Since this test uses@AutoConfigureMockMvc(addFilters = false)with extensive@MockBeanscaffolding and doesn't appear to make real HTTP connections, these properties may not be necessary.If these properties are required by other components in the Spring context, you can keep them. Otherwise, consider removing the
@TestPropertySourceannotation to simplify the test setup.
322-345: Verify the DTO passed to the service in testUpdateApp.The test mocks
appOpenApiService.updateAppbut doesn't verify that it was called with the correct DTO and operator values. Other tests in this file (e.g.,testFindAppsByIds,testFindAppsAuthorized) useArgumentCaptorto verify the exact parameters passed to service methods.Add verification using ArgumentCaptor:
mockMvc.perform(MockMvcRequestBuilders.put("/openapi/v1/apps/" + appId) .param("operator", operator) .contentType(MediaType.APPLICATION_JSON) .content(gson.toJson(requestDto))) .andExpect(MockMvcResultMatchers.status().isOk()) .andExpect(MockMvcResultMatchers.jsonPath("$.appId").value(appId)) .andExpect(MockMvcResultMatchers.jsonPath("$.name").value("App One")); + ArgumentCaptor<OpenAppDTO> dtoCaptor = ArgumentCaptor.forClass(OpenAppDTO.class); + Mockito.verify(appOpenApiService).updateApp(dtoCaptor.capture()); + + OpenAppDTO capturedDto = dtoCaptor.getValue(); + assertThat(capturedDto.getAppId()).isEqualTo(appId); + assertThat(capturedDto.getName()).isEqualTo("App One"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(78-388)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (7)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/BeanUtils.java (1)
BeanUtils(37-255)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)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/listener/AppDeletionEvent.java (1)
AppDeletionEvent(23-33)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppInfoChangedEvent.java (1)
AppInfoChangedEvent(25-35)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
Service(54-282)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerParamBindLowLevelTest.java (1)
RunWith(63-199)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(37-84)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java (1)
RestController(42-119)
⏰ 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 (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java (4)
67-78: LGTM! Constructor dependencies properly injected.The addition of
ApplicationEventPublisherandRoleInitializationServicedependencies follows best practices with constructor injection and immutable final fields.
97-101: LGTM! Type mismatch resolved correctly.The fix properly handles the List-to-Set conversion and includes defensive null checking. The compilation error noted in previous reviews has been addressed.
114-115: LGTM! Refactored to use BeanUtils appropriately.The change to use
BeanUtils.toPropertySetis cleaner and includes proper null handling. Converting the Set to List for the response maintains API compatibility.
126-136: LGTM! Migrated to OpenApiModelConverters with defensive checks.The migration from
OpenApiBeanUtilstoOpenApiModelConvertersaligns with the PR objectives. The null/empty check ingetAppsInfoprevents unnecessary processing and provides a clear early return.
| public void updateApp(OpenAppDTO openAppDTO) { | ||
| App app = convert(openAppDTO); | ||
| App updatedApp = appService.updateAppInLocal(app); | ||
| publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null validation for the input parameter.
The method lacks null checking for openAppDTO. If null is passed, the convert method will throw a NullPointerException. Add a precondition check or throw BadRequestException for better error reporting.
Apply this fix:
@Override
public void updateApp(OpenAppDTO openAppDTO) {
+ if (openAppDTO == null) {
+ throw new BadRequestException("openAppDTO cannot be null");
+ }
App app = convert(openAppDTO);
App updatedApp = appService.updateAppInLocal(app);
publisher.publishEvent(new AppInfoChangedEvent(updatedApp));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void updateApp(OpenAppDTO openAppDTO) { | |
| App app = convert(openAppDTO); | |
| App updatedApp = appService.updateAppInLocal(app); | |
| publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); | |
| } | |
| @Override | |
| public void updateApp(OpenAppDTO openAppDTO) { | |
| if (openAppDTO == null) { | |
| throw new BadRequestException("openAppDTO cannot be null"); | |
| } | |
| App app = convert(openAppDTO); | |
| App updatedApp = appService.updateAppInLocal(app); | |
| publisher.publishEvent(new AppInfoChangedEvent(updatedApp)); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 148 to 152, the method updateApp lacks null-checking for the
openAppDTO parameter which will cause a NullPointerException in convert; add an
explicit null validation at the top of the method and throw a
BadRequestException (or IllegalArgumentException per project conventions) with a
clear message like "openAppDTO must not be null" if null is passed, then proceed
to call convert, updateAppInLocal, and publish the event.
| public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) { | ||
| int pageIndex = page == null ? 0 : page; | ||
| int pageSize = (size == null || size <= 0) ? 20 : size; | ||
| Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex); | ||
| Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds; | ||
| if (targetAppIds.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
| List<App> apps = appService.findByAppIds(targetAppIds, pageable); | ||
| return OpenApiModelConverters.fromApps(apps); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen pagination parameter validation.
The pagination logic has gaps:
- Negative page: Line 161 doesn't validate against negative page values, which could cause issues with
Pageable.ofSize(pageSize).withPage(pageIndex). - Unbounded size: Line 162 has no upper limit on
size, allowing potentially large values that could cause memory or performance issues.
Apply this diff:
@Override
public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) {
- int pageIndex = page == null ? 0 : page;
- int pageSize = (size == null || size <= 0) ? 20 : size;
+ int pageIndex = (page == null || page < 0) ? 0 : page;
+ int pageSize = (size == null || size <= 0) ? 20 : Math.min(size, 500);
Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex);
Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds;
if (targetAppIds.isEmpty()) {
return Collections.emptyList();
}
List<App> apps = appService.findByAppIds(targetAppIds, pageable);
return OpenApiModelConverters.fromApps(apps);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) { | |
| int pageIndex = page == null ? 0 : page; | |
| int pageSize = (size == null || size <= 0) ? 20 : size; | |
| Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex); | |
| Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds; | |
| if (targetAppIds.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| List<App> apps = appService.findByAppIds(targetAppIds, pageable); | |
| return OpenApiModelConverters.fromApps(apps); | |
| } | |
| @Override | |
| public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) { | |
| int pageIndex = (page == null || page < 0) ? 0 : page; | |
| int pageSize = (size == null || size <= 0) ? 20 : Math.min(size, 500); | |
| Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex); | |
| Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds; | |
| if (targetAppIds.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| List<App> apps = appService.findByAppIds(targetAppIds, pageable); | |
| return OpenApiModelConverters.fromApps(apps); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 160-170, strengthen pagination validation by ensuring page is
non-negative and size is bounded: clamp pageIndex to Math.max(0, page) (treat
null as 0), set pageSize to default 20 when null or <=0, and cap pageSize to a
reasonable maximum (e.g., 100) to avoid unbounded requests; apply these adjusted
values when creating the Pageable (consider extracting the max as a constant or
config) so callers cannot pass negative pages or excessive sizes.
| public void createAppInEnv(String env, OpenAppDTO app, String operator) { | ||
| if (env == null) { | ||
| throw BadRequestException.invalidEnvFormat("null"); | ||
| } | ||
| Env envEnum; | ||
| try { | ||
| envEnum = Env.valueOf(env); | ||
| } catch (IllegalArgumentException e) { | ||
| throw BadRequestException.invalidEnvFormat(env); | ||
| } | ||
| App appEntity = convert(app); | ||
| appService.createAppInRemote(envEnum, appEntity); | ||
|
|
||
| roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), | ||
| ConfigConsts.NAMESPACE_APPLICATION, env, operator); | ||
| } |
There was a problem hiding this comment.
Critical: Add operator validation as flagged in previous review.
The method lacks validation for the operator parameter before calling roleInitializationService.initNamespaceSpecificEnvRoles at line 192. This was explicitly flagged in a previous review comment. Additionally, the app parameter is not validated for null.
Apply this fix to address both issues:
@Override
public void createAppInEnv(String env, OpenAppDTO app, String operator) {
if (env == null) {
throw BadRequestException.invalidEnvFormat("null");
}
+ if (app == null) {
+ throw new BadRequestException("app cannot be null");
+ }
+ if (operator == null || operator.trim().isEmpty()) {
+ throw new BadRequestException("operator cannot be null or blank");
+ }
Env envEnum;
try {
envEnum = Env.valueOf(env);
} catch (IllegalArgumentException e) {
throw BadRequestException.invalidEnvFormat(env);
}
App appEntity = convert(app);
appService.createAppInRemote(envEnum, appEntity);
roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(),
ConfigConsts.NAMESPACE_APPLICATION, env, operator);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void createAppInEnv(String env, OpenAppDTO app, String operator) { | |
| if (env == null) { | |
| throw BadRequestException.invalidEnvFormat("null"); | |
| } | |
| Env envEnum; | |
| try { | |
| envEnum = Env.valueOf(env); | |
| } catch (IllegalArgumentException e) { | |
| throw BadRequestException.invalidEnvFormat(env); | |
| } | |
| App appEntity = convert(app); | |
| appService.createAppInRemote(envEnum, appEntity); | |
| roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), | |
| ConfigConsts.NAMESPACE_APPLICATION, env, operator); | |
| } | |
| @Override | |
| public void createAppInEnv(String env, OpenAppDTO app, String operator) { | |
| if (env == null) { | |
| throw BadRequestException.invalidEnvFormat("null"); | |
| } | |
| if (app == null) { | |
| throw new BadRequestException("app cannot be null"); | |
| } | |
| if (operator == null || operator.trim().isEmpty()) { | |
| throw new BadRequestException("operator cannot be null or blank"); | |
| } | |
| Env envEnum; | |
| try { | |
| envEnum = Env.valueOf(env); | |
| } catch (IllegalArgumentException e) { | |
| throw BadRequestException.invalidEnvFormat(env); | |
| } | |
| App appEntity = convert(app); | |
| appService.createAppInRemote(envEnum, appEntity); | |
| roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(), | |
| ConfigConsts.NAMESPACE_APPLICATION, env, operator); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 179-194, add null-checks and validation for both the incoming app
DTO and the operator before proceeding: if app is null throw a
BadRequestException indicating a missing/invalid app; if operator is null or
blank throw a BadRequestException indicating a missing/invalid operator; perform
these checks before converting/creating the App and before calling
roleInitializationService.initNamespaceSpecificEnvRoles so no invalid values are
passed downstream.
4 participants
What's the purpose of this PR
Complete the App domain refactor end-to-end on top of generated OpenAPI models: introduce a dedicated server-side OpenAPI service for App, migrate the
OpenAPI AppController to use it, and update/add tests. This builds on PR3’s non-invasive converters and does not modify existing OpenApiBeanUtils.
Which issue(s) this PR fixes:
Fixes #5462
Brief changelog
Add server-side service interface and impl:
Replace DTO conversions with OpenApiModelConverters (no changes to OpenApiBeanUtils).
Migrate OpenAPI AppController to the new service:
Tests:
Scope limited to the App domain; no changes to other domains.
Original OpenAPI
New Reads
New Mutations
Summary by CodeRabbit