Add support for preserveCredentials while exporting API #13141
Conversation
msm1992
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw,
tgtshanika,
tharikaGitHub,
senthuran16,
hisanhunais,
HiranyaKavishani,
RakhithaRR,
npamudika,
HeshanSudarshana,
pubudu538 and
Krishanx92
as code owners
📝 WalkthroughWalkthroughA new optional boolean query parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublisherAPI
participant ApisApiServiceImpl
participant ImportExportAPI
Client->>PublisherAPI: GET /apis/export?preserveCredentials={bool}
PublisherAPI->>ApisApiServiceImpl: exportAPI(..., preserveCredentials, ...)
ApisApiServiceImpl->>ImportExportAPI: exportAPI(..., preserveCredentials)
ImportExportAPI-->>ApisApiServiceImpl: API export result
ApisApiServiceImpl-->>PublisherAPI: Response
PublisherAPI-->>Client: Exported API (with/without credentials)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
Adds a preserveCredentials flag to the API export endpoint so consumers can opt to include endpoint credentials when exporting an API.
- Introduced a new
preserveCredentialsquery parameter in all Publisher OpenAPI specs. - Extended the
exportAPIinterface and implementation to accept and defaultpreserveCredentials. - Passed the new flag into the
importExportAPI.exportAPIcall.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| components/.../rest.api.util/src/main/resources/publisher-api.yaml | Added preserveCredentials query parameter to the export path |
| components/.../rest.api.common/src/main/resources/publisher-api.yaml | Added preserveCredentials query parameter to the export path |
| components/.../rest.api.publisher.v1/src/main/resources/publisher-api.yaml | Added preserveCredentials query parameter to the export path |
| components/.../rest.api.publisher.v1/src/main/java/.../ApisApiServiceImpl.java | Updated exportAPI signature, defaulting logic, and delegated call to include preserveCredentials |
| components/.../rest.api.publisher.v1/src/gen/java/.../ApisApiService.java | Added new parameter to service interface |
| components/.../rest.api.publisher.v1/src/gen/java/.../ApisApi.java | Updated resource method to accept and forward preserveCredentials |
Comments suppressed due to low confidence (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml:4336
- [nitpick] Add a
defaultValue: "false"orschema.default: falseto thepreserveCredentialsparameter in the OpenAPI spec to document its default behavior.
- name: preserveCredentials
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java:3784
- There are no new tests covering the
preserveCredentialsflag. Consider adding unit or integration tests to verify bothtrueandfalsecases for this parameter.
public Response exportAPI(String apiId, String name, String version, String revisionNum, String providerName, String format, Boolean preserveStatus, Boolean exportLatestRevision, String gatewayEnvironment, Boolean preserveCredentials,
| if (StringUtils.isEmpty(gatewayEnvironment)) { | ||
| //If not specified status is preserved by default | ||
| preserveStatus = preserveStatus == null || preserveStatus; | ||
|
|
||
| //If not specified preserveCredentials is set to false by default | ||
| preserveCredentials = preserveCredentials != null && preserveCredentials; | ||
|
|
There was a problem hiding this comment.
The defaulting of preserveCredentials only occurs when gatewayEnvironment is empty. If a user supplies gatewayEnvironment, preserveCredentials may remain null. Consider applying preserveCredentials = Boolean.TRUE.equals(preserveCredentials); outside of the if block to ensure a consistent default.
| if (StringUtils.isEmpty(gatewayEnvironment)) { | |
| //If not specified status is preserved by default | |
| preserveStatus = preserveStatus == null || preserveStatus; | |
| //If not specified preserveCredentials is set to false by default | |
| preserveCredentials = preserveCredentials != null && preserveCredentials; | |
| // Ensure preserveCredentials is consistently defaulted | |
| preserveCredentials = Boolean.TRUE.equals(preserveCredentials); | |
| if (StringUtils.isEmpty(gatewayEnvironment)) { | |
| //If not specified status is preserved by default | |
| preserveStatus = preserveStatus == null || preserveStatus; |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml (2)
4336-4342: Add a default value for clarity.
It’s best practice to specify adefault: falseunder the boolean schema so that consumers know the fallback when the parameter is omitted:schema: type: boolean + default: false
4336-4342: Include an example for better documentation.
Consider addingexample: falseto illustrate usage in the OpenAPI spec:schema: type: boolean + example: falsecomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml (1)
4336-4342: Consider adding a default value forpreserveCredentials
Whilerequired: falsemakes the parameter optional, explicitly specifyingdefault: falsein the schema helps clients and tools understand the fallback behavior when the flag is omitted.components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/publisher-api.yaml (1)
4087-4093: Proposal: Add a default value for clarityConsider specifying
default: falseunder theschemablock so clients know what happens when the parameter is omitted:schema: type: boolean + default: false
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/ApisApi.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/ApisApiService.javais excluded by!**/gen/**
📒 Files selected for processing (4)
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/publisher-api.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (4, group4)
- GitHub Check: run-benchmark-test
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-carbon
🔇 Additional comments (6)
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml (2)
4336-4342: Ensure correct indentation to avoid YAML parsing errors.
Verify that the new parameter block aligns with the existing parameters in the list (same number of spaces before the leading- name). A misaligned indent can cause the OpenAPI spec to fail loading.
4336-4342:✅ Verification successful
Verify consistency across all Publisher API specs.
Ensure thatpreserveCredentialsis also added to thepublisher-api.yamlfiles in therest.api.publisher.v1andrest.api.utilmodules. You can run:
🏁 Script executed:
#!/bin/bash # Check presence of preserveCredentials in all Publisher API specs rg -l 'preserveCredentials' -g 'components/apimgt/**/src/main/resources/publisher-api.yaml'Length of output: 380
All Publisher API specs include
preserveCredentials
ThepreserveCredentialsquery parameter is present in:
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yamlcomponents/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/publisher-api.yamlNo further action required.
components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/publisher-api.yaml (2)
4087-4093: Schema definition and indentation look correctThe new
preserveCredentialsquery parameter is properly defined as an optional boolean with a clear description. The indentation aligns with the existing parameters.
4087-4093:✅ Verification successful
Verify propagation across all API specs
Ensure that
preserveCredentialsis also added to the Publisher API YAMLs in thecommonandpublisher.v1modules. You can run:
🏁 Script executed:
#!/bin/bash # Verify presence of preserveCredentials in all publisher-api.yaml files rg -l "preserveCredentials" -g "components/apimgt/**/publisher-api.yaml"Length of output: 361
Propagation verified across all Publisher API specs
Confirmed that
preserveCredentialsis present in thepublisher-api.yamlfiles for the following modules:
- components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/publisher-api.yaml
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml
- components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml
No further changes required.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java (2)
3784-3794: LGTM! Secure default handling implemented correctly.The parameter addition and default value handling follow established patterns in the codebase. The logic correctly defaults
preserveCredentialstofalsewhen not specified, which is the secure choice for credential preservation functionality.The null-safe boolean handling
preserveCredentials = preserveCredentials != null && preserveCredentials;is consistent with the existingpreserveStatusparameter pattern.
3803-3804:❓ Verification inconclusive
Verify downstream method signature and security controls.
The parameter passing looks correct. However, please ensure that:
- The downstream
importExportAPI.exportAPImethod signature supports thepreserveCredentialsparameter- Proper authorization checks are in place to control who can preserve credentials during export
- Exported credentials are handled securely (encrypted, access-controlled, etc.)
🏁 Script executed:
#!/bin/bash # Verify the downstream method signature supports the preserveCredentials parameter ast-grep --pattern $'exportAPI($$$, preserveCredentials, $$$)'Length of output: 63
I wasn’t able to locate the
exportAPIsignature with the previous ast-grep invocation. Let’s search the repository for both the method definition and theimportExportAPIreference:#!/bin/bash # 1. Find all occurrences of exportAPI (calls and definitions) rg -n "exportAPI" . # 2. Find where importExportAPI is declared to identify its type rg -n "importExportAPI" .
I still can’t confirm the
exportAPIsignature. Let’s locate both the method definition and theimportExportAPIdeclaration across the REST module:#!/bin/bash set -e # 1. Find all definitions and usages of exportAPI in the publisher module rg -n "exportAPI\s*\(" components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1 # 2. Find where importExportAPI is declared to identify its type rg -n "importExportAPI\s*=" -n components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1
Verify downstream signature and security controls
Please ensure the following before merging:
- Confirm that the
importExportAPI.exportAPImethod signature actually includes the newpreserveCredentialsparameter.- Verify proper authorization checks are in place to restrict who can preserve credentials during an API export.
- Ensure exported credentials are handled securely (e.g., encrypted at rest, access-controlled).
Introduces a query param preserveCredentials to export an API with endpoint credentails.