Skip to content

feat(openapi): app domain service impl, controller migration and tests #5471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hezhangjian merged 1 commit into from
Oct 12, 2025
Merged

feat(openapi): app domain service impl, controller migration and tests #5471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions ...al/src/main/java/com/ctrip/framework/apollo/openapi/server/service/AppOpenApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2025 Apollo Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.ctrip.framework.apollo.openapi.server.service;

import com.ctrip.framework.apollo.openapi.model.MultiResponseEntity;
import com.ctrip.framework.apollo.openapi.model.OpenAppDTO;
import com.ctrip.framework.apollo.openapi.model.OpenCreateAppDTO;
import com.ctrip.framework.apollo.openapi.model.OpenEnvClusterDTO;
import org.springframework.lang.NonNull;

import java.util.List;
import java.util.Set;

public interface AppOpenApiService {

void createApp(@NonNull OpenCreateAppDTO req);

List<OpenEnvClusterDTO> getEnvClusterInfo(String appId);

List<OpenAppDTO> getAllApps();

List<OpenAppDTO> getAppsInfo(List<String> appIds);

List<OpenAppDTO> getAuthorizedApps();

void updateApp(OpenAppDTO openAppDTO);

List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size);

void createAppInEnv(String env, OpenAppDTO app, String operator);

OpenAppDTO deleteApp(String appId);

MultiResponseEntity findMissEnvs(String appId);

MultiResponseEntity getAppNavTree(String appId);
}
172 changes: 161 additions & 11 deletions .../main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,38 @@

import com.ctrip.framework.apollo.common.dto.ClusterDTO;
import com.ctrip.framework.apollo.common.entity.App;
import com.ctrip.framework.apollo.common.exception.BadRequestException;
import com.ctrip.framework.apollo.common.utils.BeanUtils;
import com.ctrip.framework.apollo.openapi.api.AppOpenApiService;
import com.ctrip.framework.apollo.openapi.dto.OpenAppDTO;
import com.ctrip.framework.apollo.openapi.dto.OpenCreateAppDTO;
import com.ctrip.framework.apollo.openapi.dto.OpenEnvClusterDTO;
import com.ctrip.framework.apollo.openapi.util.OpenApiBeanUtils;
import com.ctrip.framework.apollo.core.ConfigConsts;
import com.ctrip.framework.apollo.openapi.model.OpenAppDTO;
import com.ctrip.framework.apollo.openapi.model.OpenCreateAppDTO;
import com.ctrip.framework.apollo.openapi.model.OpenEnvClusterDTO;
import com.ctrip.framework.apollo.openapi.model.MultiResponseEntity;
import com.ctrip.framework.apollo.openapi.model.OpenEnvClusterInfo;
import com.ctrip.framework.apollo.openapi.model.RichResponseEntity;
import com.ctrip.framework.apollo.openapi.util.OpenApiModelConverters;
import com.ctrip.framework.apollo.portal.component.PortalSettings;
import com.ctrip.framework.apollo.portal.entity.model.AppModel;
import com.ctrip.framework.apollo.portal.environment.Env;
import com.ctrip.framework.apollo.portal.listener.AppDeletionEvent;
import com.ctrip.framework.apollo.portal.listener.AppInfoChangedEvent;
import com.ctrip.framework.apollo.portal.service.AppService;
import com.ctrip.framework.apollo.portal.service.ClusterService;
import com.ctrip.framework.apollo.portal.service.RoleInitializationService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.data.domain.Pageable;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Service;
import org.springframework.web.client.HttpClientErrorException;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import org.springframework.stereotype.Service;
import java.util.Set;

