Implementation of OpenAPI Controllers/AppController #5467
Conversation
…upport - Add Maven OpenAPI code generation plugin with swagger dependencies now that we can use automatically generated schemas which predefined in apollo-openapi.yaml - Enhance SpringSecurityUserInfoHolder to support OpenAPI consumer context - Update AuthConfiguration to inject ConsumerService with lazy loading - Add Maven local repository to .gitignore This change enables automatic OpenAPI client/server code generation from external spec and provides dual user context support for both regular web users and OpenAPI consumers."
- Refractor the pom.xml to add java 8 support of openapi-generator plugin
- Refractor the pom.xml to add java 8 support of openapi-generator plugin
This migration aligns the app management APIs with the OpenAPI specification, fills missing API functionalities to align with the portal, and establishes a cleaner separation between generated models and DTOs. --- **Refactoring & Migration** * Replace OpenAPI DTO classes with auto-generated model classes in `AppController` * Refactor `AppController` to use new service layer and model classes * Update `OpenApiBeanUtils` to support both DTO and model conversions * Remove obsolete `ServerAppOpenApiService` class (replaced by impl version) **Service Layer Enhancements** * Add `AppOpenApiService` interface to define app management operations * Create `ServerAppOpenApiService` implementation with full functionality * Add `ServerAppNamespaceApiService` for app namespace management operations **API & Functionality Improvements** * Fill in previously missing API functionalities to match portal capabilities * Support pagination and `MultiResponseEntity` for better API responses * Add audit logging annotations and improved error handling **Testing** * Enhance test coverage with updated `AppControllerTest` ---
dosubot
bot
added
the
size:XXL
|
Warning Rate limit exceeded@tacklequestions has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (21)
WalkthroughAdds OpenAPI generation to the build; introduces many OpenAPI server interfaces and implementations (moved under .impl); refactors controllers to ResponseEntity-based RESTful endpoints; extends consumer-based user resolution; expands DTO conversion utilities; updates tests; and publishes domain events for create/update/delete operations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant AppController
participant ServerApp as ServerAppOpenApiService
participant PortalApp as AppService
participant RoleInit as RoleInitializationService
participant Publisher as EventPublisher
Client->>AppController: POST /openapi/v1/apps (OpenCreateAppDTO)
AppController->>ServerApp: createApp(req)
ServerApp->>PortalApp: createAppAndAddRolePermission(...)
PortalApp-->>ServerApp: App created
ServerApp->>RoleInit: initNamespaceRoles(...)
RoleInit-->>ServerApp: OK
ServerApp->>Publisher: publish AppInfoChangedEvent
Publisher-->>ServerApp: OK
ServerApp-->>AppController: OpenAppDTO
AppController-->>Client: 200 OK (OpenAppDTO)
sequenceDiagram
autonumber
actor Client
participant ItemController
participant ServerItem as ServerItemOpenApiService
participant ItemSvc as ItemService
Client->>ItemController: PUT /openapi/v1/.../items (OpenItemDTO)
ItemController->>ServerItem: createOrUpdateItem(...)
alt exists
ServerItem->>ItemSvc: updateItem(...)
else not found
ServerItem->>ItemSvc: createItem(...)
end
ItemSvc-->>ServerItem: OK
ServerItem-->>ItemController: void
ItemController-->>Client: 200 OK
sequenceDiagram
autonumber
actor Client
participant ReleaseController
participant ServerRelease as ServerReleaseOpenApiService
participant ReleaseSvc as ReleaseService
Client->>ReleaseController: GET /openapi/v1/envs/{env}/releases/{id}
ReleaseController->>ServerRelease: getReleaseById(env,id)
ServerRelease->>ReleaseSvc: findReleaseById(id)
ReleaseSvc-->>ServerRelease: ReleaseDTO|null
alt found
ServerRelease-->>ReleaseController: OpenReleaseDTO
ReleaseController-->>Client: 200 OK
else not found
ServerRelease-->>ReleaseController: NotFoundException
ReleaseController-->>Client: 404 Not Found
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
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)
113-122: Null-safety for SecurityContext.
getAuthentication()may be null; avoid NPE.Apply:
- Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); + if (SecurityContextHolder.getContext() == null || SecurityContextHolder.getContext().getAuthentication() == null) { + return "anonymous"; + } + Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal();apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
1-1: Resolve missing OpenAPI-generated DTOs causing apollo-portal Maven compile failureOpenAPI DTOs (OpenAppDTO, OpenCreateAppDTO, OpenEnvClusterDTO) are missing while ServerAppOpenApiService exists (apollo-portal/src/main/java/.../ServerAppOpenApiService.java). apollo-portal/pom.xml includes openapi-generator and build-helper entries, but generated sources are not present — CI likely isn't running codegen or generated sources were removed.
- Commit or re-generate the OpenAPI model sources (or add the module/dependency that provides them).
- If generation should run at build time: ensure the openapi-generator-maven-plugin runs in the generate-sources phase on CI and build-helper registers the correct generated directory before compile; verify inputSpec/outputDir settings in apollo-portal/pom.xml.
- Reconcile the openapi-generator versions in apollo-portal/pom.xml (6.6.0 vs 7.15.0) to a single, compatible version.
🧹 Nitpick comments (8)
.gitignore (1)
30-32: Keep .m2/ in .gitignore — confirm CI/builds don't expect a workspace-local .m2
- Repo-level
.m2/ignore is correct (prevents committing credentials in settings.xml).- pom.xml references
.m2/settings.xml(pom.xml:541) — verify CI/build scripts (Jenkinsfile, Azure Pipelines, GitHub Actions) do not require a workspace-local.m2; if they do, adjust.gitignoreor CI configuration.- Optional: ignore OpenAPI Generator metadata.
Apply if desired:
# Maven local repository .m2/ +# OpenAPI Generator metadata (optional) +.openapi-generator/apollo-portal/pom.xml (1)
30-30: Remote, mutable OpenAPI spec URL (repro/supply-chain risk).Pin to a tagged commit or vendor the spec into the repo to make builds deterministic and offline‑friendly.
Apply (example with pinned commit):
- <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/main/apollo-openapi.yaml</apollo.openapi.spec.url> + <apollo.openapi.spec.url>https://raw.githubusercontent.com/tacklequestions/apollo-openapi/COMMIT_SHA/apollo-openapi.yaml</apollo.openapi.spec.url>apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
29-36: Prefer explicit imports over wildcard for stability.Using
com.ctrip.framework.apollo.openapi.dto.*obscures missing type errors and risks collisions with generated models.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppNamesapceApiService.java (1)
199-201: Operator derivation may be null.
userInfoHolder.getUser()can return aUserInfowith nulluserId. Guard to avoid passing null operator tobatchDeleteByAppId.Apply:
- String operator = userInfoHolder.getUser().getUserId(); + String operator = userInfoHolder.getUser() != null ? userInfoHolder.getUser().getUserId() : null; + if (operator == null) { + throw new IllegalStateException("operator is null"); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
49-56: Trust boundary note: request attribute must be set post-auth.Using
ApolloConsumerIdfrom the request is fine if injected by a trusted auth filter. Add a brief comment and consider validating type/correlation with token to prevent spoofing.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
166-178: Per-env error handling LGTM, but include appId in message.Adds observability when failures occur across envs.
- "load env:" + env.getName() + " cluster error." + e.getMessage())); + String.format("load env:%s, appId:%s cluster error. %s", env.getName(), appId, e.getMessage())));apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
76-102: Consider extracting test data builder for better maintainabilityThe test creates mock data inline. Consider extracting a test data builder or factory methods to reduce duplication and improve test readability.
+ private static class TestDataBuilder { + static List<OpenAppDTO> createApps(String... appIds) { + return Arrays.stream(appIds) + .map(TestDataBuilder::createApp) + .collect(Collectors.toList()); + } + + static OpenAppDTO createApp(String appId) { + OpenAppDTO dto = new OpenAppDTO(); + dto.setAppId(appId); + dto.setName(appId); + return dto; + } + } + @Test public void testGetAuthorizedApps() throws Exception { final long consumerId = 123456L; Mockito.when(this.consumerAuthUtil.retrieveConsumerIdFromCtx()).thenReturn(consumerId); Set<String> authorized = new HashSet<>(Arrays.asList("app1", "app2")); Mockito.when(this.consumerService.findAppIdsAuthorizedByConsumerId(consumerId)) .thenReturn(authorized); - List<OpenAppDTO> mockApps = new ArrayList<>(); - mockApps.add(openApp("app1")); - mockApps.add(openApp("app2")); + List<OpenAppDTO> mockApps = TestDataBuilder.createApps("app1", "app2"); Mockito.when(this.serverAppOpenApiService.getAppsInfo(Mockito.anyList())) .thenReturn(mockApps);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
19-48: Consider using specific imports instead of wildcardUsing wildcard imports (
java.util.*) can lead to naming conflicts and reduced code clarity. Consider using explicit imports for better maintainability.-import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Set;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)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/ServerAppNamesapceApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java(0 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/OpenApiBeanUtils.java(4 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 with no reviewable changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (1)
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/openapi/util/OpenApiBeanUtils.java (3)
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/dto/NamespaceDTO.java (1)
NamespaceDTO(22-66)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/server/service/ServerAppNamesapceApiService.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (1)
Service(55-227)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(48-235)
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(35-123)
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/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/portal/controller/ConsumerController.java (1)
RestController(42-177)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/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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(48-235)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/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)
🪛 GitHub Actions: build
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java
[error] 219-219: Cannot find symbol: class OpenInstanceDTO
[error] 227-227: Cannot find symbol: class OpenInstanceDTO
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppNamesapceApiService.java
[error] 10-46: Cannot find symbol: AppNamespaceOpenApiService in package com.ctrip.framework.apollo.openapi.api
[error] 18-54: Cannot find symbol: AppNamespaceOpenApiService in package com.ctrip.framework.apollo.openapi.api
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/pom.xml
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java
[error] 1-1: Maven compilation failed for module apollo-portal. Command: mvn -B clean compile -Dmaven.gitcommitid.skip=true
🔇 Additional comments (9)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java (1)
49-50: LGTM: endpoint switch to query param aligns with RESTful consolidation.Please confirm the controller routes
/openapi/v1/apps?authorized=trueto the authorized-apps logic and thatServerAppOpenApiService#getAuthorizedApps()is implemented accordingly.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserInfoHolder.java (1)
41-45: Bean wiring: ensure ConsumerService is present in all active profiles.
@Lazy ConsumerServicemust exist whenauth/ldapprofiles are active. If not guaranteed, guard the constructor with@Autowired(required=false)or null‑check usage.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (2)
112-114: Bean signature change: check downstream config/tests.Both
springSecurityUserInfoHolderbeans now requireConsumerService. Ensure the bean exists forauthandldapprofiles; otherwise startup will fail.Also applies to: 216-218
87-90: Bypass URLs include/openapi/**(good).This matches the new OpenAPI flows.
apollo-portal/pom.xml (1)
281-319: Unify openapi-generator-maven-plugin versions and verify CI JDK/effective POM.
- Found conflicting versions in apollo-portal/pom.xml: top-level/pluginManagement uses 7.15.0 (lines 143–146); java8 profile uses 6.6.0 (lines 290–294); java11plus uses 7.15.0 (lines 310–314). A plugin at ~line 252 has no and will inherit management.
- Action: ensure only one version is active per JDK — either scope pluginManagement to profiles or align the versions (e.g., update java8 profile to 7.15.0 or remove/override the top-level version).
Maven not available in the sandbox (mvn: not found). On CI or a dev machine using the same JDK as your build, run:
mvn -pl apollo-portal help:effective-pom | rg -n "openapi-generator-maven-plugin|"
to confirm which version is effective.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (4)
52-52: Good alignment with OpenAPI refactoring objectivesThe migration from portal-service mocks to using OpenAPI DTOs with
ObjectMapperfor JSON serialization aligns well with the PR objectives of standardizing API design and using DTOs for inputs/outputs.
121-137: LGTM: Good coverage of authorization scenariosThe test properly validates the distinction between authorized apps (when no appIds provided) and specific app queries (with appIds parameter), ensuring the consumer service is not called unnecessarily in the latter case.
183-210: LGTM: Good validation of bad request scenariosThe tests properly validate null checks for both the app object and appId field, ensuring proper error handling.
161-181: Capture and assert consumerId passed to assignAppRoleToConsumerUse an ArgumentCaptor to capture the (long consumerId, String appId) passed to consumerService.assignAppRoleToConsumer(...) and assert consumerId == 11L and appId.equals("new-app"). File: apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (lines 161-181).
| <pluginManagement> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>org.openapitools</groupId> | ||
| <artifactId>openapi-generator-maven-plugin</artifactId> | ||
| <version>7.15.0</version> | ||
| <executions> | ||
| <execution> | ||
| <id>generate-openapi-sources</id> | ||
| <phase>generate-sources</phase> | ||
| <goals> | ||
| <goal>generate</goal> | ||
| </goals> | ||
| <configuration> | ||
| <inputSpec>${apollo.openapi.spec.url}</inputSpec> | ||
| <generatorName>spring</generatorName> | ||
| <output>${project.build.directory}/generated-sources/openapi</output> | ||
| <apiPackage>com.ctrip.framework.apollo.openapi.api</apiPackage> | ||
| <modelPackage>com.ctrip.framework.apollo.openapi.model</modelPackage> | ||
| <invokerPackage>com.ctrip.framework.apollo.openapi.invoker</invokerPackage> | ||
| <configOptions> | ||
| <interfaceOnly>true</interfaceOnly> | ||
| <useTags>true</useTags> | ||
| <dateLibrary>java8</dateLibrary> | ||
| </configOptions> | ||
| <skipValidateSpec>true</skipValidateSpec> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| </plugins> |
There was a problem hiding this comment.
OpenAPI generator execution is only in pluginManagement; it won’t run.
Executions defined under pluginManagement are not bound unless echoed under plugins. Move/duplicate the execution into the plugins section so sources are actually generated.
Apply:
<plugins>
...
- <!-- OpenAPI 代码生成插件 -->
- <plugin>
- <groupId>org.openapitools</groupId>
- <artifactId>openapi-generator-maven-plugin</artifactId>
- </plugin>
+ <!-- OpenAPI 代码生成插件 -->
+ <plugin>
+ <groupId>org.openapitools</groupId>
+ <artifactId>openapi-generator-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>generate-openapi-sources</id>
+ <phase>generate-sources</phase>
+ <goals>
+ <goal>generate</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>Also applies to: 251-256
🤖 Prompt for AI Agents
In apollo-portal/pom.xml around lines 141 to 171 (and likewise for the similar
block at lines 251-256), the openapi-generator execution is only declared inside
pluginManagement so it will not run; move or duplicate the <execution> (the
generate-openapi-sources bound to generate-sources) into the <plugins> section
so the plugin is actually executed during the build, keeping the
pluginManagement entry for defaults if desired and ensuring the <version>,
<configuration> and <executions> are present under <plugin> in <plugins>.
| <apiPackage>com.ctrip.framework.apollo.openapi.api</apiPackage> | ||
| <modelPackage>com.ctrip.framework.apollo.openapi.model</modelPackage> | ||
| <invokerPackage>com.ctrip.framework.apollo.openapi.invoker</invokerPackage> | ||
| <configOptions> | ||
| <interfaceOnly>true</interfaceOnly> | ||
| <useTags>true</useTags> | ||
| <dateLibrary>java8</dateLibrary> | ||
| </configOptions> | ||
| <skipValidateSpec>true</skipValidateSpec> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Package collision risk with existing apollo-openapi APIs.
Generated APIs target com.ctrip.framework.apollo.openapi.api, which may collide with classes from the apollo-openapi dependency. Use a dedicated package for generated code.
Apply:
- <apiPackage>com.ctrip.framework.apollo.openapi.api</apiPackage>
- <modelPackage>com.ctrip.framework.apollo.openapi.model</modelPackage>
- <invokerPackage>com.ctrip.framework.apollo.openapi.invoker</invokerPackage>
+ <apiPackage>com.ctrip.framework.apollo.openapi.generated.api</apiPackage>
+ <modelPackage>com.ctrip.framework.apollo.openapi.generated.model</modelPackage>
+ <invokerPackage>com.ctrip.framework.apollo.openapi.generated.invoker</invokerPackage>Then update imports accordingly in code.
📝 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.
| <apiPackage>com.ctrip.framework.apollo.openapi.api</apiPackage> | |
| <modelPackage>com.ctrip.framework.apollo.openapi.model</modelPackage> | |
| <invokerPackage>com.ctrip.framework.apollo.openapi.invoker</invokerPackage> | |
| <configOptions> | |
| <interfaceOnly>true</interfaceOnly> | |
| <useTags>true</useTags> | |
| <dateLibrary>java8</dateLibrary> | |
| </configOptions> | |
| <skipValidateSpec>true</skipValidateSpec> | |
| <apiPackage>com.ctrip.framework.apollo.openapi.generated.api</apiPackage> | |
| <modelPackage>com.ctrip.framework.apollo.openapi.generated.model</modelPackage> | |
| <invokerPackage>com.ctrip.framework.apollo.openapi.generated.invoker</invokerPackage> | |
| <configOptions> | |
| <interfaceOnly>true</interfaceOnly> | |
| <useTags>true</useTags> | |
| <dateLibrary>java8</dateLibrary> | |
| </configOptions> | |
| <skipValidateSpec>true</skipValidateSpec> |
🤖 Prompt for AI Agents
In apollo-portal/pom.xml around lines 158 to 166, the generated API packages are
set to com.ctrip.framework.apollo.openapi.* which risks colliding with the
existing apollo-openapi dependency; change the <apiPackage>, <modelPackage>, and
<invokerPackage> values to a dedicated namespace (for example
com.ctrip.framework.apollo.generated.openapi.api, .model, .invoker or similar
project-specific generated package) and keep <configOptions> as-is; after
updating these package names regenerate the client and update all imports/usages
across the codebase to the new package paths so code compiles against the new
non-colliding generated packages.
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
Show resolved
Hide resolved
|
|
||
| List<OpenAppDTO> getAppsInfo(List<String> appIds); | ||
|
|
||
| List<OpenAppDTO> getAuthorizedApps(); |
There was a problem hiding this comment.
Unimplemented contract likely breaks controller paths.
getAuthorizedApps() is used by the new authorized=true flow; leaving it abstract is fine, but the impl currently throws UOE. Implement it in ServerAppOpenApiService before merging.
| public List<OpenAppDTO> getAuthorizedApps() { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
Implement authorized-apps retrieval.
Throwing UOE will break /openapi/v1/apps?authorized=true. Implement via your existing consumer auth path (e.g., resolve current consumer, fetch its authorized appIds, then appService.findByAppIds).
Apply a sketch:
@Override
public List<OpenAppDTO> getAuthorizedApps() {
- throw new UnsupportedOperationException();
+ // TODO: wire ConsumerService/UserInfoHolder to get current consumer/appIds
+ // Set<String> appIds = consumerService.findAppIdsAuthorizedByCurrentConsumer();
+ // return OpenApiBeanUtils.transformFromApps(appService.findByAppIds(appIds));
+ return Collections.emptyList();
}📝 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> getAuthorizedApps() { | |
| throw new UnsupportedOperationException(); | |
| } | |
| @Override | |
| public List<OpenAppDTO> getAuthorizedApps() { | |
| // TODO: wire ConsumerService/UserInfoHolder to get current consumer/appIds | |
| // Set<String> appIds = consumerService.findAppIdsAuthorizedByCurrentConsumer(); | |
| // return OpenApiBeanUtils.transformFromApps(appService.findByAppIds(appIds)); | |
| return Collections.emptyList(); | |
| } |
| /** | ||
| * 获取单个应用信息 | ||
| * GET /openapi/v1/apps/{appId} | ||
| */ | ||
| @GetMapping("/apps/{appId}") | ||
| 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.ok(apps.get(0)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translate Chinese comments to English
/**
- * 获取单个应用信息
- * GET /openapi/v1/apps/{appId}
+ * Get single application information
+ * GET /openapi/v1/apps/{appId}
*/📝 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.
| /** | |
| * 获取单个应用信息 | |
| * GET /openapi/v1/apps/{appId} | |
| */ | |
| @GetMapping("/apps/{appId}") | |
| 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.ok(apps.get(0)); | |
| } | |
| /** | |
| * Get single application information | |
| * GET /openapi/v1/apps/{appId} | |
| */ | |
| @GetMapping("/apps/{appId}") | |
| 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.ok(apps.get(0)); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
around lines 139 to 150, the JavaDoc and inline comment above the getApp method
are written in Chinese; replace them with an English equivalent describing the
method and its HTTP mapping (e.g., "Get single application information" and "GET
/openapi/v1/apps/{appId}") so comments are consistent and readable for an
international codebase.
| /** | ||
| * 更新应用 | ||
| * PUT /openapi/v1/apps/{appId} | ||
| */ | ||
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | ||
| @PutMapping("/apps/{appId}") | ||
| @ApolloAuditLog(type = OpType.UPDATE, name = "App.update") | ||
| public ResponseEntity<OpenAppDTO> updateApp(@PathVariable String appId, @Valid @RequestBody OpenAppDTO dto) { | ||
| if (!Objects.equals(appId, dto.getAppId())) { | ||
| throw new BadRequestException("The App Id of path variable and request body is different"); | ||
| } | ||
|
|
||
| // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 | ||
| serverAppOpenApiService.updateApp(dto); | ||
|
|
||
| return ResponseEntity.ok(dto); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translate Chinese comments to English
/**
- * 更新应用
- * PUT /openapi/v1/apps/{appId}
+ * Update application
+ * PUT /openapi/v1/apps/{appId}
*/
@PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)")
@PutMapping("/apps/{appId}")
@ApolloAuditLog(type = OpType.UPDATE, name = "App.update")
public ResponseEntity<OpenAppDTO> updateApp(@PathVariable String appId, @Valid @RequestBody OpenAppDTO dto) {
if (!Objects.equals(appId, dto.getAppId())) {
throw new BadRequestException("The App Id of path variable and request body is different");
}
- // 直接调用ServerAppOpenApiService，让Service层处理DTO转换
+ // Delegate to ServerAppOpenApiService for DTO conversion and business logic
serverAppOpenApiService.updateApp(dto);📝 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.
| /** | |
| * 更新应用 | |
| * PUT /openapi/v1/apps/{appId} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | |
| @PutMapping("/apps/{appId}") | |
| @ApolloAuditLog(type = OpType.UPDATE, name = "App.update") | |
| public ResponseEntity<OpenAppDTO> updateApp(@PathVariable String appId, @Valid @RequestBody OpenAppDTO dto) { | |
| if (!Objects.equals(appId, dto.getAppId())) { | |
| throw new BadRequestException("The App Id of path variable and request body is different"); | |
| } | |
| // 直接调用ServerAppOpenApiService，让Service层处理DTO转换 | |
| serverAppOpenApiService.updateApp(dto); | |
| return ResponseEntity.ok(dto); | |
| } | |
| /** | |
| * Update application | |
| * PUT /openapi/v1/apps/{appId} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | |
| @PutMapping("/apps/{appId}") | |
| @ApolloAuditLog(type = OpType.UPDATE, name = "App.update") | |
| public ResponseEntity<OpenAppDTO> updateApp(@PathVariable String appId, @Valid @RequestBody OpenAppDTO dto) { | |
| if (!Objects.equals(appId, dto.getAppId())) { | |
| throw new BadRequestException("The App Id of path variable and request body is different"); | |
| } | |
| // Delegate to ServerAppOpenApiService for DTO conversion and business logic | |
| serverAppOpenApiService.updateApp(dto); | |
| return ResponseEntity.ok(dto); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
around lines 152 to 168, the method-level comments are in Chinese; replace them
with English equivalents so code comments are consistent and readable (e.g.,
"更新应用" -> "Update application", and the HTTP mapping comment "PUT
/openapi/v1/apps/{appId}" can remain or be restated as "PUT
/openapi/v1/apps/{appId} - update an application"). Keep the JavaDoc style and
spacing intact, preserving annotations and method behavior.
| /** | ||
| * 获取当前Consumer的应用列表（分页） | ||
| * GET /openapi/v1/apps/by-self | ||
| */ | ||
| @GetMapping("/apps/by-self") | ||
| public ResponseEntity<List<OpenAppDTO>> getAppsBySelf(Pageable page) { | ||
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | ||
| Set<String> authorizedAppIds = this.consumerService.findAppIdsAuthorizedByConsumerId(consumerId); | ||
| List<OpenAppDTO> apps = serverAppOpenApiService.getAppsBySelf(authorizedAppIds, page); | ||
| return ResponseEntity.ok(apps); | ||
| } | ||
|
|
||
| /** | ||
| * 获取应用导航树 | ||
| * GET /openapi/v1/apps/{appId}/navtree | ||
| */ | ||
| @GetMapping("/apps/{appId}/navtree") | ||
| public ResponseEntity<MultiResponseEntity<EnvClusterInfo>> getAppNavTree(@PathVariable String appId) { | ||
| MultiResponseEntity<EnvClusterInfo> response = serverAppOpenApiService.getAppNavTree(appId); | ||
| return ResponseEntity.ok(response); | ||
| } | ||
|
|
||
| return this.appOpenApiService.getAppsInfo(new ArrayList<>(appIds)); | ||
| /** | ||
| * 在指定环境创建应用 | ||
| * POST /openapi/v1/apps/envs/{env} | ||
| */ | ||
| @PreAuthorize(value = "@consumerPermissionValidator.hasCreateApplicationPermission()") | ||
| @PostMapping(value = "/apps/envs/{env}") | ||
| @ApolloAuditLog(type = OpType.CREATE, name = "App.create.forEnv") | ||
| public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | ||
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | ||
| String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); | ||
|
|
||
| serverAppOpenApiService.createAppInEnv(env, app, operator); | ||
|
|
||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| /** | ||
| * 删除应用 | ||
| * DELETE /openapi/v1/apps/{appId} | ||
| */ | ||
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | ||
| @DeleteMapping("/apps/{appId}") | ||
| @ApolloAuditLog(type = OpType.DELETE, name = "App.delete") | ||
| public ResponseEntity<Void> deleteApp(@PathVariable String appId) { | ||
| serverAppOpenApiService.deleteApp(appId); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| /** | ||
| * 查找缺失的环境 | ||
| * GET /openapi/v1/apps/{appId}/miss_envs | ||
| */ | ||
| @GetMapping("/apps/{appId}/miss_envs") | ||
| public ResponseEntity<MultiResponseEntity<String>> findMissEnvs(@PathVariable String appId) { | ||
| MultiResponseEntity<String> response = serverAppOpenApiService.findMissEnvs(appId); | ||
| return ResponseEntity.ok(response); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translate all remaining Chinese comments to English
All method-level Javadoc comments should be in English for consistency.
/**
- * 获取当前Consumer的应用列表（分页）
- * GET /openapi/v1/apps/by-self
+ * Get current consumer's application list (paginated)
+ * GET /openapi/v1/apps/by-self
*/
/**
- * 获取应用导航树
- * GET /openapi/v1/apps/{appId}/navtree
+ * Get application navigation tree
+ * GET /openapi/v1/apps/{appId}/navtree
*/
/**
- * 在指定环境创建应用
- * POST /openapi/v1/apps/envs/{env}
+ * Create application in specified environment
+ * POST /openapi/v1/apps/envs/{env}
*/
/**
- * 删除应用
- * DELETE /openapi/v1/apps/{appId}
+ * Delete application
+ * DELETE /openapi/v1/apps/{appId}
*/
/**
- * 查找缺失的环境
- * GET /openapi/v1/apps/{appId}/miss_envs
+ * Find missing environments
+ * GET /openapi/v1/apps/{appId}/miss_envs
*/📝 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.
| /** | |
| * 获取当前Consumer的应用列表（分页） | |
| * GET /openapi/v1/apps/by-self | |
| */ | |
| @GetMapping("/apps/by-self") | |
| public ResponseEntity<List<OpenAppDTO>> getAppsBySelf(Pageable page) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| Set<String> authorizedAppIds = this.consumerService.findAppIdsAuthorizedByConsumerId(consumerId); | |
| List<OpenAppDTO> apps = serverAppOpenApiService.getAppsBySelf(authorizedAppIds, page); | |
| return ResponseEntity.ok(apps); | |
| } | |
| /** | |
| * 获取应用导航树 | |
| * GET /openapi/v1/apps/{appId}/navtree | |
| */ | |
| @GetMapping("/apps/{appId}/navtree") | |
| public ResponseEntity<MultiResponseEntity<EnvClusterInfo>> getAppNavTree(@PathVariable String appId) { | |
| MultiResponseEntity<EnvClusterInfo> response = serverAppOpenApiService.getAppNavTree(appId); | |
| return ResponseEntity.ok(response); | |
| } | |
| return this.appOpenApiService.getAppsInfo(new ArrayList<>(appIds)); | |
| /** | |
| * 在指定环境创建应用 | |
| * POST /openapi/v1/apps/envs/{env} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.hasCreateApplicationPermission()") | |
| @PostMapping(value = "/apps/envs/{env}") | |
| @ApolloAuditLog(type = OpType.CREATE, name = "App.create.forEnv") | |
| public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); | |
| serverAppOpenApiService.createAppInEnv(env, app, operator); | |
| return ResponseEntity.ok().build(); | |
| } | |
| /** | |
| * 删除应用 | |
| * DELETE /openapi/v1/apps/{appId} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | |
| @DeleteMapping("/apps/{appId}") | |
| @ApolloAuditLog(type = OpType.DELETE, name = "App.delete") | |
| public ResponseEntity<Void> deleteApp(@PathVariable String appId) { | |
| serverAppOpenApiService.deleteApp(appId); | |
| return ResponseEntity.ok().build(); | |
| } | |
| /** | |
| * 查找缺失的环境 | |
| * GET /openapi/v1/apps/{appId}/miss_envs | |
| */ | |
| @GetMapping("/apps/{appId}/miss_envs") | |
| public ResponseEntity<MultiResponseEntity<String>> findMissEnvs(@PathVariable String appId) { | |
| MultiResponseEntity<String> response = serverAppOpenApiService.findMissEnvs(appId); | |
| return ResponseEntity.ok(response); | |
| } | |
| } | |
| /** | |
| * Get current consumer's application list (paginated) | |
| * GET /openapi/v1/apps/by-self | |
| */ | |
| @GetMapping("/apps/by-self") | |
| public ResponseEntity<List<OpenAppDTO>> getAppsBySelf(Pageable page) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| Set<String> authorizedAppIds = this.consumerService.findAppIdsAuthorizedByConsumerId(consumerId); | |
| List<OpenAppDTO> apps = serverAppOpenApiService.getAppsBySelf(authorizedAppIds, page); | |
| return ResponseEntity.ok(apps); | |
| } | |
| /** | |
| * Get application navigation tree | |
| * GET /openapi/v1/apps/{appId}/navtree | |
| */ | |
| @GetMapping("/apps/{appId}/navtree") | |
| public ResponseEntity<MultiResponseEntity<EnvClusterInfo>> getAppNavTree(@PathVariable String appId) { | |
| MultiResponseEntity<EnvClusterInfo> response = serverAppOpenApiService.getAppNavTree(appId); | |
| return ResponseEntity.ok(response); | |
| } | |
| /** | |
| * Create application in specified environment | |
| * POST /openapi/v1/apps/envs/{env} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.hasCreateApplicationPermission()") | |
| @PostMapping(value = "/apps/envs/{env}") | |
| @ApolloAuditLog(type = OpType.CREATE, name = "App.create.forEnv") | |
| public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); | |
| serverAppOpenApiService.createAppInEnv(env, app, operator); | |
| return ResponseEntity.ok().build(); | |
| } | |
| /** | |
| * Delete application | |
| * DELETE /openapi/v1/apps/{appId} | |
| */ | |
| @PreAuthorize(value = "@consumerPermissionValidator.isAppAdmin(#appId)") | |
| @DeleteMapping("/apps/{appId}") | |
| @ApolloAuditLog(type = OpType.DELETE, name = "App.delete") | |
| public ResponseEntity<Void> deleteApp(@PathVariable String appId) { | |
| serverAppOpenApiService.deleteApp(appId); | |
| return ResponseEntity.ok().build(); | |
| } | |
| /** | |
| * Find missing environments | |
| * GET /openapi/v1/apps/{appId}/miss_envs | |
| */ | |
| @GetMapping("/apps/{appId}/miss_envs") | |
| public ResponseEntity<MultiResponseEntity<String>> findMissEnvs(@PathVariable String appId) { | |
| MultiResponseEntity<String> response = serverAppOpenApiService.findMissEnvs(appId); | |
| return ResponseEntity.ok(response); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
around lines 170-229, the method-level Javadoc comments are written in Chinese;
update each comment block immediately above the methods (getAppsBySelf,
getAppNavTree, createAppInEnv, deleteApp, findMissEnvs) to clear, idiomatic
English descriptions that explain the endpoint purpose and HTTP mapping (e.g.,
"Get the current consumer's application list (paged) - GET
/openapi/v1/apps/by-self", "Get application navigation tree - GET
/openapi/v1/apps/{appId}/navtree", "Create an application in the specified
environment - POST /openapi/v1/apps/envs/{env}", "Delete an application - DELETE
/openapi/v1/apps/{appId}", "Find missing environments - GET
/openapi/v1/apps/{appId}/miss_envs"); keep the annotations and method signatures
unchanged.
| public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | ||
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | ||
| String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); | ||
|
|
||
| serverAppOpenApiService.createAppInEnv(env, app, operator); | ||
|
|
||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
Add null check for consumer retrieval
The code retrieves consumer by ID but doesn't check if the consumer exists before accessing its name. This could lead to a NullPointerException.
@PreAuthorize(value = "@consumerPermissionValidator.hasCreateApplicationPermission()")
@PostMapping(value = "/apps/envs/{env}")
@ApolloAuditLog(type = OpType.CREATE, name = "App.create.forEnv")
public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) {
long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx();
- String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName();
+ com.ctrip.framework.apollo.openapi.entity.Consumer consumer = this.consumerService.getConsumerByConsumerId(consumerId);
+ if (consumer == null) {
+ throw new BadRequestException("Consumer not found for id: " + consumerId);
+ }
+ String operator = consumer.getName();
serverAppOpenApiService.createAppInEnv(env, app, 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 ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| String operator = this.consumerService.getConsumerByConsumerId(consumerId).getName(); | |
| serverAppOpenApiService.createAppInEnv(env, app, operator); | |
| return ResponseEntity.ok().build(); | |
| } | |
| public ResponseEntity<Void> createAppInEnv(@PathVariable String env, @Valid @RequestBody OpenAppDTO app) { | |
| long consumerId = this.consumerAuthUtil.retrieveConsumerIdFromCtx(); | |
| com.ctrip.framework.apollo.openapi.entity.Consumer consumer = this.consumerService.getConsumerByConsumerId(consumerId); | |
| if (consumer == null) { | |
| throw new BadRequestException("Consumer not found for id: " + consumerId); | |
| } | |
| String operator = consumer.getName(); | |
| serverAppOpenApiService.createAppInEnv(env, app, operator); | |
| return ResponseEntity.ok().build(); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
around lines 199-206, add a null-check after retrieving the consumer: if
consumerService.getConsumerByConsumerId(consumerId) returns null, handle it
explicitly (e.g., return ResponseEntity.status(HttpStatus.NOT_FOUND).build() or
throw a suitable runtime/REST exception with a clear message) instead of calling
getName() on a potentially null object; proceed to call
serverAppOpenApiService.createAppInEnv(env, app, operator) only when consumer is
non-null.
| public void testCreateAppInEnv() throws Exception { | ||
| // Allow create application permission for openapi endpoint | ||
| Mockito.when(consumerPermissionValidator.hasCreateApplicationPermission()).thenReturn(true); | ||
| Mockito.when(consumerAuthUtil.retrieveConsumerIdFromCtx()).thenReturn(100L); | ||
|
|
||
| com.ctrip.framework.apollo.openapi.entity.Consumer c = | ||
| new com.ctrip.framework.apollo.openapi.entity.Consumer(); | ||
| c.setName("operator-x"); | ||
| Mockito.when(consumerService.getConsumerByConsumerId(100L)).thenReturn(c); | ||
|
|
||
| OpenAppDTO app = openApp("env-app"); | ||
|
|
||
| this.mockMvc.perform( | ||
| MockMvcRequestBuilders.post("/openapi/v1/apps/envs/DEV") | ||
| .contentType("application/json") | ||
| .content(new ObjectMapper().writeValueAsString(app)) | ||
| ) | ||
| .andDo(MockMvcResultHandlers.print()) | ||
| .andExpect(status().isOk()); | ||
|
|
||
| // verify invocation with captured DTO instead of instance equality | ||
| ArgumentCaptor<OpenAppDTO> appCaptor = ArgumentCaptor.forClass(OpenAppDTO.class); | ||
| Mockito.verify(serverAppOpenApiService, times(1)) | ||
| .createAppInEnv(Mockito.eq("DEV"), appCaptor.capture(), Mockito.eq("operator-x")); | ||
| OpenAppDTO capturedApp = appCaptor.getValue(); | ||
| org.junit.Assert.assertEquals("env-app", capturedApp.getAppId()); | ||
| org.junit.Assert.assertEquals("env-app", capturedApp.getName()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify operator name extraction logic
The test correctly verifies that the operator name is extracted from the consumer and passed to the service. However, ensure that the actual implementation handles null consumer scenarios gracefully.
🏁 Script executed:
#!/bin/bash
# Check how the ConsumerService handles null or missing consumers
rg -A 5 "getConsumerByConsumerId" --type java apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/Length of output: 1596
Handle null consumer when extracting operator name
AppController.java calls consumerService.getConsumerByConsumerId(consumerId).getName() but ConsumerService.getConsumerByConsumerId(...) can return null (consumerRepository.findById(...).orElse(null)) — this will NPE. Fix by null-checking the consumer (or change the service to return Optional/throw a clear NotFound) and add a unit test for the missing-consumer path. Files: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java, apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerService.java.
- fix a Map<String> generation problem by adjusting apollo-openapi.yaml - change build-helper-maven-plugin version from 3.5.0 to 3.4.0 to adapt to the maven wrapper
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apollo-portal/pom.xml (2)
166-166: Enable spec validation to catch contract drift earlySetting skipValidateSpec to true hides spec issues. Enable validation (at least in CI).
- <skipValidateSpec>true</skipValidateSpec> + <skipValidateSpec>false</skipValidateSpec>
30-30: Pin the OpenAPI spec URL to a commit or vendor it to avoid flaky buildsBuilding against main makes builds non‑reproducible and can break unexpectedly.
Consider pinning to a commit SHA URL or vendoring the spec into the repo, then referencing a local path property.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (3)
160-166: NPE risk when ruleItems or nested lists are nullopenGrayReleaseRuleDTO.getRuleItems() and item.getClientIpList()/getClientLabelList() may be null.
Apply:
- Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = - new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()); + Set<OpenGrayReleaseRuleItemDTO> openGrayReleaseRuleItemDTOSet = + openGrayReleaseRuleDTO.getRuleItems() == null + ? Collections.emptySet() + : new HashSet<>(openGrayReleaseRuleDTO.getRuleItems()); @@ - Set<String> clientIpList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); - Set<String> clientLabelList = new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList()); + Set<String> clientIpList = openGrayReleaseRuleItemDTO.getClientIpList() == null + ? Collections.emptySet() + : new HashSet<>(openGrayReleaseRuleItemDTO.getClientIpList()); + Set<String> clientLabelList = openGrayReleaseRuleItemDTO.getClientLabelList() == null + ? Collections.emptySet() + : new HashSet<>(openGrayReleaseRuleItemDTO.getClientLabelList());
185-188: Nit: parameter name ‘Cluster’ should be lowerCamelCaseUse cluster to avoid shadowing type naming.
- public static OpenClusterDTO transformFromClusterDTO(ClusterDTO Cluster) { - Preconditions.checkArgument(Cluster != null); - return BeanUtils.transform(OpenClusterDTO.class, Cluster); + public static OpenClusterDTO transformFromClusterDTO(ClusterDTO cluster) { + Preconditions.checkArgument(cluster != null); + return BeanUtils.transform(OpenClusterDTO.class, cluster);
19-27: Generator is configured — ensure generated models are available or isolate themapollo-portal/pom.xml configures openapi-generator to write models into com.ctrip.framework.apollo.openapi.model and build-helper-maven-plugin adds ${project.build.directory}/generated-sources/openapi to sources, so imports compile when the generate-sources step runs (see apollo-portal/pom.xml).
- Action: ensure CI/build runs generate-sources (or commit/generated sources or document IDE setup) so OpenEnvClusterDTO/OpenInstanceDTO are present.
- Optional: move generated models to a distinct package (e.g., com.ctrip.framework.apollo.openapi.generated.model) or switch imports to existing DTOs under com.ctrip.framework.apollo.openapi.dto.* to avoid surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/OpenApiBeanUtils.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/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/util/OpenApiBeanUtils.java (4)
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/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/openapi/server/service/AppOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/EnvClusterInfo.java (1)
EnvClusterInfo(24-48)
🪛 GitHub Actions: build
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
[error] Command mvn -B clean compile -Dmaven.gitcommitid.skip=true failed due to compilation errors in apollo-portal module.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java
[error] Command mvn -B clean compile -Dmaven.gitcommitid.skip=true failed due to compilation errors in apollo-portal module.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
[error] Command mvn -B clean compile -Dmaven.gitcommitid.skip=true failed due to compilation errors in apollo-portal module.
apollo-portal/pom.xml
[error] Command mvn -B clean compile -Dmaven.gitcommitid.skip=true failed due to compilation errors in apollo-portal module.
🔇 Additional comments (13)
apollo-portal/pom.xml (4)
158-166: Generated package names risk collisions; use a dedicated namespaceUsing com.ctrip.framework.apollo.openapi.* for generated types can collide with classes from apollo-openapi. Switch to a generated.* subpackage and update imports.
- <apiPackage>com.ctrip.framework.apollo.openapi.api</apiPackage> - <modelPackage>com.ctrip.framework.apollo.openapi.model</modelPackage> - <invokerPackage>com.ctrip.framework.apollo.openapi.invoker</invokerPackage> + <apiPackage>com.ctrip.framework.apollo.openapi.generated.api</apiPackage> + <modelPackage>com.ctrip.framework.apollo.openapi.generated.model</modelPackage> + <invokerPackage>com.ctrip.framework.apollo.openapi.generated.invoker</invokerPackage>
257-277: LGTM: build-helper correctly adds generated sourcesThis points to ${project.build.directory}/generated-sources/openapi/src/main/java which matches the openapi-generator output.
281-319: Profile versions OK; ensure consistency with plugin blockYou manage generator versions per JDK. After moving executions into , ensure the aligns with the active profile in CI.
141-171: Generator executions are only in pluginManagement; they won’t runThe openapi-generator goals are bound only under pluginManagement. They won’t execute unless you also bind them under . This is likely why apollo-portal fails to compile with missing openapi.model types.
Apply this diff to bind executions under :
@@ <!-- OpenAPI 代码生成插件 --> <plugin> <groupId>org.openapitools</groupId> <artifactId>openapi-generator-maven-plugin</artifactId> + <version>7.15.0</version> + <executions> + <execution> + <id>generate-openapi-sources</id> + <phase>generate-sources</phase> + <goals> + <goal>generate</goal> + </goals> + <configuration> + <inputSpec>${apollo.openapi.spec.url}</inputSpec> + <generatorName>spring</generatorName> + <output>${project.build.directory}/generated-sources/openapi</output> + <apiPackage>com.ctrip.framework.apollo.openapi.generated.api</apiPackage> + <modelPackage>com.ctrip.framework.apollo.openapi.generated.model</modelPackage> + <invokerPackage>com.ctrip.framework.apollo.openapi.generated.invoker</invokerPackage> + <configOptions> + <interfaceOnly>true</interfaceOnly> + <useTags>true</useTags> + <dateLibrary>java8</dateLibrary> + <useSpringBoot3>true</useSpringBoot3> + <useJakartaEe>true</useJakartaEe> + </configOptions> + <skipValidateSpec>false</skipValidateSpec> + </configuration> + </execution> + </executions> </plugin>apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (5)
54-61: LGTM: batch item transform mirrors existing single-item helperNull/empty guarded, streams used correctly.
93-94: LGTM: boolean accessors adjusted to setIsPublicMatches the model change.
124-128: LGTM: boolean accessors adjusted to setIsLockedMatches the model change.
134-138: LGTM: NamespaceDTO → OpenNamespaceDTO transformConsistent with other transform helpers.
207-225: OpenInstanceDTO references likely do not exist; causes compilation failureCI indicates missing OpenInstanceDTO. Unless the spec defines it and you generate it, these helpers won’t compile.
Minimal compile-fix (remove for now):
- /** - * 将InstanceDTO转换为OpenInstanceDTO - */ - public static OpenInstanceDTO transformFromInstanceDTO(final InstanceDTO instanceDTO) { - Preconditions.checkArgument(instanceDTO != null); - return BeanUtils.transform(OpenInstanceDTO.class, instanceDTO); - } - - /** - * 批量转换InstanceDTO列表为OpenInstanceDTO列表 - */ - public static List<OpenInstanceDTO> transformFromInstanceDTOs(final List<InstanceDTO> instanceDTOs) { - if (CollectionUtils.isEmpty(instanceDTOs)) { - return Collections.emptyList(); - } - return instanceDTOs.stream() - .map(OpenApiBeanUtils::transformFromInstanceDTO) - .collect(Collectors.toList()); - }If the spec contains Instance, keep these and import the generated class from com.ctrip.framework.apollo.openapi.generated.model after fixing the generator.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java (2)
40-40: Ensure getAuthorizedApps is implemented before mergeControllers relying on authorized=true will break if the impl throws.
30-32: Don’t ship a default method that always throwsMake createApp abstract; force implementation and avoid runtime UOE.
- default void createApp(OpenCreateAppDTO req) { - throw new UnsupportedOperationException(); - } + void createApp(OpenCreateAppDTO req);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (2)
100-111: LGTM: env→clusters mapping with stable orderingUsing BeanUtils.toPropertySet("name", clusterDTOs) and ArrayList ensures deduplication and predictable order.
130-133: Implement authorized-apps retrieval instead of throwingThis breaks /openapi/v1/apps?authorized=true.
Sketch:
@Override public List<OpenAppDTO> getAuthorizedApps() { - throw new UnsupportedOperationException(); + // Resolve current consumer/user and fetch authorized appIds, then map to DTOs + // Example: + // Set<String> appIds = consumerService.findAppIdsAuthorizedByCurrentConsumer(); + // List<App> apps = appService.findByAppIds(appIds); + // return OpenApiBeanUtils.transformFromApps(apps); + return java.util.Collections.emptyList(); }Additional changes required (outside this hunk):
// Add field and constructor arg private final ConsumerService consumerService; // Inject it in the constructor and wire via @Service/@Autowired as applicable.
| <!-- OpenAPI 代码生成插件 --> | ||
| <plugin> | ||
| <groupId>org.openapitools</groupId> | ||
| <artifactId>openapi-generator-maven-plugin</artifactId> | ||
| </plugin> | ||
|
|
There was a problem hiding this comment.
OpenAPI plugin declared without executions
This lacks , so nothing runs. See earlier diff to add here.
🤖 Prompt for AI Agents
In apollo-portal/pom.xml around lines 251 to 256, the
openapi-generator-maven-plugin is declared but has no <executions> configured so
it never runs; add an <executions> section that binds the appropriate generator
goal(s) (e.g., generate) to the desired lifecycle phase (e.g., generate-sources)
and configure the plugin's <configuration> (inputSpec, output, generatorName,
apiPackage, modelPackage, etc.) inside those executions so the OpenAPI code
generation runs during the build.
| private App convert(OpenAppDTO dto) { | ||
| return App.builder() | ||
| .appId(dto.getAppId()) | ||
| .name(dto.getName()) | ||
| .ownerName(dto.getOwnerName()) | ||
| .orgId(dto.getOrgId()) | ||
| .orgName(dto.getOrgName()) | ||
| .ownerEmail(dto.getOwnerEmail()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
App.builder() likely does not exist; compile error
App in apollo-common typically has setters, not a Lombok builder.
Apply:
- private App convert(OpenAppDTO dto) {
- return App.builder()
- .appId(dto.getAppId())
- .name(dto.getName())
- .ownerName(dto.getOwnerName())
- .orgId(dto.getOrgId())
- .orgName(dto.getOrgName())
- .ownerEmail(dto.getOwnerEmail())
- .build();
- }
+ private App convert(OpenAppDTO dto) {
+ App app = new App();
+ app.setAppId(dto.getAppId());
+ app.setName(dto.getName());
+ app.setOwnerName(dto.getOwnerName());
+ app.setOrgId(dto.getOrgId());
+ app.setOrgName(dto.getOrgName());
+ app.setOwnerEmail(dto.getOwnerEmail());
+ return app;
+ }📝 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.
| private App convert(OpenAppDTO dto) { | |
| return App.builder() | |
| .appId(dto.getAppId()) | |
| .name(dto.getName()) | |
| .ownerName(dto.getOwnerName()) | |
| .orgId(dto.getOrgId()) | |
| .orgName(dto.getOrgName()) | |
| .ownerEmail(dto.getOwnerEmail()) | |
| .build(); | |
| } | |
| private App convert(OpenAppDTO dto) { | |
| App app = new App(); | |
| app.setAppId(dto.getAppId()); | |
| app.setName(dto.getName()); | |
| app.setOwnerName(dto.getOwnerName()); | |
| app.setOrgId(dto.getOrgId()); | |
| app.setOrgName(dto.getOrgName()); | |
| app.setOwnerEmail(dto.getOwnerEmail()); | |
| return app; | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
around lines 77 to 86, the code uses App.builder() which does not exist (App
uses setters), so replace the builder pattern with creating a new App instance
and calling its setters for appId, name, ownerName, orgId, orgName and
ownerEmail (e.g. new App(); app.setAppId(dto.getAppId()); ...), or use an
available constructor if one exists; ensure imports remain correct and remove
any Lombok/builder references.
| public void createApp(OpenCreateAppDTO req) { | ||
| App app = null; | ||
| if (req.getApp() != null) { | ||
| app = convert(req.getApp()); | ||
| } | ||
| appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Null-safety and validation for createApp
Guard null req/app/admins; otherwise NPE.
@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 IllegalArgumentException("app is required");
+ }
+ App app = convert(req.getApp());
+ Set<String> admins = req.getAdmins() == null ? new HashSet<>() : new HashSet<>(req.getAdmins());
+ appService.createAppAndAddRolePermission(app, admins);
}📝 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 createApp(OpenCreateAppDTO req) { | |
| App app = null; | |
| if (req.getApp() != null) { | |
| app = convert(req.getApp()); | |
| } | |
| appService.createAppAndAddRolePermission(app, new HashSet<>(req.getAdmins())); | |
| } | |
| @Override | |
| public void createApp(OpenCreateAppDTO req) { | |
| if (req == null || req.getApp() == null) { | |
| throw new IllegalArgumentException("app is required"); | |
| } | |
| App app = convert(req.getApp()); | |
| Set<String> admins = req.getAdmins() == null ? new HashSet<>() : new HashSet<>(req.getAdmins()); | |
| appService.createAppAndAddRolePermission(app, admins); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java
around lines 92 to 99, the method createApp assumes req, req.getApp(), and
req.getAdmins() are non-null and will NPE; add null checks and validation:
validate req is not null (throw IllegalArgumentException or Api-specific
exception), validate req.getApp() is not null before convert (or throw), ensure
req.getAdmins() is not null/empty and convert to a non-null Set before calling
appService.createAppAndAddRolePermission; perform these checks early and
return/throw with a clear message so createAppAndAddRolePermission never
receives null arguments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppNamespaceOpenApiService.java (9)
22-26: Add English Javadoc (bilingual) for wider contributor/user reach.Consider adding an English summary alongside the existing Chinese comments.
19-21: Import @nullable to mark documented null returns.You document null return cases below; annotate them to make contracts explicit.
Apply:
import com.ctrip.framework.apollo.openapi.model.OpenAppNamespaceDTO; +import org.springframework.lang.Nullable; import java.util.List;
37-44: Annotate nullable return for findPublicAppNamespace.Javadoc says “returns null if not exists”; make it explicit.
- OpenAppNamespaceDTO findPublicAppNamespace(String namespaceName); + @Nullable + OpenAppNamespaceDTO findPublicAppNamespace(String namespaceName);
45-53: Annotate nullable return for findByAppIdAndName; clarify name normalization.Consider defining case/prefix normalization in Javadoc (e.g., trimmed, case-sensitive) to avoid subtle bugs.
- OpenAppNamespaceDTO findByAppIdAndName(String appId, String namespaceName); + @Nullable + OpenAppNamespaceDTO findByAppIdAndName(String appId, String namespaceName);
54-61: Specify ordering of results (deterministic API).Document or guarantee sort order (e.g., by namespaceName asc) so clients can rely on stable pagination and diffs.
62-68: Avoid unbounded findAll() in OpenAPI service.Unbounded “findAll” can be expensive and conflicts with the refactor objective to standardize pagination. Prefer a paged variant and consider deprecating this method.
76-84: Clarify uniqueness scope and prefix semantics.Document whether uniqueness is within the app or global, and whether “appendNamespacePrefix” affects the uniqueness check.
85-101: Avoid boolean flag parameter (flag argument anti‑pattern).Split into two explicit methods (e.g., createAppNamespace and createAppNamespaceWithPrefix) or carry the option in the DTO to improve readability and future extensibility.
119-125: Batch delete: gate with permissions, transaction, and audit.This is destructive; ensure admin-only permissions, transactional integrity, and audit logging in the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppNamespaceOpenApiService.java(1 hunks)
🔇 Additional comments (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppNamespaceOpenApiService.java (4)
69-75: Confirm idempotency and return value for createDefaultAppNamespace.Recommend idempotent behavior and optionally returning the created/existing namespace for observability.
102-109: Define import semantics (idempotent/upsert, conflict policy).Please document whether import overwrites existing namespaces, skips duplicates, or errors; and whether it’s idempotent.
110-118: Protect against deleting default/publicly referenced namespaces.Ensure implementation forbids deleting “application” default or public namespaces referenced by other apps, with clear error codes/messages per the refactor goals.
27-27: Naming consistency: verify implementation class name (possible typo "Namesapce")Confirm implementor(s) of AppNamespaceOpenApiService use "Namespace" (not "Namesapce") and ensure "OpenApi" vs "Api" suffix is used consistently; check apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/.
…tion pattern **Service Layer Changes:** - Add interface definitions for all OpenAPI services - ClusterOpenApiService, InstanceOpenApiService, ItemOpenApiService - NamespaceOpenApiService, OrganizationOpenApiService, ReleaseOpenApiService - Move concrete implementations to impl package - ServerClusterOpenApiService → impl/ServerClusterOpenApiService - ServerNamespaceOpenApiService → impl/ServerNamespaceOpenApiService - ServerOrganizationOpenApiService → impl/ServerOrganizationOpenApiService - Reimplement service classes with enhanced functionality - ServerInstanceOpenApiService: Complete rewrite (37→95 lines) - ServerItemOpenApiService: Major expansion (115→284 lines) - ServerReleaseOpenApiService: Significant enhancement (71→134 lines) **Controller Layer Enhancements:** - ClusterController: Enhanced with additional endpoints (+73 lines) - InstanceController: Expanded functionality (+76 lines) - ItemController: Major feature additions (+271 lines) - NamespaceBranchController: Significant enhancements (+198 lines) - NamespaceController: Comprehensive expansion (+267 lines) - OrganizationController: Minor improvements (+22 lines) - ReleaseController: Enhanced capabilities (+143 lines) **Summary:** - Added 6 new service interfaces (242 lines) - Restructured service implementations into impl package - Enhanced 7 controllers with 873 additional lines of functionality - Improved separation of concerns with interface-implementation pattern - Total: +1,115 lines added, -413 lines removed across 22 files
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (35)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
41-43: Potentially unbounded response; consider pagination
getByReleasesNotIn(...)returns a full List without paging. For large namespaces this can be heavy. Consider offering a paged variant or document server-side limits.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java (1)
75-79: Guard null/empty releaseIds and use safe envPrevent a potential NPE in downstream services and avoid unnecessary calls for empty filters.
- public List<OpenInstanceDTO> getByReleasesNotIn(String env, String appId, String clusterName, - String namespaceName, Set<Long> releaseIds) { - List<InstanceDTO> portalInstances = instanceService.getByReleasesNotIn(Env.valueOf(env), appId, clusterName, namespaceName, releaseIds); - return OpenApiBeanUtils.transformFromInstanceDTOs(portalInstances); - } + public List<OpenInstanceDTO> getByReleasesNotIn(String env, String appId, String clusterName, + String namespaceName, Set<Long> releaseIds) { + if (releaseIds == null || releaseIds.isEmpty()) { + return java.util.Collections.emptyList(); + } + final Env targetEnv = safeEnv(env); + List<InstanceDTO> portalInstances = instanceService.getByReleasesNotIn(targetEnv, appId, clusterName, namespaceName, releaseIds); + return OpenApiBeanUtils.transformFromInstanceDTOs(portalInstances); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ReleaseOpenApiService.java (1)
25-37: Avoid concrete-service coupling: promote query methods to an interfaceController injects the concrete ServerReleaseOpenApiService for query methods (findAllReleases/findActiveReleases/compareRelease). Move these into this interface (or a new ReleaseQueryOpenApiService) to depend on abstractions and ease testing.
Example (additions shown):
public interface ReleaseOpenApiService { OpenReleaseDTO publishNamespace(String appId, String env, String clusterName, String namespaceName, NamespaceReleaseDTO releaseDTO); OpenReleaseDTO getLatestActiveRelease(String appId, String env, String clusterName, String namespaceName); void rollbackRelease(String env, long releaseId, String operator); OpenReleaseDTO getReleaseById(String env, long releaseId); + + // Consider returning OpenReleaseDTOs instead of portal BOs for OpenAPI + java.util.List<OpenReleaseDTO> findAllReleases(String appId, String env, String clusterName, + String namespaceName, int page, int size); + + java.util.List<OpenReleaseDTO> findActiveReleases(String appId, String env, String clusterName, + String namespaceName, int page, int size); + + com.ctrip.framework.apollo.portal.entity.vo.ReleaseCompareResult compareRelease(String env, + long baseReleaseId, + long toCompareReleaseId); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java (4)
61-71: Clarify not-found semantics for “latest active release”Returning null here causes a 200 with null body upstream. Prefer 404 (or 204) semantics for absence, either by throwing NotFoundException or handling in controller.
Would you like me to update the controller to return 404 when no latest release is found?
103-105: Don’t expose portal internals (ReleaseBO) via OpenAPIfindAllReleases currently leaks portal BOs. Return OpenReleaseDTO (or a dedicated OpenAPI model) and transform BO.baseInfo instead.
Apply:
- public List<ReleaseBO> findAllReleases(String appId, String env, String clusterName, String namespaceName, int page, int size) { - return releaseService.findAllReleases(appId, Env.valueOf(env), clusterName, namespaceName, page, size); - } + public List<OpenReleaseDTO> findAllReleases(String appId, String env, String clusterName, String namespaceName, int page, int size) { + List<ReleaseBO> bos = releaseService.findAllReleases(appId, Env.valueOf(env), clusterName, namespaceName, page, size); + return bos.stream() + .map(ReleaseBO::getBaseInfo) + .map(OpenApiBeanUtils::transformFromReleaseDTO) + .collect(java.util.stream.Collectors.toList()); + }Note: Update the controller signature accordingly (see controller comment).
117-122: Public methods missing from the interfaceThese query methods are only on the impl. Promote to an interface to avoid concrete wiring in controllers.
131-133: Interface completeness and cohesioncompareRelease should also be part of the service interface (or a dedicated query interface) for consistency.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (9)
65-85: Depend on abstractions, not the concrete implInjecting ServerReleaseOpenApiService ties the controller to implementation details. Depend on an interface (add missing methods to ReleaseOpenApiService or introduce ReleaseQueryOpenApiService).
92-92: Enable bean validation on request bodyAnnotate the DTO with @Valid to standardize validation.
- @RequestBody NamespaceReleaseDTO model) { + @Valid @RequestBody NamespaceReleaseDTO model) {
120-125: Check permission first and return 404 when not foundPrevent unnecessary data fetch and clarify absence semantics.
- OpenReleaseDTO latestActiveRelease = this.releaseOpenApiService.getLatestActiveRelease(appId, env, clusterName, namespaceName); - if (userPermissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName)) { - throw new AccessDeniedException("Access is denied"); - } - return ResponseEntity.ok(latestActiveRelease); + if (userPermissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName)) { + throw new AccessDeniedException("Access is denied"); + } + OpenReleaseDTO latestActiveRelease = this.releaseOpenApiService.getLatestActiveRelease(appId, env, clusterName, namespaceName); + if (latestActiveRelease == null) { + return ResponseEntity.notFound().build(); + } + return ResponseEntity.ok(latestActiveRelease);
134-134: Enable bean validation on request body (merge)Consistent with other endpoints.
- @RequestBody NamespaceReleaseDTO model) { + @Valid @RequestBody NamespaceReleaseDTO model) {
142-145: Avoid unnecessary uppercasing before Env.valueOfEnv.valueOf handles normalization. Drop toUpperCase to keep inputs intact.
- ReleaseDTO mergedRelease = namespaceBranchService.merge(appId, Env.valueOf(env.toUpperCase()), + ReleaseDTO mergedRelease = namespaceBranchService.merge(appId, Env.valueOf(env), clusterName, namespaceName, branchName, model.getReleaseTitle(), model.getReleaseComment(), Boolean.TRUE.equals(model.getIsEmergencyPublish()), deleteBranch, model.getReleasedBy());
160-160: Enable bean validation on request body (gray release)Add @Valid for consistency.
- @PathVariable String branchName, @RequestBody NamespaceReleaseDTO model) { + @PathVariable String branchName, @Valid @RequestBody NamespaceReleaseDTO model) {
191-191: Enable bean validation on request body (gray-del release)Add @Valid for consistency.
- @PathVariable String branchName, @RequestBody NamespaceGrayDelReleaseDTO model) { + @PathVariable String branchName, @Valid @RequestBody NamespaceGrayDelReleaseDTO model) {
204-205: Normalize env consistentlyUse Env.valueOf(env).toString() instead of manual uppercasing.
- releaseModel.setEnv(env.toUpperCase()); + releaseModel.setEnv(Env.valueOf(env).toString());
269-282: OpenAPI endpoint should not return portal BOsReturning List exposes internal view objects. Return OpenReleaseDTO (or a dedicated OpenAPI model) and transform accordingly.
Controller change:
- public ResponseEntity<List<ReleaseBO>> findAllReleases(...){ + public ResponseEntity<List<OpenReleaseDTO>> findAllReleases(...){ if (userPermissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName)) { - return ResponseEntity.ok(Collections.emptyList()); + return ResponseEntity.ok(Collections.emptyList()); } - - List<ReleaseBO> releases = serverReleaseOpenApiService.findAllReleases(appId, env, clusterName, namespaceName, page, size); - return ResponseEntity.ok(releases); + List<OpenReleaseDTO> releases = serverReleaseOpenApiService.findAllReleases(appId, env, clusterName, namespaceName, page, size); + return ResponseEntity.ok(releases); }Service change (see impl file comment for method signature/body).
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
53-58: Consider standardizing the Javadoc language.The Javadoc comments are in Chinese while the rest of the codebase uses English. Consider using English for consistency.
- /** - * 删除集群 - * @param env 环境 - * @param appId 应用ID - * @param clusterName 集群名称 - */ + /** + * Delete cluster + * @param env environment + * @param appId application ID + * @param clusterName cluster name + */apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java (1)
34-38: Consider documenting the fillItemDetail parameter.The
@since 2.4.0annotation is helpful, but thefillItemDetailparameter's purpose isn't immediately clear. Consider adding parameter documentation./** * Retrieves a single namespace + * @param fillItemDetail whether to include configuration items in the response * @since 2.4.0 */ OpenNamespaceDTO getNamespace(String appId, String env, String clusterName, String namespaceName, boolean fillItemDetail);Similarly for
getNamespaces:/** * Retrieves a list namespaces + * @param fillItemDetail whether to include configuration items in the response * @since 2.4.0 */ List<OpenNamespaceDTO> getNamespaces(String appId, String env, String clusterName, boolean fillItemDetail);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (2)
53-63: Standardize the Javadoc language and format.The Javadoc is in Chinese and includes the endpoint URL. For consistency with the rest of the codebase, use English and consider removing the URL since it's already defined in the annotation.
- /** - * 获取指定集群信息 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} - */ + /** + * Get cluster information + */Apply similar changes to other methods:
- /** - * 创建集群 - * POST /openapi/v1/envs/{env}/apps/{appId}/clusters - */ + /** + * Create a cluster + */ - /** - * 删除集群 - * DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} - */ + /** + * Delete a cluster + */
42-50: Consider using constructor injection for consistency.The controller has both
ClusterOpenApiService(interface) andServerClusterOpenApiService(implementation) injected. Consider using only the interface to maintain proper abstraction boundaries.private final UserService userService; private final ClusterOpenApiService clusterOpenApiService; - private final ServerClusterOpenApiService serverClusterOpenApiService; public ClusterController( UserService userService, - ClusterOpenApiService clusterOpenApiService, - ServerClusterOpenApiService serverClusterOpenApiService) { + ClusterOpenApiService clusterOpenApiService) { this.userService = userService; this.clusterOpenApiService = clusterOpenApiService; - this.serverClusterOpenApiService = serverClusterOpenApiService; }And update the delete method:
- serverClusterOpenApiService.deleteCluster(env, appId, clusterName); + clusterOpenApiService.deleteCluster(env, appId, clusterName);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java (1)
25-45: Avoid depending on concrete impl in controllers; promote advanced methods to an interface (or an extension).ItemController injects both ItemOpenApiService and ServerItemOpenApiService because several item ops (diff/sync/text/branch/order/syntax) are only on the impl. Please expose these via an interface (e.g., extend ItemOpenApiService or introduce ItemAdvancedOpenApiService) so controllers don’t depend on a concrete class.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppNamespaceApiService.java (1)
213-216: Guard against NPE when resolving operator from UserInfoHolder.userInfoHolder.getUser() may be null (non-interactive OpenAPI calls), causing NPE.
Apply this diff:
@Override public void batchDeleteByAppId(String appId) { - String operator = userInfoHolder.getUser().getUserId(); + if (userInfoHolder.getUser() == null) { + throw new IllegalStateException("No logged-in user in context"); + } + String operator = userInfoHolder.getUser().getUserId(); appNamespaceService.batchDeleteByAppId(appId, operator); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerItemOpenApiService.java (2)
181-191: Null-safe comparator for lastModifiedTime.Guard against possible null DataChangeLastModifiedTime.
Apply this diff:
- // 按最后修改时间排序 - items.sort((o1, o2) -> { - if (o1.getDataChangeLastModifiedTime().after(o2.getDataChangeLastModifiedTime())) { - return -1; - } - if (o1.getDataChangeLastModifiedTime().before(o2.getDataChangeLastModifiedTime())) { - return 1; - } - return 0; - }); + // 按最后修改时间排序（空值最后） + items.sort((o1, o2) -> { + final java.util.Date d1 = o1.getDataChangeLastModifiedTime(); + final java.util.Date d2 = o2.getDataChangeLastModifiedTime(); + if (d1 == null && d2 == null) return 0; + if (d1 == null) return 1; + if (d2 == null) return -1; + return d2.compareTo(d1); + });
233-243: Reuse the null-safe comparator here too.Apply this diff:
- if ("lastModifiedTime".equals(orderBy)) { - items.sort((o1, o2) -> { - if (o1.getDataChangeLastModifiedTime().after(o2.getDataChangeLastModifiedTime())) { - return -1; - } - if (o1.getDataChangeLastModifiedTime().before(o2.getDataChangeLastModifiedTime())) { - return 1; - } - return 0; - }); - } + if ("lastModifiedTime".equals(orderBy)) { + items.sort((o1, o2) -> { + final java.util.Date d1 = o1.getDataChangeLastModifiedTime(); + final java.util.Date d2 = o2.getDataChangeLastModifiedTime(); + if (d1 == null && d2 == null) return 0; + if (d1 == null) return 1; + if (d2 == null) return -1; + return d2.compareTo(d1); + }); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java (4)
60-73: Avoid concrete impl dependency in controller.Leverage only the interface(s) by moving advanced item ops into an interface (or an extension) so you can inject by type and drop ServerItemOpenApiService here.
123-128: Handle invalid Base64 for encoded key.Decoding can throw IllegalArgumentException; return 400 instead of 500.
Apply this diff:
@GetMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key:.+}") public ResponseEntity<OpenItemDTO> getItemByEncodedKey(@PathVariable String appId, @PathVariable String env, @PathVariable String clusterName, @PathVariable String namespaceName, @PathVariable String key) { - return this.getItem(appId, env, clusterName, namespaceName, - new String(Base64.getDecoder().decode(key.getBytes(StandardCharsets.UTF_8)))); + try { + String decoded = new String(Base64.getDecoder().decode(key.getBytes(StandardCharsets.UTF_8))); + return this.getItem(appId, env, clusterName, namespaceName, decoded); + } catch (IllegalArgumentException ex) { + throw new BadRequestException("key is not valid Base64"); + } }
154-156: Return 201 Created on successful create.Aligns with REST and the refactor goals.
Apply this diff:
- OpenItemDTO createdItem = this.itemOpenApiService.createItem(appId, env, clusterName, namespaceName, item); - return ResponseEntity.ok(createdItem); + OpenItemDTO createdItem = this.itemOpenApiService.createItem(appId, env, clusterName, namespaceName, item); + return ResponseEntity.status(org.springframework.http.HttpStatus.CREATED).body(createdItem);
118-121: Fix comment to match the actual path.Docstring mentions query param, but the mapping uses path /encodedItems/{key}.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (4)
94-99: Return 404 when branch not found (instead of 400).BadRequest here is misleading; 404 is appropriate.
Apply this diff:
- if (namespaceBO == null) { - throw new BadRequestException("Namespace branch not found"); - } - return ResponseEntity.ok(OpenApiBeanUtils.transformFromNamespaceBO(namespaceBO)); + if (namespaceBO == null) { + return ResponseEntity.notFound().build(); + } + return ResponseEntity.ok(OpenApiBeanUtils.transformFromNamespaceBO(namespaceBO));
108-110: Remove @ResponseStatus when returning ResponseEntity.Avoid conflicting status sources; ResponseEntity already sets 201.
Apply this diff:
- @ResponseStatus(HttpStatus.CREATED)
135-137: Same here: drop @ResponseStatus to avoid duplication.Apply this diff:
- @ResponseStatus(HttpStatus.NO_CONTENT)
235-237: Unify operator header name with mergeBranch (“X-Apollo-Operator”).Keep header names consistent across endpoints.
Apply this diff:
- @RequestBody OpenGrayReleaseRuleDTO rules, - @RequestHeader("operator") String operator) { + @RequestBody OpenGrayReleaseRuleDTO rules, + @RequestHeader("X-Apollo-Operator") String operator) {apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (3)
262-269: Deterministic response order (optional)Sort the result for stable outputs.
Apply this diff:
Set<String> missingNamespaces = findMissingNamespaceNames(appId, env, clusterName); - List<String> result = Lists.newArrayList(missingNamespaces); + List<String> result = Lists.newArrayList(missingNamespaces); + result.sort(String::compareToIgnoreCase); return ResponseEntity.ok(result);
81-83: Add @Valid to request body to leverage bean validationEnsures model-level constraints are enforced uniformly per the refactor goals.
Apply this diff:
- public ResponseEntity<OpenAppNamespaceDTO> createNamespace(@PathVariable String appId, - @RequestBody OpenAppNamespaceDTO appNamespaceDTO) { + public ResponseEntity<OpenAppNamespaceDTO> createNamespace(@PathVariable String appId, + @RequestBody @javax.validation.Valid OpenAppNamespaceDTO appNamespaceDTO) {
126-150: Unify 404 vs 400 semantics across “not found” casesSome endpoints return 404 (loadNamespace, getNamespaceLock) while getAppNamespace used 400 via exception (now suggested to 404). Please standardize across the controller per API spec.
Consider adding/using a NotFoundException mapped to 404, or consistently returning ResponseEntity.notFound().
Also applies to: 183-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
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/InstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/OrganizationOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ReleaseOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java(0 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java(0 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiService.java(0 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppNamespaceApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerItemOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerNamespaceOpenApiService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerOrganizationOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java(7 hunks)
💤 Files with no reviewable changes (3)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerInstanceOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerReleaseOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerItemOpenApiService.java
🧰 Additional context used
🧬 Code graph analysis (13)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java (3)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/PageDTO.java (1)
PageDTO(27-59)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerItemOpenApiService.java (7)
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/dto/PageDTO.java (1)
PageDTO(27-59)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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java (7)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/NotFoundException.java (1)
NotFoundException(21-73)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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/NamespaceReleaseModel.java (1)
NamespaceReleaseModel(23-102)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ReleaseCompareResult.java (1)
ReleaseCompareResult(26-46)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (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/openapi/v1/controller/ClusterController.java (1)
RestController(36-113)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/NamespaceGrayDelReleaseModel.java (1)
NamespaceGrayDelReleaseModel(21-31)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceReleaseModel.java (1)
NamespaceReleaseModel(23-102)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ReleaseCompareResult.java (1)
ReleaseCompareResult(26-46)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/ConfigPublishEvent.java (1)
ConfigPublishEvent(23-185)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java (1)
Validated(52-379)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (1)
Validated(49-214)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.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/exception/NotFoundException.java (1)
NotFoundException(21-73)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/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/NamespaceIdentifier.java (1)
NamespaceIdentifier(23-76)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 (5)
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/v1/controller/InstanceController.java (1)
RestController(32-108)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java (1)
RestController(36-74)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppNamespaceApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationController.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
RestController(36-113)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (1)
RestController(32-108)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (1)
RestController(50-314)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (5)
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/RequestPrecondition.java (1)
RequestPrecondition(24-43)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/ConfigPublishEvent.java (1)
ConfigPublishEvent(23-185)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.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/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/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(39-226)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/environment/Env.java (1)
Env(34-237)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppNamespaceDeletionEvent.java (1)
AppNamespaceDeletionEvent(23-33)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (21)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/OrganizationOpenApiService.java (1)
1-31: LGTM! Well-structured interface following OpenAPI refactoring objectives.This new interface aligns perfectly with the PR objectives to separate concerns and establish a clear service layer. The interface follows clean architecture principles by defining the contract for organization management operations and using the generated OpenAPI model classes (
OpenOrganizationDto).apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerOrganizationOpenApiService.java (3)
17-17: Proper package reorganization aligns with service layer separation.The move to the
implpackage correctly separates interface contracts from their implementations, consistent with the broader refactoring pattern seen across the OpenAPI services.
19-20: Import updates correctly reflect the OpenAPI model migration.The imports have been updated to use the new OpenAPI model package (
com.ctrip.framework.apollo.openapi.model.OpenOrganizationDto) and the refactored service interface path, which aligns with the PR's objective to replace DTO classes with auto-generated model classes.
35-38: Implementation delegates correctly to utility method.The
getOrganizations()method correctly implements the interface contract and delegates toOpenApiBeanUtils.transformFromOrganizations()for the transformation logic, maintaining separation of concerns.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationController.java (2)
19-24: Import updates align with OpenAPI model migration and RESTful design.The imports correctly reflect the migration to generated OpenAPI models and the adoption of ResponseEntity for full HTTP response control, which directly addresses the PR objectives to unify RESTful style and use standardized response wrappers.
39-49: Excellent RESTful endpoint design with proper documentation and response wrapping.The refactored endpoint demonstrates several best practices:
- Uses proper REST URL naming with plural nouns (
/organizations) without action verbs- Wraps response in ResponseEntity for consistent HTTP response control including status codes, headers, and body
- Uses ResponseEntity.ok() for clean, expressive syntax returning HTTP 200 status
- Includes comprehensive Javadoc documentation
- Method name corrected from singular
getOrganization()to pluralgetOrganizations()for consistencyThis implementation directly fulfills the PR objectives from issue #5462 to unify RESTful style and standardize responses using ResponseEntity wrappers.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (6)
19-30: LGTM! Well-organized imports for new functionality.The imports are properly organized and correspond to the new OpenAPI instance management features being added. The
Splitter.on(",").omitEmptyStrings().trimResults()configuration provides robust parsing by automatically removing empty strings and trimming whitespace from comma-separated values.
36-36: LGTM! Appropriate Guava Splitter configuration.The static Splitter configuration is well-designed for parsing comma-separated release IDs. The combination of
omitEmptyStrings()andtrimResults()ensures that inputs like "1, 2, , 3," are properly parsed to ["1", "2", "3"], which is ideal for handling user input that may contain empty values or extra whitespace.
41-45: LGTM! Constructor properly updated for new service layer.The constructor correctly accepts and stores both the existing
InstanceOpenApiServiceand the newServerInstanceOpenApiService, enabling the controller to support both legacy and new OpenAPI endpoints.
61-68: LGTM! Well-implemented paginated instance retrieval by release.The endpoint follows RESTful conventions and properly delegates to the server service layer. The method signature correctly handles pagination parameters with sensible defaults (page=0, size=20).
74-84: LGTM! Comprehensive namespace-based instance search with filtering.The endpoint provides flexible instance querying capabilities with optional
instanceAppIdfiltering and proper pagination support. The method signature and delegation to the service layer are well-structured.
90-107: LGTM! Robust implementation with proper input validation.The endpoint correctly:
- Uses the
paramsattribute to ensure theexcludeReleasesparameter is required- Parses comma-separated release IDs using the well-configured Splitter
- Validates that the parsed release ID set is not empty
- Provides a clear error message for invalid input
- Properly delegates to the service layer
The validation logic prevents potential issues with empty or malformed input, ensuring reliable API behavior.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/InstanceOpenApiService.java (1)
35-43: Clarify/standardize pagination semantics in the contractPlease confirm page is zero-based (Spring Pageable semantics) and size > 0 per the OpenAPI spec. If different, document here and normalize in the controller.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java (2)
88-90: Confirm page index base before propagating
PageDTO#getPage()is zero-based (Spring Pageable). Ensure OpenPageDTOOpenInstanceDTO expects the same. If the OpenAPI schema is one-based, adjust here (+1) to avoid contract drift.
84-95: Make the transformer impl-private and null-safeAvoid exposing this helper; handle null pageDTO and empty content safely.
- @Override - public OpenPageDTOOpenInstanceDTO transformToOpenPageDTO(PageDTO<InstanceDTO> pageDTO) { - List<OpenInstanceDTO> openContent = OpenApiBeanUtils.transformFromInstanceDTOs(pageDTO.getContent()); - OpenPageDTOOpenInstanceDTO openPageDTOOpenInstanceDTO = new OpenPageDTOOpenInstanceDTO(); - openPageDTOOpenInstanceDTO.setPage(pageDTO.getPage()); - openPageDTOOpenInstanceDTO.setSize(pageDTO.getSize()); - openPageDTOOpenInstanceDTO.setTotal(pageDTO.getTotal()); - openPageDTOOpenInstanceDTO.setContent(openContent); - - return openPageDTOOpenInstanceDTO; - } + private static OpenPageDTOOpenInstanceDTO transformToOpenPageDTO(PageDTO<InstanceDTO> pageDTO) { + OpenPageDTOOpenInstanceDTO openPage = new OpenPageDTOOpenInstanceDTO(); + if (pageDTO == null) { + openPage.setPage(0); + openPage.setSize(0); + openPage.setTotal(0L); + openPage.setContent(java.util.Collections.emptyList()); + return openPage; + } + List<OpenInstanceDTO> openContent = pageDTO.hasContent() + ? OpenApiBeanUtils.transformFromInstanceDTOs(pageDTO.getContent()) + : java.util.Collections.emptyList(); + openPage.setPage(pageDTO.getPage()); + openPage.setSize(pageDTO.getSize()); + openPage.setTotal(pageDTO.getTotal()); + openPage.setContent(openContent); + return openPage; + }Likely an incorrect or invalid review comment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (1)
307-313: Consider permission masking for compare resultsAlign with how list endpoints hide configs when the user lacks access (or document why compare is exempt).
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ClusterOpenApiService.java (1)
24-31: LGTM! Clean interface design for cluster operations.The interface follows RESTful principles with clear separation of concerns, aligning well with the PR objective to establish a clearer separation between generated models and DTOs.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/NamespaceOpenApiService.java (1)
28-54: LGTM! Well-designed interface with proper default methods.The interface provides a clean API surface for namespace operations with convenient default methods that set
fillItemDetailtotrue, maintaining backward compatibility while adding flexibility.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (1)
103-111: Confirm OpenAPI cluster delete permission vs portalOpenAPI delete uses @consumerPermissionValidator.isAppAdmin(#appId) (ConsumerPermissionValidator.isSuperAdmin() = false → relies on consumer ASSIGN_ROLE permission), while the portal delete uses @userPermissionValidator.isSuperAdmin() (requires super admin). Files: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java and apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java — confirm this divergence is intentional.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerNamespaceOpenApiService.java (1)
78-80: Verify getAppendNamespacePrefix() return type and null-safety before passing to createAppNamespaceInLocalcreateAppNamespaceInLocal(AppNamespace, boolean) (apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppNamespaceService.java:134) expects a primitive boolean; ServerNamespaceOpenApiService (apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerNamespaceOpenApiService.java:80) passes appNamespaceDTO.getAppendNamespacePrefix() — if the DTO accessor returns Boolean (nullable) this will NPE on auto‑unboxing. Ensure the DTO returns a primitive boolean or null‑guard/convert the value (e.g., Boolean.TRUE.equals(...)).
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (1)
198-208: Incorrect — service throws on missing namespace; null-guard unnecessary AppNamespaceOpenApiService.deleteAppNamespace delegates to AppNamespaceService.deleteAppNamespace, which throws BadRequestException.appNamespaceNotExists when the namespace is missing (apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppNamespaceService.java); deletedAppNamespace will not be null and transformToAppNamespace won't NPE. Change service/exception mapping if you prefer 404 or idempotent 204.Likely an incorrect or invalid review comment.
| @Override | ||
| public void deleteCluster(String env, String appId, String clusterName) { | ||
| clusterService.deleteCluster(Env.valueOf(env), appId, clusterName); | ||
| } |
There was a problem hiding this comment.
Add validation for invalid environment strings.
The Env.valueOf(env) call will throw an IllegalArgumentException if the environment string is invalid. This should be handled gracefully with a more user-friendly error message.
@Override
public void deleteCluster(String env, String appId, String clusterName) {
+ if (!Env.exists(env)) {
+ throw BadRequestException.invalidEnvFormat(env);
+ }
clusterService.deleteCluster(Env.valueOf(env), appId, clusterName);
}Also apply the same validation to other methods:
@Override
public OpenClusterDTO getCluster(String appId, String env, String clusterName) {
+ if (!Env.exists(env)) {
+ throw BadRequestException.invalidEnvFormat(env);
+ }
ClusterDTO clusterDTO = clusterService.loadCluster(appId, Env.valueOf(env), clusterName);
return clusterDTO == null ? null : OpenApiBeanUtils.transformFromClusterDTO(clusterDTO);
}
@Override
public OpenClusterDTO createCluster(String env, OpenClusterDTO openClusterDTO) {
+ if (!Env.exists(env)) {
+ throw BadRequestException.invalidEnvFormat(env);
+ }
ClusterDTO toCreate = OpenApiBeanUtils.transformToClusterDTO(openClusterDTO);
ClusterDTO createdClusterDTO = clusterService.createCluster(Env.valueOf(env), toCreate);
return OpenApiBeanUtils.transformFromClusterDTO(createdClusterDTO);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerClusterOpenApiService.java
around lines 59 to 62, the call Env.valueOf(env) can throw
IllegalArgumentException for invalid environment strings; validate the env input
before converting (or catch IllegalArgumentException) and throw a clear,
user-friendly exception (e.g., BadRequest with a message like "Invalid
environment: <env>") so the API returns a graceful error; apply the same
validation/catching pattern to all other methods in this class that call
Env.valueOf(env).
| @Override | ||
| public int getInstanceCountByNamespace(String appId, String env, String clusterName, String namespaceName) { | ||
| return instanceService.getInstanceCountByNamespace(appId, Env.valueOf(env), clusterName, namespaceName); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden env parsing; avoid IllegalArgumentException from valueOf
Env.valueOf(env) throws on invalid input. Use Env.transformEnv(env) and validate against Env.UNKNOWN to produce consistent 4xx errors per the refactor goals.
Apply this diff:
- public int getInstanceCountByNamespace(String appId, String env, String clusterName, String namespaceName) {
- return instanceService.getInstanceCountByNamespace(appId, Env.valueOf(env), clusterName, namespaceName);
- }
+ public int getInstanceCountByNamespace(String appId, String env, String clusterName, String namespaceName) {
+ final Env targetEnv = safeEnv(env);
+ return instanceService.getInstanceCountByNamespace(appId, targetEnv, clusterName, namespaceName);
+ }Add this helper in the class (outside the shown range):
private static Env safeEnv(String env) {
Env e = Env.transformEnv(env);
if (e == Env.UNKNOWN) {
throw new IllegalArgumentException("Env " + env + " is invalid");
}
return e;
}🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java
around lines 45-49, the code uses Env.valueOf(env) which throws
IllegalArgumentException on invalid input; add a private static helper
safeEnv(String env) that calls Env.transformEnv(env), throws a new
IllegalArgumentException("Env " + env + " is invalid") if the result is
Env.UNKNOWN, and returns the valid Env otherwise, then replace Env.valueOf(env)
with safeEnv(env) in getInstanceCountByNamespace so invalid env strings produce
a controlled 4xx-style error.
| @Override | ||
| public OpenPageDTOOpenInstanceDTO getByRelease(String env, long releaseId, int page, int size) { | ||
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(Env.valueOf(env), releaseId, page, size); | ||
|
|
||
| return transformToOpenPageDTO(portalPageDTO); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the safe env resolver here as well
Align env handling with the helper for consistent behavior.
- public OpenPageDTOOpenInstanceDTO getByRelease(String env, long releaseId, int page, int size) {
- PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(Env.valueOf(env), releaseId, page, size);
+ public OpenPageDTOOpenInstanceDTO getByRelease(String env, long releaseId, int page, int size) {
+ final Env targetEnv = safeEnv(env);
+ PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(targetEnv, releaseId, page, size);
return transformToOpenPageDTO(portalPageDTO);
}📝 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.
| @Override | |
| public OpenPageDTOOpenInstanceDTO getByRelease(String env, long releaseId, int page, int size) { | |
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(Env.valueOf(env), releaseId, page, size); | |
| return transformToOpenPageDTO(portalPageDTO); | |
| } | |
| @Override | |
| public OpenPageDTOOpenInstanceDTO getByRelease(String env, long releaseId, int page, int size) { | |
| final Env targetEnv = safeEnv(env); | |
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByRelease(targetEnv, releaseId, page, size); | |
| return transformToOpenPageDTO(portalPageDTO); | |
| } |
🤖 Prompt for AI Agents
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java
around lines 53 to 58: currently the code calls Env.valueOf(env) directly;
replace that call with the project's safe env resolver helper used elsewhere
(the same method used across the codebase to parse/validate env strings) so env
parsing behavior is consistent and safe — e.g., call the helper method that
safely resolves/validates the env string and pass its result into
instanceService.getByRelease, and handle the helper's null/invalid result
according to the project convention (throw or return an empty page) rather than
using Env.valueOf directly.
| public OpenPageDTOOpenInstanceDTO getByNamespace(String env, String appId, String clusterName, | ||
| String namespaceName, String instanceAppId, | ||
| int page, int size) { | ||
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByNamespace(Env.valueOf(env), appId, clusterName, namespaceName, instanceAppId, page, size); | ||
| return transformToOpenPageDTO(portalPageDTO); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent env parsing
Same change for namespace query.
- public OpenPageDTOOpenInstanceDTO getByNamespace(String env, String appId, String clusterName,
+ public OpenPageDTOOpenInstanceDTO getByNamespace(String env, String appId, String clusterName,
String namespaceName, String instanceAppId,
int page, int size) {
- PageDTO<InstanceDTO> portalPageDTO = instanceService.getByNamespace(Env.valueOf(env), appId, clusterName, namespaceName, instanceAppId, page, size);
+ final Env targetEnv = safeEnv(env);
+ PageDTO<InstanceDTO> portalPageDTO = instanceService.getByNamespace(targetEnv, appId, clusterName, namespaceName, instanceAppId, page, size);
return transformToOpenPageDTO(portalPageDTO);
}📝 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 OpenPageDTOOpenInstanceDTO getByNamespace(String env, String appId, String clusterName, | |
| String namespaceName, String instanceAppId, | |
| int page, int size) { | |
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByNamespace(Env.valueOf(env), appId, clusterName, namespaceName, instanceAppId, page, size); | |
| return transformToOpenPageDTO(portalPageDTO); | |
| } | |
| public OpenPageDTOOpenInstanceDTO getByNamespace(String env, String appId, String clusterName, | |
| String namespaceName, String instanceAppId, | |
| int page, int size) { | |
| final Env targetEnv = safeEnv(env); | |
| PageDTO<InstanceDTO> portalPageDTO = instanceService.getByNamespace(targetEnv, appId, clusterName, namespaceName, instanceAppId, page, size); | |
| return transformToOpenPageDTO(portalPageDTO); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerInstanceOpenApiService.java
around lines 64-69, the method currently uses Env.valueOf(env) which is
inconsistent with other parsing logic; change it to use the same parsing helper
(e.g. Env.fromString(env) or the project's canonical Env.parse/convert method)
and ensure the code handles invalid inputs the same way as other endpoints
(validate the returned Env and throw or return a proper error if parsing fails)
so namespace queries follow the consistent env parsing behavior.
| @Override | ||
| public void updateItem(String appId, String env, String clusterName, String namespaceName, | ||
| OpenItemDTO itemDTO) { | ||
| ItemDTO toUpdateItem = itemService | ||
| .loadItem(Env.valueOf(env), appId, clusterName, namespaceName, itemDTO.getKey()); | ||
| //protect. only value,type,comment,lastModifiedBy can be modified | ||
| toUpdateItem.setComment(itemDTO.getComment()); | ||
| toUpdateItem.setType(itemDTO.getType()); | ||
| toUpdateItem.setValue(itemDTO.getValue()); | ||
| toUpdateItem.setDataChangeLastModifiedBy(itemDTO.getDataChangeLastModifiedBy()); | ||
|
|
||
| itemService.updateItem(appId, Env.valueOf(env), clusterName, namespaceName, toUpdateItem); | ||
| } |
There was a problem hiding this comment.
Fix NPE when updating a non-existent item; return 404 instead.
toUpdateItem can be null; setters will NPE. Return a NotFound instead.
Apply this diff:
public void updateItem(String appId, String env, String clusterName, String namespaceName,
OpenItemDTO itemDTO) {
- ItemDTO toUpdateItem = itemService
- .loadItem(Env.valueOf(env), appId, clusterName, namespaceName, itemDTO.getKey());
+ ItemDTO toUpdateItem = itemService
+ .loadItem(Env.valueOf(env), appId, clusterName, namespaceName, itemDTO.getKey());
+ if (toUpdateItem == null) {
+ throw com.ctrip.framework.apollo.common.exception.NotFoundException
+ .itemNotFound(appId, clusterName, namespaceName, itemDTO.getKey());
+ }
//protect. only value,type,comment,lastModifiedBy can be modified
toUpdateItem.setComment(itemDTO.getComment());
toUpdateItem.setType(itemDTO.getType());
toUpdateItem.setValue(itemDTO.getValue());
toUpdateItem.setDataChangeLastModifiedBy(itemDTO.getDataChangeLastModifiedBy());
itemService.updateItem(appId, Env.valueOf(env), clusterName, namespaceName, toUpdateItem);
}📝 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.
| @Override | |
| public void updateItem(String appId, String env, String clusterName, String namespaceName, | |
| OpenItemDTO itemDTO) { | |
| ItemDTO toUpdateItem = itemService | |
| .loadItem(Env.valueOf(env), appId, clusterName, namespaceName, itemDTO.getKey()); | |
| //protect. only value,type,comment,lastModifiedBy can be modified | |
| toUpdateItem.setComment(itemDTO.getComment()); | |
| toUpdateItem.setType(itemDTO.getType()); | |
| toUpdateItem.setValue(itemDTO.getValue()); | |
| toUpdateItem.setDataChangeLastModifiedBy(itemDTO.getDataChangeLastModifiedBy()); | |
| itemService.updateItem(appId, Env.valueOf(env), clusterName, namespaceName, toUpdateItem); | |
| } | |
| @Override | |
| public void updateItem(String appId, String env, String clusterName, String namespaceName, | |
| OpenItemDTO itemDTO) { | |
| ItemDTO toUpdateItem = itemService | |
| .loadItem(Env.valueOf(env), appId, clusterName, namespaceName, itemDTO.getKey()); | |
| if (toUpdateItem == null) { | |
| throw com.ctrip.framework.apollo.common.exception.NotFoundException | |
| .itemNotFound(appId, clusterName, namespaceName, itemDTO.getKey()); | |
| } | |
| //protect. only value,type,comment,lastModifiedBy can be modified | |
| toUpdateItem.setComment(itemDTO.getComment()); | |
| toUpdateItem.setType(itemDTO.getType()); | |
| toUpdateItem.setValue(itemDTO.getValue()); | |
| toUpdateItem.setDataChangeLastModifiedBy(itemDTO.getDataChangeLastModifiedBy()); | |
| itemService.updateItem(appId, Env.valueOf(env), clusterName, namespaceName, toUpdateItem); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerItemOpenApiService.java
around lines 88 to 100, the method updateItem calls setters on toUpdateItem
which may be null causing an NPE; add a null check after loading the item and if
toUpdateItem is null throw an appropriate NotFound (HTTP 404) exception with a
clear message (e.g., item not found for appId/cluster/namespace/key) so the API
returns 404 instead of causing a server NPE.
| @GetMapping(value = "/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/lock") | ||
| public OpenNamespaceLockDTO getNamespaceLock(@PathVariable String appId, @PathVariable String env, | ||
| @PathVariable String clusterName, @PathVariable | ||
| String namespaceName) { | ||
| return this.namespaceOpenApiService.getNamespaceLock(appId, env, clusterName, namespaceName); | ||
| public ResponseEntity<OpenNamespaceLockDTO> getNamespaceLock(@PathVariable String appId, @PathVariable String env, | ||
| @PathVariable String clusterName, | ||
| @PathVariable String namespaceName) { | ||
| OpenNamespaceLockDTO lock = this.namespaceOpenApiService.getNamespaceLock(appId, env, clusterName, namespaceName); |
There was a problem hiding this comment.
Path var should accept dots (.)
Without .+, namespaces containing '.' won’t match this endpoint.
Apply this diff:
- @GetMapping(value = "/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/lock")
+ @GetMapping(value = "/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName:.+}/lock")📝 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 = "/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/lock") | |
| public OpenNamespaceLockDTO getNamespaceLock(@PathVariable String appId, @PathVariable String env, | |
| @PathVariable String clusterName, @PathVariable | |
| String namespaceName) { | |
| return this.namespaceOpenApiService.getNamespaceLock(appId, env, clusterName, namespaceName); | |
| public ResponseEntity<OpenNamespaceLockDTO> getNamespaceLock(@PathVariable String appId, @PathVariable String env, | |
| @PathVariable String clusterName, | |
| @PathVariable String namespaceName) { | |
| OpenNamespaceLockDTO lock = this.namespaceOpenApiService.getNamespaceLock(appId, env, clusterName, namespaceName); | |
| @GetMapping(value = "/openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName:.+}/lock") | |
| public ResponseEntity<OpenNamespaceLockDTO> getNamespaceLock(@PathVariable String appId, @PathVariable String env, | |
| @PathVariable String clusterName, | |
| @PathVariable String namespaceName) { | |
| OpenNamespaceLockDTO lock = this.namespaceOpenApiService.getNamespaceLock(appId, env, clusterName, namespaceName); |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java
around lines 141 to 145, the @GetMapping path variable for namespaceName does
not allow dots so requests for namespaces containing '.' won't match; update the
mapping to use a regex pattern that accepts dots (e.g. change the path segment
to {namespaceName:.+}) and annotate the method parameter explicitly with
@PathVariable("namespaceName") to ensure Spring binds the full value including
dots.
| @GetMapping("/openapi/v1/apps/{appId}/appnamespaces/{namespaceName:.+}") | ||
| public ResponseEntity<OpenAppNamespaceDTO> getAppNamespace(@PathVariable String appId, @PathVariable String namespaceName) { | ||
| OpenAppNamespaceDTO appNamespace = appNamespaceOpenApiService.findByAppIdAndName(appId, namespaceName); | ||
|
|
||
| if (appNamespace == null) { | ||
| throw BadRequestException.appNamespaceNotExists(appId, namespaceName); | ||
| } | ||
|
|
||
| return ResponseEntity.ok(appNamespace); | ||
| } |
There was a problem hiding this comment.
Return 404 for not found instead of 400 Bad Request
Aligns with REST semantics and with other endpoints in this controller.
Apply this diff:
@GetMapping("/openapi/v1/apps/{appId}/appnamespaces/{namespaceName:.+}")
public ResponseEntity<OpenAppNamespaceDTO> getAppNamespace(@PathVariable String appId, @PathVariable String namespaceName) {
OpenAppNamespaceDTO appNamespace = appNamespaceOpenApiService.findByAppIdAndName(appId, namespaceName);
-
- if (appNamespace == null) {
- throw BadRequestException.appNamespaceNotExists(appId, namespaceName);
- }
-
- return ResponseEntity.ok(appNamespace);
+ if (appNamespace == null) {
+ return ResponseEntity.notFound().build();
+ }
+ return ResponseEntity.ok(appNamespace);
}📝 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("/openapi/v1/apps/{appId}/appnamespaces/{namespaceName:.+}") | |
| public ResponseEntity<OpenAppNamespaceDTO> getAppNamespace(@PathVariable String appId, @PathVariable String namespaceName) { | |
| OpenAppNamespaceDTO appNamespace = appNamespaceOpenApiService.findByAppIdAndName(appId, namespaceName); | |
| if (appNamespace == null) { | |
| throw BadRequestException.appNamespaceNotExists(appId, namespaceName); | |
| } | |
| return ResponseEntity.ok(appNamespace); | |
| } | |
| @GetMapping("/openapi/v1/apps/{appId}/appnamespaces/{namespaceName:.+}") | |
| public ResponseEntity<OpenAppNamespaceDTO> getAppNamespace(@PathVariable String appId, @PathVariable String namespaceName) { | |
| OpenAppNamespaceDTO appNamespace = appNamespaceOpenApiService.findByAppIdAndName(appId, namespaceName); | |
| if (appNamespace == null) { | |
| return ResponseEntity.notFound().build(); | |
| } | |
| return ResponseEntity.ok(appNamespace); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java
around lines 183 to 192, the handler currently throws
BadRequestException.appNamespaceNotExists when the namespace is not found;
change this to return a 404 Not Found instead. Replace the BadRequestException
throw with the appropriate NotFoundException (or construct/throw an existing
helper like NotFoundException.appNamespaceNotExists) or return
ResponseEntity.notFound().build() so the endpoint responds with HTTP 404
consistent with other controller methods.
| @GetMapping("/openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName}/instances") | ||
| public ResponseEntity<List<OpenNamespaceDTO>> getPublicAppNamespaceInstances(@PathVariable String env, | ||
| @PathVariable String publicNamespaceName, | ||
| @RequestParam(name = "page", defaultValue = "0") int page, | ||
| @RequestParam(name = "size", defaultValue = "10") int size) { | ||
| List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(Env.valueOf(env), publicNamespaceName, page, size) | ||
| .stream() |
There was a problem hiding this comment.
Accept dots in publicNamespaceName and validate env/pagination
Add .+ to handle dots; add basic page/size validation; convert env safely.
Apply this diff:
- @GetMapping("/openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName}/instances")
+ @GetMapping("/openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName:.+}/instances")
public ResponseEntity<List<OpenNamespaceDTO>> getPublicAppNamespaceInstances(@PathVariable String env,
@PathVariable String publicNamespaceName,
@RequestParam(name = "page", defaultValue = "0") int page,
@RequestParam(name = "size", defaultValue = "10") int size) {
- List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(Env.valueOf(env), publicNamespaceName, page, size)
+ RequestPrecondition.checkArguments(page >= 0, "page must be >= 0");
+ RequestPrecondition.checkArguments(size > 0 && size <= 500, "size must be in (0, 500]");
+ final Env envObj;
+ try {
+ envObj = Env.valueOf(env);
+ } catch (IllegalArgumentException ex) {
+ throw BadRequestException.invalidEnvFormat(env);
+ }
+ List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(envObj, publicNamespaceName, page, size)
.stream()
.map(OpenApiBeanUtils::transformFromNamespaceDTO)
.collect(Collectors.toList());
return ResponseEntity.ok(instances);
}📝 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("/openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName}/instances") | |
| public ResponseEntity<List<OpenNamespaceDTO>> getPublicAppNamespaceInstances(@PathVariable String env, | |
| @PathVariable String publicNamespaceName, | |
| @RequestParam(name = "page", defaultValue = "0") int page, | |
| @RequestParam(name = "size", defaultValue = "10") int size) { | |
| List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(Env.valueOf(env), publicNamespaceName, page, size) | |
| .stream() | |
| @GetMapping("/openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName:.+}/instances") | |
| public ResponseEntity<List<OpenNamespaceDTO>> getPublicAppNamespaceInstances(@PathVariable String env, | |
| @PathVariable String publicNamespaceName, | |
| @RequestParam(name = "page", defaultValue = "0") int page, | |
| @RequestParam(name = "size", defaultValue = "10") int size) { | |
| RequestPrecondition.checkArguments(page >= 0, "page must be >= 0"); | |
| RequestPrecondition.checkArguments(size > 0 && size <= 500, "size must be in (0, 500]"); | |
| final Env envObj; | |
| try { | |
| envObj = Env.valueOf(env); | |
| } catch (IllegalArgumentException ex) { | |
| throw BadRequestException.invalidEnvFormat(env); | |
| } | |
| List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(envObj, publicNamespaceName, page, size) | |
| .stream() | |
| .map(OpenApiBeanUtils::transformFromNamespaceDTO) | |
| .collect(Collectors.toList()); | |
| return ResponseEntity.ok(instances); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java
around lines 227-233, update the @GetMapping path to allow dots in
publicNamespaceName by changing the placeholder to {publicNamespaceName:.+};
validate request params by rejecting negative page or size and capping size
(e.g. enforce size>0, page>=0 and max size constant) and return 400 on invalid
values; convert env safely by wrapping Env.valueOf(env) in try/catch (or use a
safe parser) and return a 400 Bad Request when env is invalid; keep the rest of
the implementation (calling namespaceService) intact after these validations.
| @GetMapping("/openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/public-association") | ||
| public ResponseEntity<OpenNamespaceDTO> getPublicNamespaceAssociation(@PathVariable String appId, |
There was a problem hiding this comment.
Accept dots in namespaceName for association endpoint
Consistency with other endpoints; avoids 404 for names containing '.'.
Apply this diff:
- @GetMapping("/openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/public-association")
+ @GetMapping("/openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName:.+}/public-association")📝 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("/openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/public-association") | |
| public ResponseEntity<OpenNamespaceDTO> getPublicNamespaceAssociation(@PathVariable String appId, | |
| @GetMapping("/openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName:.+}/public-association") | |
| public ResponseEntity<OpenNamespaceDTO> getPublicNamespaceAssociation(@PathVariable String appId, |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java
around lines 243-244, the @GetMapping path variable for namespaceName doesn't
accept dots and will 404 for names with '.'; change the mapping segment from
{namespaceName} to a regex that allows dots (e.g. {namespaceName:.+}) so the
PathVariable will capture names containing '.' and keep the method signature
unchanged.
| private Set<String> findMissingNamespaceNames(String appId, String env, String clusterName) { | ||
| // 获取配置数据库中的Namespace列表 | ||
| List<OpenNamespaceDTO> configDbNamespaces = namespaceService.findNamespaces(appId, Env.valueOf(env), clusterName) | ||
| .stream() | ||
| .map(OpenApiBeanUtils::transformFromNamespaceDTO) | ||
| .collect(Collectors.toList()); | ||
| List<AppNamespace> portalDbAppNamespaces = appNamespaceService.findByAppId(appId); | ||
|
|
||
| Set<String> configDbNamespaceNames = configDbNamespaces.stream() | ||
| .map(OpenNamespaceDTO::getNamespaceName) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| Set<String> portalDbPrivateAppNamespaceNames = Sets.newHashSet(); | ||
|
|
||
| for (AppNamespace appNamespace : portalDbAppNamespaces) { | ||
| if (!appNamespace.isPublic()) { | ||
| portalDbPrivateAppNamespaceNames.add(appNamespace.getName()); | ||
| } | ||
| } | ||
|
|
||
| // 私有namespace应该全部存在 | ||
| return Sets.difference(portalDbPrivateAppNamespaceNames, configDbNamespaceNames); | ||
| } |
There was a problem hiding this comment.
Integrity check misses config-DB AppNamespace gaps; align with portal logic
Current logic only checks “portal private app namespaces – config namespaces”. It omits “portal app namespaces – config app namespaces” (i.e., missing AppNamespace records in config DB). It also does an extra transform just to extract names.
Suggested approach:
- Fetch config DB app namespaces for the app in env.
- Compute:
- missingAppNamespaceNames = (portalDbAllAppNamespaceNames - configDbAppNamespaceNames)
- missingNamespaceNames = (portalDbPrivateAppNamespaceNames - configDbNamespaceNames)
- Return union of both sets.
- Optional: avoid transforming NamespaceDTO → OpenNamespaceDTO just to read names.
Example sketch (service calls may differ; adjust to your interfaces):
Set<String> configDbAppNamespaceNames = namespaceOpenApiService
.getAppNamespaces(appId, Env.valueOf(env)) // or a dedicated service
.stream().map(OpenAppNamespaceDTO::getName).collect(toSet());
Set<String> portalDbAllAppNamespaceNames = appNamespaceService.findByAppId(appId)
.stream().map(AppNamespace::getName).collect(toSet());
Set<String> missingAppNamespaceNames = Sets.difference(portalDbAllAppNamespaceNames, configDbAppNamespaceNames);
Set<String> configDbNamespaceNames = namespaceService.findNamespaces(appId, Env.valueOf(env), clusterName)
.stream().map(NamespaceDTO::getNamespaceName).collect(toSet()); // use DTO directly if available
Set<String> portalDbPrivateAppNamespaceNames = appNamespaceService.findByAppId(appId).stream()
.filter(ns -> !ns.isPublic()).map(AppNamespace::getName).collect(toSet());
return Sets.union(missingAppNamespaceNames, Sets.difference(portalDbPrivateAppNamespaceNames, configDbNamespaceNames));🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java
around lines 290 to 312, the current method only computes portal private app
namespaces missing in config DB and also performs an unnecessary DTO transform
just to read names; update it to additionally fetch config-DB AppNamespace names
for the app in the given env, compute missingAppNamespaceNames =
portalAllAppNamespaceNames - configDbAppNamespaceNames, compute
missingNamespaceNames = portalPrivateAppNamespaceNames - configDbNamespaceNames
(use NamespaceDTO.getNamespaceName or NamespaceDTO directly to avoid the extra
OpenNamespaceDTO transform), then return the union of missingAppNamespaceNames
and missingNamespaceNames; remove the redundant transform and reuse
appNamespaceService.findByAppId(appId) results to derive
portalAllAppNamespaceNames and portalPrivateAppNamespaceNames.
tacklequestions
force-pushed
the
openapi-controllers/appcontroller
branch
from
9b0a2fc to
8b7f807
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (50)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationControllerTest.java (1)
54-55: Optionally assert no other interactions with the serviceAdds a small guard against unintended calls.
- verify(organizationOpenApiService, times(1)).getOrganizations(); + verify(organizationOpenApiService, times(1)).getOrganizations(); + verifyNoMoreInteractions(organizationOpenApiService);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java (3)
48-52: Unify response type; consider a clearer REST path for “count”.Returning a bare int breaks the otherwise consistent ResponseEntity-based responses. Also, a path ending in “/instances” typically implies listing, not counting.
Apply this diff to wrap the response; if the OpenAPI spec allows, consider renaming the path to “.../instances/count” to reflect semantics.
- public int getInstanceCountByNamespace(@PathVariable String appId, @PathVariable String env, - @PathVariable String clusterName, @PathVariable String namespaceName) { - return this.instanceOpenApiService.getInstanceCountByNamespace(appId, env, clusterName, namespaceName); - } + public ResponseEntity<Integer> getInstanceCountByNamespace(@PathVariable String appId, @PathVariable String env, + @PathVariable String clusterName, @PathVariable String namespaceName) { + int count = this.instanceOpenApiService.getInstanceCountByNamespace(appId, env, clusterName, namespaceName); + return ResponseEntity.ok(count); + }Please confirm whether the OpenAPI spec mandates the current URL; if not, consider “/instances/count”.
58-65: Add pagination guardrails (bounds check).page/size are unvalidated; negative or huge values will stress the service.
Apply this diff to enforce sane bounds (adjust max as needed):
public ResponseEntity<OpenPageDTOOpenInstanceDTO> getInstancesByRelease(@PathVariable String env, @PathVariable Long releaseId, @RequestParam(defaultValue = "0") int page, @RequestParam(defaultValue = "20") int size) { + if (page < 0 || size <= 0 || size > 500) { + throw new BadRequestException("Invalid pagination params: page must be >= 0 and 1 <= size <= 500"); + } OpenPageDTOOpenInstanceDTO instances = instanceOpenApiService.getByRelease(env, releaseId, page, size); return ResponseEntity.ok(instances); }If you prefer consistency with the rest of the portal, consider switching to Pageable, as used in AppController.getAppsBySelf.
71-81: Rename “instances_search”; add pagination guardrails.
- The suffix “_search” is non‑RESTful and inconsistent with the refactor goals. Prefer “…/instances” with filters via query params (e.g., instanceAppId).
- Add bounds checks for page/size.
Apply this diff for pagination validation; consider path rename if the OpenAPI spec permits.
@GetMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/instances_search") public ResponseEntity<OpenPageDTOOpenInstanceDTO> getInstancesByNamespace(@PathVariable String env, @PathVariable String appId, @PathVariable String clusterName, @PathVariable String namespaceName, @RequestParam(required = false) String instanceAppId, @RequestParam(defaultValue = "0") int page, @RequestParam(defaultValue = "20") int size) { + if (page < 0 || size <= 0 || size > 500) { + throw new BadRequestException("Invalid pagination params: page must be >= 0 and 1 <= size <= 500"); + } OpenPageDTOOpenInstanceDTO instances = instanceOpenApiService.getByNamespace(env, appId, clusterName, namespaceName, instanceAppId, page, size); return ResponseEntity.ok(instances); }Please confirm whether the current path name is dictated by the OpenAPI contract.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java (2)
105-107: Missing @OverRide annotation on public interface methods.Methods
findAllReleasesandfindActiveReleasesappear to be implementing interface methods but lack@Overrideannotations. This makes the API contract unclear.Add
@Overrideannotations to interface implementation methods:+ @Override public List<OpenReleaseBO> findAllReleases(String appId, String env, String clusterName, String namespaceName, int page, int size) {+ @Override public List<OpenReleaseDTO> findActiveReleases(String appId, String env, String clusterName, String namespaceName, int page, int size) {
119-124: Consider using method references for cleaner stream operations.The stream operation can be simplified using method references for better readability.
Apply this diff to use method references:
List<OpenReleaseDTO> releases = serverReleaseOpenApiService.findActiveReleases(appId, env, clusterName, namespaceName, page, size); return releases.stream() - .map(OpenApiBeanUtils::transformFromReleaseDTO) + .map(OpenApiBeanUtils::transformFromReleaseDTO) .collect(java.util.stream.Collectors.toList());Actually, the current code already uses method references correctly. The import can be optimized though:
- import java.util.stream.Collectors; return releases.stream() .map(OpenApiBeanUtils::transformFromReleaseDTO) - .collect(java.util.stream.Collectors.toList()); + .collect(Collectors.toList());apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (1)
58-60: Replace Chinese comments with English for international codebase consistency.All comments should be in English to maintain consistency across the codebase.
/** - * 创建应用 - * POST /openapi/v1/apps + * Create application + * POST /openapi/v1/apps */apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java (15)
62-65: Translate Chinese comments to English./** - * 获取单个配置项 - * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} + * Get single configuration item + * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} */
76-79: Translate Chinese comments to English./** - * 通过查询参数获取配置项（支持编码的key） - * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items?key={key}&encoded={true|false} + * Get configuration item through encoded key + * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key} */
88-91: Translate Chinese comments to English./** - * 创建新的配置项 - * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items + * Create new configuration item + * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items */
116-119: Translate Chinese comments to English./** - * 更新配置项 - * PUT openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} + * Update configuration item + * PUT openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} */
155-158: Translate Chinese comments to English./** - * 通过编码的key更新配置项 - * PUT openapi/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key} + * Update configuration item through encoded key + * PUT openapi/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key} */
170-173: Translate Chinese comments to English./** - * 删除配置项 - * DELETE openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} + * Delete configuration item + * DELETE openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key} */
189-192: Translate Chinese comments to English./** - * 通过编码的key删除配置项 - * DELETE openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key} + * Delete configuration item through encoded key + * DELETE openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/encodedItems/{key} */
202-205: Translate Chinese comments to English./** - * 通过namespace查询item - * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items + * Query items by namespace + * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items */
216-219: Translate Chinese comments to English./** - * 通过文本批量修改配置项 - * PUT openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:batchUpdate + * Batch update items through text + * PUT openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:batchUpdate */
229-232: Translate Chinese comments to English./** - * 获取分支下的配置项 - * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/items + * Get items under branch + * GET openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/items */
241-244: Translate Chinese comments to English./** - * 对比命名空间配置差异 - * POST openapi/v1/namespaces/items:compare + * Compare namespace configuration differences + * POST openapi/v1/namespaces/items:compare */
267-270: Translate Chinese comments to English./** - * 同步配置项到多个命名空间 - * POST openapi/v1/apps/{appId}/namespaces/{namespaceName}/items:sync + * Sync items to multiple namespaces + * POST openapi/v1/apps/{appId}/namespaces/{namespaceName}/items:sync */
276-281: Translate Chinese inline comments to English.OpenNamespaceIdentifier noPermissionNamespace = null; - // 检查用户是否拥有每个命名空间的修改权限 + // Check if user has modify permission for each namespace boolean hasPermission = true; for (OpenNamespaceIdentifier namespaceIdentifier : model.getSyncToNamespaces()) { - // 一旦用户没有其中一个命名空间的修改权限，就中断循环 + // Break the loop once user lacks modify permission for any namespace hasPermission = consumerPermissionValidator.hasModifyNamespacePermission(
301-304: Translate Chinese comments to English./** - * 验证配置文本语法 - * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:validate + * Validate configuration text syntax + * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:validate */
316-319: Translate Chinese comments to English./** - * 撤销配置项更改 - * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:revert + * Revert item changes + * POST openapi/v1/apps/{appId}/env/{env}/clusters/{clusterName}/namespaces/{namespaceName}/items:revert */apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (17)
75-78: Translate Chinese comments to English./** - * 创建AppNamespace - * POST /openapi/v1/apps/{appId}/appnamespaces + * Create AppNamespace + * POST /openapi/v1/apps/{appId}/appnamespaces */
110-113: Translate Chinese comments to English./** - * 获取指定集群下的所有Namespace - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces + * Get all namespaces under specified cluster + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces */
122-125: Translate Chinese comments to English./** - * 获取指定的Namespace - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName} + * Get specified namespace + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName} */
137-140: Translate Chinese comments to English./** - * 获取Namespace的锁状态 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/lock + * Get namespace lock status + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/lock */
152-157: Translate Chinese comments to English.- // ======================= AppNamespace资源管理 ======================= + // ======================= AppNamespace Resource Management ======================= /** - * 获取所有公共AppNamespace - * GET /openapi/v1/appnamespaces?public=true + * Get all public AppNamespaces + * GET /openapi/v1/appnamespaces?public=true */
169-172: Translate Chinese comments to English./** - * 获取指定应用的AppNamespace - * GET /openapi/v1/apps/{appId}/appnamespaces + * Get AppNamespaces for specified application + * GET /openapi/v1/apps/{appId}/appnamespaces */
179-182: Translate Chinese comments to English./** - * 获取指定的AppNamespace - * GET /openapi/v1/apps/{appId}/appnamespaces/{namespaceName} + * Get specified AppNamespace + * GET /openapi/v1/apps/{appId}/appnamespaces/{namespaceName} */
194-197: Translate Chinese comments to English./** - * 删除AppNamespace - * DELETE /openapi/v1/apps/{appId}/appnamespaces/{namespaceName} + * Delete AppNamespace + * DELETE /openapi/v1/apps/{appId}/appnamespaces/{namespaceName} */
211-216: Translate Chinese comments to English.- // ======================= 发布状态管理 ======================= + // ======================= Release Status Management ======================= /** - * 获取应用下所有Namespace的发布状态 - * GET /openapi/v1/apps/{appId}/namespaces/releases/status + * Get release status for all namespaces under application + * GET /openapi/v1/apps/{appId}/namespaces/releases/status */
223-226: Translate Chinese comments to English./** - * 获取公共AppNamespace的所有实例 - * GET /openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName}/instances + * Get all instances of public AppNamespace + * GET /openapi/v1/envs/{env}/appnamespaces/{publicNamespaceName}/instances */
228-237: Add validation for pagination parameters and safe env conversion.public ResponseEntity<List<OpenNamespaceDTO>> getPublicAppNamespaceInstances(@PathVariable String env, @PathVariable String publicNamespaceName, @RequestParam(name = "page", defaultValue = "0") int page, @RequestParam(name = "size", defaultValue = "10") int size) { + RequestPrecondition.checkArguments(page >= 0, "page must be >= 0"); + RequestPrecondition.checkArguments(size > 0 && size <= 500, "size must be in (0, 500]"); + final Env envObj; + try { + envObj = Env.valueOf(env); + } catch (IllegalArgumentException ex) { + throw BadRequestException.invalidEnvFormat(env); + } - List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(Env.valueOf(env), publicNamespaceName, page, size) + List<OpenNamespaceDTO> instances = namespaceService.getPublicAppNamespaceAllNamespaces(envObj, publicNamespaceName, page, size) .stream() .map(OpenApiBeanUtils::transformFromNamespaceDTO) .collect(Collectors.toList()); return ResponseEntity.ok(instances); }
239-242: Translate Chinese comments to English./** - * 获取关联的公共Namespace - * GET /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/public-association + * Get associated public namespace + * GET /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/public-association */
256-261: Translate Chinese comments to English.- // ======================= Namespace完整性检查和修复 ======================= + // ======================= Namespace Integrity Check and Repair ======================= /** - * 检查缺失的Namespace - * GET /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/integrity-check + * Check missing namespaces + * GET /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/integrity-check */
271-274: Translate Chinese comments to English./** - * 删除关联的Namespace - * DELETE /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/links + * Delete associated namespace + * DELETE /openapi/v1/apps/{appId}/envs/{env}/clusters/{clusterName}/namespaces/{namespaceName}/links */
285-289: Translate Chinese comments to English.- // ======================= 私有辅助方法 ======================= + // ======================= Private Helper Methods ======================= /** - * 查找缺失的Namespace名称 + * Find missing namespace names */
291-292: Remove unnecessary transformation to extract namespace names.Directly use NamespaceDTO.getNamespaceName() instead of transforming to OpenNamespaceDTO.
- // 获取配置数据库中的Namespace列表 - List<OpenNamespaceDTO> configDbNamespaces = namespaceService.findNamespaces(appId, Env.valueOf(env), clusterName) - .stream() - .map(OpenApiBeanUtils::transformFromNamespaceDTO) - .collect(Collectors.toList()); + // Get namespace list from config database + List<com.ctrip.framework.apollo.common.dto.NamespaceDTO> configDbNamespaces = namespaceService.findNamespaces(appId, Env.valueOf(env), clusterName); List<AppNamespace> portalDbAppNamespaces = appNamespaceService.findByAppId(appId); Set<String> configDbNamespaceNames = configDbNamespaces.stream() - .map(OpenNamespaceDTO::getNamespaceName) + .map(com.ctrip.framework.apollo.common.dto.NamespaceDTO::getNamespaceName) .collect(Collectors.toSet());Also applies to: 298-300
310-311: Fix Chinese comment.- // 私有namespace应该全部存在 + // Private namespaces should all existapollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (4)
49-52: Translate Chinese comments to English./** - * 获取指定集群信息 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} + * Get specified cluster information + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} */
62-65: Translate Chinese comments to English./** - * 创建集群 - * POST /openapi/v1/envs/{env}/apps/{appId}/clusters + * Create cluster + * POST /openapi/v1/envs/{env}/apps/{appId}/clusters */
95-98: Translate Chinese comments to English./** - * 删除集群 - * DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} + * Delete cluster + * DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName} */
66-93: Consider returning 201 Created for createCluster endpoint.For RESTful consistency, resource creation endpoints should return 201 Created rather than 200 OK.
@PreAuthorize(value = "@consumerPermissionValidator.hasCreateClusterPermission(#appId)") @PostMapping(value = "/apps/{appId}/clusters") public ResponseEntity<OpenClusterDTO> createCluster(@PathVariable String env, @PathVariable String appId, @Valid @RequestBody OpenClusterDTO cluster) { // ... validation code ... OpenClusterDTO createdCluster = this.clusterOpenApiService.createCluster(env, cluster); - return ResponseEntity.ok(createdCluster); + return ResponseEntity.status(HttpStatus.CREATED).body(createdCluster); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (7)
78-81: Translate Chinese comments to English./** - * 获取命名空间分支信息 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches + * Get namespace branch information + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches */
94-98: Translate Chinese comments to English./** - * 创建命名空间分支 - * POST /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches - * 原先operator是必选，现在从authorization可以获取，为了保持兼容性留下但改为非必须 + * Create namespace branch + * POST /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches + * Previously operator was required, now it can be obtained from authorization. Kept as optional for compatibility */
122-125: Translate Chinese comments to English./** - * 删除命名空间分支 - * DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName} + * Delete namespace branch + * DELETE /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName} */
156-160: Translate Chinese comments to English./** - * 合并分支到主分支 - * PATCH /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName} - * 使用 PATCH 方法表示部分更新操作（将分支状态从"独立"更新为"合并"） + * Merge branch to main branch + * PATCH /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName} + * Using PATCH method to indicate partial update operation (updating branch state from "independent" to "merged") */
195-198: Translate Chinese comments to English./** - * 获取分支灰度发布规则 - * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/rules + * Get branch gray release rules + * GET /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/rules */
212-215: Translate Chinese comments to English./** - * 更新分支灰度发布规则 - * PUT /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/rules + * Update branch gray release rules + * PUT /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/rules */
87-92: Consider safe conversion for environment name.The code uses
Env.valueOf(env.toUpperCase())which can throw IllegalArgumentException if the environment name is invalid. Consider using the saferEnv.transformEnv()method or adding proper error handling.- NamespaceBO namespaceBO = namespaceBranchService.findBranch(appId, Env.valueOf(env.toUpperCase()), clusterName, namespaceName); + Env envObj; + try { + envObj = Env.valueOf(env.toUpperCase()); + } catch (IllegalArgumentException e) { + throw BadRequestException.invalidEnvFormat(env); + } + NamespaceBO namespaceBO = namespaceBranchService.findBranch(appId, envObj, clusterName, namespaceName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
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/ItemOpenApiService.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/server/service/impl/ServerItemOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java(6 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/openapi/v1/controller/ClusterController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java(7 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)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchControllerTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ItemOpenApiService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceControllerTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseControllerTest.java
🧰 Additional context used
🧬 Code graph analysis (15)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationControllerTest.java (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java (1)
RunWith(45-150)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java (1)
RunWith(58-321)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java (6)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/NotFoundException.java (1)
NotFoundException(21-73)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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceReleaseModel.java (1)
NamespaceReleaseModel(23-102)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerItemOpenApiService.java (1)
Service(51-304)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerTest.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/v1/controller/ReleaseController.java (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceGrayDelReleaseModel.java (1)
NamespaceGrayDelReleaseModel(21-31)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/model/NamespaceReleaseModel.java (1)
NamespaceReleaseModel(23-102)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/ConfigPublishEvent.java (1)
ConfigPublishEvent(23-185)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (1)
Validated(49-214)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java (1)
Validated(36-65)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java (9)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/utils/InputValidator.java (1)
InputValidator(26-47)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/environment/Env.java (1)
Env(34-237)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterControllerTest.java (1)
RunWith(45-150)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemControllerTest.java (1)
RunWith(44-366)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchControllerTest.java (1)
RunWith(45-227)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/OrganizationControllerTest.java (1)
RunWith(36-58)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseControllerTest.java (1)
RunWith(53-255)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java (1)
RunWith(57-330)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerAppOpenApiService.java (5)
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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)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/v1/controller/ItemController.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/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(35-109)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/server/service/impl/ServerItemOpenApiService.java (6)
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/dto/PageDTO.java (1)
PageDTO(27-59)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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)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/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (6)
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-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/util/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)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/ConfigPublishEvent.java (1)
ConfigPublishEvent(23-185)
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/util/OpenApiBeanUtils.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/InstanceController.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/openapi/v1/controller/AppController.java (1)
RestController(40-220)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (5)
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/OpenApiBeanUtils.java (1)
OpenApiBeanUtils(42-445)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/environment/Env.java (1)
Env(34-237)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/listener/AppNamespaceDeletionEvent.java (1)
AppNamespaceDeletionEvent(23-33)
| public void rollbackRelease(String env, long releaseId, String operator) { | ||
| operator = userInfoHolder.getUser().getName(); | ||
| releaseService.rollback(Env.valueOf(env), releaseId, operator); |
There was a problem hiding this comment.
Potential issue: Operator parameter ignored.
The operator parameter is immediately overwritten with userInfoHolder.getUser().getName(). This suggests the parameter should either be removed from the interface, or there should be logic to use the provided operator when available.
Apply this fix to respect the provided operator parameter:
@Override
public void rollbackRelease(String env, long releaseId, String operator) {
- operator = userInfoHolder.getUser().getName();
+ if (operator == null || operator.trim().isEmpty()) {
+ operator = userInfoHolder.getUser().getName();
+ }
releaseService.rollback(Env.valueOf(env), releaseId, 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 rollbackRelease(String env, long releaseId, String operator) { | |
| operator = userInfoHolder.getUser().getName(); | |
| releaseService.rollback(Env.valueOf(env), releaseId, operator); | |
| @Override | |
| public void rollbackRelease(String env, long releaseId, String operator) { | |
| if (operator == null || operator.trim().isEmpty()) { | |
| operator = userInfoHolder.getUser().getName(); | |
| } | |
| releaseService.rollback(Env.valueOf(env), releaseId, operator); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/impl/ServerReleaseOpenApiService.java
around lines 75–77, the method unconditionally overwrites the incoming operator
parameter with userInfoHolder.getUser().getName(); change this to respect a
provided operator by using the incoming operator when it is non-null and not
blank, and only fall back to userInfoHolder.getUser().getName() when the
parameter is null/empty (trim before checking); then pass that chosen operator
into releaseService.rollback(Env.valueOf(env), releaseId, operator).
| @GetMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/instances_not_in", | ||
| params = "excludeReleases") | ||
| public ResponseEntity<List<OpenInstanceDTO>> getInstancesExcludingReleases(@PathVariable String env, | ||
| @PathVariable String appId, | ||
| @PathVariable String clusterName, | ||
| @PathVariable String namespaceName, | ||
| @RequestParam("excludeReleases") String releaseIds) { | ||
| Set<Long> releaseIdSet = RELEASES_SPLITTER.splitToList(releaseIds).stream() | ||
| .map(Long::parseLong) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| if (CollectionUtils.isEmpty(releaseIdSet)) { | ||
| throw new BadRequestException("excludeReleases parameter cannot be empty"); | ||
| } | ||
|
|
||
| List<OpenInstanceDTO> instances = instanceOpenApiService.getByReleasesNotIn(env, appId, clusterName, namespaceName, releaseIdSet); | ||
| return ResponseEntity.ok(instances); | ||
| } |
There was a problem hiding this comment.
Guard against NumberFormatException; tighten empty check; consider output size limits.
Parsing excludeReleases will 500 on non-numeric input. Convert invalid input into 400 Bad Request and avoid the indirect null/empty check.
Apply this diff:
- Set<Long> releaseIdSet = RELEASES_SPLITTER.splitToList(releaseIds).stream()
- .map(Long::parseLong)
- .collect(Collectors.toSet());
-
- if (CollectionUtils.isEmpty(releaseIdSet)) {
- throw new BadRequestException("excludeReleases parameter cannot be empty");
- }
+ Set<Long> releaseIdSet;
+ try {
+ releaseIdSet = RELEASES_SPLITTER.splitToList(releaseIds).stream()
+ .map(Long::parseLong)
+ .collect(Collectors.toSet());
+ } catch (NumberFormatException e) {
+ throw new BadRequestException("excludeReleases must be a comma-separated list of numeric ids");
+ }
+
+ if (releaseIdSet.isEmpty()) {
+ throw new BadRequestException("excludeReleases parameter cannot be empty");
+ }Optional: This endpoint returns an unpaged List, which can be very large. Consider adding pagination or a “limit” param with a sane maximum.
📝 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}/namespaces/{namespaceName}/instances_not_in", | |
| params = "excludeReleases") | |
| public ResponseEntity<List<OpenInstanceDTO>> getInstancesExcludingReleases(@PathVariable String env, | |
| @PathVariable String appId, | |
| @PathVariable String clusterName, | |
| @PathVariable String namespaceName, | |
| @RequestParam("excludeReleases") String releaseIds) { | |
| Set<Long> releaseIdSet = RELEASES_SPLITTER.splitToList(releaseIds).stream() | |
| .map(Long::parseLong) | |
| .collect(Collectors.toSet()); | |
| if (CollectionUtils.isEmpty(releaseIdSet)) { | |
| throw new BadRequestException("excludeReleases parameter cannot be empty"); | |
| } | |
| List<OpenInstanceDTO> instances = instanceOpenApiService.getByReleasesNotIn(env, appId, clusterName, namespaceName, releaseIdSet); | |
| return ResponseEntity.ok(instances); | |
| } | |
| @GetMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/instances_not_in", | |
| params = "excludeReleases") | |
| public ResponseEntity<List<OpenInstanceDTO>> getInstancesExcludingReleases(@PathVariable String env, | |
| @PathVariable String appId, | |
| @PathVariable String clusterName, | |
| @PathVariable String namespaceName, | |
| @RequestParam("excludeReleases") String releaseIds) { | |
| Set<Long> releaseIdSet; | |
| try { | |
| releaseIdSet = RELEASES_SPLITTER.splitToList(releaseIds).stream() | |
| .map(Long::parseLong) | |
| .collect(Collectors.toSet()); | |
| } catch (NumberFormatException e) { | |
| throw new BadRequestException("excludeReleases must be a comma-separated list of numeric ids"); | |
| } | |
| if (releaseIdSet.isEmpty()) { | |
| throw new BadRequestException("excludeReleases parameter cannot be empty"); | |
| } | |
| List<OpenInstanceDTO> instances = instanceOpenApiService.getByReleasesNotIn(env, appId, clusterName, namespaceName, releaseIdSet); | |
| return ResponseEntity.ok(instances); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/InstanceController.java
around lines 87 to 104, tighten validation on the excludeReleases parameter and
guard against NumberFormatException by first verifying the raw request param is
not null/empty, splitting it, then parsing each token inside a try/catch (or
using a safe parse utility) and throwing a BadRequestException with a clear
message when any token is non-numeric; after successful parsing require the
resulting Set<Long> to be non-empty before proceeding; additionally (optional)
enforce a sane maximum on the number of release IDs (or add a limit param) to
avoid returning or processing excessively large result sets.
| private boolean checkNamespaceSyncModel(OpenNamespaceSyncModel namespaceSyncModel) { | ||
| if (CollectionUtils.isEmpty(namespaceSyncModel.getSyncToNamespaces()) || CollectionUtils.isEmpty(namespaceSyncModel.getSyncItems())) { | ||
| return false; | ||
| } | ||
| for (OpenNamespaceIdentifier namespaceIdentifier : namespaceSyncModel.getSyncToNamespaces()) { | ||
| if (checkNamespaceIdentifier(namespaceIdentifier)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix logic error in checkNamespaceSyncModel method.
The logic in line 334-336 is inverted - when checkNamespaceIdentifier returns true (which currently means empty fields), the method returns false. This is backwards.
private boolean checkNamespaceSyncModel(OpenNamespaceSyncModel namespaceSyncModel) {
if (CollectionUtils.isEmpty(namespaceSyncModel.getSyncToNamespaces()) || CollectionUtils.isEmpty(namespaceSyncModel.getSyncItems())) {
return false;
}
for (OpenNamespaceIdentifier namespaceIdentifier : namespaceSyncModel.getSyncToNamespaces()) {
- if (checkNamespaceIdentifier(namespaceIdentifier)) {
+ if (!checkNamespaceIdentifier(namespaceIdentifier)) {
return false;
}
}
return true;
}📝 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.
| private boolean checkNamespaceSyncModel(OpenNamespaceSyncModel namespaceSyncModel) { | |
| if (CollectionUtils.isEmpty(namespaceSyncModel.getSyncToNamespaces()) || CollectionUtils.isEmpty(namespaceSyncModel.getSyncItems())) { | |
| return false; | |
| } | |
| for (OpenNamespaceIdentifier namespaceIdentifier : namespaceSyncModel.getSyncToNamespaces()) { | |
| if (checkNamespaceIdentifier(namespaceIdentifier)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| private boolean checkNamespaceSyncModel(OpenNamespaceSyncModel namespaceSyncModel) { | |
| if (CollectionUtils.isEmpty(namespaceSyncModel.getSyncToNamespaces()) || CollectionUtils.isEmpty(namespaceSyncModel.getSyncItems())) { | |
| return false; | |
| } | |
| for (OpenNamespaceIdentifier namespaceIdentifier : namespaceSyncModel.getSyncToNamespaces()) { | |
| if (!checkNamespaceIdentifier(namespaceIdentifier)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java
around lines 329-339, the for-loop condition is inverted: currently it returns
false when checkNamespaceIdentifier(...) returns true, but
checkNamespaceIdentifier(...) actually signals an invalid/empty identifier when
it returns true; flip the condition to return false only when the identifier is
invalid (i.e., negate the call) so the method rejects invalid identifiers and
only returns true when all identifiers are valid.
| private boolean checkNamespaceIdentifier(OpenNamespaceIdentifier namespaceIdentifier) { | ||
| return StringUtils.isContainEmpty(namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(), namespaceIdentifier.getClusterName()); | ||
| } |
There was a problem hiding this comment.
Fix logic error in namespace identifier validation.
The method checkNamespaceIdentifier is checking the wrong field (clusterName twice instead of namespaceName), and the logic is inverted - it returns true when fields are empty, but the caller expects false for invalid data.
private boolean checkNamespaceIdentifier(OpenNamespaceIdentifier namespaceIdentifier) {
- return StringUtils.isContainEmpty(namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(), namespaceIdentifier.getClusterName());
+ return !StringUtils.isContainEmpty(namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(), namespaceIdentifier.getNamespaceName());
}📝 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.
| private boolean checkNamespaceIdentifier(OpenNamespaceIdentifier namespaceIdentifier) { | |
| return StringUtils.isContainEmpty(namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(), namespaceIdentifier.getClusterName()); | |
| } | |
| private boolean checkNamespaceIdentifier(OpenNamespaceIdentifier namespaceIdentifier) { | |
| return !StringUtils.isContainEmpty(namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(), namespaceIdentifier.getNamespaceName()); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java
around lines 341-343, the validation wrongly checks clusterName twice and
returns true when fields are empty; change the checked fields to
namespaceIdentifier.getEnv(), namespaceIdentifier.getClusterName(),
namespaceIdentifier.getNamespaceName() and invert the result so the method
returns false if any of those are empty (i.e., return true only when none are
empty).
…tests of all controllers controllers: migrate App/Cluster/Instance/Item/Namespace(Branch)/Release to OpenAPI-generated models; unify request/response mapping, validation, and error handling. services: adjust AppOpenApiService and ItemOpenApiService signatures; update ServerApp/Item/Release implementations; fix edge cases found during integration. util: enhance OpenApiBeanUtils with null-safety and mapping helpers to reduce boilerplate. security: tweak SpringSecurityUserInfoHolder to streamline user context handling. tests: add controller tests for Cluster/Instance/Item/Organization/Release; update Namespace/NamespaceBranch tests to cover typical and edge scenarios. intent: align the portal OpenAPI layer with generated types and improve maintainability; no intended API behavior changes beyond model types and validation fixes
tacklequestions
force-pushed
the
openapi-controllers/appcontroller
branch
from
8b7f807 to
af8efa3
Compare
What's the purpose of this PR
Migrate AppController to use generated OpenAPI models.
This migration aligns the app management APIs with the OpenAPI specification,
fills missing API functionalities to align with the portal,
and establishes a cleaner separation between generated models and DTOs.
Which issue(s) this PR fixes:
Fixes #5462 #5449
Brief changelog
Summary by CodeRabbit
New Features
Refactor
Chores
Tests