feat(openapi): cluster domain service impl, controller migration and tests#5472
Conversation
dosubot
Bot
added
the
size:XL
WalkthroughController endpoints were refactored to implement a generated API interface and return ResponseEntity; DTO conversions switched from OpenApiBeanUtils to OpenApiModelConverters; a deleteCluster operation was added to controller, server service interface and implementation; unit and parameter-binding tests for GET/POST/DELETE were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as ClusterController
participant Service as ClusterOpenApiService
participant Impl as ServerClusterOpenApiService
participant Core as ClusterService
rect rgb(235,245,255)
Note over Controller: getCluster / createCluster (DTO conversion)
Client->>Controller: HTTP GET/POST (env, appId, clusterName / OpenClusterDTO)
Controller->>Service: getCluster/createCluster(...)
Service->>Impl: delegate
Impl->>Core: read/create ClusterDTO
Core-->>Impl: ClusterDTO
Impl-->>Service: OpenClusterDTO (via OpenApiModelConverters)
Service-->>Controller: OpenClusterDTO
Controller-->>Client: 200 OK (ResponseEntity)
end
sequenceDiagram
autonumber
actor Client
participant Controller as ClusterController
participant Auth as PermissionValidator/UserService
participant Service as ClusterOpenApiService
participant Impl as ServerClusterOpenApiService
participant Core as ClusterService
rect rgb(255,245,235)
Note over Controller: deleteCluster (audit + auth)
Client->>Controller: HTTP DELETE (env, appId, clusterName, operator)
Controller->>Auth: validate operator/authz
alt authorized
Controller->>Service: deleteCluster(env, appId, clusterName)
Service->>Impl: delegate
Impl->>Core: deleteCluster(...)
Core-->>Impl: void
Impl-->>Service: void
Service-->>Controller: void
Controller-->>Client: 200 OK
else unauthorized
Controller-->>Client: 403/4xx
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tacklequestions
force-pushed
the
pr/a05-cluster
branch
from
835bc9a to
049dc71
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
112-120: Guard against null Authentication to avoid NPESecurityContext or Authentication can be null; calling getPrincipal unguarded risks NPE on unauthenticated requests.
Apply:
- private String getCurrentUsername() { - Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); + private String getCurrentUsername() { + var context = SecurityContextHolder.getContext(); + if (context == null || context.getAuthentication() == null) { + return "anonymousUser"; + } + Object principal = context.getAuthentication().getPrincipal(); if (principal instanceof UserDetails) { return ((UserDetails) principal).getUsername(); } if (principal instanceof Principal) { return ((Principal) principal).getName(); } return String.valueOf(principal); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
39-47: Ambiguous ClusterOpenApiService injection in ClusterController: two beans implement ClusterOpenApiService (legacy ServerClusterOpenApiServiceOld and new ServerClusterOpenApiService), causing NoUniqueBeanDefinitionException at runtime; annotate the intended implementation with @primary or inject with @qualifier.
🧹 Nitpick comments (21)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (2)
102-105: Don’t swallow exceptions silently; log at debugSilent fallback makes diagnosing auth propagation issues hard. Log at debug/trace.
Apply:
- } catch (Exception e) { - // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 - return null; - } + } catch (Exception e) { + // 如果获取Consumer信息失败，返回null，让系统回退到默认方式 + if (logger.isDebugEnabled()) { + logger.debug("Failed to resolve OpenAPI Consumer from request, falling back to Spring Security principal.", e); + } + return null; + }Add logger (outside this range):
import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class SpringSecurityUserInfoHolder implements UserInfoHolder { private static final Logger logger = LoggerFactory.getLogger(SpringSecurityUserInfoHolder.class); // ... }
65-68: Comment language consistencyProject code/comments are predominantly in English. Consider switching these method comments to English for consistency.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java (1)
25-29: Consider adding Javadoc for interface methodsWhile the interface is well-structured, adding Javadoc comments for each method would improve API documentation and clarify expected behavior, especially regarding:
- Parameter validation requirements
- Return value semantics (e.g., when
getClusterreturns null)- Exception handling expectations
public interface ClusterOpenApiService { + /** + * Retrieves cluster information. + * @param appId application ID + * @param env environment name + * @param clusterName cluster name + * @return cluster DTO or null if not found + */ OpenClusterDTO getCluster(String appId, String env, String clusterName); + /** + * Creates a new cluster. + * @param env environment name + * @param openClusterDTO cluster data to create + * @return created cluster DTO + * @throws IllegalArgumentException if validation fails + */ OpenClusterDTO createCluster(String env, OpenClusterDTO openClusterDTO); + /** + * Deletes a cluster. + * @param env environment name + * @param appId application ID + * @param clusterName cluster name + * @throws NotFoundException if cluster doesn't exist + */ void deleteCluster(String env, String appId, String clusterName);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (2)
22-22: Remove unnecessary empty lineThere's an extra empty line after imports that can be removed for consistency.
import java.util.Collections; - import javax.persistence.EntityManagerFactory;
112-114: Document the use of @lazy for ConsumerServiceThe
@Lazyannotation is used forConsumerServiceinjection in bothauthandldapprofiles. Consider adding a comment explaining why lazy initialization is needed here (likely to avoid circular dependencies).@Bean @ConditionalOnMissingBean(UserInfoHolder.class) + // @Lazy is used to avoid circular dependency with ConsumerService initialization public UserInfoHolder springSecurityUserInfoHolder(UserService userService, @Lazy ConsumerService consumerService) { return new SpringSecurityUserInfoHolder(userService, consumerService); }Also applies to: 216-218
apollo-portal/pom.xml (2)
140-172: Consider adding skip configuration for OpenAPI generationAdd a property to skip OpenAPI generation during development builds to improve build performance.
<properties> <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url> + <openapi.skip>false</openapi.skip> <github.path>${project.artifactId}</github.path>And in the plugin configuration:
<configuration> <inputSpec>${apollo.openapi.spec.url}</inputSpec> + <skip>${openapi.skip}</skip> <generatorName>spring</generatorName>
271-273: Redundant source path in commentThe comment mentions the path that's already clear from the configuration below.
-<!-- 注意这层 src/main/java --> +<!-- Add generated OpenAPI sources to compilation -->apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (4)
42-45: Improve class-level documentationThe current Javadoc could be more descriptive about the purpose and usage patterns of this converter class.
/** - * Non-invasive converters for OpenAPI generated model classes. - * This class mirrors/OpenApiBeanUtils functions but targets com.ctrip.framework.apollo.openapi.model.* types. + * Utility class for converting between internal Apollo domain objects and OpenAPI generated model classes. + * + * <p>This class provides bidirectional conversion methods for all OpenAPI model types used in the + * Apollo Portal OpenAPI implementation. It handles null checks, collection transformations, and + * maintains data integrity during conversions.</p> + * + * <p>Note: This class parallels OpenApiBeanUtils functionality but specifically targets + * com.ctrip.framework.apollo.openapi.model.* types generated from the OpenAPI specification.</p> */
115-124: Inefficient collection conversion in fromReleaseBOThe conversion from Set to List for items could be simplified using streams.
- Set<com.ctrip.framework.apollo.portal.entity.bo.KVEntity> items = releaseBO.getItems(); - List<KVEntity> itemsList = new ArrayList<>(); - if (!CollectionUtils.isEmpty(items)) { - for (com.ctrip.framework.apollo.portal.entity.bo.KVEntity item : items) { - KVEntity kvEntity = new KVEntity(); - kvEntity.setKey(item.getKey()); - kvEntity.setValue(item.getValue()); - itemsList.add(kvEntity); - } - } - openReleaseBO.setItems(itemsList); + Set<com.ctrip.framework.apollo.portal.entity.bo.KVEntity> items = releaseBO.getItems(); + if (!CollectionUtils.isEmpty(items)) { + List<KVEntity> itemsList = items.stream() + .map(item -> { + KVEntity kvEntity = new KVEntity(); + kvEntity.setKey(item.getKey()); + kvEntity.setValue(item.getValue()); + return kvEntity; + }) + .collect(Collectors.toList()); + openReleaseBO.setItems(itemsList); + } else { + openReleaseBO.setItems(Collections.emptyList()); + }
248-257: Inefficient Set operations in toGrayReleaseRuleDTOCreating a new HashSet from the list just to iterate over it is unnecessary. Also, using forEach with side effects inside could be replaced with a more functional approach.
- Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()); - openGrayReleaseRuleItemDTOSet.forEach(openGrayReleaseRuleItemDTO -> { - String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId(); - Set<String> clientIpList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); - Set<String> clientLabelList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()); - GrayReleaseRuleItemDTO ruleItem = new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList); - grayReleaseRuleDTO.addRuleItem(ruleItem); - }); + if (openGrayReleaseRuleDTO.getRuleItems() != null) { + openGrayReleaseRuleDTO.getRuleItems().forEach(openGrayReleaseRuleItemDTO -> { + String clientAppId = openGrayReleaseRuleItemDTO.getClientAppId(); + Set<String> clientIpList = openGrayReleaseRuleItemDTO.getClientIpList() != null + ? new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()) + : new HashSet<>(); + Set<String> clientLabelList = openGrayReleaseRuleItemDTO.getClientLabelList() != null + ? new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()) + : new HashSet<>(); + GrayReleaseRuleItemDTO ruleItem = new GrayReleaseRuleItemDTO(clientAppId, clientIpList, clientLabelList); + grayReleaseRuleDTO.addRuleItem(ruleItem); + }); + }
47-48: Consider making GSON thread-safe with GsonBuilder configurationWhile Gson instances are thread-safe, consider using GsonBuilder to configure specific behaviors like null handling and date formatting for consistency.
- private static final Gson GSON = new Gson(); + private static final Gson GSON = new GsonBuilder() + .serializeNulls() + .setDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ") + .create();apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (1)
40-43: Ensure consistent error handling in OpenApiService#get methods
AllgetXXXmethods (e.g. getItem, getCluster, getLatestActiveRelease, getNamespace…) currently returnnullwhen the underlying DTO is missing. Either:
- Retain this null‐return pattern for all endpoints (controllers should map
null→404), or- Uniformly refactor every
getXXXto throw a 404 exception and update controllers accordingly.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (3)
66-93: Use 201 Created for successful creation (optionally set Location header)Creation should respond with 201; 200 is suboptimal. Optionally set Location to the new resource URL.
Apply:
- OpenClusterDTO createdCluster = this.clusterOpenApiService.createCluster(env, cluster); - return ResponseEntity.ok(createdCluster); + OpenClusterDTO createdCluster = this.clusterOpenApiService.createCluster(env, cluster); + return ResponseEntity.status(201).body(createdCluster);If you want a Location header, also import ServletUriComponentsBuilder and build the URI; I can provide the snippet. Based on objectives.
99-107: Return 204 No Content on deleteDELETE should respond with 204 No Content when successful.
Apply:
- clusterOpenApiService.deleteCluster(env, appId, clusterName); - return ResponseEntity.ok().build(); + clusterOpenApiService.deleteCluster(env, appId, clusterName); + return ResponseEntity.noContent().build();
66-93: Consider unified response wrapperTo align with linked objectives (issue #5462), consider wrapping responses as ResponseEntity<ApiResponse> instead of raw DTOs, with consistent error payloads.
Based on objectives.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (2)
40-44: Map invalid env to a 400-style error instead of IllegalArgumentExceptionEnv.valueOf(env) throws IllegalArgumentException for unknown env, which may surface as 500 if not mapped. Prefer validating and throwing a BadRequestException.invalidEnvFormat to produce a clear 400.
Apply:
+import com.ctrip.framework.apollo.common.exception.BadRequestException; @@ @Override public OpenClusterDTO getCluster(String appId, String env, String clusterName) { - ClusterDTO clusterDTO = clusterService.loadCluster(appId, Env.valueOf(env), clusterName); + if (!Env.exists(env)) { + throw BadRequestException.invalidEnvFormat(env); + } + ClusterDTO clusterDTO = clusterService.loadCluster(appId, Env.valueOf(env), clusterName); return clusterDTO == null ? null : OpenApiModelConverters.fromClusterDTO(clusterDTO); }Consider the same guard in createCluster and deleteCluster for consistency.
59-63: Delete delegation LGTMDelegation to ClusterService with Env.valueOf is straightforward. Add the env validation as suggested above for consistency.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java (4)
63-79: Add a 404 test for non-existent clusterCurrently only the happy-path GET is tested. Add a test expecting 404 when service returns null.
Example method to add:
@Test public void testGetCluster_NotFound() throws Exception { String appId = "test-app"; String env = "DEV"; String clusterName = "missing"; when(clusterOpenApiService.getCluster(appId, env, clusterName)).thenReturn(null); this.mockMvc.perform( MockMvcRequestBuilders.get("/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}", env, appId, clusterName)) .andDo(MockMvcResultHandlers.print()) .andExpect(status().isNotFound()); }
100-109: Adjust expectation to 201 if create uses CreatedIf you adopt 201 for creation, update the assertion accordingly.
Apply:
- .andExpect(status().isOk()) + .andExpect(status().isCreated()) .andExpect(jsonPath("$.appId", is(appId))) .andExpect(jsonPath("$.name", is(clusterName)));
141-149: Adjust expectation to 204 if delete uses No ContentIf DELETE returns 204, update the assertion accordingly.
Apply:
- .andExpect(status().isOk()); + .andExpect(status().isNoContent());
45-47: Ensure method security is active in slice tests@PreAuthorize is used in the controller. In @WebMvcTest slices, method security may be disabled unless imported. Consider importing your security config or enabling method security for the test slice to exercise the permission checks.
📜 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/ClusterOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.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/ClusterController.java(3 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/ClusterControllerTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (1)
Service(30-51)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/openapi/v1/controller/ClusterController.java (3)
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/InputValidator.java (1)
InputValidator(26-47)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/RequestPrecondition.java (1)
RequestPrecondition(24-43)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.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/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/ServerClusterOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
Service(31-63)
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/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)
🔇 Additional comments (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (2)
39-43: Constructor injection with @lazy is fineUsing @lazy for ConsumerService helps avoid early init/cycles. LGTM.
Please confirm no circular dependency remains at runtime with the new OpenAPI service wiring.
47-56: Approve code changes; add missing tests for SpringSecurityUserInfoHolder. Tests should cover: valid OpenAPI consumer (consumerUserInfo path), invalid/missing OpenAPI consumer (fallback to Spring Security), and non-OpenAPI requests (Spring Security path).apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
210-210: Drop null-check ongetEnv()
getEnv()invokesEnv.valueOf(env)and never returns null, so guarding the call beforetoString()isn’t needed.Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java (1)
18-19: Ignore import path warning. The importcom.ctrip.framework.apollo.openapi.model.OpenClusterDTOmatches the<modelPackage>in apollo-portal/pom.xml; no change required.Likely an incorrect or invalid review comment.
apollo-portal/pom.xml (1)
48-62: Ensure Jakarta EE 9+ runtime and Jackson ≥2.12 compatibilityFile: apollo-portal/pom.xml Lines: 48–62
- Confirm your runtime/JEE libs target Jakarta EE 9+ APIs
- Upgrade Jackson to ≥ 2.12 with the jakarta classifier
- Exclude any legacy javax-namespace swagger artifacts to prevent class-path conflicts
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (1)
58-82: Imports follow project conventions: grouped as JDK → javax → third-party (org.hibernate, org.springframework) → Apollo modules (com.ctrip.framework.apollo.*); no reordering needed.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
46-51: Converters usage LGTMConversion between OpenClusterDTO and ClusterDTO is clean and consistent.
| <artifactId>apollo-portal</artifactId> | ||
| <name>Apollo Portal</name> | ||
| <properties> | ||
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
External OpenAPI spec URL may cause build reliability issues
Using an external URL for the OpenAPI spec can cause build failures if the URL is unavailable. Consider:
- Storing the spec file locally in the repository
- Using a fallback mechanism
- Adding build-time validation
Consider storing the OpenAPI spec locally:
-<apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url>
+<apollo.openapi.spec.file>${project.basedir}/src/main/resources/openapi/apollo-openapi.yaml</apollo.openapi.spec.file>And update the plugin configuration:
-<inputSpec>${apollo.openapi.spec.url}</inputSpec>
+<inputSpec>${apollo.openapi.spec.file}</inputSpec>🏁 Script executed:
#!/bin/bash
# Check if the OpenAPI spec URL is accessible
curl -s -o /dev/null -w "%{http_code}" https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yamlLength of output: 3
Store OpenAPI spec locally to avoid external dependency
External URLs may become unavailable and break builds. Add the spec under src/main/resources/openapi, update the POM to reference the local file:
- <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url>
+ <apollo.openapi.spec.file>${project.basedir}/src/main/resources/openapi/apollo-openapi.yaml</apollo.openapi.spec.file>And in your plugin config:
- <inputSpec>${apollo.openapi.spec.url}</inputSpec>
+ <inputSpec>${apollo.openapi.spec.file}</inputSpec>📝 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.
| <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url> | |
| <!-- in the <properties> section --> | |
| <properties> | |
| … | |
| <apollo.openapi.spec.file>${project.basedir}/src/main/resources/openapi/apollo-openapi.yaml</apollo.openapi.spec.file> | |
| … | |
| </properties> | |
| <!-- in the OpenAPI Generator plugin configuration --> | |
| <build> | |
| <plugins> | |
| <plugin> | |
| <groupId>org.openapitools</groupId> | |
| <artifactId>openapi-generator-maven-plugin</artifactId> | |
| <version>${openapi.generator.version}</version> | |
| <executions> | |
| <execution> | |
| <goals> | |
| <goal>generate</goal> | |
| </goals> | |
| <configuration> | |
| <inputSpec>${apollo.openapi.spec.file}</inputSpec> | |
| <!-- other configuration --> | |
| </configuration> | |
| </execution> | |
| </executions> | |
| </plugin> | |
| </plugins> | |
| </build> |
🤖 Prompt for AI Agents
In apollo-portal/pom.xml around line 30, the project references the OpenAPI spec
via an external URL; add a local copy of the spec under
src/main/resources/openapi (e.g., apollo-openapi.yaml) and update the POM
property apollo.openapi.spec.url to point to the classpath/resource path (or
relative file path) instead of the external URL; also update any plugin/config
sections that consume that property to use the local resource (or include
src/main/resources in the plugin’s config), and ensure the resource is packaged
by verifying <resources> includes src/main/resources so builds no longer depend
on the external URL.
| @Service | ||
| public class ServerClusterOpenApiService implements ClusterOpenApiService { | ||
|
|
||
| private final ClusterService clusterService; | ||
|
|
||
| public ServerClusterOpenApiService(ClusterService clusterService) { | ||
| this.clusterService = clusterService; | ||
| } |
There was a problem hiding this comment.
Make this the primary bean to resolve injection ambiguity
Mark this implementation as @primary to avoid NoUniqueBeanDefinitionException alongside the legacy @service("ServerClusterOpenApiServiceOld") bean.
Apply:
-import org.springframework.stereotype.Service;
+import org.springframework.stereotype.Service;
+import org.springframework.context.annotation.Primary;
@@
-@Service
+@Service
+@Primary
public class ServerClusterOpenApiService implements ClusterOpenApiService {📝 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.
| @Service | |
| public class ServerClusterOpenApiService implements ClusterOpenApiService { | |
| private final ClusterService clusterService; | |
| public ServerClusterOpenApiService(ClusterService clusterService) { | |
| this.clusterService = clusterService; | |
| } | |
| ++ b/apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java | |
| @@ | |
| import org.springframework.stereotype.Service; | |
| import org.springframework.context.annotation.Primary; | |
| @@ | |
| @Service | |
| @Primary | |
| public class ServerClusterOpenApiService implements ClusterOpenApiService { | |
| private final ClusterService clusterService; | |
| public ServerClusterOpenApiService(ClusterService clusterService) { | |
| this.clusterService = clusterService; | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java
around lines 31 to 38, the class should be marked as the primary bean to resolve
injection ambiguity with the legacy Service("ServerClusterOpenApiServiceOld")
bean; add the @Primary annotation to the class declaration (and import
org.springframework.context.annotation.Primary) so Spring will prefer this
implementation when autowiring ClusterOpenApiService.
| Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE); | ||
| openReleaseDTO.setConfigurations(configs); | ||
| return openReleaseDTO; |
There was a problem hiding this comment.
Add null check for configurations field
The fromJson call could return null if the configurations field is null or invalid JSON.
OpenReleaseDTO openReleaseDTO = BeanUtils.transform(OpenReleaseDTO.class, release);
- Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE);
- openReleaseDTO.setConfigurations(configs);
+ if (release.getConfigurations() != null) {
+ Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE);
+ openReleaseDTO.setConfigurations(configs != null ? configs : new HashMap<>());
+ } else {
+ openReleaseDTO.setConfigurations(new HashMap<>());
+ }📝 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.
| Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE); | |
| openReleaseDTO.setConfigurations(configs); | |
| return openReleaseDTO; | |
| OpenReleaseDTO openReleaseDTO = BeanUtils.transform(OpenReleaseDTO.class, release); | |
| if (release.getConfigurations() != null) { | |
| Map<String, String> configs = GSON.fromJson(release.getConfigurations(), TYPE); | |
| openReleaseDTO.setConfigurations(configs != null ? configs : new HashMap<>()); | |
| } else { | |
| openReleaseDTO.setConfigurations(new HashMap<>()); | |
| } | |
| return openReleaseDTO; |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java
around lines 106 to 108, the result of
GSON.fromJson(release.getConfigurations(), TYPE) can be null when configurations
is null or invalid JSON; after the fromJson call check if configs is null and if
so assign an empty map (e.g., Collections.emptyMap() or new HashMap<>()) before
calling openReleaseDTO.setConfigurations(configs); optionally consider catching
JsonSyntaxException around the parse to handle invalid JSON and set the empty
map in that case as well.
| @GetMapping(value = "/apps/{appId}/clusters/{clusterName:.+}") | ||
| public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env, | ||
| @PathVariable String appId, | ||
| @PathVariable String clusterName) { | ||
| OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName); | ||
| return ResponseEntity.ok(cluster); | ||
| } |
There was a problem hiding this comment.
Return 404 when cluster not found
Current code returns 200 OK with a null body. REST should return 404 Not Found if the cluster doesn’t exist.
Apply:
- public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env,
- @PathVariable String appId,
- @PathVariable String clusterName) {
- OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName);
- return ResponseEntity.ok(cluster);
- }
+ public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env,
+ @PathVariable String appId,
+ @PathVariable String clusterName) {
+ OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName);
+ if (cluster == null) {
+ return ResponseEntity.notFound().build();
+ }
+ return ResponseEntity.ok(cluster);
+ }📝 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.
| @GetMapping(value = "/apps/{appId}/clusters/{clusterName:.+}") | |
| public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env, | |
| @PathVariable String appId, | |
| @PathVariable String clusterName) { | |
| OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName); | |
| return ResponseEntity.ok(cluster); | |
| } | |
| @GetMapping(value = "/apps/{appId}/clusters/{clusterName:.+}") | |
| public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env, | |
| @PathVariable String appId, | |
| @PathVariable String clusterName) { | |
| OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName); | |
| if (cluster == null) { | |
| return ResponseEntity.notFound().build(); | |
| } | |
| return ResponseEntity.ok(cluster); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java
around lines 53-59, the controller currently returns 200 OK with a null body
when the cluster isn't found; change the method to detect a null OpenClusterDTO
and return a 404 Not Found response (e.g., return
ResponseEntity.notFound().build()) instead of ResponseEntity.ok(null) so callers
receive the correct HTTP status when the cluster does not exist.
| HttpServletRequest request = attributes.getRequest(); | ||
| String requestURI = request.getRequestURI(); | ||
|
|
||
| // 只对OpenAPI请求处理Consumer用户信息 | ||
| if (!requestURI.startsWith("/openapi/")) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
OpenAPI path check should account for contextPath
getRequestURI() includes the context path. startsWith("/openapi/") will fail when the app is deployed under a non-root context (e.g., "/portal"). Compare against contextPath + "/openapi/".
Apply:
- // 只对OpenAPI请求处理Consumer用户信息
- if (!requestURI.startsWith("/openapi/")) {
+ // 只对OpenAPI请求处理Consumer用户信息（考虑 contextPath）
+ String contextPath = request.getContextPath();
+ String openApiPrefix = (contextPath == null || contextPath.isEmpty() ? "" : contextPath) + "/openapi/";
+ if (!requestURI.startsWith(openApiPrefix)) {
return null;
}📝 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.
| HttpServletRequest request = attributes.getRequest(); | |
| String requestURI = request.getRequestURI(); | |
| // 只对OpenAPI请求处理Consumer用户信息 | |
| if (!requestURI.startsWith("/openapi/")) { | |
| return null; | |
| } | |
| HttpServletRequest request = attributes.getRequest(); | |
| String requestURI = request.getRequestURI(); | |
| // 只对OpenAPI请求处理Consumer用户信息（考虑 contextPath） | |
| String contextPath = request.getContextPath(); | |
| String openApiPrefix = (contextPath == null || contextPath.isEmpty() ? "" : contextPath) + "/openapi/"; | |
| if (!requestURI.startsWith(openApiPrefix)) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
around lines 75 to 81, the code uses
request.getRequestURI().startsWith("/openapi/") which fails when the application
runs under a non-root context; replace the raw "/openapi/" check with one that
prefixes the servlet context path: obtain String contextPath =
request.getContextPath() (may be empty for root) and check
request.getRequestURI().startsWith(contextPath + "/openapi/") (or
normalize/concatenate safely) so OpenAPI routes are correctly detected
regardless of deployment context.
| // 获取Consumer ID | ||
| Object consumerIdObj = request.getAttribute("Authorization"); | ||
| if (consumerIdObj == null) { | ||
| return null; | ||
| } | ||
|
|
||
| long consumerId = Long.parseLong(consumerIdObj.toString()); | ||
| Consumer consumer = consumerService.getConsumerByConsumerId(consumerId); | ||
| if (consumer == null) { |
There was a problem hiding this comment.
🧩 Analysis chain
Authorization attribute misuse; resolve Consumer from validated token or canonical attribute
Reading a numeric Consumer ID from request attribute "Authorization" is incorrect. "Authorization" is an HTTP header (typically a Bearer token), not a request attribute. Parsing it as long will often throw and silently fall back due to the catch-all, producing unexpected identity behavior for OpenAPI calls.
Prefer one of:
- Option A (attribute set by auth filter): read a canonical attribute key like "consumerId" (or the constant used by your OpenAPI auth filter), keep getConsumerByConsumerId.
- Option B (token-based): read the Authorization header, extract the token, and resolve the Consumer by token via ConsumerService.
Proposed minimal fix (Option A) if your filter sets a consumerId attribute:
- // 获取Consumer ID
- Object consumerIdObj = request.getAttribute("Authorization");
+ // 获取Consumer ID（由OpenAPI认证过滤器注入的 request attribute）
+ Object consumerIdObj = request.getAttribute("consumerId");
if (consumerIdObj == null) {
return null;
}
long consumerId = Long.parseLong(consumerIdObj.toString());
Consumer consumer = consumerService.getConsumerByConsumerId(consumerId);If you already resolve Consumers by token, apply Option B:
- Object consumerIdObj = request.getAttribute("Authorization");
- if (consumerIdObj == null) {
- return null;
- }
-
- long consumerId = Long.parseLong(consumerIdObj.toString());
- Consumer consumer = consumerService.getConsumerByConsumerId(consumerId);
+ String authHeader = request.getHeader("Authorization");
+ if (authHeader == null || authHeader.isEmpty()) {
+ return null;
+ }
+ String token = authHeader.startsWith("Bearer ") ? authHeader.substring(7) : authHeader;
+ // 根据实际可用的 service API 选择方法名
+ Consumer consumer = consumerService.getConsumerByToken(token); // e.g. getConsumerByToken(...)Run this script to confirm which attribute/header your auth filter actually sets and which ConsumerService API exists:
Also applies to: 95-101
🏁 Script executed:
#!/bin/bash
# Inspect how OpenAPI auth propagates consumer identity
rg -n -C3 -g '!**/target/**' -P 'setAttribute\(\s*"(?i:authorization|consumerId)"'
rg -n -C3 -P 'RequestContextHolder|getHeader\(\s*"Authorization"'
rg -n -C3 -P 'interface\s+ConsumerService|class\s+\w*ConsumerService'
rg -n -C3 -P '\bConsumerService\b.*\('Length of output: 36413
🏁 Script executed:
#!/bin/bash
# Display ConsumerAuthUtil.java to see how consumer ID is retrieved
sed -n '1,200p' apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
echo "-----"
# Display ConsumerService.java to inspect available methods
sed -n '1,200p' apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java
echo "-----"
# Display SpringSecurityUserInfoHolder.java to view getConsumerUserInfo implementation
sed -n '1,200p' apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.javaLength of output: 14693
Use the correct consumer ID attribute (ApolloConsumerId) or header token
- Replace
Object consumerIdObj = request.getAttribute("Authorization");
with
Object consumerIdObj = request.getAttribute(ConsumerAuthUtil.CONSUMER_ID);
(importcom.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil) - Or parse the header instead:
String authHeader = request.getHeader("Authorization"); if (!StringUtils.hasText(authHeader)) return null; String token = authHeader.startsWith("Bearer ") ? authHeader.substring(7) : authHeader; Long consumerId = consumerService.getConsumerIdByToken(token); if (consumerId == null) return null; Consumer consumer = consumerService.getConsumerByConsumerId(consumerId);
Apply to both /openapi/ blocks in getConsumerUserInfo().
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
around lines 83 to 91, the code reads the wrong request attribute
("Authorization") to get the consumer ID; replace it to use the correct
attribute ConsumerAuthUtil.CONSUMER_ID (import
com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil) or alternatively read
the Authorization header, extract the Bearer token, call
consumerService.getConsumerIdByToken(token) to resolve the Long consumerId, then
call consumerService.getConsumerByConsumerId(consumerId); apply the same fix to
both /openapi/ blocks inside getConsumerUserInfo().
049dc71 to
6d86cc2
Compare
dosubot
Bot
added
size:XXL
tacklequestions
force-pushed
the
pr/a05-cluster
branch
from
6d86cc2 to
e166e53
Compare
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
31-32: Add @primary annotation to resolve potential bean conflicts.As noted in a previous review, this implementation should be marked as
@Primaryto ensure it's the preferred bean when autowiringClusterOpenApiServiceby type, avoiding ambiguity with the legacy beanServerClusterOpenApiServiceOld.Apply this diff:
import org.springframework.stereotype.Service; +import org.springframework.context.annotation.Primary; -@Service +@Service +@Primary public class ServerClusterOpenApiService implements ClusterOpenApiService {apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
53-59: Return 404 when cluster not found.The controller returns
200 OKwith a null body when the cluster is not found. Per REST conventions, it should return404 Not Foundto indicate the resource doesn't exist.Apply this diff:
public ResponseEntity<OpenClusterDTO> getCluster(@PathVariable String env, @PathVariable String appId, @PathVariable String clusterName) { OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName); + if (cluster == null) { + return ResponseEntity.notFound().build(); + } return ResponseEntity.ok(cluster); }
🧹 Nitpick comments (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (1)
31-31: Consider renaming to avoid class name collision.Having two classes named
ServerClusterOpenApiServicein different packages (this one in.server.serviceand the new one in.server.service.impl) creates confusion and makes code navigation harder. Consider renaming this legacy class toLegacyServerClusterOpenApiServiceorServerClusterOpenApiServiceV1to make the distinction clearer.Apply this diff to rename the class:
-public class ServerClusterOpenApiService implements ClusterOpenApiService { +public class LegacyServerClusterOpenApiService implements ClusterOpenApiService {Note: You'll also need to update the constructor name on line 35.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
53-58: Consider using English for Javadoc comments.The Javadoc is written in Chinese, which is inconsistent with the rest of the codebase (e.g., English comments in other files). For international collaboration and maintainability, consider translating to English:
/** - * 删除集群 - * @param env 环境 - * @param appId 应用ID - * @param clusterName 集群名称 + * Delete a cluster. + * @param env environment + * @param appId application ID + * @param clusterName cluster name */apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (2)
95-107: Consider using 204 No Content for DELETE operations.The method returns
200 OKwith an empty body. REST best practices suggest204 No Contentis more appropriate for successful DELETE operations that don't return a response body.Apply this diff:
public ResponseEntity<Void> deleteCluster(@PathVariable String env, @PathVariable String appId, @PathVariable String clusterName) { clusterOpenApiService.deleteCluster(env, appId, clusterName); - return ResponseEntity.ok().build(); + return ResponseEntity.noContent().build(); }Otherwise, the authorization check and audit logging are correctly implemented.
49-52: Consider using English for Javadoc comments.The Javadoc comments are in Chinese (lines 50-51, 63-64, 96-97), which is inconsistent with the rest of the codebase. For better international collaboration and maintainability, consider translating to English:
Example for getCluster:
/** - * 获取指定集群信息 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} + * Get cluster information + * @param env environment + * @param appId application ID + * @param clusterName cluster name + * @return cluster DTO */Also applies to: 62-65, 95-98
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java (1)
81-149: LGTM: Well-structured tests cover key scenarios.The existing tests properly validate:
testCreateCluster: Happy path with authorization and user validation (lines 82-109)testCreateClusterWithAppIdMismatch: Input validation for appId consistency (lines 112-133)testDeleteCluster: Successful deletion with admin authorization (lines 136-149)Mocking strategy and assertions are appropriate.
Consider adding tests for additional edge cases to improve coverage:
- Invalid cluster name format (should return 400)
- Non-existent operator user (should return 400)
- Unauthorized cluster creation attempt (permission denied)
- Unauthorized cluster deletion attempt (non-admin)
Example for invalid cluster name:
@Test public void testCreateClusterWithInvalidName() throws Exception { String appId = "test-app"; String env = "DEV"; String invalidClusterName = "invalid@name"; String operator = "apollo"; OpenClusterDTO clusterDTO = new OpenClusterDTO(); clusterDTO.setAppId(appId); clusterDTO.setName(invalidClusterName); clusterDTO.setDataChangeCreatedBy(operator); when(consumerPermissionValidator.hasCreateClusterPermission(appId)).thenReturn(true); this.mockMvc.perform( MockMvcRequestBuilders.post("/openapi/v1/envs/{env}/apps/{appId}/clusters", env, appId) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(clusterDTO)) ) .andDo(MockMvcResultHandlers.print()) .andExpect(status().isBadRequest()); }
📜 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/ClusterOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java(1 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/ClusterOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.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/ServerClusterOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
Service(31-63)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (3)
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/InputValidator.java (1)
InputValidator(26-47)apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/RequestPrecondition.java (1)
RequestPrecondition(24-43)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (1)
Service(30-51)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiModelConverters.java (1)
OpenApiModelConverters(77-426)
⏰ 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 (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
40-44: LGTM: Null return delegates 404 handling to controller.The method correctly returns null when the cluster is not found, allowing the controller layer to map this to an HTTP 404 response. Ensure the controller (ClusterController) handles this case appropriately.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
66-93: LGTM: Comprehensive validation and authorization.The
createClusterendpoint properly validates:
- Path vs payload appId consistency (line 72-75)
- Required fields are non-empty (line 80-81)
- Cluster name format (line 83-85)
- Operator user existence (line 87-89)
Authorization is correctly enforced via
@PreAuthorizechecking cluster creation permissions.
| @Test | ||
| public void testGetCluster() throws Exception { | ||
| String appId = "test-app"; | ||
| String env = "DEV"; | ||
| String clusterName = "default"; | ||
| OpenClusterDTO clusterDTO = new OpenClusterDTO(); | ||
| clusterDTO.setAppId(appId); | ||
| clusterDTO.setName(clusterName); | ||
|
|
||
| when(clusterOpenApiService.getCluster(appId, env, clusterName)).thenReturn(clusterDTO); | ||
|
|
||
| this.mockMvc.perform(MockMvcRequestBuilders.get("/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}", env, appId, clusterName)) | ||
| .andDo(MockMvcResultHandlers.print()) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.appId", is(appId))) | ||
| .andExpect(jsonPath("$.name", is(clusterName))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test case for cluster not found (404).
The test only covers the happy path where the cluster exists. Add a test case to verify the controller returns 404 Not Found when the cluster doesn't exist:
@Test
public void testGetClusterNotFound() throws Exception {
String appId = "test-app";
String env = "DEV";
String clusterName = "nonexistent";
when(clusterOpenApiService.getCluster(appId, env, clusterName)).thenReturn(null);
this.mockMvc.perform(MockMvcRequestBuilders.get("/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}",
env, appId, clusterName))
.andDo(MockMvcResultHandlers.print())
.andExpect(status().isNotFound());
}This test is essential to verify the controller correctly handles the not-found case once the controller is fixed to return 404.
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java
around lines 63 to 79, add a new test that verifies the controller returns 404
when a cluster is not found: create a test method (e.g., testGetClusterNotFound)
that sets appId/env/clusterName (use a nonexistent name), mock
clusterOpenApiService.getCluster(appId, env, clusterName) to return null,
perform the same MockMvc GET to
/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}, and assert the
response status isNotFound(); place this test next to the existing
testGetCluster method.
9094846 to
7f3b123
Compare
dosubot
Bot
added
size:XL
tacklequestions
force-pushed
the
pr/a05-cluster
branch
3 times, most recently
from
b057923 to
b7a6c49
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
47-50: Return 404 when cluster not found.The method returns
200 OKwith a null body when the cluster doesn't exist. RESTful convention dictates returning404 Not Foundfor missing resources.Apply this diff:
@Override public ResponseEntity<OpenClusterDTO> getCluster(String appId, String clusterName, String env) { - return ResponseEntity.ok(this.clusterOpenApiService.getCluster(appId, env, clusterName)); + OpenClusterDTO cluster = this.clusterOpenApiService.getCluster(appId, env, clusterName); + if (cluster == null) { + return ResponseEntity.notFound().build(); + } + return ResponseEntity.ok(cluster); }
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java (1)
20-27: Add interface-level Javadoc and consider parameter order consistency.The interface would benefit from:
- Interface-level Javadoc describing the service's purpose
- Method-level Javadoc for each operation
Additionally, the parameter order varies across methods:
getCluster:appId, env, clusterNamecreateCluster:env, openClusterDTO(appId is in DTO)deleteCluster:env, appId, clusterNameWhile this may be intentional based on usage patterns, consider standardizing the order (e.g., consistently placing
envfirst orappIdfirst) to improve API consistency and reduce confusion.Example Javadoc:
+/** + * Server-side OpenAPI service interface for cluster operations. + * Provides methods to retrieve, create, and delete clusters across environments. + */ public interface ClusterOpenApiService { + /** + * Retrieves a cluster by application ID, environment, and cluster name. + * + * @param appId the application ID + * @param env the environment name + * @param clusterName the cluster name + * @return the cluster DTO, or null if not found + */ OpenClusterDTO getCluster(String appId, String env, String clusterName); + /** + * Creates a new cluster in the specified environment. + * + * @param env the environment name + * @param openClusterDTO the cluster data + * @return the created cluster DTO + */ OpenClusterDTO createCluster(String env, OpenClusterDTO openClusterDTO); + /** + * Deletes a cluster from the specified environment. + * + * @param env the environment name + * @param appId the application ID + * @param clusterName the cluster name + */ void deleteCluster(String env, String appId, String clusterName); }
📜 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/ClusterOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerParamBindLowLevelTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerParamBindLowLevelTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (2)
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/openapi/v1/controller/ClusterController.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
RestController(45-199)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java (1)
RestController(36-74)apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-138)
⏰ 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/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java (4)
20-21: LGTM!The import change from
OpenApiBeanUtilstoOpenApiModelConvertersaligns with the PR's objective to use the new converter utility for DTO transformations.
41-41: LGTM!The conversion method call is updated correctly. The null check before conversion ensures that null is returned when the cluster doesn't exist, which is then handled by the controller layer.
46-48: LGTM!The bidirectional conversion using
OpenApiModelConvertersis correct. The converter methods include null checks viaPreconditions.checkArgument, ensuring robustness.
51-54: LGTM!The
deleteClustermethod correctly delegates to the underlyingclusterServiceand follows the same pattern as other methods in this service (e.g., usingEnv.valueOf(env)).apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (2)
52-76: LGTM!The
createClustermethod properly:
- Validates that path variable
appIdmatches the payloadappId- Checks for non-empty cluster name and operator
- Validates cluster name format
- Verifies operator exists in the user service
- Returns the created cluster wrapped in
ResponseEntityThe validation logic is thorough and follows the pattern established in other controllers (e.g.,
AppController).
78-94: LGTM!The
deleteClustermethod correctly:
- Applies authorization check via
@PreAuthorize- Adds audit logging via
@ApolloAuditLog- Validates operator is not empty
- Verifies the operator exists
- Delegates deletion to the service
- Returns appropriate response
The implementation follows the established pattern seen in
AppController.deleteApp.
tacklequestions
force-pushed
the
pr/a05-cluster
branch
from
b7a6c49 to
5e87c5e
Compare
dosubot
Bot
added
size:L
4 participants
What's the purpose of this PR
Refactor the Cluster domain end-to-end on top of generated OpenAPI models: add a server-side OpenAPI service interface and implementation for Cluster,
migrate the OpenAPI ClusterController to use it, and add/update tests. Keep the legacy service bean (named) to avoid Java 8 bean conflicts.
Which issue(s) this PR fixes:
Fixes #5462
Brief changelog
Add server-side interface: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java
Add implementation: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java
Migrate controller: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java
Add tests: apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java
Retain legacy service: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerClusterOpenApiService.java with
@service("ServerClusterOpenApiServiceOld") to ensure Java 8 build compatibility
No changes to OpenApiBeanUtils; scope limited to Cluster domain; base on PR03 (converters)
openapi: GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} → portal: GET /apps/{appId}/envs/{env}/clusters/{clusterName} — 查询指定集群详情（original openapi） (apollo-portal/target/generated-sources/openapi/
src/main/java/com/ctrip/framework/apollo/openapi/api/ClusterManagementApi.java:154, apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java:68)
openapi: POST /openapi/v1/envs/{env}/apps/{appId}/clusters → portal: POST /apps/{appId}/envs/{env}/clusters — 创建集群（original openapi） (apollo-portal/target/generated-sources/openapi/src/main/java/com/ctrip/framework/
apollo/openapi/api/ClusterManagementApi.java:45, apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java:47)
openapi: DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} → portal: DELETE /apps/{appId}/envs/{env}/clusters/{clusterName} — 删除集群 (apollo-portal/target/generated-sources/openapi/src/main/java/com/ctrip/framework/apollo/openapi/api/ClusterManagementApi.java:112, apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/
ClusterController.java:59)
Summary by CodeRabbit
New Features
Refactor
Tests