/**
* @author wxq
Expand All @@ -43,14 +60,21 @@ public class ServerAppOpenApiService implements AppOpenApiService {
private final PortalSettings portalSettings;
private final ClusterService clusterService;
private final AppService appService;
private final ApplicationEventPublisher publisher;
private final RoleInitializationService roleInitializationService;
private static final Logger logger = LoggerFactory.getLogger(ServerAppOpenApiService.class);

public ServerAppOpenApiService(
PortalSettings portalSettings,
ClusterService clusterService,
AppService appService) {
AppService appService,
ApplicationEventPublisher publisher,
RoleInitializationService roleInitializationService) {
this.portalSettings = portalSettings;
this.clusterService = clusterService;
this.appService = appService;
this.publisher = publisher;
this.roleInitializationService = roleInitializationService;
}

private App convert(OpenAppDTO dto) {
Expand All @@ -70,7 +94,11 @@ private App convert(OpenAppDTO dto) {
@Override
public void createApp(OpenCreateAppDTO req) {
App app = convert(req.getApp());
appService.createAppAndAddRolePermission(app, req.getAdmins());
List<String> admins = req.getAdmins();
if (admins == null) {
admins = Collections.emptyList();
}
appService.createAppAndAddRolePermission(app, new HashSet<>(admins));
}

@Override
Expand All @@ -83,7 +111,8 @@ public List<OpenEnvClusterDTO> getEnvClusterInfo(String appId) {

envCluster.setEnv(env.getName());
List<ClusterDTO> clusterDTOs = clusterService.findClusters(env, appId);
envCluster.setClusters(BeanUtils.toPropertySet("name", clusterDTOs));
Set<String> clusterNames = clusterDTOs == null ? Collections.emptySet() : BeanUtils.toPropertySet("name", clusterDTOs);
envCluster.setClusters(new ArrayList<>(clusterNames));

envClusters.add(envCluster);
}
Expand All @@ -94,17 +123,138 @@ public List<OpenEnvClusterDTO> getEnvClusterInfo(String appId) {
@Override
public List<OpenAppDTO> getAllApps() {
final List<App> apps = this.appService.findAll();
return OpenApiBeanUtils.transformFromApps(apps);
return OpenApiModelConverters.fromApps(apps);
}

@Override
public List<OpenAppDTO> getAppsInfo(List<String> appIds) {
if (appIds == null || appIds.isEmpty()) {
return Collections.emptyList();
}
final List<App> apps = this.appService.findByAppIds(new HashSet<>(appIds));
return OpenApiBeanUtils.transformFromApps(apps);
return OpenApiModelConverters.fromApps(apps);
}

@Override
public List<OpenAppDTO> getAuthorizedApps() {
throw new UnsupportedOperationException();
}

/**
* Updating Application Information - Using OpenAPI DTOs
* @param openAppDTO OpenAPI application DTO
*/
@Override
public void updateApp(OpenAppDTO openAppDTO) {
App app = convert(openAppDTO);
App updatedApp = appService.updateAppInLocal(app);
publisher.publishEvent(new AppInfoChangedEvent(updatedApp));
}
Comment on lines +148 to +152

@coderabbitai coderabbitai Bot Oct 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add null validation for the input parameter.

The method lacks null checking for openAppDTO. If null is passed, the convert method will throw a NullPointerException. Add a precondition check or throw BadRequestException for better error reporting.

Apply this fix:

 @Override
 public void updateApp(OpenAppDTO openAppDTO) {
+  if (openAppDTO == null) {
+    throw new BadRequestException("openAppDTO cannot be null");
+  }
   App app = convert(openAppDTO);
   App updatedApp = appService.updateAppInLocal(app);
   publisher.publishEvent(new AppInfoChangedEvent(updatedApp));
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void updateApp(OpenAppDTO openAppDTO) {
App app = convert(openAppDTO);
App updatedApp = appService.updateAppInLocal(app);
publisher.publishEvent(new AppInfoChangedEvent(updatedApp));
}
@Override
public void updateApp(OpenAppDTO openAppDTO) {
if (openAppDTO == null) {
throw new BadRequestException("openAppDTO cannot be null");
}
App app = convert(openAppDTO);
App updatedApp = appService.updateAppInLocal(app);
publisher.publishEvent(new AppInfoChangedEvent(updatedApp));
}
🤖 Prompt for AI Agents 
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 148 to 152, the method updateApp lacks null-checking for the
openAppDTO parameter which will cause a NullPointerException in convert; add an
explicit null validation at the top of the method and throw a
BadRequestException (or IllegalArgumentException per project conventions) with a
clear message like "openAppDTO must not be null" if null is passed, then proceed
to call convert, updateAppInLocal, and publish the event.


/**
* Get the current user's app list (paginated)
* @param page Pagination parameter
* @return App list
*/
@Override
public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) {
int pageIndex = page == null ? 0 : page;
int pageSize = (size == null || size <= 0) ? 20 : size;
Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex);
Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds;
if (targetAppIds.isEmpty()) {
return Collections.emptyList();
}
List<App> apps = appService.findByAppIds(targetAppIds, pageable);
return OpenApiModelConverters.fromApps(apps);
}
Comment on lines +160 to +170

