Feature: Support application-level export configuration #5517
Conversation
dosubot
bot
added
the
size:XL
WalkthroughAdds app-and-cluster-scoped export/import: new REST endpoints and service methods, ZIP path handling (ignore-user-dir), UI tabbed export/import flows with cluster lookup and permission checks, conflictAction validation, new i18n keys, and tests for the app-level flow. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/Browser
participant Ctrl as ConfigExportController
participant Svc as ConfigsExportService / ConfigsImportService
participant Cl as ClusterService / Repos
rect rgb(230,245,255)
Note over UI,Ctrl: App Config Export
UI->>Ctrl: GET /apps/{appId}/envs/{env}/clusters/{cluster}/export
Ctrl->>Ctrl: authorize isAppAdmin(appId)
Ctrl->>Svc: exportAppConfigByEnvAndCluster(appId, env, cluster, outputStream)
Svc->>Cl: validate app & cluster existence
Svc->>Svc: exportNamespaces(..., ignoreUserDir=true) -> writeNamespacesToZip(use genNamespacePathIgnoreUser)
Svc-->>Ctrl: stream ZIP bytes
Ctrl->>UI: HTTP file download (application/zip)
end
rect rgb(245,255,230)
Note over UI,Ctrl: App Config Import
UI->>Ctrl: POST /apps/{appId}/envs/{env}/clusters/{cluster}/import (file, conflictAction)
Ctrl->>Ctrl: validate conflictAction & permissions
Ctrl->>Svc: importAppConfigFromZipFile(appId, env, cluster, zipStream, ignoreConflict)
Svc->>Svc: parse ZIP entries, validate paths, collect namespaces
Svc->>Cl: doImport(namespaces, ignoreConflict)
Svc-->>Ctrl: import result
Ctrl->>UI: success/error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/resources/static/config_export.html (1)
265-271: Duplicate script include for AppUtils.js.
AppUtils.jsis included twice at lines 265 and 271. Remove one of them.<script type="application/javascript" src="scripts/AppUtils.js"></script> <script type="application/javascript" src="scripts/PageCommon.js"></script> <script type="application/javascript" src="scripts/directive/directive.js"></script> <script type="application/javascript" src="scripts/valdr.js"></script> -<script type="application/javascript" src="scripts/AppUtils.js"></script> <script type="application/javascript" src="scripts/services/OrganizationService.js"></script>
🧹 Nitpick comments (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
231-333: Test coverage looks good, but consider usingassertThrowsfor cleaner exception testing.The test method comprehensively covers:
- Export failure when app doesn't exist
- Export failure when cluster doesn't exist
- Successful export
- Import failure scenarios with validation
- Successful import with proper verification
The exception testing pattern using try-catch with
Assert.assertEqualsworks but could be modernized with JUnit'sassertThrows:BadRequestException ex = Assert.assertThrows(BadRequestException.class, () -> configsExportService.exportAppConfigByEnvAndCluster(appId2, env, clusterName1, fileOutputStream)); Assert.assertEquals("App not found: " + appId2, ex.getMessage());apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java (1)
135-154: New endpoint looks well-structured. Consider handling invalid environment names gracefully.The endpoint correctly:
- Uses app-level permission validation (
@unifiedPermissionValidator.isAppAdmin)- Generates a descriptive filename with timestamp
- Logs the download action with relevant context
However,
Env.valueOf(env)at line 151 will throwIllegalArgumentExceptionif the environment name is invalid, resulting in HTTP 500. For consistency with other validation errors, consider catching this and returning a 400 Bad Request:try (OutputStream outputStream = response.getOutputStream()) { + Env envEnum; + try { + envEnum = Env.valueOf(env); + } catch (IllegalArgumentException e) { + throw new BadRequestException("invalid env format:%s", env); + } - configsExportService.exportAppConfigByEnvAndCluster(appId, Env.valueOf(env), clusterName, + configsExportService.exportAppConfigByEnvAndCluster(appId, envEnum, clusterName, outputStream); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
196-198: Lost error context whenreadContentreturns null.When
readContentfails due to anIOException, it logs the error and returnsnull. Here, you throw aBadRequestExceptionwithout the original error context. Consider propagating more context or logging at this level.String content = readContent(dataZip); if (content == null) { - throw new BadRequestException("Failed to read file content."); + throw new BadRequestException("Failed to read file content for: %s", filePath); }apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js (2)
145-156: Unreliable success notification timing.The
setTimeoutapproach for showing the success toast assumes the redirect will succeed, but the actual file download could fail after the redirect. The toast shows regardless of the actual download outcome.Consider using the
$window.location.hrefassignment inside the.then()without the setTimeout, as the HEAD check already validates accessibility. The success feedback should rely on the HEAD response, not a timed delay after redirect.}).then(function(response) { $window.location.href = exportUrl; - setTimeout(function() { - toastr.success($translate.instant('ConfigExport.ExportSuccess')); - }, 1000); + toastr.success($translate.instant('ConfigExport.ExportSuccess')); }).catch(function(response) {
180-188: Deprecated.success()and.error()callbacks.The
.success()and.error()callbacks are deprecated in AngularJS $http. This is inconsistent withexportAppConfigwhich uses the modern.then()and.catch()pattern.- }).success(function (data) { - toastr.success(data, $translate.instant('ConfigExport.ImportSuccess')); - }).error(function (data, status) { + }).then(function (response) { + toastr.success(response.data, $translate.instant('ConfigExport.ImportSuccess')); + }).catch(function (response) { + var status = response.status; + var data = response.data; if (status === 403) { toastr.warning($translate.instant('ConfigExport.NoPermissionTip')); return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java(1 hunks)apollo-portal/src/main/resources/static/config_export.html(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js(4 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.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/portal/service/ConfigsImportService.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
ConfigFileUtils(39-213)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/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 (16)
CHANGES.md (1)
27-27: LGTM!The changelog entry is properly formatted and follows the existing pattern for feature entries.
apollo-portal/src/main/resources/static/views/common/nav.html (1)
80-80: LGTM!The Config Export link is correctly added to the non-admin tools menu, enabling app administrators to access the new app-level export/import functionality.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
182-189: LGTM!The new utility method follows the established pattern and correctly omits the
ownerNamecomponent for app-level exports. The implementation is consistent with existing path generation methods.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
102-105: LGTM!The new test method follows the existing test structure pattern in this file.
apollo-portal/src/main/resources/static/config_export.html (2)
99-106: TheconflictActionmodel is shared between Env and App tabs.Both tab sections bind to the same
ng-model="conflictAction". Changing the conflict action in one tab will affect the other. If this is intentional for UX consistency, consider adding a note or tooltip. If tabs should have independent conflict action settings, use separate model variables likeenvConflictActionandappConflictAction.Also applies to: 188-195
36-46: Tab implementation looks good overall.The tabbed interface correctly gates the Env tab behind root permission while keeping the App tab accessible to app administrators. The active state logic appropriately defaults non-admin users to the App tab.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java (1)
127-128: LGTM!Clean refactor using method reference instead of lambda.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (3)
19-19: LGTM!Appropriate import for client-side validation errors.
168-171: LGTM!Good consistency improvement changing to
ServiceException, which aligns with other error handling in this service (e.g., line 100).
214-225: LGTM!The error handling and delegation to
doImportwith empty lists for apps/clusters is appropriate for app-scoped imports where only namespace data is being imported.apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js (1)
113-131: LGTM!The
getClusterInfofunction properly validates required inputs before making the API call, clears previous state, and appropriately handles both success and error cases by toggling the button state.apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
796-822: LGTM!The new i18n keys follow the existing naming conventions and provide appropriate Chinese translations for the new app-level export/import feature.
apollo-portal/src/main/resources/static/i18n/en.json (1)
796-822: LGTM!The English translations are consistent with the Chinese counterparts and appropriately describe the new app-level configuration export/import functionality.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (1)
97-110: LGTM on authorization and structure.The new app-scoped import endpoint correctly uses
@PreAuthorizewithisAppAdminto restrict access to application administrators, which aligns with the PR objective of requiring administrator privileges for the application.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (2)
113-138: LGTM on the new export method structure.The
exportAppConfigByEnvAndClustermethod correctly validates the app and cluster existence before attempting export, uses try-with-resources for proper resource management of theZipOutputStream, and appropriately setsignoreUserDir=trueto generate simplified paths for app-level exports.
297-300: LGTM on path generation logic.The conditional path generation based on
ignoreUserDirflag correctly differentiates between environment-level exports (which include owner directory structure) and app-level exports (which use a simplified path structure). This aligns with the different use cases for each export type.
| List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Potential unhandled IllegalArgumentException from Env::valueOf.
Per the Env.valueOf implementation in the relevant code snippets, it throws IllegalArgumentException if the environment name doesn't exist. This would result in an HTTP 500 response instead of a user-friendly 400 Bad Request.
Consider wrapping with proper exception handling:
- List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
- .collect(Collectors.toList());
+ List<Env> importEnvs;
+ try {
+ importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
+ .collect(Collectors.toList());
+ } catch (IllegalArgumentException e) {
+ throw new BadRequestException("Invalid environment: " + e.getMessage());
+ }📝 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.
| List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf) | |
| .collect(Collectors.toList()); | |
| List<Env> importEnvs; | |
| try { | |
| importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf) | |
| .collect(Collectors.toList()); | |
| } catch (IllegalArgumentException e) { | |
| throw new BadRequestException("Invalid environment: " + e.getMessage()); | |
| } |
| configsImportService.importAppConfigFromZipFile(appId, Env.valueOf(env), clusterName, | ||
| zipInputStream, ignoreConflictNamespace); |
There was a problem hiding this comment.
Same IllegalArgumentException risk for single env parsing.
The Env.valueOf(env) call can throw IllegalArgumentException for invalid environment names, resulting in HTTP 500 instead of 400.
+ Env envObj;
+ try {
+ envObj = Env.valueOf(env);
+ } catch (IllegalArgumentException e) {
+ throw new BadRequestException("Invalid environment: " + env);
+ }
byte[] bytes = file.getBytes();
try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
- configsImportService.importAppConfigFromZipFile(appId, Env.valueOf(env), clusterName,
+ configsImportService.importAppConfigFromZipFile(appId, envObj, clusterName,
zipInputStream, ignoreConflictNamespace);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 107-108, the direct call to Env.valueOf(env) can throw
IllegalArgumentException for invalid env names causing a 500; validate or safely
parse the env string and convert it to an Env in a try-catch (or use a safe
helper) and if parsing fails return a 400 Bad Request (e.g., throw or return an
appropriate client error with a clear message) before calling
configsImportService.importAppConfigFromZipFile.
| try { | ||
| this.exportNamespaces(env, app, cluster, zipOutputStream, true); | ||
| } catch (BadRequestException badRequestException) { | ||
| // ignore | ||
| } catch (Exception e) { | ||
| logger.error("export namespace error. appId = {}, env = {}, cluster = {}", app.getAppId(), | ||
| env.getName(), cluster.getName(), e); | ||
| } |
There was a problem hiding this comment.
Silently ignoring BadRequestException may hide legitimate errors.
Catching and ignoring BadRequestException without any logging could mask issues like missing namespaces or configuration problems. While the env-level export (line 258) has the same pattern, for app-level export this is the primary operation, so failures are more significant.
Consider at least logging a warning:
try {
this.exportNamespaces(env, app, cluster, zipOutputStream, true);
} catch (BadRequestException badRequestException) {
- // ignore
+ logger.warn("export namespace warning. appId = {}, env = {}, cluster = {}, message = {}",
+ app.getAppId(), env.getName(), cluster.getName(), badRequestException.getMessage());
} catch (Exception e) {🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
around lines 126-133, the catch block for BadRequestException currently swallows
the exception; change it to log a warning (including appId, env, cluster and the
exception) instead of ignoring it so legitimate problems are visible; use
logger.warn with a descriptive message and pass the exception (or at least
exception.getMessage()) to preserve stack/diagnostic info while keeping current
control flow.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (1)
103-138: LogBadRequestExceptionin app-level export instead of silently ignoring.
exportAppConfigByEnvAndClusterdoes the right thing in validating the app and cluster and streaming a ZIP, but the inner catch block fully swallowsBadRequestException:try { this.exportNamespaces(env, app, cluster, zipOutputStream, true); } catch (BadRequestException badRequestException) { // ignore }For app-level export this is the primary operation; if namespace export fails (e.g. due to bad data), the client gets an apparently-successful download with missing content and no server-side signal.
At least logging a warning with appId/env/cluster and the exception message would make such situations traceable, similar to how other exceptions are logged.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (1)
87-88: Handle invalid env names instead of lettingEnv.valueOfpropagate.
Env.valueOfthrowsIllegalArgumentExceptionfor unknown environments, which will currently bubble up as a 500. For both the multi-env import (/configs/import) and app-level import (/apps/{appId}/envs/{env}/clusters/{clusterName}/import), this should be treated as a client error.Wrap the parsing and convert invalid values into a
BadRequestException(or use a safe helper such as atransformEnv/exists check), e.g.:List<Env> importEnvs; try { importEnvs = Splitter.on(ENV_SEPARATOR) .splitToList(envs) .stream() .map(Env::valueOf) .collect(Collectors.toList()); } catch (IllegalArgumentException ex) { throw new BadRequestException("Invalid environment(s): " + envs); }And similarly for the single
envpath variable before callingEnv.valueOf(env).Also applies to: 107-108
🧹 Nitpick comments (3)
apollo-portal/src/main/resources/static/config_export.html (1)
36-43: Consider marking the App tab<li>active for non-root users.When
hasRootPermission == false, the Env tab<li>is removed and only the App tab remains, but it never gets theactiveclass even though theapp_configpane is markedin active. This can make the selected tab state look inconsistent.You could conditionally add
activefor the App tab when!hasRootPermission, e.g. extend theng-classto include anactivestate in that case.Also applies to: 126-131
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
102-105: Don’t swallow exceptions in the “happy path” import.The new
testAppConfigExportImportScenariodoes a good job covering the error paths and the successful flow, but the last call toimportAppConfigFromZipFilecatchesExceptionand only prints the stack trace. That can hide real regressions.Consider letting unexpected exceptions fail the test, e.g. by removing the try/catch or adding an explicit
Assert.failin the catch block.Also applies to: 231-333
apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js (1)
133-157: Reconsider the HEAD+GET pattern for app-config export.
exportAppConfigfirst issues aHEADto the export URL and, on success, immediately performs a full GET to trigger the download. This guards against showing a download dialog when the user lacks permission (403), but it also doubles the number of HTTP requests and server-side work for successful exports.If performance or server load becomes a concern, consider either:
- Relying on the GET directly and handling 4xx responses in the client, or
- Introducing a lightweight permission-check endpoint instead of probing the full export URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java(1 hunks)apollo-portal/src/main/resources/static/config_export.html(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js(4 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java
- CHANGES.md
- apollo-portal/src/main/resources/static/i18n/zh-CN.json
- apollo-portal/src/main/resources/static/i18n/en.json
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java
🧰 Additional context used
🧬 Code graph analysis (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.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 (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (1)
48-49: Conflict-action validation is clear and centralised.Using
CONFLICT_ACTION_IGNORE/COVERplusvalidateConflictActionkeeps the allowed values explicit and shared across both import endpoints. This helps avoid magic strings and makes it easy to extend later if new actions are added.Also applies to: 82-86, 101-105, 112-116
apollo-portal/src/main/resources/static/views/common/nav.html (1)
80-80: Adding ConfigExport to non-admin tools is consistent with app-level permissions.Exposing
/config_export.htmlfor non-root users while keeping actual operations guarded by app-admin/super-admin checks on the backend is a sensible UX improvement.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (1)
267-282: ignoreUserDir flag and namespace path generation look correct.Threading
ignoreUserDirthroughexportNamespacesintowriteNamespacesToZipand switching betweengenNamespacePathIgnoreUser(appId, env, configFileName)and the existinggenNamespacePath(ownerName, appId, env, configFileName)cleanly separates the app-level vs. owner-scoped export layouts without changing the shared ZIP-writing logic.Also applies to: 284-301
| List<Env> exportEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Validate env values before calling Env.valueOf in export endpoints.
Both the bulk export (/configs/export) and the new app-level export (/apps/{appId}/envs/{env}/clusters/{clusterName}/export) call Env.valueOf on user input. For unknown env names this throws IllegalArgumentException, which will be surfaced as a 500.
To keep the API contract clear, consider translating invalid envs into a client error instead, e.g.:
List<Env> exportEnvs;
try {
exportEnvs = Splitter.on(ENV_SEPARATOR)
.splitToList(envs)
.stream()
.map(Env::valueOf)
.collect(Collectors.toList());
} catch (IllegalArgumentException ex) {
throw new BadRequestException("Invalid environment(s): " + envs);
}And similarly wrap the single env path variable before passing it to Env.valueOf in exportAppConfig.
Also applies to: 137-152
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java
around lines 127-128 (and similarly lines 137-152), the code calls Env.valueOf
on unvalidated user input which will throw IllegalArgumentException and surface
as a 500; change the flow to validate and translate invalid env names into a
client error by catching IllegalArgumentException when mapping split envs to Env
(or when converting the single env path variable) and throw a
BadRequestException (or equivalent 4xx) with a clear message like "Invalid
environment(s): <input>" so invalid inputs return a 400 instead of a 500.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
162-172: Avoid uncaughtArrayIndexOutOfBoundsExceptionfor malformed ZIP paths (the loop isn’t guarded).
importDataFromZipFile’s entry parsing assumes path lengths (e.g.,info[2],info[3]). If a ZIP contains unexpected paths, this can crash before reaching thedoImport(...)try/catch.Consider validating
info.lengthfor the “cluster/namespace” branch and throwingBadRequestException(or skipping unknown entries) early.String[] info = filePath.replace('\\', '/').split("/"); @@ - } else { - String env = info[2]; - fileName = info[3]; + } else { + // expected: ownerName/appId/env/file => 4 parts + if (info.length != 4) { + throw new BadRequestException("Invalid file path in ZIP."); + } + String env = info[2]; + fileName = info[3]; for (Env importEnv : importEnvs) {
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (1)
103-138: Don’t silently swallowBadRequestExceptionin app-scoped export (log at least).try { this.exportNamespaces(env, app, cluster, zipOutputStream, true); } catch (BadRequestException badRequestException) { - // ignore + logger.warn("export namespace warning. appId = {}, env = {}, cluster = {}, message = {}", + app.getAppId(), env.getName(), cluster.getName(), badRequestException.getMessage()); } catch (Exception e) {
🧹 Nitpick comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
102-105: Test name is misleading (no “fillItemDetail” toggle here).
Minor nit: either rename the test or make it actually parameterize thefillItemDetailbehavior like the env-scoped tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java(1 hunks)apollo-portal/src/main/resources/static/config_export.html(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js(4 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java
- CHANGES.md
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
- apollo-portal/src/main/resources/static/views/common/nav.html
- apollo-portal/src/main/resources/static/config_export.html
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
ConfigFileUtils(39-213)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
ConfigFileUtils(39-213)
⏰ 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 (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
174-227: App-scoped import ZIP validation looks solid (appId/env/cluster prefix + empty ZIP guard).The checks on
info.length == 3,info[0] == appId,info[1] == env, andfileNameprefixappId+cluster+are the right fail-fast gates for the new app-level ZIP format.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
182-190: LGTM: path helper aligns with app-scoped ZIP layout.apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js (1)
17-32: Good: app-scoped UI state is introduced cleanly (cluster,appConfigBtnDisabled).apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (1)
267-301: LGTM:ignoreUserDirthreaded correctly into ZIP path generation.apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
796-822: Chinese translations are well-aligned with English counterparts.The Chinese translations in lines 796-822 accurately mirror the English versions. The splitting of environment-level and app-level export/import flows is well-expressed in Chinese:
- "按环境操作" (Operate by Environment) vs. "按应用集群操作" (Operate by Application Cluster) effectively conveys scope distinction
- All new keys have appropriate Chinese equivalents that maintain semantic consistency
apollo-portal/src/main/resources/static/i18n/en.json (2)
796-822: Old keyConfigExport.TitleTipshas been successfully removed and all UI references updated.Verification confirms no remaining references to the old key in the codebase. The new scoped keys (
ConfigExport.Env.TitleTipsandConfigExport.App.TitleTips) are properly used inconfig_export.htmlat lines 50 and 130 respectively. The migration is complete.
796-822: i18n translations look good; locale file completeness confirmed.The English translations in lines 796-822 are clear and well-written. The new keys follow a consistent hierarchical naming pattern and appropriately support the app-and-environment-scoped export/import feature. Both supported locale files (en.json and zh-CN.json) contain the new ConfigExport keys in sync—each has 28 ConfigExport entries with corresponding translations.
| $scope.exportAppConfig = function () { | ||
| if (!$scope.cluster.appId || !$scope.cluster.env || !$scope.cluster.name || !$scope.cluster.info) { | ||
| toastr.warning($translate.instant('ConfigExport.PleaseEnterAppIdAndEnvAndCluster')); | ||
| return; | ||
| } | ||
|
|
||
| var exportUrl = AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId + | ||
| '/envs/' + $scope.cluster.env + '/clusters/' + $scope.cluster.name + '/export'; | ||
|
|
||
| $http({ | ||
| method: 'HEAD', | ||
| url: exportUrl | ||
| }).then(function(response) { | ||
| $window.location.href = exportUrl; | ||
| setTimeout(function() { | ||
| toastr.success($translate.instant('ConfigExport.ExportSuccess')); | ||
| }, 1000); | ||
| }).catch(function(response) { | ||
| if (response.status === 403) { | ||
| toastr.warning($translate.instant('ConfigExport.NoPermissionTip')); | ||
| return; | ||
| } | ||
| toastr.error($translate.instant('ConfigExport.ExportFailed')); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Encode URL path segments for app-scoped export.
appId/env/cluster are user inputs; if they contain special chars (spaces, #, /, etc.) the request can break.
- var exportUrl = AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId +
- '/envs/' + $scope.cluster.env + '/clusters/' + $scope.cluster.name + '/export';
+ var exportUrl = AppUtil.prefixPath() + '/apps/' + encodeURIComponent($scope.cluster.appId) +
+ '/envs/' + encodeURIComponent($scope.cluster.env) + '/clusters/' + encodeURIComponent($scope.cluster.name) + '/export';🤖 Prompt for AI Agents
In
apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js
around lines 133 to 157, the export URL is built by concatenating user-provided
appId, env and cluster name directly into the path which can break if those
values contain special characters; update the URL construction to encode each
path segment using encodeURIComponent (i.e. encode appId, env, and cluster.name
separately) and use that encodedUrl for both the HEAD $http call and
$window.location.href redirect so the request and redirect work correctly for
inputs with spaces, slashes or other reserved characters.
| $scope.importAppConfig = function () { | ||
| if (!$scope.cluster.appId || !$scope.cluster.env || !$scope.cluster.name || !$scope.cluster.info) { | ||
| toastr.warning($translate.instant('ConfigExport.PleaseEnterAppIdAndEnvAndCluster')); | ||
| return; | ||
| } | ||
|
|
||
| var file = document.getElementById("appFileUpload").files[0]; | ||
| if (file == null) { | ||
| toastr.warning($translate.instant('ConfigExport.UploadFileTip')); | ||
| return; | ||
| } | ||
|
|
||
| var form = new FormData(); | ||
| form.append('file', file); | ||
| $http({ | ||
| method: 'POST', | ||
| url: AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId + '/envs/' + $scope.cluster.env + | ||
| '/clusters/' + $scope.cluster.name + '/import?conflictAction=' + $scope.conflictAction, | ||
| data: form, | ||
| headers: {'Content-Type': undefined}, | ||
| transformRequest: angular.identity | ||
| }).success(function (data) { | ||
| toastr.success(data, $translate.instant('ConfigExport.ImportSuccess')); | ||
| }).error(function (data, status) { | ||
| if (status === 403) { | ||
| toastr.warning($translate.instant('ConfigExport.NoPermissionTip')); | ||
| return; | ||
| } | ||
| toastr.error(data, $translate.instant('ConfigExport.ImportFailed')); | ||
| }); | ||
| toastr.info($translate.instant('ConfigExport.ImportingTip')); | ||
| }; |
There was a problem hiding this comment.
Encode URL path segments for app-scoped import (same concern as export).
- url: AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId + '/envs/' + $scope.cluster.env +
- '/clusters/' + $scope.cluster.name + '/import?conflictAction=' + $scope.conflictAction,
+ url: AppUtil.prefixPath() + '/apps/' + encodeURIComponent($scope.cluster.appId) + '/envs/' + encodeURIComponent($scope.cluster.env) +
+ '/clusters/' + encodeURIComponent($scope.cluster.name) + '/import?conflictAction=' + encodeURIComponent($scope.conflictAction),🤖 Prompt for AI Agents
In
apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js
around lines 159 to 190, the POST URL used for app-scoped import concatenates
user-controlled values (appId, env, cluster.name and the conflictAction query
value) directly into the path which can break URLs or introduce injection; wrap
each path segment and the query value with encodeURIComponent when building the
URL (i.e. encode appId, env, cluster.name for the path and encode conflictAction
for the query string) so the request URL is properly escaped before making the
$http call.
...portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java
Show resolved
Hide resolved
|
Example of application administrator using export and import functions |
|
Example of system administrator using export and import functions |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/resources/static/i18n/en.json (1)
795-823: Tighten wording/capitalization for new ConfigExport strings (minor UX polish).A few new labels/tips could be more idiomatic/consistent:
"Operate By ..."→"Operate by ...""Operate By Application Cluster"→ consider"Operate by Application/Cluster"(or similar)"ConfigExport.ClusterNameTips"says “before export” but this page is export/import—consider “before export/import”.
♻️ Duplicate comments (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
102-105: Test can produce false positives + leaks/stream reuse can corrupt ZIP output.
try/catchassertions don’t fail if no exception is thrown.- Reusing the same
FileOutputStreamacross multiple exports can concatenate outputs and break the later import.- Multiple streams aren’t closed;
printStackTrace()can mask test failure.A tighter pattern:
- FileOutputStream fileOutputStream = new FileOutputStream(filePath); - - try { - configsExportService.exportAppConfigByEnvAndCluster(appId2, env, clusterName1, - fileOutputStream); - } catch (BadRequestException e) { - Assert.assertEquals("App not found: " + appId2, e.getMessage()); - } + Assert.assertThrows(BadRequestException.class, () -> { + try (FileOutputStream out = new FileOutputStream(filePath)) { + configsExportService.exportAppConfigByEnvAndCluster(appId2, env, clusterName1, out); + } + }); + + try (FileOutputStream out = new FileOutputStream(filePath)) { + configsExportService.exportAppConfigByEnvAndCluster(appId1, env, clusterName1, out); + } + + try (FileInputStream fis = new FileInputStream(filePath); + ZipInputStream zis = new ZipInputStream(fis)) { + configsImportService.importAppConfigFromZipFile(appId1, env, clusterName1, zis, false); + }Also applies to: 231-333
apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js (1)
133-157: Encode app/env/cluster path segments (andconflictAction) when building URLs.Current concatenation will break for special characters in user input. Suggested change:
- var exportUrl = AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId + - '/envs/' + $scope.cluster.env + '/clusters/' + $scope.cluster.name + '/export'; + var exportUrl = AppUtil.prefixPath() + '/apps/' + encodeURIComponent($scope.cluster.appId) + + '/envs/' + encodeURIComponent($scope.cluster.env) + '/clusters/' + encodeURIComponent($scope.cluster.name) + '/export'; ... - url: AppUtil.prefixPath() + '/apps/' + $scope.cluster.appId + '/envs/' + $scope.cluster.env + - '/clusters/' + $scope.cluster.name + '/import?conflictAction=' + $scope.conflictAction, + url: AppUtil.prefixPath() + '/apps/' + encodeURIComponent($scope.cluster.appId) + '/envs/' + encodeURIComponent($scope.cluster.env) + + '/clusters/' + encodeURIComponent($scope.cluster.name) + '/import?conflictAction=' + encodeURIComponent($scope.conflictAction),Also applies to: 159-190
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (2)
80-94: Handle invalid/whitespace envs:Env.valueOfcan still surface as 500.
Splitter...map(Env::valueOf)will throwIllegalArgumentExceptionfor bad values (and also for values with leading/trailing spaces). ConsidertrimResults().omitEmptyStrings()plus translating invalid envs toBadRequestException(400).
97-109: SameEnv.valueOf(env)500 risk for app-scoped import endpoint.For
/apps/{appId}/envs/{env}/.../import, invalidenvwill currently bubble asIllegalArgumentException→ likely 500. Convert to a 400BadRequestException(and consider normalizing case/whitespace).
🧹 Nitpick comments (3)
apollo-portal/src/main/resources/static/views/common/nav.html (1)
74-82: Consider hiding “ConfigExport” unless user has app-admin permission (UX).Right now it’s shown to all non-root users; if the page/actions require app-admin, consider gating the menu entry accordingly (or showing a clearer “permission required” tip on page load).
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
182-189: Prefer “/” for ZIP entry paths (avoid OS-dependent separators).If this path is written into ZIP entries, using
File.separatorcan yield\on Windows. Consider standardizing ZIP entry paths:- return String.join(File.separator, appId, env.getName(), configFilename); + return String.join("/", appId, env.getName(), configFilename);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
174-227: App import can “succeed” while silently skipping failed namespaces/items.
importAppConfigFromZipFile(...)delegates todoImport(...), which catches exceptions per namespace in parallel and only logs them. For an app-scoped import endpoint, consider collecting failures and throwing aServiceException(or returning a summary) when any namespace import fails (or when all fail).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java(1 hunks)apollo-portal/src/main/resources/static/config_export.html(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(2 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(2 hunks)apollo-portal/src/main/resources/static/scripts/controller/ConfigExportController.js(4 hunks)apollo-portal/src/main/resources/static/views/common/nav.html(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java
- apollo-portal/src/main/resources/static/i18n/zh-CN.json
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
🧰 Additional context used
🧬 Code graph analysis (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/ConfigFileUtils.java (1)
ConfigFileUtils(39-213)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/environment/Env.java (1)
Env(34-237)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (2)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/exception/BadRequestException.java (1)
BadRequestException(22-140)apollo-portal/src/main/java/com/ctrip/framework/apollo/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 (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsImportService.java (1)
162-172: Switching toServiceExceptionhere is reasonable.This makes import failures consistent with other service-layer errors and preserves stack traces.
| byte[] bytes = file.getBytes(); | ||
| try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) { | ||
| configsImportService.importDataFromZipFile(importEnvs, zipInputStream, false); | ||
| configsImportService.importDataFromZipFile(importEnvs, zipInputStream, | ||
| ignoreConflictNamespace); | ||
| } |
There was a problem hiding this comment.
Avoid file.getBytes() for ZIP uploads (heap pressure / DoS risk).
Both ZIP endpoints buffer the entire upload into memory before creating ZipInputStream. Prefer streaming:
- byte[] bytes = file.getBytes();
- try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
+ try (ZipInputStream zipInputStream = new ZipInputStream(file.getInputStream())) {
...
}Optionally also validate “zip-ness” (content-type/filename) and enforce/verify upload size limits.
Also applies to: 105-109
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 90-94 (and similarly 105-109), the code currently calls
file.getBytes() which loads the entire upload into memory; change it to stream
the upload instead by obtaining an InputStream (e.g. file.getInputStream() or
the servlet request InputStream) and wrap that in a ZipInputStream so the ZIP is
processed incrementally. Additionally enforce/verify upload size limits before
processing, and perform basic ZIP validation (check filename/content-type and/or
the ZIP magic header) before iterating entries to reject non-ZIP or oversized
uploads. Ensure resources are closed via try-with-resources on the
InputStream/ZipInputStream.
| private void validateConflictAction(String conflictAction) { | ||
| if (!conflictAction.equals(CONFLICT_ACTION_COVER) | ||
| && !conflictAction.equals(CONFLICT_ACTION_IGNORE)) { | ||
| throw new BadRequestException("ConflictAction is incorrect."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make conflictAction validation a bit more robust/clear.
Minor hardening/readability:
- Prefer constant-first equals to be null-safe:
CONFLICT_ACTION_IGNORE.equals(conflictAction) - Consider returning a more actionable 400 message (e.g., include allowed values:
ignore|cover).
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 112 to 117, make the conflictAction validation null-safe and more
actionable by using constant-first equals checks (e.g.,
CONFLICT_ACTION_IGNORE.equals(conflictAction) ||
CONFLICT_ACTION_COVER.equals(conflictAction)) and, if invalid, throw
BadRequestException with a clearer message that includes the allowed values (for
example: "ConflictAction is incorrect. Allowed values: ignore|cover"). Ensure
you do not introduce additional behavioral changes beyond the null-safe equals
order and improved error text.
| <ul class="nav nav-tabs"> | ||
| <li role="presentation" ng-class="{false:'disabled'}[allowSwitchingTabs]" class="active" ng-if="hasRootPermission"> | ||
| <a ng-disabled="!allowSwitchingTabs" style="border-left-style:none" href="#env_config" data-toggle="tab">{{'ConfigExport.Env.Tab' | translate}}</a> | ||
| </li> | ||
| <li role="presentation" ng-class="{false:'disabled'}[allowSwitchingTabs]"> | ||
| <a ng-disabled="!allowSwitchingTabs" href="#app_config" data-toggle="tab">{{'ConfigExport.App.Tab' | translate}}</a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
Tab header “active” state doesn’t match pane when hasRootPermission is false.
When env tab is absent, consider making the app tab <li> active by default so the UI state is consistent (header + pane).
Also applies to: 126-127
🤖 Prompt for AI Agents
In apollo-portal/src/main/resources/static/config_export.html around lines 36 to
43 (also apply same change to lines 126-127), the second tab header is never
marked active when hasRootPermission is false because the first <li> is
conditionally absent; change the active handling to be conditional: remove the
hard-coded class="active" from the first <li> and replace with ng-class
expressions that set 'active' on the env tab when hasRootPermission is true and
set 'active' on the app tab when hasRootPermission is false (e.g.
ng-class="{'active': hasRootPermission}" for env and ng-class="{'active':
!hasRootPermission}" for app), ensuring ng-disabled/allowSwitchingTabs logic
remains unchanged.
| <button class="btn btn-info" | ||
| ng-click="getClusterInfo()">{{'Common.Search' | translate }}</button> | ||
| </div> |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submit.
- <button class="btn btn-info"
+ <button type="button" class="btn btn-info"
ng-click="getClusterInfo()">{{'Common.Search' | translate }}</button>📝 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.
| <button class="btn btn-info" | |
| ng-click="getClusterInfo()">{{'Common.Search' | translate }}</button> | |
| </div> | |
| <button type="button" class="btn btn-info" | |
| ng-click="getClusterInfo()">{{'Common.Search' | translate }}</button> | |
| </div> |
🤖 Prompt for AI Agents
In apollo-portal/src/main/resources/static/config_export.html around lines 161
to 163, the <button> used for triggering getClusterInfo() lacks an explicit
type, causing it to default to "submit" and potentially submit enclosing forms;
update the button element to include type="button" (i.e., add an explicit type
attribute) so clicking it invokes the Angular click handler without submitting
the form.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
Show resolved
Hide resolved
2 participants
What's the purpose of this PR
Feature: Support exporting and importing configurations for specified applications and clusters
Brief changelog
Requires administrator privileges for the application.
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.mvn spotless:applyto format your code.CHANGESlog.Summary by CodeRabbit
New Features
Behavior
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.