@coderabbitai coderabbitai Bot Oct 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Strengthen pagination parameter validation.

The pagination logic has gaps:

  1. Negative page: Line 161 doesn't validate against negative page values, which could cause issues with Pageable.ofSize(pageSize).withPage(pageIndex).
  2. Unbounded size: Line 162 has no upper limit on size, allowing potentially large values that could cause memory or performance issues.

Apply this diff:

 @Override
 public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) {
-  int pageIndex = page == null ? 0 : page;
-  int pageSize = (size == null || size <= 0) ? 20 : size;
+  int pageIndex = (page == null || page < 0) ? 0 : page;
+  int pageSize = (size == null || size <= 0) ? 20 : Math.min(size, 500);
   Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex);
   Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds;
   if (targetAppIds.isEmpty()) {
     return Collections.emptyList();
   }
   List<App> apps = appService.findByAppIds(targetAppIds, pageable);
   return OpenApiModelConverters.fromApps(apps);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) {
int pageIndex = page == null ? 0 : page;
int pageSize = (size == null || size <= 0) ? 20 : size;
Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex);
Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds;
if (targetAppIds.isEmpty()) {
return Collections.emptyList();
}
List<App> apps = appService.findByAppIds(targetAppIds, pageable);
return OpenApiModelConverters.fromApps(apps);
}
@Override
public List<OpenAppDTO> getAppsBySelf(Set<String> appIds, Integer page, Integer size) {
int pageIndex = (page == null || page < 0) ? 0 : page;
int pageSize = (size == null || size <= 0) ? 20 : Math.min(size, 500);
Pageable pageable = Pageable.ofSize(pageSize).withPage(pageIndex);
Set<String> targetAppIds = appIds == null ? Collections.emptySet() : appIds;
if (targetAppIds.isEmpty()) {
return Collections.emptyList();
}
List<App> apps = appService.findByAppIds(targetAppIds, pageable);
return OpenApiModelConverters.fromApps(apps);
}
🤖 Prompt for AI Agents 
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 160-170, strengthen pagination validation by ensuring page is
non-negative and size is bounded: clamp pageIndex to Math.max(0, page) (treat
null as 0), set pageSize to default 20 when null or <=0, and cap pageSize to a
reasonable maximum (e.g., 100) to avoid unbounded requests; apply these adjusted
values when creating the Pageable (consider extracting the max as a constant or
config) so callers cannot pass negative pages or excessive sizes.


/**
* Create an application in a specified environment
* @param env Environment
* @param app Application information
* @param operator Operator
*/
@Override
public void createAppInEnv(String env, OpenAppDTO app, String operator) {
if (env == null) {
throw BadRequestException.invalidEnvFormat("null");
}
Env envEnum;
try {
envEnum = Env.valueOf(env);
} catch (IllegalArgumentException e) {
throw BadRequestException.invalidEnvFormat(env);
}
App appEntity = convert(app);
appService.createAppInRemote(envEnum, appEntity);

roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(),
ConfigConsts.NAMESPACE_APPLICATION, env, operator);
}
Comment on lines +179 to +194

@coderabbitai coderabbitai Bot Oct 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Add operator validation as flagged in previous review.

The method lacks validation for the operator parameter before calling roleInitializationService.initNamespaceSpecificEnvRoles at line 192. This was explicitly flagged in a previous review comment. Additionally, the app parameter is not validated for null.

Apply this fix to address both issues:

 @Override
 public void createAppInEnv(String env, OpenAppDTO app, String operator) {
   if (env == null) {
     throw BadRequestException.invalidEnvFormat("null");
   }
+  if (app == null) {
+    throw new BadRequestException("app cannot be null");
+  }
+  if (operator == null || operator.trim().isEmpty()) {
+    throw new BadRequestException("operator cannot be null or blank");
+  }
   Env envEnum;
   try {
     envEnum = Env.valueOf(env);
   } catch (IllegalArgumentException e) {
     throw BadRequestException.invalidEnvFormat(env);
   }
   App appEntity = convert(app);
   appService.createAppInRemote(envEnum, appEntity);

   roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(),
           ConfigConsts.NAMESPACE_APPLICATION, env, operator);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void createAppInEnv(String env, OpenAppDTO app, String operator) {
if (env == null) {
throw BadRequestException.invalidEnvFormat("null");
}
Env envEnum;
try {
envEnum = Env.valueOf(env);
} catch (IllegalArgumentException e) {
throw BadRequestException.invalidEnvFormat(env);
}
App appEntity = convert(app);
appService.createAppInRemote(envEnum, appEntity);
roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(),
ConfigConsts.NAMESPACE_APPLICATION, env, operator);
}
@Override
public void createAppInEnv(String env, OpenAppDTO app, String operator) {
if (env == null) {
throw BadRequestException.invalidEnvFormat("null");
}
if (app == null) {
throw new BadRequestException("app cannot be null");
}
if (operator == null || operator.trim().isEmpty()) {
throw new BadRequestException("operator cannot be null or blank");
}
Env envEnum;
try {
envEnum = Env.valueOf(env);
} catch (IllegalArgumentException e) {
throw BadRequestException.invalidEnvFormat(env);
}
App appEntity = convert(app);
appService.createAppInRemote(envEnum, appEntity);
roleInitializationService.initNamespaceSpecificEnvRoles(appEntity.getAppId(),
ConfigConsts.NAMESPACE_APPLICATION, env, operator);
}
🤖 Prompt for AI Agents 
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/server/service/ServerAppOpenApiService.java
around lines 179-194, add null-checks and validation for both the incoming app
DTO and the operator before proceeding: if app is null throw a
BadRequestException indicating a missing/invalid app; if operator is null or
blank throw a BadRequestException indicating a missing/invalid operator; perform
these checks before converting/creating the App and before calling
roleInitializationService.initNamespaceSpecificEnvRoles so no invalid values are
passed downstream.


/**
* Delete an application
* @param appId application ID
* @return the deleted application
*/
@Override
public OpenAppDTO deleteApp(String appId) {
App app = appService.deleteAppInLocal(appId);
publisher.publishEvent(new AppDeletionEvent(app));
return OpenApiModelConverters.fromApp(app);
}

/**
* Find missing environments
* @param appId application ID
* @return list of missing environments
*/
public MultiResponseEntity findMissEnvs(String appId) {
List<RichResponseEntity> entities = new ArrayList<>();
MultiResponseEntity response = new MultiResponseEntity(HttpStatus.OK.value(), entities);
for (Env env : portalSettings.getActiveEnvs()) {
try {
appService.load(env, appId);
} catch (Exception e) {
RichResponseEntity entity;
if (e instanceof HttpClientErrorException &&
((HttpClientErrorException) e).getStatusCode() == HttpStatus.NOT_FOUND) {
entity = new RichResponseEntity(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase());
entity.setBody(env.toString());
} else {
entity = new RichResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR.value(),
"load env:" + env.getName() + " cluster error." + e.getMessage());
}
response.addEntitiesItem(entity);
}
}
return response;
}

/**
* Find AppNavTree
* @param appId
* @return list of EnvClusterInfos
*/
@Override
public MultiResponseEntity getAppNavTree(String appId) {
List<RichResponseEntity> entities = new ArrayList<>();
MultiResponseEntity response = new MultiResponseEntity(HttpStatus.OK.value(),entities);
List<Env> envs = portalSettings.getActiveEnvs();
for (Env env : envs) {
try {
OpenEnvClusterInfo openEnvClusterInfo = OpenApiModelConverters.fromEnvClusterInfo(appService.createEnvNavNode(env, appId));
RichResponseEntity entity = new RichResponseEntity(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase());
entity.setBody(openEnvClusterInfo);
response.addEntitiesItem(entity);
} catch (Exception e) {
logger.warn("Failed to load env {} navigation for app {}", env, appId, e);
RichResponseEntity entity = new RichResponseEntity(HttpStatus.INTERNAL_SERVER_ERROR.value(),
"load env:" + env.getName() + " cluster error." + e.getMessage());
response.addEntitiesItem(entity);
}
}
return response;
}
}
Loading