Intra-vendor model routing feature enhancements #12948
Conversation
…ds under metadata
…st-alpha Intra-vendor model routing feature enhancements
ashera96
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
Fix API Deployment errors
…st-alpha Fix apiKeyValue missing issue in synapse artifact
…st-alpha Intra-vendor model routing feature improvements
…st-alpha Improve error handling and set primary endpoint IDs on AI API creation flow
…st-alpha Add endpoint URL validation
Add support for failover policy
…ORGANIZATION in AM_API_ENDPOINTS
…st-alpha Add unique constraint on API_UUID, ENDPOINT_NAME, REVISION_UUID, and ORGANIZATION in AM_API_ENDPOINTS
WalkthroughThe changes span extensive refactoring and enhancements across the API management codebase. Documentation has been improved with updated JavaDoc comments and clearer constant values. New DTO classes and renamed models standardize endpoint representations, while metadata entries in LLM provider services have been revised to include boolean flags. Mediator classes now separate inbound/outbound flows and introduce failover handling. Additionally, SQL schema changes add unique constraints to improve data integrity. Template, export/import, and mapping utilities have been updated to support enhanced security, configuration management, and endpoint sequence handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AIAPIMediator
participant FailoverMediator
participant GatewayEndpoint
Client->>AIAPIMediator: Send API Request
AIAPIMediator->>AIAPIMediator: processInboundRequest()
AIAPIMediator->>FailoverMediator: Invoke failover handling
FailoverMediator->>GatewayEndpoint: Retrieve active endpoint details
GatewayEndpoint-->>FailoverMediator: Return endpoint info
FailoverMediator-->>AIAPIMediator: Provide failover configuration
AIAPIMediator->>AIAPIMediator: processOutboundResponse()
AIAPIMediator-->>Client: Return API Response
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (64)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/analytics/SynapseAnalyticsDataProvider.java (2)
600-637: Consider adding null check for response size parsing.The response size parsing could throw a NumberFormatException if the Content-Length header contains non-numeric values.
if (headers != null && headers.get(HttpHeaders.CONTENT_LENGTH) != null) { - responseSize = Integer.parseInt(headers.get(HttpHeaders.CONTENT_LENGTH).toString()); + try { + responseSize = Integer.parseInt(headers.get(HttpHeaders.CONTENT_LENGTH).toString()); + } catch (NumberFormatException e) { + log.warn("Invalid Content-Length header value", e); + } }
616-625: Consider consolidating exception handling.The IOException and XMLStreamException blocks have identical error handling logic. Consider consolidating them to reduce code duplication.
- } catch (IOException ex) { - //In case of an exception, it won't be propagated up,and set response size to 0 - log.error("Error occurred while building the message to" + - " calculate the response body size", ex); - } catch (XMLStreamException ex) { + } catch (IOException | XMLStreamException ex) { log.error("Error occurred while building the message to calculate the response" + " body size", ex); - }components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java (1)
87-90: Consider adding header value validation.While the addition of remaining token count metadata via headers is a good enhancement, consider adding validation to ensure the header value is a valid non-negative integer.
Example validation in a helper method:
private void validateRemainingTokenCount(String headerValue) { try { int tokenCount = Integer.parseInt(headerValue); if (tokenCount < 0) { throw new APIManagementException("Remaining token count must be non-negative"); } } catch (NumberFormatException e) { throw new APIManagementException("Invalid remaining token count format"); } }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
214-233: Good implementation, consider adding parameter validation and logging.The implementation follows the class's patterns and is well-documented. Consider these improvements:
- Add null checks for parameters
- Validate required API fields
- Add debug logging for better troubleshooting
Apply this diff to add the suggested improvements:
public static void setEndpointSequencesToBeRemoved(API api, GatewayAPIDTO gatewayAPIDTO) { + if (api == null || gatewayAPIDTO == null) { + throw new IllegalArgumentException("API and GatewayAPIDTO parameters cannot be null"); + } + + if (log.isDebugEnabled()) { + log.debug("Removing endpoint sequences for API: " + api.getId()); + } String productionEndpointSequence = APIUtil.getEndpointSequenceName(api) + AIAPIConstants.ENDPOINT_SEQUENCE + APIConstants.APIEndpoint.PRODUCTION; gatewayAPIDTO.setSequencesToBeRemove( addStringToList(productionEndpointSequence, gatewayAPIDTO.getSequencesToBeRemove())); String sandboxEndpointSequence = APIUtil.getEndpointSequenceName(api) + AIAPIConstants.ENDPOINT_SEQUENCE + APIConstants.APIEndpoint.SANDBOX; gatewayAPIDTO.setSequencesToBeRemove( addStringToList(sandboxEndpointSequence, gatewayAPIDTO.getSequencesToBeRemove())); + if (log.isDebugEnabled()) { + log.debug("Successfully removed endpoint sequences for API: " + api.getId()); + } }components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java (2)
787-807: Consider enhancing error descriptions for better debugging.While the error messages are clear, some error descriptions could be more detailed to aid in troubleshooting:
ERROR_UPDATING_API_ENDPOINTcould include more context about what failedERROR_ADDING_API_ENDPOINTcould specify what type of error occurredAPI_ENDPOINT_URL_INVALIDcould include validation criteria in the description- ERROR_UPDATING_API_ENDPOINT(902041, "Error while updating the API endpoint.", 500, - "Error while updating the API endpoint."), + ERROR_UPDATING_API_ENDPOINT(902041, "Error while updating the API endpoint.", 500, + "Error while updating the API endpoint. Details: %s"), - ERROR_ADDING_API_ENDPOINT(902043, "Failed to add endpoint to API.", 500, - "Error while adding API endpoint."), + ERROR_ADDING_API_ENDPOINT(902043, "Failed to add endpoint to API.", 500, + "Error while adding API endpoint. Reason: %s"), - API_ENDPOINT_URL_INVALID(902049, "Endpoint URL is invalid", 400, - "Endpoint URL is invalid"); + API_ENDPOINT_URL_INVALID(902049, "Endpoint URL is invalid", 400, + "Endpoint URL is invalid. URL must be a valid HTTP/HTTPS URL.");
787-807: Add section comment for endpoint-related error codes.For consistency with other sections (like "Auth related codes", "Labels related codes"), add a clear section comment for the endpoint-related error codes.
+ // API Endpoint related codes API_ENDPOINT_NOT_FOUND(902040, "Cannot find the required API endpoint details.", 404,components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java (1)
24696-24696: Fix typo in error message.There's a typo in the error message: "mappnigs" should be "mappings".
- handleException("Error while adding primary endpoint mappnigs for API : " + apiUUID, e); + handleException("Error while adding primary endpoint mappings for API : " + apiUUID, e);components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
8217-8257: Consider simplifying the primary endpoint mapping logic.The current implementation has some opportunities for improvement:
- Add null check for
api.getOrganization()to avoid potential NPE- Consider extracting the endpoint ID construction logic into a separate helper method
- The fallback logic could be simplified by combining the checks
Here's a suggested refactor:
private void populateAPIPrimaryEndpointsMapping(API api, String uuid) throws APIManagementException { String organization = api.getOrganization(); + if (organization == null) { + throw new APIManagementException("Organization cannot be null"); + } String currentApiUuid; String revisionUuid = null; APIRevision apiRevision = checkAPIUUIDIsARevisionUUID(uuid); if (apiRevision != null && apiRevision.getApiUUID() != null) { currentApiUuid = apiRevision.getApiUUID(); revisionUuid = apiRevision.getRevisionUUID(); } else { currentApiUuid = uuid; } + // Helper method to construct endpoint ID + private String constructEndpointId(String apiUuid, String type) { + return apiUuid + APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR + type; + } List<String> endpointIds = apiMgtDAO.getPrimaryEndpointUUIDByAPIId(currentApiUuid); if (endpointIds != null && !endpointIds.isEmpty()) { String prodEndpointId = constructEndpointId(currentApiUuid, APIConstants.APIEndpoint.PRODUCTION); String sandboxEndpointId = constructEndpointId(currentApiUuid, APIConstants.APIEndpoint.SANDBOX); for (String endpointId : endpointIds) { if (endpointId.equals(prodEndpointId)) { api.setPrimaryProductionEndpointId(endpointId); } else if (endpointId.equals(sandboxEndpointId)) { api.setPrimarySandboxEndpointId(endpointId); } } // Fallback only if endpoints not found in the list if (api.getPrimaryProductionEndpointId() == null) { api.setPrimaryProductionEndpointId( apiMgtDAO.getPrimaryEndpointUUIDByApiIdAndEnv(currentApiUuid, APIConstants.APIEndpoint.PRODUCTION, revisionUuid, organization)); } if (api.getPrimarySandboxEndpointId() == null) { api.setPrimarySandboxEndpointId( apiMgtDAO.getPrimaryEndpointUUIDByApiIdAndEnv(currentApiUuid, APIConstants.APIEndpoint.SANDBOX, revisionUuid, organization)); } } }features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql (1)
1598-1599: Ensure the new unique constraint meets data integrity requirements.The added unique constraint on
(API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)guarantees that duplicate endpoint entries across an organization and API revision are prevented. Please verify that the existing data complies with this constraint before deployment and that the behavior for potentialNULLvalues in theENDPOINT_NAMEcolumn is as expected. If you require that every endpoint have a non-null name, consider adding aNOT NULLmodifier to this column.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql (1)
1537-1538: Unique Constraint Addition on AM_API_ENDPOINTSThe new unique constraint on the combination of
(API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)increases data integrity by preventing duplicate endpoint entries for a given API revision and organization. Please verify that this grouping truly represents the unique identity of an endpoint in your business model and that no legacy data violates this rule.Optional Improvement: Consider explicitly naming the constraint to improve maintainability and clarity (e.g., use
CONSTRAINT UK_AM_API_ENDPOINTS UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)). This makes future modifications or error messages more descriptive.A sample diff for naming the constraint:
- UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION) + CONSTRAINT UK_AM_API_ENDPOINTS UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)Overall, these changes align with your objectives of enhancing data integrity across the API management schema.
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql (1)
1722-1723: Enforce Endpoint Uniqueness via Added ConstraintThe added unique constraint
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)
is a well-considered enhancement. It enforces data integrity by preventing duplicate endpoint configurations across API UUIDs, endpoint names, revisions, and organizations. This aligns with similar changes made in other database scripts and supports multi-tenant consistency.A few points for consideration:
- Nullable Column: Currently,
ENDPOINT_NAMEis defined without a NOT NULL constraint. In MySQL, multiple NULLs can bypass uniqueness. Verify that this is the intended behavior or consider markingENDPOINT_NAMEas NOT NULL if a valid endpoint name is always required.- Application Impact: Ensure that any application logic or ORM mappings that reference the AM_API_ENDPOINTS table are updated to account for this new uniqueness constraint.
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql (1)
1874-1875: Unique Constraint Addition on AM_API_ENDPOINTSThe added unique constraint
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)reinforces data integrity by ensuring that no two rows share the same combination of API identifier, endpoint name, revision identifier, and organization. This change supports the intra-vendor model routing enhancements by preventing duplicate endpoint configurations.
A couple of points to consider:
- Verify that the
ORGANIZATIONcolumn’s nullability aligns with business expectations. In PostgreSQL, multiple NULL values are not considered equal in a unique constraint, which might allow duplicates ifORGANIZATIONis not explicitly constrained to be non-null.- Ensure that similar schema changes in other database scripts (H2, DB2, MySQL, MSSQL, Oracle) maintain consistent behavior regarding this unique constraint.
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql (1)
1755-1756: Unique Constraint Addition ReviewThe new unique constraint
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)is correctly added to enforce data integrity by ensuring that each endpoint configuration for a given API (as defined by the combination of API_UUID, ENDPOINT_NAME, REVISION_UUID, and ORGANIZATION) remains unique. This change supports the objective of preventing duplicate endpoint configurations for enhanced routing behavior.
Points to Consider:
- Nullability of ENDPOINT_NAME: Currently, the
ENDPOINT_NAMEcolumn is defined asVARCHAR(255)without an explicit NOT NULL constraint. In SQL Server, unique constraints treat multiple NULLs as distinct. If the intention is to strictly prevent duplicate entries, you may want to enforceENDPOINT_NAMEas NOT NULL.- Constraint Naming: For easier maintenance and debugging, consider explicitly naming the unique constraint (e.g., using a name like
UQ_AM_API_ENDPOINTS_APIUUID_ENDPOINTNAME_REVISION_ORG). This makes future troubleshooting or schema modifications more straightforward.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql (1)
2484-2485: Enhance Data Integrity with Unique Constraint in AM_API_ENDPOINTSThe addition of the unique constraint
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)
is a robust enhancement that prevents duplicate records for the same API endpoint based on the combination of API_UUID, ENDPOINT_NAME, REVISION_UUID, and ORGANIZATION. This constraint works alongside the existing primary key on(ENDPOINT_UUID, REVISION_UUID)to further safeguard data integrity.Suggestion: Verify if the
ENDPOINT_NAMEcolumn should be allowed to remain nullable. In Oracle, multiple NULL values in a unique constraint are not regarded as duplicates—which might be acceptable or could lead to unintended duplicates if the business logic expects an endpoint name to always be provided. If uniqueness should be enforced regardless, consider specifyingNOT NULLforENDPOINT_NAME.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql (1)
2484-2485: Review the Newly Added Unique ConstraintThe new unique constraint:
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)enforces that no two records in the
AM_API_ENDPOINTStable share the same combination of API identifier, endpoint name, revision identifier, and organization. This is a positive step toward improving data integrity by preventing accidental duplicate endpoint entries.Points to Consider:
- Business Logic Alignment: Please verify that this composite uniqueness precisely reflects your business requirements. In particular, ensure that an endpoint’s uniqueness should indeed depend on the
ENDPOINT_NAMEeven when it's allowed to be NULL.- Column Nullability: The
ENDPOINT_NAMEcolumn is defined without an explicitNOT NULLconstraint. Since Oracle permits multiple NULLs in a unique constraint, if you intend that every endpoint record must have a non-null name (and therefore the uniqueness should be strictly enforced), you might consider altering the column to beNOT NULLor using a function-based unique index to handle NULL values.- Data Migration Impact: Ensure that existing data complies with this new constraint before deployment, or plan for data cleanup/migration if necessary.
components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/util/APIMUtil.java (2)
180-189: Update method documentation to include the new parameter behavior.The JavaDoc should document that credentials are not preserved during export by default.
* @param apiId API ID * @param revisionNo Revision number, if empty latest revision will be exported * @param organization Organization + * @note Credentials are not preserved during export by default * @return API project zip as a byte array * @throws APIMGovernanceException If an error occurs while getting the API project
491-492: Address the TODO comment for API type mapping.The comment indicates incomplete mapping between API types and ExtendedArtifactType. This should be addressed to ensure comprehensive type support.
Would you like me to help create an issue to track the completion of API type mappings?
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java (2)
78-79: Document the suspend duration unit conversion.The multiplication by 1000 suggests a conversion from seconds to milliseconds. This should be documented for clarity.
messageContext.setProperty(APIConstants.AIAPIConstants.TARGET_ENDPOINT, nextEndpoint.getEndpointId()); messageContext.setProperty(APIConstants.AIAPIConstants.TARGET_MODEL, nextEndpoint.getModel()); + // Convert suspend duration from seconds to milliseconds messageContext.setProperty(APIConstants.AIAPIConstants.SUSPEND_DURATION, endpoints.getSuspendDuration() * 1000);
91-93: Update JavaDoc to reflect the new return type.The JavaDoc still references RBEndpointDTO but the method now returns ModelEndpointDTO.
/** * Selects an endpoint using a round-robin selection mechanism. * * @param endpoints List of active endpoints. - * @return The selected RBEndpointDTO based on round-robin order. + * @return The selected ModelEndpointDTO based on round-robin order. */ private ModelEndpointDTO getRoundRobinEndpoint(List<ModelEndpointDTO> endpoints) {components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
4520-4530: Add input validation and improve documentation for endpoint sequence name generationThe new utility method needs some improvements:
+ /** + * Return the endpoint sequence name for an API formatted as <API_NAME>--<API_VERSION> + * Used for managing endpoint sequences during API undeployment. + * + * @param api API object to generate sequence name for + * @return Formatted endpoint sequence name + * @throws APIManagementException if API object or required fields are null + */ public static String getEndpointSequenceName(API api) { + if (api == null || api.getId() == null || + api.getId().getApiName() == null || + api.getId().getVersion() == null) { + throw new APIManagementException("API name and version are required"); + } + StringBuilder sb = new StringBuilder(); + sb.append(api.getId().getApiName()) + .append("--") + .append(api.getId().getVersion()); + return sb.toString(); }
- Add input validation to handle null API object and fields
- Add JavaDoc explaining the format and usage
- Use StringBuilder for better performance with string concatenation
- Consider adding unit tests to verify the formatting
10258-10300: Improve URL validation methods with better abstraction and error handlingThe endpoint URL validation methods need some improvements:
- public static boolean validateEndpointURLs(ArrayList<String> endpoints) { + public static boolean validateEndpointURLs(List<String> endpoints) { for (String endpoint : endpoints) { if (!validateEndpointURL(endpoint)) { return false; } } return true; } + private static final String JMS_PREFIX = "jms:"; + private static final String CONSUL_PREFIX = "consul("; public static boolean validateEndpointURL(String endpoint) { long validatorOptions = UrlValidator.ALLOW_2_SLASHES + UrlValidator.ALLOW_ALL_SCHEMES + UrlValidator.ALLOW_LOCAL_URLS; RegexValidator authorityValidator = new RegexValidator(".*"); UrlValidator urlValidator = new UrlValidator(authorityValidator, validatorOptions); - if (!endpoint.startsWith("jms:") && !endpoint.startsWith("consul(") && !endpoint.contains("{") && + if (!endpoint.startsWith(JMS_PREFIX) && !endpoint.startsWith(CONSUL_PREFIX) && !endpoint.contains("{") && !endpoint.contains("}") && !urlValidator.isValid(endpoint)) { try { URL endpointUrl = new URL(endpoint); URI endpointUri = new URI(endpointUrl.getProtocol(), endpointUrl.getAuthority(), endpointUrl.getPath(), endpointUrl.getQuery(), null); if (!urlValidator.isValid(endpointUri.toString())) { - log.error("Invalid endpoint url " + endpointUrl); + log.error("Invalid endpoint URL format: " + endpointUrl); return false; } } catch (URISyntaxException | MalformedURLException e) { - log.error("Error while parsing the endpoint url " + endpoint); + log.error("Error parsing endpoint URL '" + endpoint + "': " + e.getMessage()); return false; } } return true; }
- Use List interface instead of ArrayList for better abstraction
- Add constants for URL prefixes to avoid magic strings
- Improve error messages with more details
- Add JavaDoc explaining validation rules and special cases
- Consider adding unit tests for different URL formats
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/LLMProviderMetadata.java (1)
39-44: Enhance the deprecation notice with more context.While the deprecation notice correctly references the new constructor, it would be helpful to explain why this constructor is deprecated and the benefits of using the new one.
/** - * @deprecated This constructor is deprecated. Use {@link #LLMProviderMetadata(String, String, String, boolean)} - * instead. + * @deprecated This constructor is deprecated as it doesn't support specifying whether the metadata attribute is + * required. Use {@link #LLMProviderMetadata(String, String, String, boolean)} instead to explicitly + * control the required status of the metadata attribute. */components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/ImportExportAPIServiceImpl.java (1)
58-128: Consider documenting the security implications of credential preservation.The addition of the
preserveCredentialsparameter enhances the API export functionality by providing control over credential handling. Consider:
- Adding documentation about the security implications of preserving credentials during export
- Implementing audit logging for credential preservation actions
- Adding configuration options to control this feature at a global level
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (4)
75-79: Document the time unit conversion.The suspend duration is multiplied by 1000, suggesting a conversion from seconds to milliseconds. This should be documented for clarity.
messageContext.setProperty(APIConstants.AIAPIConstants.TARGET_ENDPOINT, nextEndpoint.getEndpointId()); messageContext.setProperty(APIConstants.AIAPIConstants.TARGET_MODEL, nextEndpoint.getModel()); messageContext.setProperty(APIConstants.AIAPIConstants.SUSPEND_DURATION, - endpoints.getSuspendDuration() * 1000); + endpoints.getSuspendDuration() * 1000); // Convert seconds to milliseconds
91-93: Update JavaDoc to reflect new return type.The JavaDoc still references RBEndpointDTO but the method now returns ModelEndpointDTO.
- * @return The selected RBEndpointDTO based on weight. + * @return The selected ModelEndpointDTO based on weight.
98-102: Extract minimum weight as a constant.The minimum weight value of 0.1f is hardcoded. Consider extracting it as a private static final constant for better maintainability.
+ private static final float MIN_ENDPOINT_WEIGHT = 0.1f; + private ModelEndpointDTO getWeightedRandomEndpoint(List<ModelEndpointDTO> endpoints) { float totalWeight = 0.0f; List<Float> cumulativeWeights = new ArrayList<>(); for (ModelEndpointDTO endpoint : endpoints) { - double weight = Math.max(endpoint.getWeight(), 0.1f); + double weight = Math.max(endpoint.getWeight(), MIN_ENDPOINT_WEIGHT); totalWeight += (float) weight; cumulativeWeights.add(totalWeight); }
104-105: Use ThreadLocalRandom for better thread safety.Replace Random with ThreadLocalRandom for improved performance in concurrent scenarios.
- Random random = new Random(); - float randomValue = random.nextFloat() * totalWeight; + float randomValue = java.util.concurrent.ThreadLocalRandom.current().nextFloat() * totalWeight;components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java (1)
86-89:❓ Verification inconclusive
Consider adding error handling for missing remaining token count.
Good addition of remaining token count metadata. Since this is an optional field (required=false) and sourced from headers, consider adding error handling in the consuming code to gracefully handle cases where this value is missing.
Run this script to check if error handling exists for missing header values:
🏁 Script executed:
#!/bin/bash # Search for error handling related to remaining token count rg -A 5 "LLM_PROVIDER_SERVICE_METADATA_REMAINING_TOKEN_COUNT" --type javaLength of output: 4684
Review Update: Ensure Graceful Handling of Missing Token Count
The new metadata entry for the remaining token count is a valuable addition. However, our repository search indicates that none of the related provider services (OpenAiLLMProviderService, AzureOpenAiLLMProviderService, and MistralAiLLMProviderService) include explicit error handling for scenarios where the corresponding header value is absent. To safeguard against potential issues when the remaining token count isn’t provided, please consider incorporating error handling in the consuming code. This could involve checking for the presence of the header value before its usage and applying a sensible fallback when it’s missing.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/dto/EndpointDTO.java (1)
2-2: Update the copyright year.The copyright year is set to 2025, which is in the future. Consider updating it to the current year (2024) or the year when the code was created.
- * Copyright (c) 2025, WSO2 LLC (http://www.wso2.org) All Rights Reserved. + * Copyright (c) 2024, WSO2 LLC (http://www.wso2.org) All Rights Reserved.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java (2)
45-47: Fix inconsistent class name in debug log.The debug log message refers to "FallbackDecisionMediator" but the class name is "FailoverMediator".
- log.debug("FallbackDecisionMediator: Initialized."); + log.debug("FailoverMediator: Initialized.");
75-78: Extract time unit conversion to constants.The conversion from seconds to milliseconds is hardcoded. Consider extracting this to a constant or utility method.
+ private static final int MILLISECONDS_PER_SECOND = 1000; + messageContext.setProperty(AIAPIConstants.SUSPEND_DURATION, - policyConfig.getSuspendDuration() * 1000); + policyConfig.getSuspendDuration() * MILLISECONDS_PER_SECOND); messageContext.setProperty(AIAPIConstants.ENDPOINT_TIMEOUT, - policyConfig.getRequestTimeout() * 1000); + policyConfig.getRequestTimeout() * MILLISECONDS_PER_SECOND);components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/RBPolicyConfigDTO.java (3)
2-2: Update the copyright year.The copyright year is set to 2025, which is in the future. Please update it to the current year (2024) or earlier.
- * Copyright (c) 2025, WSO2 LLC (http://www.wso2.org) All Rights Reserved. + * Copyright (c) 2024, WSO2 LLC (http://www.wso2.org) All Rights Reserved.
26-30: Consider making the class immutable and adding validation.To improve robustness and thread safety, consider:
- Making the class immutable by:
- Making fields final
- Using a constructor instead of setters
- Returning defensive copies of lists
- Adding validation for the suspend duration and endpoint lists
Here's a suggested implementation:
public class RBPolicyConfigDTO { - private List<ModelEndpointDTO> production; - private List<ModelEndpointDTO> sandbox; - private Long suspendDuration; + private final List<ModelEndpointDTO> production; + private final List<ModelEndpointDTO> sandbox; + private final Long suspendDuration; + + public RBPolicyConfigDTO(List<ModelEndpointDTO> production, + List<ModelEndpointDTO> sandbox, + Long suspendDuration) { + this.production = production != null ? + new ArrayList<>(production) : new ArrayList<>(); + this.sandbox = sandbox != null ? + new ArrayList<>(sandbox) : new ArrayList<>(); + if (suspendDuration != null && suspendDuration < 0) { + throw new IllegalArgumentException( + "Suspend duration cannot be negative"); + } + this.suspendDuration = suspendDuration; + }
37-40: Return defensive copies of lists to prevent modification of internal state.The list getters currently return direct references to the internal lists, which could allow external code to modify the internal state. Consider returning defensive copies instead.
public List<ModelEndpointDTO> getProduction() { - return production; + return production != null ? + new ArrayList<>(production) : new ArrayList<>(); } public List<ModelEndpointDTO> getSandbox() { - return sandbox; + return sandbox != null ? + new ArrayList<>(sandbox) : new ArrayList<>(); }Also applies to: 57-60
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/ModelEndpointDTO.java (2)
26-28: Consider adding validation for weight field.The weight field is used for load balancing but lacks validation. Consider adding validation in the setter to ensure the weight is non-negative.
private double weight; + +/** + * Validates that the weight is non-negative. + * + * @param weight the weight to validate + * @throws IllegalArgumentException if weight is negative + */ +private void validateWeight(double weight) { + if (weight < 0) { + throw new IllegalArgumentException("Weight must be non-negative"); + } +}
85-88: Add weight validation in setter.Add validation to ensure weight is non-negative.
public void setWeight(double weight) { + validateWeight(weight); this.weight = weight; }components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyDeploymentConfigDTO.java (2)
22-31: Consider adding validation for fallback endpoints.While the structure is good, consider adding validation to ensure:
- The target model endpoint is not null
- The fallback endpoints list is not empty when provided
+/** + * Validates the fallback model endpoints. + * + * @param endpoints the endpoints to validate + * @throws IllegalArgumentException if the list is empty + */ +private void validateFallbackEndpoints(List<ModelEndpointDTO> endpoints) { + if (endpoints != null && endpoints.isEmpty()) { + throw new IllegalArgumentException("Fallback endpoints list cannot be empty"); + } +}
68-71: Add validation in setter.Add validation to ensure fallback endpoints list is not empty when provided.
public void setFallbackModelEndpoints(List<ModelEndpointDTO> fallbackModelEndpoints) { + validateFallbackEndpoints(fallbackModelEndpoints); this.fallbackModelEndpoints = fallbackModelEndpoints; }components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java (2)
47-47: Add documentation for theOM_ELEMENT_NAMEconstant.The purpose and usage context of this constant is not clear. Please add JavaDoc comments to explain its intended use.
103-122: Consider grouping endpoint-related constants in a nested class.The growing number of endpoint and failover related constants suggests they could be better organized in a dedicated nested class, similar to how AI-specific constants are grouped in
AIAPIConstants.Example refactoring:
public class APIConstants { // ... other constants ... + public static class EndpointConstants { + public static final String TARGET_MODEL = "TARGET_MODEL"; + public static final String TARGET_ENDPOINT = "TARGET_ENDPOINT"; + public static final String FAILOVER_TARGET_MODEL = "FAILOVER_TARGET_MODEL"; + // ... other endpoint related constants ... + } public static class AIAPIConstants { // ... existing AI constants ... } }components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java (1)
2506-2536: Consider additional null checks and exception handling for better resilience.
- If the JSON/YAML file is missing the "data" key or contains invalid JSON,
new JsonParser().parse(jsonContent).getAsJsonObject().get(APIConstants.DATA)can throw aNullPointerExceptionor a parsing exception.- You may want to handle potential JSON parsing errors (e.g.,
JsonSyntaxException) and provide a more descriptive error message or ignore the file gracefully.- In scenarios where “data” is absent or empty, logging a warning (instead of debug) can help clarify potential misconfiguration to end-users.
Below is an illustrative diff to add basic null checks. Feel free to adapt:
- JsonElement endpointsElement = new JsonParser().parse(jsonContent).getAsJsonObject().get(APIConstants.DATA); - JsonArray endpointsArray = endpointsElement.getAsJsonArray(); + JsonObject rootObject; + try { + rootObject = JsonParser.parseString(jsonContent).getAsJsonObject(); + } catch (JsonSyntaxException ex) { + throw new APIManagementException("Invalid JSON structure in endpoint file", ex); + } + JsonElement endpointsElement = rootObject.get(APIConstants.DATA); + if (endpointsElement == null || !endpointsElement.isJsonArray()) { + log.warn("No valid endpoints found under the 'data' key, skipping"); + return new ArrayList<>(); + } + JsonArray endpointsArray = endpointsElement.getAsJsonArray();components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (3)
57-57: Remove the unused import.It appears that
FailoverPolicyDeploymentConfigDTOis never referenced in this class. Removing it can keep the code clean.
1762-1776: Consider returning an empty list instead ofnull.This helper method returns
nullif no active endpoints remain. Returning an empty list allows the caller to avoid null checks.
1836-1836: Update the Javadoc to match theModelEndpointDTOparameter.The method’s Javadoc currently references
RBEndpointDTO, but the parameter type isModelEndpointDTO. Aligning the documentation prevents confusion.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java (3)
3369-3371: Minor readability note on method return.This closing block simply delegates to another method. While functionally acceptable, consider adding a comment indicating that the parent method immediately delegates to
handleEndpointSecurity(endpointSecurity)for clarity.
3380-3416: Refactor repeated sandbox/production code blocks.The code that zeros out credentials for sandbox and production endpoints repeats almost identically. Consider extracting this into a helper method to avoid duplication, reduce error-proneness, and improve clarity.
3417-3447: Consider centralizing sandbox/production decryption logic.This method re-implements similar logic for both sandbox and production endpoints. You could factor out the common code to minimize repetition and reduce maintenance overhead. Also, consider clarifying how errors in one environment should affect the other in case of decryption failures.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointsContext.java (1)
74-78: Context population with endpoint data.
Consider populating an empty list rather thannullto avoid velocity template issues ifthis.endpointsis uninitialized.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.java (2)
367-402: New method for endpoint configuration string generation.
- Reusing the existing Velocity context is effective.
- Consider warning or logging if
endpointsis empty.- Testing various endpoint scenarios (e.g., single, multiple, none) could improve reliability.
404-425: Dedicated context creation for endpoints.
This design centralizes endpoint context handling. Consider merging logic withcreateAIAPIConfigContextif there's overlap to avoid duplication.components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java (2)
2-2: Update the copyright year.The copyright year is set to 2025, which is in the future. Please update it to the current year.
-* Copyright (c) 2025 WSO2 LLC. (http://www.wso2.org) All Rights Reserved. +* Copyright (c) 2024 WSO2 LLC. (http://www.wso2.org) All Rights Reserved.
73-106: Improve getter methods documentation and formatting.The getter methods could be improved by:
- Adding JavaDoc documentation
- Removing unnecessary blank lines
+ /** + * Gets the deployment stage of the endpoint. + * + * @return The deployment stage (PRODUCTION or SANDBOX) + */ public String getDeploymentStage() { - return deploymentStage; }components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java (2)
997-1024: Robust default endpoint selection for AI APIs.
This logic properly selects default production/sandbox endpoints and adds them to the gateway sequences. Consider verifying cases where no default endpoint exists to avoid partial or empty configurations.
1303-1309: Endpoint naming may benefit from normalization.
Current naming patterngetEndpointKey(api) + "_API_LLMEndpoint_" + endpointDTO.getId()might cause collisions ifendpointDTO.getId()includes special characters.Consider a quick slugging mechanism to avoid unexpected special characters:
- endpoint.setName(getEndpointKey(api) + "_API_LLMEndpoint_" + endpointDTO.getId()); + String normalizedId = endpointDTO.getId().replaceAll("[^A-Za-z0-9_\\-]", "_"); + endpoint.setName(getEndpointKey(api) + "_API_LLMEndpoint_" + normalizedId);components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (5)
92-130: Consider narrower exception handling
CatchingExceptionat a broad level may obscure handling specific, actionable errors. Consider catching only known exceptions (e.g.,APIManagementException,IOException) or rethrowing a custom wrapper to better handle different failure scenarios.
164-193: Function name clarity
RenamingprepareForFailoverIfNeededto something more explicit, likeprepareRequestForFailoverIfNeeded, could help future maintainers quickly grasp its role.
269-287: Handling non-JSON
Returningnullfor unsupported content types is suitable. Consider adding a debug log to indicate that extraction was bypassed for easier troubleshooting.
312-338: Potential performance overhead
Repeatedly parsing large JSON payloads via JsonPath could become expensive. If performance becomes critical, adopting a streaming approach or caching partial parses may help.
795-817: Consolidate payload logic
Consider refactoring repeated content-type checks to a helper to reduce branching here.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.java (5)
930-939: Clarify method scope in Javadoc.The Javadoc accurately describes the high-level purpose of encrypting API key credentials. However, consider clarifying that both production and sandbox endpoint credentials are handled within the same logic, just conditioned by deployment stage. This helps make the documentation more explicit.
940-1017: Reduce duplication in the encryption logic.These lines duplicate some of the existing encryption checks for production and sandbox endpoints, mirroring the logic seen in other methods. Extracting common encryption functionality into a helper method would simplify maintenance and reduce the chance of divergent behavior across the codebase.
+ // Example snippet of a possible refactor: + private static void encryptApiKey(String keyType, + Map endpointSecurityMap, + String oldSecret, + CryptoUtil cryptoUtil) throws CryptoException, APIManagementException { + // Shared encryption logic goes here... + }
1030-1135: Avoid repeated production/sandbox checks.Similar to the API key credential encryption method, this OAuth encryption block repeats production vs. sandbox conditionals for each environment. Consider refactoring them into a reusable utility. This also helps ensure a uniform approach to error handling and logging.
3308-3395: Factor out repeated encryption logic.In
updateAPIEndpoint, a sizeable portion of production/sandbox credential checks overlap with methods already provided for encrypting credentials. Consolidating this logic into a shared helper would reduce code repetition and potential inconsistencies.
3408-3452: Centralize environment validation.While this method correctly checks for a valid environment stage (
PRODUCTIONorSANDBOX), the encryption logic again replicates patterns found in other methods. Centralizing environment detection and encryption in one place will keep the code DRY and more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/DesignAssistantAPIPayloadResponseDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/DesignAssistantChatQueryDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/DesignAssistantChatResponseDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/DesignAssistantGenAPIPayloadDTO.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/DesignAssistantApi.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/DesignAssistantApiService.javais excluded by!**/gen/**
📒 Files selected for processing (56)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIAdmin.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java(4 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIProvider.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/LLMProviderMetadata.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/dto/EndpointDTO.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyDeploymentConfigDTO.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/ModelEndpointDTO.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/RBEndpointsPolicyDTO.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/RBPolicyConfigDTO.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/API.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/InMemoryAPIDeployer.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/Utils.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/analytics/SynapseAnalyticsDataProvider.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/DataHolder.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/TenantServiceCreator.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java(8 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/util/APIMUtil.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java(4 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java(7 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/template/APITemplateBuilder.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/ImportExportAPIServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java(12 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java(4 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ExportUtils.java(5 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.java(8 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointsContext.java(3 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(4 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/azure_api.yaml(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2(0 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql(1 hunks)features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql(1 hunks)
🔥 Files not summarized due to errors (1)
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/azure_api.yaml: Error: Disallowed special token found: <|endoftext|>
💤 Files with no reviewable changes (5)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/RBEndpointsPolicyDTO.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/Utils.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/DataHolder.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/internal/TenantServiceCreator.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2
✅ Files skipped from review due to trivial changes (2)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIAdmin.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/API.java
🔇 Additional comments (110)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/InMemoryAPIDeployer.java (1)
365-365: LGTM! The endpoint sequence cleanup is properly integrated.The addition of
setEndpointSequencesToBeRemovedensures proper cleanup of endpoint sequences during API undeployment, which is consistent with the existing pattern of removing custom sequences and backend configurations.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/analytics/SynapseAnalyticsDataProvider.java (1)
451-500: LGTM! The metadata key change improves clarity.The change from
LLM_PROVIDER_SERVICE_METADATA_MODELtoLLM_PROVIDER_SERVICE_METADATA_RESPONSE_MODELbetter reflects the specific nature of the model metadata being retrieved. The implementation maintains good practices:
- Uses Optional for null-safe handling
- Consistent error handling across all token count retrievals
- Clear separation of metadata and token usage information
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java (2)
68-70: LGTM! Good separation of request model metadata.The separation of model metadata into request-specific entries with optional requirement improves clarity and flexibility in the metadata structure.
71-74: LGTM! Good addition of response model metadata.The addition of required response model metadata complements the request/response separation and ensures proper validation.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIProvider.java (2)
1601-1610: LGTM! Documentation improvements enhance clarity.The JavaDoc changes improve clarity and consistency by:
- Using more consistent terminology in the return description
- Simplifying the exception description while maintaining its meaning
1613-1618: LGTM! Documentation improvements enhance consistency.The JavaDoc changes improve the documentation by:
- Making the method description more concise
- Ensuring consistent capitalization in parameter descriptions
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/GatewayUtils.java (1)
22-22: LGTM!The import follows the existing pattern and is correctly used in the new method.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java (1)
787-807: Error code structure and HTTP status codes are well-defined.The new endpoint-related error codes:
- Follow the established 902xxx numbering pattern
- Use appropriate HTTP status codes (404 for not found, 400 for invalid requests, 500 for server errors)
- Maintain consistency with the existing error code structure
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java (7)
19110-19116: LGTM! SQL constant name change aligns with key-type-based endpoint retrieval.The change from environment-based to key-type-based endpoint retrieval is consistent with the broader changes in endpoint handling.
20680-20686: LGTM! Consistent SQL constant name change for revision-specific endpoint retrieval.The change maintains consistency with the key-type-based approach introduced earlier.
24443-24449: LGTM! Improved try-with-resources formatting.The formatting change enhances code readability while maintaining the same functionality.
24533-24540: LGTM! Improved method signature formatting.The formatting change enhances readability of the long method signature.
24604-24614: LGTM! Enhanced error message with API UUID.The error message now provides better context for debugging by including the API UUID.
24667-24729: LGTM! Well-implemented new methods for endpoint management.The new methods are well-structured with:
- Efficient batch operations for default mappings
- Proper transaction and resource handling
- Clear error messages
24743-24749: LGTM! Enhanced error message with API UUID.The error message now provides better context for debugging by including the API UUID.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
664-674: LGTM! Clean implementation of primary endpoint mapping for AI APIs.The method follows single responsibility principle and has good separation of concerns by delegating the actual endpoint mapping to the DAO layer.
components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sql (1)
1322-1323: Enhanced Unique Constraint on AM_API_ENDPOINTS TableThe new unique constraint—
UNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)—has been added immediately after the primary key declaration. This improvement strengthens data integrity by ensuring that every combination of API_UUID, ENDPOINT_NAME, REVISION_UUID, and ORGANIZATION is unique. Please verify that no legacy data violates this rule and that the insertion logic in tests (and possibly in production) correctly handles this constraint.features/apimgt/org.wso2.carbon.apimgt.keymanager.feature/src/main/resources/sql/h2.sql (1)
1323-1324: Consistent Unique Constraint Enhancement in AM_API_ENDPOINTSIn this file the same unique constraint on the combination of
(API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)has been introduced. This change is consistent with other database dialects used within the project and reinforces the integrity of endpoint records. Be sure to test that all DML operations respect this new constraint and that any potential conflicts are properly handled.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql (1)
2352-2353: Unique Constraint Addition in AM_API_ENDPOINTSThe new unique constraint on
(API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)is a valuable enhancement to enforce data integrity. This additional constraint will help prevent duplicate endpoint entries across different APIs, revisions, and organizations. Please ensure that any existing data complies with this rule before applying the migration to avoid runtime errors or conflicts.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql (1)
2480-2481: Unique Constraint Addition on AM_API_ENDPOINTS:
The new constraintUNIQUE (API_UUID, ENDPOINT_NAME, REVISION_UUID, ORGANIZATION)ensures that duplicate endpoint configurations for the same API (distinguished by its revision and organization) cannot be inserted. This change enhances data integrity by enforcing an extra uniqueness criterion beyond the existing primary key.
Points to consider:
- NULL values: Verify whether
ENDPOINT_NAMEandORGANIZATIONshould allow NULLs. In Oracle, unique constraints permit multiple rows with NULLs, which might inadvertently allow duplicates if these fields are not required. If uniqueness is critical for these columns, consider addingNOT NULLconstraints.- Consistency Across Dialects: Confirm that a similar constraint is implemented for other supported databases so that schema behavior remains consistent.
components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/util/APIMUtil.java (1)
205-208:❓ Verification inconclusive
Verify the impact of disabling credential preservation.
The
falseparameter for credential preservation might affect existing API exports. Please ensure this change aligns with security requirements and doesn't break existing functionality.
🏁 Script executed:
#!/bin/bash # Search for other instances of exportApi to ensure consistent credential handling ast-grep --pattern 'ExportUtils.exportApi($$$)'Length of output: 2218
Check Consistency of Credential Preservation in API Exports
In
APIMUtil.java(lines 205-208) the export call is using a hard-codedfalsefor credential preservation, whereas similar export calls inImportExportAPIServiceImpl.javapass a dynamicpreserveCredentialsvalue. Please verify whether intentionally disabling credential preservation in this context aligns with the security requirements and legacy behavior. Determine if this discrepancy is deliberate or if the credential preservation flag should be updated to ensure consistent functionality.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java (1)
26-27: LGTM! Import changes align with the new data model.The updated imports correctly reflect the transition to the new endpoint configuration model.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/LLMProviderMetadata.java (4)
34-35: LGTM! Good choice of default value.The default value of
truemaintains backward compatibility, ensuring existing metadata entries remain valid.
53-61: LGTM! Well-structured constructor.The constructor maintains a consistent parameter order and properly handles JSON serialization/deserialization.
93-99: LGTM! Methods follow Java conventions.The getter uses the correct
isprefix for boolean fields, and both methods are properly implemented.
23-100:✅ Verification successful
Verify updates in dependent classes.
The AI summary indicates changes in related files. Let's verify that all usages of
LLMProviderMetadatahave been updated to handle the newrequiredfield.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of LLMProviderMetadata to ensure they're updated # Find all Java files that use LLMProviderMetadata echo "Files using LLMProviderMetadata:" rg -l "LLMProviderMetadata" --type java # Find constructor usages to check if any still use the deprecated version echo -e "\nConstructor usages:" rg "new LLMProviderMetadata\(" --type java -A 3Length of output: 15864
All dependent usages verified: Updated constructor consistently applied.
After reviewing the invoking files, we confirmed that all usages of
LLMProviderMetadatahave been updated to use the constructor with the additionalrequiredfield. No instances of the deprecated three-parameter constructor remain.
- In AzureOpenAiLLMProviderService.java, MistralAiLLMProviderService.java, and OpenAiLLMProviderService.java, all constructor calls now include the fourth boolean parameter.
- The dependent classes have been updated in accordance with the new API design.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java (2)
67-73: Great enhancement: Splitting model metadata into request and response.The separation of model metadata into distinct request and response entries with appropriate boolean flags improves clarity and enables better request/response handling.
77-89:✅ Verification successful
Verify header-based token count implementation.
The standardization of token count metadata with boolean flags and the addition of header-based remaining token count looks good. However, let's verify the implementation consistency.
Let's check if the header constant is defined and used consistently across other LLM providers:
🏁 Script executed:
#!/bin/bash # Check if the header constant is defined and used consistently rg "LLM_PROVIDER_SERVICE_METADATA_IDENTIFIER_REMAINING_TOKEN_COUNT" -A 2 rg "INPUT_SOURCE_HEADER.*REMAINING_TOKEN_COUNT" -A 2Length of output: 2354
Verification: Header-based token count metadata is consistent.
- Confirmed that the constant
LLM_PROVIDER_SERVICE_METADATA_IDENTIFIER_REMAINING_TOKEN_COUNTis defined inAPIConstants.javawith the expected value ("x-ratelimit-remaining-tokens").- Verified that this constant is used in the same manner in
AzureOpenAiLLMProviderService,OpenAiLLMProviderService, andMistralAiLLMProviderServicewith a consistent boolean flag setting (false).- The distinct handling—using
INPUT_SOURCE_HEADERfor the remaining token count (as opposed to payload for other token counts)—appears intentional and aligns with similar implementations across providers.No further modifications are needed.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/ImportExportAPIServiceImpl.java (2)
104-105: LGTM! Parameter correctly propagated to export utility.The
preserveCredentialsparameter is properly passed through toExportUtils.exportApi, maintaining consistency with the method signature and enabling credential preservation during API export.
126-127:✅ Verification successful
Verify credential preservation implementation across the codebase.
The
preserveCredentialsparameter is correctly propagated toExportUtils.exportApi. Both overloaded methods now consistently handle credential preservation during API export.Let's verify the implementation across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of credential preservation across the codebase # Check if ExportUtils.exportApi method signature matches ast-grep --pattern $'class ExportUtils { $$$ exportApi($$$, preserveCredentials) { $$$ } $$$ }' # Check for any direct credential handling in ExportUtils rg -A 5 'preserveCredentials' --type javaLength of output: 53597
Credential Preservation Implementation Verified
The
preserveCredentialsparameter is consistently propagated throughout the export APIs. Our static analysis confirms that:
- Both overloaded methods in
ExportUtils.exportApinow accept and pass thepreserveCredentialsparameter as expected.- Related components (e.g.,
APIMappingUtil.fromAPItoDTO,ImportExportAPIServiceImpl) correctly use the parameter to control credential handling.No further modifications are necessary.
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (3)
26-27: LGTM! Import changes align with model standardization.The updated imports reflect the transition to a more standardized model-centric approach for endpoint management.
66-66: LGTM! Enhanced debug logging.The added debug log message improves observability and follows good logging practices.
72-72: LGTM! Proper active endpoints retrieval.The method call and null/empty checks are implemented correctly.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java (2)
67-73: LGTM! Improved metadata clarity with request/response separation.The changes enhance clarity by explicitly separating request and response model metadata, making the code more maintainable and self-documenting.
77-85: LGTM! Token count metadata fields marked as required.The changes appropriately mark token count metadata fields as required, which aligns with LLM provider requirements for tracking token usage.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/dto/EndpointDTO.java (4)
21-24: Well-documented class!The class documentation clearly describes the purpose and main attributes of the DTO.
32-50: Clean and consistent getter/setter implementation!The getter and setter methods for the
idfield follow Java bean conventions and are well-documented.
52-70: Clean and consistent getter/setter implementation!The getter and setter methods for the
namefield follow Java bean conventions and are well-documented.
27-28:❓ Verification inconclusive
Verify the impact of field renaming across the codebase.
The renaming of fields from
endpointUuidtoidandendpointNametonameimproves clarity and follows common DTO patterns.Let's verify all references to these fields have been updated:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old field names rg -i "endpointUuid|endpointName" --type javaLength of output: 35291
Impact of Field Renaming – Verify Consistency Across Data Models
The renaming in
EndpointDTO(fromendpointUuidtoidand fromendpointNametoname) clearly improves clarity and aligns well with DTO patterns. However, our repository search shows that several related classes (for example, inSimplifiedEndpoint.javaandAPIEndpointInfo.java) still use the legacy identifiers. In these classes the mapping is performed correctly (e.g., assigningthis.endpointUuid = endpointDTO.getId()), but the internal field names remain unchanged.Please verify the following:
- Mapping Consistency: Confirm that having legacy names (like
endpointUuidandendpointName) in downstream models is intentional and does not introduce any issues (e.g., in serialization, logging, or API integrations).- Future Harmonization: If uniformity is desired for maintainability, consider updating these fields and corresponding getters/setters in the affected classes:
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.javacomponents/apimgt/org.wso2/carbon/apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/APIEndpointInfo.java- Other mapping utilities (e.g., in
APIMappingUtil.javaand related files) that refer to the old names.This review approves the
EndpointDTOchanges, but please manually verify that these naming discrepancies across models cause no unintended issues.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java (1)
83-87: LGTM!Correctly indicates that this mediator is not content-aware as it only handles metadata and configuration.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (2)
3284-3284: LGTM! Added constant for XML/JSON element name handling.The constant
OM_ELEMENT_NAMEis appropriately placed and follows the naming conventions.
3306-3313: Improved endpoint configuration with clearer constant names.The changes in the
APIEndpointclass:
- Removed ambiguous
PRODUCTIONandSANDBOXconstants- Added more descriptive constants for default endpoint names and configuration keys
- Maintains backward compatibility with existing
PRIMARY_ENDPOINT_ID_SEPARATORThe new constants provide better clarity and maintainability.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/ModelEndpointDTO.java (1)
21-24: LGTM! Clear class renaming and documentation.The class has been appropriately renamed from
RBEndpointDTOtoModelEndpointDTOto better reflect its purpose in model endpoint management. The class-level JavaDoc is clear and concise.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java (4)
289-290: Confirm the accuracy of the final parameter value.Here, the third parameter of
APIMappingUtil.fromAPIEndpointToDTOis set tofalse. Verify that ignoring endpoint secrets is intended in this context. If you need to preserve credentials for future references, consider setting it totrue.
321-350: Read-only and primary endpoint check logic successfully guards against unintended deletions.The newly introduced conditions correctly forbid deleting original endpoints defined within the API configuration and block deleting endpoints set as primary. The flow is logically sound, clarifies its intent, and ensures resource integrity.
379-389: Preventing updates to read-only endpoints is correct and consistent.Similar to the deletion checks, the code properly prevents modifying read-only endpoints (i.e., original production/sandbox). This preserves API endpoint integrity and maintains correctness.
433-433: Verify the final argument usage when retrieving API endpoints.By passing
falsetoPublisherCommonUtils.getAPIEndpoint, endpoint secrets will be excluded. If that's intentional, no change is needed; otherwise, confirm whether retrieving credentials is necessary.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java (2)
747-748: LGTM! Simple and focused query for retrieving endpoint mappings.The new query provides a clean way to retrieve endpoint UUIDs by API UUID.
760-768:❓ Verification inconclusive
Verify the impact of changing ENV to KEY_TYPE in the query condition.
The query has been updated to filter by KEY_TYPE instead of ENV. While the change aligns with the intra-vendor model routing feature, please ensure that:
- All calling code has been updated to pass KEY_TYPE instead of ENV
- No existing functionality depends on the ENV-based filtering
🏁 Script executed:
#!/bin/bash # Search for any remaining references to ENV in endpoint-related code rg -A 5 "ENV.*endpoint"Length of output: 25
Action Required: Verify Query Parameter Update Impact
The SQL query now uses KEY_TYPE in place of ENV. Our automated search for "ENV" references in endpoint-related code didn't yield any results. However, please ensure that:
- All calling code has been updated: Confirm that every invocation of this query now passes a KEY_TYPE value instead of ENV.
- No functional dependency remains on ENV-based filtering: Verify that no legacy functionality still implicitly expects ENV in the query condition.
Please perform a thorough manual review of all related code paths to ensure the change hasn’t introduced unintended side effects.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java (2)
56-56: LGTM!The new
INPUT_SOURCE_HEADERconstant follows the established pattern and complements the existingINPUT_SOURCE_PAYLOADconstant.
87-94: LGTM!The new token count metadata constants follow the established pattern and use standard rate limit header format for the identifier.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ExportUtils.java (5)
72-72: Import APIEndpointDTO Confirmed.
This new import forAPIEndpointDTOis consistent with the subsequent changes where the DTO is used to handle endpoint export.
165-165: New parameter documentation is valid.
The new@param preserveCredentialsJavadoc tag correctly documents the boolean parameter's purpose.
171-172: Ensure consistent usage of the new parameter.
ThepreserveCredentialsparameter in theexportApisignature is now fully documented. Verify that any downstream invocation includes appropriate checks if credentials are not to be preserved.
225-226: Passing new parameters to the updated methods looks good.
CallingaddOperationPoliciesToArchivewith(api, currentApiUuid)andaddAPIEndpointsToArchivewith thepreserveCredentialsargument ensures the new export logic is used. Confirm there are no null or optional parameter edge cases.
1300-1301: Appropriate inclusion of preserveCredentials in dependent API export.
ForwardingpreserveCredentialsin theexportApicall for dependent APIs ensures consistent credential handling across all exports.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java (1)
2498-2505: Doc comment is clear and well-written.
No issues noted. The description accurately reflects the method’s intent and parameters.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (2)
1785-1794: Guard against potential null references inpolicyConfig.If
policyConfig.getProduction()orpolicyConfig.getSandbox()is unexpectedlynull, a NullPointerException could occur when choosing endpoints. A quick null check can help avoid runtime errors.
1796-1811: Similarly guard against potential null references in the failover policy configuration.Ensure
policyConfigDTO.getProduction()orpolicyConfigDTO.getSandbox()is nevernull, and that.getFallbackModelEndpoints()is valid. Consider logging or throwing an error if the selected list cannot be obtained.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/EndpointsContext.java (3)
21-21: No concerns with the new import.
33-34: Validate endpoints for null or empty cases.
Ensure the code handlesnullor empty lists to avoid potential NullPointerExceptions down the line.
39-44: Constructor refactor looks good.
AcceptingSimplifiedEndpointobjects aligns smoothly with the new endpoint model.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.java (7)
61-61: New AI endpoints sequence template constant.
Implementation and naming look consistent.
99-105: JavaDoc provides clear context.
No further issues identified.
109-110: Refactored AI API configuration method signature.
Switching toSimplifiedEndpointfor production/sandbox endpoints clarifies usage.
136-138: Context injection for AI endpoints.
Verify null handling forproductionEndpoint/sandboxEndpointto prevent potential template errors.
271-272: Renamed method togetConfigStringEndpointConfigTemplate.
The new name is consistent with its functionality.
357-365: Concise and helpful Javadoc.
Solid documentation for the new endpoint configuration method.
549-553: New template path for AI API endpoints sequence.
Please confirm the corresponding template file exists in the repository.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/template/APITemplateBuilder.java (4)
22-22: Import ofSimplifiedEndpoint.
No issues found.
30-31: Updated AI API config signature usingSimplifiedEndpoints.
Enhanced clarity by distinguishing production and sandbox usage.
35-35: Renamed togetConfigStringEndpointConfigTemplate.
Name reflects functionality more accurately.
39-39: IntroducedgetStringForEndpoints.
Useful for generating endpoint configurations from a simplified model.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/TemplateBuilderUtil.java (7)
22-22: New import statements look appropriate.
These imports align well with the introduced functionality for AI APIs and endpoint modeling.Also applies to: 36-36, 41-41, 53-53, 88-88, 90-90
567-567: Invocation of addEndpointsFromConfig.
CallingaddEndpointsFromConfighere correctly populates EndpointDTO objects when AI API subtype conditions are met.
639-662: Check for shared EndpointConfig usage.
The method safely checks for null before adding production/sandbox endpoints. However, be cautious if both endpoints share the sameendpointConfigobject; any mutations might affect both.Would you like a script to confirm that each environment references a separate config instance where needed?
664-682: Potential shared state in createEndpointDTO.
This method sets the sameEndpointConfigDTOreference on each newEndpointDTO. If multiple endpoints use the same config object, changes in one might inadvertently affect the other.Would you like me to generate a script to scan for places where the same instance is modified?
881-881: New endpoint list parameter in createAPIGatewayDTOtoPublishAPI.
Ensure that any existing callers pass the newly requiredendpointListargument (or null if not applicable) to avoid breakage.
1034-1034: Conditional addition of endpoints for non-WS APIs.
This aligns with the updated approach of passing in the new endpoint list, ensuring no accidental inclusion for WebSocket APIs.
1318-1318: Confirm intent when passing null arguments to the template builder.
InvokinggetConfigStringEndpointConfigTemplate(type, null, null)may yield incomplete endpoint config. Verify this is correct for cases with no endpoint ID or config.components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (26)
19-19: New imports
These additions forPathNotFoundException,NhttpConstants,PassThroughConstants, andModelEndpointDTOlook appropriate and align with their usage below.Also applies to: 31-32, 39-39
132-163: Validate flow logic
The quick return paths (e.g., whentargetModel != null) can skip failover preparation. Ensure this matches desired behavior, so that valid but incorrect model values do not inadvertently bypass failover.
195-209: Straightforward extraction
Removing surrounding quotes from the extracted request model is cleanly implemented.
219-230: Property preservation steps
Storing the request payload, headers, method, URL postfix, and model in the Synapse context is well-structured.
242-257: Load balancing logic
The approach to build the message and modify the request payload for the specified target model is clear. Just verify that all edge cases (e.g., missing metadata) are handled.
295-300: No issues
The method succinctly checks and flags unsupported content types.
341-397: Cascading failover logic
The call tollmProviderService.getResponseMetadatacan transform themetadataMap; ensure that partial failures or exceptions there won't leave metadata in an inconsistent state.
406-414: Successful response handling
SettingTARGET_ENDPOINTtoEXIT_ENDPOINTon 2xx statuses is consistent with the design.
425-442: Failover gating
Logic checks for a validrequestModeland existing failover endpoints before calling the failover handler. This reads well.
454-466: Endpoint suspension
Suspending an endpoint by ID and model with a configurable duration appears straightforward.
477-502: Failover indexing
Double-check that indexing logic correctly handles an emptyfailoverEndpointslist or out-of-bounds issues if attempts exceed list size.
511-515: Index retrieval
Retrieving the endpoint index fromCURRENT_ENDPOINT_INDEXas an integer or defaulting to 0 is a clean fallback.
524-529: Context update
SettingCURRENT_ENDPOINT_INDEXandTARGET_ENDPOINTin one method is readable and maintainable.
540-551: Building failover request
Ensure updating the JSON payload withupdateJsonPayloaddoes not inadvertently drop fields if multiple modifications occur in series.
562-568: Rebuilding JSON payload
Reconstructing the JSON payload for Axis2 context is handled properly.
579-595: Conditional payload modification
The function ensures that payload changes only happen for a validINPUT_SOURCE_PAYLOAD. This is well-structured.
603-612: Request metadata updates
Syncing the REST URL, HTTP method, and transport headers from the stored properties is consistent with the upstream logic.
620-625: Target model metadata
Locating request model metadata by searching relevant attributes is concise and readable.
634-646: Metadata search
Using streams to find matchingLLMProviderMetadataobjects is clear. Implementation looks good.
659-677: Payload modification
The logic to branch on content type (JSON vs. text) and update the target field is correct.
688-697: JSON path errors
If the JSONPath expression is invalid, the code just returns. Consider adding a warning log or fallback.
708-716: Text payload modification
Straightforward approach to updating the SOAP body's first element text content.
776-787: Payload extraction
Properly returns the payload for the first metadata that referencesINPUT_SOURCE_PAYLOAD.
825-837: Query parameter extraction
Logic is correct for splitting on “?” then parsing pairs.
845-860: Parse query parameters
Implementation handles incomplete pairs gracefully by assigning an empty value. Good coverage.
868-873: Header extraction
Getting transport headers directly into a Map is a clean approach.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.java (4)
1022-1025: Javadoc parameters look good.The parameter documentation and exception tags are consistent with the method signature and clarify usage.
3230-3231: Straightforward endpoint list retrieval.This code snippet concisely retrieves endpoint metadata and maps it to a data transfer object. No major issues detected.
3234-3236: Documentation and object initialization logic look fine.The Javadoc lines clearly communicate the purpose of retrieving the default production or sandbox endpoint. The code constructing the
APIEndpointInfoinstance and setting its fields is concise.Also applies to: 3242-3244, 3253-3255
3269-3296: Validate fallback for API endpoints.When the endpoint is not found in the database, the code automatically falls back to reading from the primary production/sandbox config. While this seems intentional, consider verifying that this behavior is correct for all consumer scenarios. If endpoints are expected to be explicitly defined, returning a 404 might be advisable instead of falling back to the default.
...pimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java
Outdated
Show resolved
Hide resolved
...teway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java
Outdated
Show resolved
Hide resolved
....apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java
Outdated
Show resolved
Hide resolved
...o2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/RBPolicyConfigDTO.java
Show resolved
Hide resolved
...bon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java
Outdated
Show resolved
Hide resolved
...bon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java
Outdated
Show resolved
Hide resolved
...so2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java
Show resolved
Hide resolved
| private String endpointUuid; | ||
| private boolean endpointSecurityEnabled; | ||
| private String endpointName; | ||
| private String apiKeyIdentifier; | ||
| private String apiKeyValue; | ||
| private String apiKeyIdentifierType; | ||
| private String deploymentStage; | ||
| private static final String PRODUCTION = "PRODUCTION"; | ||
| private static final String SANDBOX = "SANDBOX"; | ||
| private static final Log log = LogFactory.getLog(SimplifiedEndpoint.class); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the class immutable and enhance security.
Since this class handles sensitive data (API keys) and appears to be a DTO:
- Consider making the class immutable by marking fields as
final. - Consider implementing security measures to protect sensitive data:
- Mask sensitive fields in logs
- Clear sensitive data from memory when no longer needed
- Consider using secure string handling for sensitive fields
- private String endpointUuid;
- private boolean endpointSecurityEnabled;
- private String endpointName;
- private String apiKeyIdentifier;
- private String apiKeyValue;
- private String apiKeyIdentifierType;
- private String deploymentStage;
+ private final String endpointUuid;
+ private final boolean endpointSecurityEnabled;
+ private final String endpointName;
+ private final String apiKeyIdentifier;
+ @SensitiveData
+ private final String apiKeyValue;
+ private final String apiKeyIdentifierType;
+ private final String deploymentStage;📝 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 String endpointUuid; | |
| private boolean endpointSecurityEnabled; | |
| private String endpointName; | |
| private String apiKeyIdentifier; | |
| private String apiKeyValue; | |
| private String apiKeyIdentifierType; | |
| private String deploymentStage; | |
| private static final String PRODUCTION = "PRODUCTION"; | |
| private static final String SANDBOX = "SANDBOX"; | |
| private static final Log log = LogFactory.getLog(SimplifiedEndpoint.class); | |
| private final String endpointUuid; | |
| private final boolean endpointSecurityEnabled; | |
| private final String endpointName; | |
| private final String apiKeyIdentifier; | |
| @SensitiveData | |
| private final String apiKeyValue; | |
| private final String apiKeyIdentifierType; | |
| private final String deploymentStage; | |
| private static final String PRODUCTION = "PRODUCTION"; | |
| private static final String SANDBOX = "SANDBOX"; | |
| private static final Log log = LogFactory.getLog(SimplifiedEndpoint.class); |
...so2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java
Outdated
Show resolved
Hide resolved
Add missing java doc comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java (1)
72-94:⚠️ Potential issueAdd error handling and null checks in mediate method.
The method has several potential issues:
- No error handling for JSON deserialization (duplicate of previous comment)
- Magic numbers used for time conversions
- Missing null checks for activeEndpoints
Apply these improvements:
public boolean mediate(MessageContext messageContext) { + private static final int MILLISECONDS_IN_SECOND = 1000; + if (log.isDebugEnabled()) { log.debug("FailoverMediator mediation started."); } DataHolder.getInstance().initCache(GatewayUtils.getAPIKeyForEndpoints(messageContext)); - FailoverPolicyConfigDTO policyConfig = new Gson().fromJson(failoverConfigs, FailoverPolicyConfigDTO.class); + FailoverPolicyConfigDTO policyConfig; + try { + policyConfig = new Gson().fromJson(failoverConfigs, FailoverPolicyConfigDTO.class); + if (policyConfig == null) { + log.error("Failed to parse failover configuration: null config"); + return false; + } + } catch (JsonSyntaxException e) { + log.error("Failed to parse failover configuration", e); + return false; + } List<ModelEndpointDTO> activeEndpoints = GatewayUtils.getActiveEndpoints(policyConfig, messageContext); + if (activeEndpoints == null) { + log.error("No active endpoints available"); + return false; + } ModelEndpointDTO targetEndpointModel = APIConstants.API_KEY_TYPE_PRODUCTION.equals(messageContext.getProperty(APIConstants.API_KEY_TYPE)) ? policyConfig.getProduction().getTargetModelEndpoint() : policyConfig.getSandbox().getTargetModelEndpoint(); + if (targetEndpointModel == null) { + log.error("Target endpoint model is null"); + return false; + } // Update the message context with failover-related properties messageContext.setProperty(AIAPIConstants.FAILOVER_TARGET_ENDPOINT, targetEndpointModel.getEndpointId()); messageContext.setProperty(AIAPIConstants.FAILOVER_TARGET_MODEL, targetEndpointModel.getModel()); messageContext.setProperty(AIAPIConstants.FAILOVER_ENDPOINTS, activeEndpoints); - messageContext.setProperty(AIAPIConstants.SUSPEND_DURATION, policyConfig.getSuspendDuration() * 1000); - messageContext.setProperty(AIAPIConstants.ENDPOINT_TIMEOUT, policyConfig.getRequestTimeout() * 1000); + messageContext.setProperty(AIAPIConstants.SUSPEND_DURATION, policyConfig.getSuspendDuration() * MILLISECONDS_IN_SECOND); + messageContext.setProperty(AIAPIConstants.ENDPOINT_TIMEOUT, policyConfig.getRequestTimeout() * MILLISECONDS_IN_SECOND);
🧹 Nitpick comments (4)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java (1)
34-42: Enhance class-level documentation with thread safety details.Consider adding thread safety information to the class-level JavaDoc, as this mediator might be accessed concurrently in a multi-threaded environment.
/** * Mediator responsible for handling AI API failover policies. This mediator processes failover configurations, * determines the active endpoints, and sets relevant properties in the message context. + * + * <p>Thread Safety: This class is thread-safe as it maintains no mutable state beyond the configuration + * which is set during initialization and remains constant during execution.</p> */components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (3)
2-2: Update the copyright year.The copyright year is set to 2025, which is ahead of the current date (February 2025). Consider updating it to 2024 or using a range (e.g., 2023-2024) to reflect the actual development period.
-* Copyright (c) 2025 WSO2 LLC. (http://www.wso2.org) All Rights Reserved. +* Copyright (c) 2024 WSO2 LLC. (http://www.wso2.org) All Rights Reserved.
125-128: Enhance error handling with specific exception types.The catch block catches all exceptions, which may mask specific issues. Consider catching and handling specific exceptions separately for better error tracking and resolution.
- } catch (Exception e) { - log.error("Error during mediation.", e); - return false; - } + } catch (APIManagementException e) { + log.error("API management error during mediation.", e); + return false; + } catch (XMLStreamException | IOException e) { + log.error("Error processing request/response payload.", e); + return false; + } catch (Exception e) { + log.error("Unexpected error during mediation.", e); + return false; + }
414-418: Extract HTTP status code range as constants.The HTTP status code range check uses magic numbers. Consider extracting these values as constants for better maintainability and clarity.
+ private static final int HTTP_SUCCESS_MIN = 200; + private static final int HTTP_SUCCESS_MAX = 300; + private boolean handleSuccessfulResponse(MessageContext messageContext, int statusCode) { - if (statusCode >= 200 && statusCode < 300) { + if (statusCode >= HTTP_SUCCESS_MIN && statusCode < HTTP_SUCCESS_MAX) { messageContext.setProperty(APIConstants.AIAPIConstants.TARGET_ENDPOINT, APIConstants.AIAPIConstants.EXIT_ENDPOINT); return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java(1 hunks)
🔇 Additional comments (3)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java (2)
1-31: LGTM!The license header and package declarations are properly structured, with well-organized imports.
60-62: Consider adding cleanup logic in destroy method.The empty destroy method might need cleanup logic if the mediator acquires any resources during its lifecycle.
Please verify if any resources need to be cleaned up when the mediator is destroyed.
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (1)
57-57: Well-structured implementation with good separation of concerns!The changes to the AIAPIMediator class demonstrate excellent modularity and robust error handling. The implementation properly separates inbound and outbound processing, handles failover scenarios effectively, and includes comprehensive documentation.
...bon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java
Outdated
Show resolved
Hide resolved
Handle fail and exception scenarios
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (1)
483-508: Add unit tests for the failover handling logic.The failover implementation is complex and critical. Consider adding comprehensive unit tests to verify:
- Endpoint suspension behavior
- Request rebuilding process
- Edge cases in the failover sequence
Would you like me to help generate unit test cases for the failover handling implementation?
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (1)
102-111: Consider using Java 8 Stream API for weight calculations.The logic is correct but could be more concise using streams:
- float totalWeight = 0.0f; - List<Float> cumulativeWeights = new ArrayList<>(); - - for (ModelEndpointDTO endpoint : endpoints) { - double weight = Math.max(endpoint.getWeight(), 0.1f); - totalWeight += (float) weight; - cumulativeWeights.add(totalWeight); - } + List<Float> cumulativeWeights = new ArrayList<>(); + float totalWeight = endpoints.stream() + .map(endpoint -> (float) Math.max(endpoint.getWeight(), 0.1f)) + .reduce(0.0f, + (sum, weight) -> { + cumulativeWeights.add(sum + weight); + return sum + weight; + });components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java (2)
21-24: Add more detailed class-level documentation.The current JavaDoc is too brief. Consider adding:
- Purpose of failover policies
- Description of key configuration parameters
- Example usage
/** * DTO representing the configuration for failover policies in API Gateway. + * + * This class encapsulates configuration for both production and sandbox environments, + * including request timeout and suspend duration settings for failed endpoints. + * + * Example usage: + * <pre> + * FailoverPolicyConfigDTO config = new FailoverPolicyConfigDTO(); + * config.setRequestTimeout(30000L); // 30 seconds + * config.setSuspendDuration(300000L); // 5 minutes + * </pre> */
120-125: Consider enhancing duration validation.The current validation only checks if the duration is positive. Consider adding:
- Upper bound validation to prevent unreasonably large values
- Null check with default value assignment
private void validateDuration(Long duration, String fieldName) { + if (duration == null) { + throw new IllegalArgumentException(fieldName + " cannot be null"); + } if (duration != null && duration <= 0) { throw new IllegalArgumentException(fieldName + " must be positive"); } + if (duration > 3600000) { // 1 hour + throw new IllegalArgumentException(fieldName + " cannot exceed 1 hour"); + } }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (1)
1761-1776: Consider adding metrics for endpoint filtering.The endpoint filtering logic is well-structured but lacks observability. Consider adding metrics to track:
- Number of suspended endpoints
- Number of active endpoints
- Filtering duration
private static List<ModelEndpointDTO> filterActiveEndpoints(List<ModelEndpointDTO> selectedEndpoints, org.apache.synapse.MessageContext messageContext) { + long startTime = System.currentTimeMillis(); + int totalEndpoints = selectedEndpoints != null ? selectedEndpoints.size() : 0; + int activeEndpointCount = 0; if (selectedEndpoints == null || selectedEndpoints.isEmpty()) { return null; } List<ModelEndpointDTO> activeEndpoints = new ArrayList<>(); for (ModelEndpointDTO endpoint : selectedEndpoints) { if (!DataHolder.getInstance().isEndpointSuspended(getAPIKeyForEndpoints(messageContext), getEndpointKey(endpoint))) { activeEndpoints.add(endpoint); + activeEndpointCount++; } } + + if (log.isDebugEnabled()) { + log.debug("Endpoint filtering completed in " + (System.currentTimeMillis() - startTime) + + "ms. Active: " + activeEndpointCount + "/" + totalEndpoints); + } return activeEndpoints.isEmpty() ? null : activeEndpoints; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java
🔇 Additional comments (11)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/AIAPIMediator.java (3)
19-19: LGTM! Import statements are well-organized.The new imports enhance error handling and support new functionality for model endpoint management.
Also applies to: 31-32, 39-39
665-667: LGTM! Method signatures improved to use exceptions for error handling.The change from Boolean return types to void with exception handling provides better error information and aligns with best practices.
Also applies to: 696-697, 724-725
703-712: LGTM! Error handling implemented as suggested.The JSON parsing operations now have proper error handling with specific exception catches and appropriate error logging.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/SimplifiedEndpoint.java (3)
25-31: Consider making the class immutable and enhance security.Since this class handles sensitive data (API keys) and appears to be a DTO:
- Consider making the class immutable by marking fields as
final.- Consider implementing security measures to protect sensitive data:
- Mask sensitive fields in logs
- Clear sensitive data from memory when no longer needed
- Consider using secure string handling for sensitive fields
41-77: Well-structured constructor with proper validation!The constructor implementation is robust with:
- Proper null checks and validation
- Clear error messages
- Well-organized security configuration handling
- Early returns for better readability
120-131: Mask sensitive data in toString output.The toString method currently exposes sensitive data (apiKeyValue). Consider masking it in the output.
public String toString() { return "SimplifiedEndpointDTO{" + " endpointUuid='" + endpointUuid + '\'' + ", endpointSecurityEnabled=" + endpointSecurityEnabled + ", endpointName='" + endpointName + '\'' + ", apiKeyIdentifier='" + apiKeyIdentifier + '\'' + - ", apiKeyValue='" + apiKeyValue + '\'' + + ", apiKeyValue='********'" + ", apiKeyIdentifierType='" + apiKeyIdentifierType + '\'' + ", deploymentStage='" + deploymentStage + '\'' + '}'; }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (4)
18-29: LGTM! Import changes align with the refactoring.The updated imports reflect the transition from RB-prefixed DTOs to more generic Model-prefixed DTOs and include JsonSyntaxException for enhanced error handling.
66-68: LGTM! Enhanced debug logging.The added debug message improves observability of the mediator's execution flow.
72-80: Implementation matches previous suggestion for JSON error handling.The added try-catch block properly handles JsonSyntaxException during configuration parsing, setting appropriate error response and logging.
82-88:❓ Verification inconclusive
Verify the suspend duration calculation.
While the type changes from RBEndpointDTO to ModelEndpointDTO look good, please confirm that:
- The suspend duration from RBPolicyConfigDTO is in seconds
- Multiplying by MILLISECONDS_IN_SECOND constant produces the correct duration
🏁 Script executed:
#!/bin/bash # Search for MILLISECONDS_IN_SECOND constant definition and usage rg -A 2 "MILLISECONDS_IN_SECOND" --type javaLength of output: 2682
Suspend Duration Conversion and Endpoint Type Verification
- The
MILLISECONDS_IN_SECONDconstant is correctly defined as1000and is used consistently in this file (as well as inRoundRobinMediatorandFailoverMediator) to convert a suspend duration from seconds to milliseconds.- The conversion logic in the snippet (multiplying
endpoints.getSuspendDuration()byMILLISECONDS_IN_SECOND) is appropriate—as long as the upstream suspend duration value (from the policy configuration, e.g., RBPolicyConfigDTO) is indeed provided in seconds.- Please verify that the suspend duration value supplied by RBPolicyConfigDTO (or its current equivalent) is in seconds to confirm that the multiplication achieves the intended millisecond conversion.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java (1)
26-31: LGTM! Good use of default values.The default values for timeouts are well-chosen and clearly documented through constant names.
| public String getApiKeyValue() { | ||
|
|
||
| return apiKeyValue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secure the API key value getter.
The getApiKeyValue() method exposes sensitive data. Consider:
- Adding a warning annotation (e.g.,
@Sensitive) - Implementing access logging for security auditing
- Returning a masked or encrypted version by default with a separate method for getting the raw value when absolutely necessary
+ @Sensitive
+ @SecurityAudit
public String getApiKeyValue() {
+ if (log.isDebugEnabled()) {
+ log.debug("Accessing API key value for endpoint: " + endpointUuid);
+ }
return apiKeyValue;
}
+
+ /**
+ * Returns the raw API key value. Use with caution.
+ * @param context SecurityContext for validation
+ * @return raw API key value
+ */
+ public String getRawApiKeyValue(SecurityContext context) {
+ // Add security context validation here
+ return apiKeyValue;
+ }📝 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 String getApiKeyValue() { | |
| return apiKeyValue; | |
| } | |
| @Sensitive | |
| @SecurityAudit | |
| public String getApiKeyValue() { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Accessing API key value for endpoint: " + endpointUuid); | |
| } | |
| return apiKeyValue; | |
| } | |
| /** | |
| * Returns the raw API key value. Use with caution. | |
| * @param context SecurityContext for validation | |
| * @return raw API key value | |
| */ | |
| public String getRawApiKeyValue(SecurityContext context) { | |
| // Add security context validation here | |
| return apiKeyValue; | |
| } |
...bon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/gateway/FailoverPolicyConfigDTO.java
Show resolved
Hide resolved
...2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java
Outdated
Show resolved
Hide resolved
...2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java
Outdated
Show resolved
Hide resolved
...2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java
Outdated
Show resolved
Hide resolved
Fix review comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (1)
1798-1815:⚠️ Potential issueAdd null checks to avoid potential NullPointerExceptions.
This method does not validatepolicyConfigDTOor its production/sandbox fields, which can lead to runtime errors. This repeats a previously noted requirement.Apply the following patch to maintain consistency:
+ if (policyConfigDTO == null) { + throw new IllegalArgumentException("FailoverPolicyConfigDTO cannot be null"); + } List<ModelEndpointDTO> selectedEndpoints = APIConstants.API_KEY_TYPE_PRODUCTION .equals(messageContext.getProperty(APIConstants.API_KEY_TYPE)) ? policyConfigDTO.getProduction().getFallbackModelEndpoints() : policyConfigDTO.getSandbox().getFallbackModelEndpoints(); + if (selectedEndpoints == null) { + throw new IllegalArgumentException("FailoverPolicyDeploymentConfigDTO cannot be null"); + } return filterActiveEndpoints(selectedEndpoints, messageContext);
🧹 Nitpick comments (3)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java (2)
3380-3415: Refactor suggestion: Extract repeated endpoint security handling logic.The code has duplicate logic for handling sandbox and production endpoint security. Consider extracting the common logic into a separate private method to improve maintainability.
-private static JSONObject handleEndpointSecurity(JSONObject endpointSecurity) { +private static JSONObject handleEndpointSecurity(JSONObject endpointSecurity) { + return handleEndpointSecurityInternal(endpointSecurity, + Arrays.asList(APIConstants.ENDPOINT_SECURITY_SANDBOX, APIConstants.ENDPOINT_SECURITY_PRODUCTION)); +} + +private static JSONObject handleEndpointSecurityInternal(JSONObject endpointSecurity, List<String> types) { JSONObject endpointSecurityElement = new JSONObject(); endpointSecurityElement.putAll(endpointSecurity); - if (endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_SANDBOX) != null) { - JSONObject sandboxEndpointSecurity = new JSONObject( - (Map) endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_SANDBOX)); - if (APIConstants.ENDPOINT_SECURITY_TYPE_OAUTH.equalsIgnoreCase((String) sandboxEndpointSecurity - .get(APIConstants.ENDPOINT_SECURITY_TYPE))) { - sandboxEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_CLIENT_SECRET, ""); - } - if (sandboxEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_PASSWORD) != null) { - sandboxEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_PASSWORD, ""); - } - if (sandboxEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE) != null) { - sandboxEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, ""); - } - endpointSecurityElement.put(APIConstants.ENDPOINT_SECURITY_SANDBOX, sandboxEndpointSecurity); - } - if (endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_PRODUCTION) != null) { - JSONObject productionEndpointSecurity = new JSONObject( - (Map) endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_PRODUCTION)); - if (APIConstants.ENDPOINT_SECURITY_TYPE_OAUTH.equalsIgnoreCase((String) productionEndpointSecurity - .get(APIConstants.ENDPOINT_SECURITY_TYPE))) { - productionEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_CLIENT_SECRET, ""); - } - if (productionEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_PASSWORD) != null) { - productionEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_PASSWORD, ""); - } - if (productionEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE) != null) { - productionEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, ""); - } - endpointSecurityElement.put(APIConstants.ENDPOINT_SECURITY_PRODUCTION, productionEndpointSecurity); + for (String type : types) { + if (endpointSecurityElement.get(type) != null) { + JSONObject typeEndpointSecurity = new JSONObject( + (Map) endpointSecurityElement.get(type)); + if (APIConstants.ENDPOINT_SECURITY_TYPE_OAUTH.equalsIgnoreCase((String) typeEndpointSecurity + .get(APIConstants.ENDPOINT_SECURITY_TYPE))) { + typeEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_CLIENT_SECRET, ""); + } + if (typeEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_PASSWORD) != null) { + typeEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_PASSWORD, ""); + } + if (typeEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE) != null) { + typeEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, ""); + } + endpointSecurityElement.put(type, typeEndpointSecurity); + } } return endpointSecurityElement; }
3417-3447: Refactor suggestion: Extract repeated decryption logic.Similar to the previous comment, the decryption logic is duplicated for sandbox and production endpoints. Consider extracting the common logic.
-private static JSONObject handleEndpointSecurityDecrypt(JSONObject endpointSecurity) { +private static JSONObject handleEndpointSecurityDecrypt(JSONObject endpointSecurity) { + return handleEndpointSecurityDecryptInternal(endpointSecurity, + Arrays.asList(APIConstants.ENDPOINT_SECURITY_SANDBOX, APIConstants.ENDPOINT_SECURITY_PRODUCTION)); +} + +private static JSONObject handleEndpointSecurityDecryptInternal(JSONObject endpointSecurity, List<String> types) { JSONObject endpointSecurityElement = new JSONObject(); endpointSecurityElement.putAll(endpointSecurity); CryptoUtil cryptoUtil = CryptoUtil.getDefaultCryptoUtil(); try { - if (endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_SANDBOX) != null) { - JSONObject sandboxEndpointSecurity = new JSONObject( - (Map) endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_SANDBOX)); - String apiKeyValue = (String) sandboxEndpointSecurity.get(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE); - if (StringUtils.isNotEmpty(apiKeyValue)) { - sandboxEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, - new String(cryptoUtil.base64DecodeAndDecrypt(apiKeyValue))); - } - endpointSecurityElement.put(APIConstants.ENDPOINT_SECURITY_SANDBOX, sandboxEndpointSecurity); - } - if (endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_PRODUCTION) != null) { - JSONObject productionEndpointSecurity = new JSONObject( - (Map) endpointSecurityElement.get(APIConstants.ENDPOINT_SECURITY_PRODUCTION)); - String apiKeyValue = (String) productionEndpointSecurity.get( - APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE); - if (StringUtils.isNotEmpty(apiKeyValue)) { - productionEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, - new String(cryptoUtil.base64DecodeAndDecrypt(apiKeyValue))); - } - endpointSecurityElement.put(APIConstants.ENDPOINT_SECURITY_PRODUCTION, productionEndpointSecurity); + for (String type : types) { + if (endpointSecurityElement.get(type) != null) { + JSONObject typeEndpointSecurity = new JSONObject( + (Map) endpointSecurityElement.get(type)); + String apiKeyValue = (String) typeEndpointSecurity.get( + APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE); + if (StringUtils.isNotEmpty(apiKeyValue)) { + typeEndpointSecurity.put(APIConstants.ENDPOINT_SECURITY_API_KEY_VALUE, + new String(cryptoUtil.base64DecodeAndDecrypt(apiKeyValue))); + } + endpointSecurityElement.put(type, typeEndpointSecurity); + } } } catch (CryptoException e) { log.error("Error while decrypting client credentials", e); } return endpointSecurityElement; }components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (1)
1755-1776: Prefer returning an empty list instead of null.
Returningnullmight lead toNullPointerExceptionin the calling methods. It’s often more predictable to return an empty list.- return activeEndpoints.isEmpty() ? null : activeEndpoints; + return activeEndpoints;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java(4 hunks)
🔇 Additional comments (7)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/APIMappingUtil.java (3)
3361-3370: LGTM! Endpoint security handling for API model looks good.The method correctly handles endpoint security based on tenant domain and preserveCredentials flag.
3372-3378: LGTM! New organization-based endpoint security handling is well implemented.The method provides a clean abstraction for handling endpoint security based on organization and preserveCredentials flag.
3560-3570: LGTM! Endpoint security handling in API endpoint mapping is well implemented.The methods correctly handle endpoint security configuration during DTO conversion, including decryption and credential preservation based on organization settings.
Also applies to: 3572-3591
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (4)
56-57: No issues with the new imports.
They appear straightforward and required for the newly introduced methods.
59-60: No issues with these additional imports.
They align well with the new endpoint-related functionality.
1778-1797: Consider verifying non-null endpoint lists.
After validatingpolicyConfigitself, you might also ensure thatpolicyConfig.getProduction()orpolicyConfig.getSandbox()is not null, preventing unexpected NullPointerExceptions.
1839-1851: Good validation of endpoint parameters.
This properly addresses previous feedback on ensuring non-null endpoint ID and model.
… have a primary endpoint id
…st-alpha Intra vendor model routing post alpha bug fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (2)
8141-8154: LGTM! Consider adding debug logging.The method effectively handles endpoint cleanup. Consider adding debug logging to help with troubleshooting.
private void removeAPIEndpoints(String apiUUID) throws APIManagementException { try { + if (log.isDebugEnabled()) { + log.debug("Removing endpoints for API: " + apiUUID); + } apiMgtDAO.deleteAPIPrimaryEndpointMappings(apiUUID); apiMgtDAO.deleteAPIEndpointsByApiUUID(apiUUID); } catch (APIManagementException e) { throw new APIManagementException("Error while removing endpoints for API " + apiUUID, e); } }
8235-8275: Consider refactoring to improve readability and maintainability.The method handles endpoint mapping population well but could be improved:
- Extract the endpoint ID validation logic into a separate method
- Add constants for the repeated endpoint type checks
- Consider using early returns to reduce nesting
private void populateAPIPrimaryEndpointsMapping(API api, String uuid) throws APIManagementException { // ... existing revision check code ... + private boolean isProductionEndpoint(String endpointId, String apiUuid) { + return endpointId.equals(apiUuid + + APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR + + APIConstants.APIEndpoint.PRODUCTION); + } + + private boolean isSandboxEndpoint(String endpointId, String apiUuid) { + return endpointId.equals(apiUuid + + APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR + + APIConstants.APIEndpoint.SANDBOX); + } List<String> endpointIds = apiMgtDAO.getPrimaryEndpointUUIDByAPIId(currentApiUuid); if (endpointIds == null || endpointIds.isEmpty()) { return; } // Set default endpoints for (String endpointId : endpointIds) { if (isProductionEndpoint(endpointId, currentApiUuid)) { api.setPrimaryProductionEndpointId(endpointId); } else if (isSandboxEndpoint(endpointId, currentApiUuid)) { api.setPrimarySandboxEndpointId(endpointId); } } // Get additional endpoints if needed if (api.getPrimaryProductionEndpointId() == null) { api.setPrimaryProductionEndpointId( apiMgtDAO.getPrimaryEndpointUUIDByApiIdAndEnv( currentApiUuid, APIConstants.APIEndpoint.PRODUCTION, revisionUuid, organization ) ); } if (api.getPrimarySandboxEndpointId() == null) { api.setPrimarySandboxEndpointId( apiMgtDAO.getPrimaryEndpointUUIDByApiIdAndEnv( currentApiUuid, APIConstants.APIEndpoint.SANDBOX, revisionUuid, organization ) ); } }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java (3)
24495-24510: Add logging to track the number of endpoints deleted.The method handles database operations correctly, but adding logging would help with monitoring and debugging.
public void deleteAPIEndpointsByApiUUID(String apiUUID) throws APIManagementException { try (Connection connection = APIMgtDBUtil.getConnection()) { connection.setAutoCommit(false); try (PreparedStatement statement = connection.prepareStatement( SQLConstants.APIEndpointsSQLConstants.DELETE_API_ENDPOINTS_BY_API_UUID)) { statement.setString(1, apiUUID); - statement.executeUpdate(); + int deletedCount = statement.executeUpdate(); + log.debug("Deleted " + deletedCount + " endpoints for API UUID: " + apiUUID); connection.commit(); } catch (SQLException e) { connection.rollback();
24627-24652: Enhance error messages with operation context.The error messages could be more specific about the operation being performed to aid in troubleshooting.
- handleException("Error while updating primary endpoint mappings for API : " + api.getUuid(), e); + handleException("Error while updating primary endpoint mappings (delete and recreate) for API : " + api.getUuid(), e); - handleException("Error while deleting primary endpoint mappings of API : " + apiUUID, e); + handleException("Error while deleting all primary endpoint mappings during cleanup for API : " + apiUUID, e);
24711-24739: Extract endpoint ID separator into a constant.The endpoint ID separator is used multiple times and should be extracted into a constant for better maintainability.
+ private static final String ENDPOINT_ID_SEPARATOR = APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR; public void addDefaultPrimaryEndpointMappings(API api) throws APIManagementException { String apiUUID = api.getUuid(); try (Connection connection = APIMgtDBUtil.getConnection()) { connection.setAutoCommit(false); String addPrimaryEndpointMappingQuery = SQLConstants.APIEndpointsSQLConstants.ADD_PRIMARY_ENDPOINT_MAPPING; try (PreparedStatement addPrimaryMapping = connection.prepareStatement(addPrimaryEndpointMappingQuery)) { // add primary production endpoint mapping addPrimaryMapping.setString(1, apiUUID); addPrimaryMapping.setString(2, - apiUUID + APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR + APIConstants.APIEndpoint.PRODUCTION); + apiUUID + ENDPOINT_ID_SEPARATOR + APIConstants.APIEndpoint.PRODUCTION); addPrimaryMapping.addBatch(); // add primary sandbox endpoint mapping addPrimaryMapping.setString(1, apiUUID); addPrimaryMapping.setString(2, - apiUUID + APIConstants.APIEndpoint.PRIMARY_ENDPOINT_ID_SEPARATOR + APIConstants.APIEndpoint.SANDBOX); + apiUUID + ENDPOINT_ID_SEPARATOR + APIConstants.APIEndpoint.SANDBOX); addPrimaryMapping.addBatch();components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java (1)
66-89: LGTM! The metadata structure improvements enhance clarity and functionality.The separation of model metadata into request/response entries and the addition of boolean flags improve the clarity of metadata processing. The new remaining token count metadata via headers is a valuable addition for token usage tracking.
Consider extracting the metadata keys and boolean flags into constants for better maintainability:
+ private static final boolean METADATA_ENABLED = true; + private static final boolean METADATA_DISABLED = false; + List<LLMProviderMetadata> llmProviderMetadata = new ArrayList<>(); llmProviderMetadata.add(new LLMProviderMetadata( APIConstants.AIAPIConstants.LLM_PROVIDER_SERVICE_METADATA_REQUEST_MODEL, APIConstants.AIAPIConstants.INPUT_SOURCE_PAYLOAD, - APIConstants.AIAPIConstants.LLM_PROVIDER_SERVICE_METADATA_IDENTIFIER_MODEL, false)); + APIConstants.AIAPIConstants.LLM_PROVIDER_SERVICE_METADATA_IDENTIFIER_MODEL, METADATA_DISABLED));components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java (1)
92-97:❓ Verification inconclusive
Verify model names and consider version-specific validation.
The model names ("gpt-4o", "gpt-4o-mini", "o3-mini") don't match standard OpenAI model names. Additionally, consider:
- Adding version validation
- Making the list configurable through external configuration
Consider this improvement:
- // Set default model List - List<String> modelList = new ArrayList<>(); - modelList.add("gpt-4o"); - modelList.add("gpt-4o-mini"); - modelList.add("o3-mini"); - llmProvider.setModelList(modelList); + // Load models from configuration + List<String> modelList = ConfigurationManager.getInstance() + .getOpenAIModels(); + if (modelList.isEmpty()) { + // Fallback to default models + modelList = Arrays.asList( + "gpt-4", + "gpt-3.5-turbo", + "gpt-3.5-turbo-16k" + ); + } + llmProvider.setModelList(modelList);
🌐 Web query:
What are the current available models in OpenAI API?💡 Result:
The current available models in the OpenAI ecosystem (as of February 2025) include several specialized families with distinct capabilities:
Core Language Models
GPT-4o Series (Omni models):
gpt-4o(2024-11-20): Latest multimodal GA model handling text/images with 128k token contextgpt-4o-mini(2024-07-18): Compact version for cost-sensitive applications- Enhanced features: JSON mode, parallel function calling[1]
GPT-4 Turbo:
gpt-4-turbo-2024-04-09: Vision-enabled successor to previous preview models- 128k token context with image/text processing[1]
GPT-3.5 Series:
gpt-35-turbo(0125): Latest iteration with improved format adherencegpt-35-turbo-instruct: Legacy completions endpoint alternative[1]Specialized Models
Embeddings:
text-embedding-3-small/large: State-of-the-art text vectorization[7]Multimodal Models:
- DALL-E 3: Image generation from text prompts[1]
- Whisper V2: Speech-to-text transcription/translation (
whisper-1)[4][8]- Text-to-Speech: Preview synthetic voice generation[1]
Experimental & Limited Access
o-Series Reasoning Models:
o3-mini(2025): Enhanced problem-solving for STEM taskso1: Multimodal version with image/text processing[1]Third-Party Integrations
Google's Gemini 1.5 Flash can be accessed through OpenAI-compatible endpoints by modifying API base URLs[3].
Key limitations:
- Vision features in GPT-4 Turbo require standard deployments (not provisioned)[1]
- Fine-tuning availability varies by region and model version[1]
- Older models like
gpt-4-0314are being phased out[1]For production use cases requiring stability:
# Recommended model selection for common tasks CHAT_MODEL = "gpt-4o" if multimodal else "gpt Citations: - 1: https://learn.microsoft.com/en-us/azure/ai-services/openai/concepts/models - 2: https://holypython.com/python-api-tutorial/listing-all-available-openai-models-openai-api/ - 3: https://ai.google.dev/gemini-api/docs/openai - 4: https://platform.openai.com/docs/api-reference/introduction - 5: https://openai.com/api/ - 6: https://platform.openai.com/docs/guides/text-generation - 7: https://platform.openai.com/docs/guides/embeddings - 8: https://learn.microsoft.com/en-us/rest/api/azureopenai/models/list?view=rest-azureopenai-2024-10-21 --- </details> **Model List Verified – Consider Configurability & Version Validation** The current default model names ("gpt-4o", "gpt-4o-mini", and "o3-mini") have been confirmed as valid according to the latest OpenAI API details (February 2025). While the names are correct, it would be beneficial to enhance maintainability by externalizing this list and, if feasible, adding version-specific validation logic. - Verify that the default model list can be overridden via configuration. - Consider adding checks against API version metadata to ensure compatibility. For example, you could refactor the code as follows: ```diff - // Set default model List - List<String> modelList = new ArrayList<>(); - modelList.add("gpt-4o"); - modelList.add("gpt-4o-mini"); - modelList.add("o3-mini"); - llmProvider.setModelList(modelList); + // Load models from configuration; fallback to verified default models + List<String> modelList = ConfigurationManager.getInstance().getOpenAIModels(); + if (modelList.isEmpty()) { + modelList = Arrays.asList("gpt-4o", "gpt-4o-mini", "o3-mini"); + } + llmProvider.setModelList(modelList);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java(9 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java(3 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(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java
🔇 Additional comments (7)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java (3)
4747-4749: LGTM! Clean and focused endpoint mapping query.The new query provides a simple way to get endpoint UUIDs for an API.
4760-4769: LGTM! Well-structured join query with improved filtering.The renamed query now correctly uses KEY_TYPE instead of ENV for filtering, which better reflects its purpose. The join logic and conditions are clear and well-organized.
4773-4773: LGTM! Clean endpoint cleanup query.Simple and focused query for removing all endpoints associated with an API.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
664-674: LGTM! Well-structured method for handling AI API primary endpoints.The method has good separation of concerns and follows single responsibility principle by only handling primary endpoint mappings for AI APIs.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java (1)
19110-19116:❓ Verification inconclusive
Verify the impact of switching from environment-based to key-type-based endpoint mapping.
The SQL query has been updated to use key type instead of environment for endpoint mapping. Ensure that all dependent code and database migrations handle this change correctly.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old environment-based query constant # and verify the existence of database migration scripts. # Search for any remaining references to the old query constant rg "GET_API_PRIMARY_ENDPOINT_UUID_BY_API_UUID_AND_ENV" # Look for database migration scripts fd -g "*sql" -x grep -l "AM_API_PRIMARY_EP_MAPPING"Length of output: 1523
Impact of Switching to Key-Type Based Endpoint Mapping
The SQL query update appears to correctly transition from environment-based to key-type-based endpoint mapping. The absence of any remaining references to
GET_API_PRIMARY_ENDPOINT_UUID_BY_API_UUID_AND_ENVin the codebase is reassuring. However, several SQL migration scripts (e.g., files in the./components/apimgt/org.wso2.carbon.apimgt.keymgt/src/test/resources/dbscripts/h2.sqland in various feature directories) still referenceAM_API_PRIMARY_EP_MAPPING. Please manually verify that these migration scripts have been updated to correctly support the new key-type-based mapping, ensuring that dependent modules handle the change without issues.components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java (2)
86-89: Verify security implications of exposing token count in headers.The remaining token count is exposed through headers. This might leak sensitive information about API usage and quotas.
Please confirm:
- Is this information necessary in headers?
- Are there any security implications of exposing token counts?
- Is rate limiting implemented to prevent token count enumeration attacks?
67-85: Well-structured metadata with clear request/response separation!The metadata entries are well-organized with:
- Clear separation between request and response models
- Consistent pattern for token count tracking
- Proper boolean flags for required fields
| // Set default model List | ||
| List<String> modelList = new ArrayList<>(); | ||
| modelList.add("mistral-small-latest"); | ||
| modelList.add("mistral-medium"); | ||
| modelList.add("open-mistral-7b"); | ||
| llmProvider.setModelList(modelList); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the default models are up-to-date.
Please ensure that these are the latest supported models from Mistral AI.
🌐 Web query:
What are the latest available models from Mistral AI?
💡 Result:
As of February 2025, Mistral AI's latest models include several advanced offerings across different categories:
Core Models
-
Mistral Saba 25.02
Released February 17, this model specializes in languages from the Middle East and South Asia while maintaining strong multilingual capabilities[3][4]. It balances performance with efficiency for regional applications. -
Mistral Small v3 (25.01)
A January release featuring:- Apache 2.0 license for open use
- Competitive performance with larger models like Llama3-70B
- Optimized latency (150 tokens/s)
- High accuracy (81% MMLU)[9][10]
Specialized Models
-
Codestral 25.01
January update to code-generation model:- Supports >80 programming languages
- Enhanced speed (2.5× faster than predecessors)
- Specializes in code completion/FIM/testing[1][2]
-
Pixtral Large (24.11)
Multimodal November release combining:- Visual encoder for image understanding
- Enhanced long-context handling
- Integrated with text generation capabilities[3][7]
Edge-Optimized Models
- Ministral Family
October releases for local device deployment:Model Parameters Key Features Ministral 8B 8B 128k context for complex edge tasks Ministral3B 3B Low-latency mobile/robotics use[5][8]
Earlier November releases like Mistral Large24.11 remain current with improved reasoning/function calling[2], while foundational open-source models like Mathstral7B continue receiving updates[3].
These models reflect Mistral's focus on both frontier capabilities (through Large/Saba) and practical deployment efficiency (Small/Ministerial lines)[6][9].
Citations:
- 1: https://cloud.google.com/blog/products/ai-machine-learning/announcing-new-mistral-large-model-on-vertex-ai
- 2: https://cloud.google.com/vertex-ai/generative-ai/docs/partner-models/mistral
- 3: https://docs.mistral.ai
- 4: https://docs.mistral.ai/getting-started/changelog/
- 5: https://techcrunch.com/2024/10/16/mistral-releases-new-ai-models-optimized-for-edge-devices/
- 6: https://aws.amazon.com/bedrock/mistral/
- 7: https://en.wikipedia.org/wiki/Mistral_AI
- 8: https://learn.microsoft.com/en-us/azure/ai-studio/how-to/deploy-models-mistral
- 9: https://mistral.ai/news/mistral-small-3
- 10: https://docs.mistral.ai/getting-started/models/models_overview/
Update Default Model List to Reflect Latest Mistral AI Models
The current default list in MistralAiLLMProviderService.java (lines 92-97) contains:
"mistral-small-latest""mistral-medium""open-mistral-7b"
However, as of February 2025, the latest Mistral AI models include offerings such as Mistral Small v3 (25.01) and Mistral Saba 25.02, with no direct reference to a "medium" model or "open-mistral-7b". Please review and update the default model list to align with the current model naming and versions provided by Mistral AI.
Fix policy invoking when only single deployment stage is defined
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (1)
99-101: Consider extracting the milliseconds conversion to a utility method.The milliseconds conversion could be moved to a utility method to improve reusability and maintainability.
- messageContext.setProperty(APIConstants.AIAPIConstants.SUSPEND_DURATION, - endpoints.getSuspendDuration() * APIConstants.AIAPIConstants.MILLISECONDS_IN_SECOND); + messageContext.setProperty(APIConstants.AIAPIConstants.SUSPEND_DURATION, + GatewayUtils.convertToMilliseconds(endpoints.getSuspendDuration()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/FailoverMediator.java
- components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/RoundRobinMediator.java
🔇 Additional comments (7)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/utils/GatewayUtils.java (3)
56-60: LGTM!The new imports are correctly added to support the intra-vendor model routing feature enhancements.
1754-1776: LGTM!The implementation correctly filters active endpoints by:
- Validating input parameters
- Creating a new list for active endpoints
- Filtering out suspended endpoints
- Handling edge cases (null/empty input, no active endpoints)
1794-1812: Input validation has been added as previously suggested.The implementation now includes proper validation for:
- Null endpoint object
- Empty endpoint ID
- Empty model name
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/mediators/WeightedRoundRobinMediator.java (4)
19-19: LGTM! Import changes align with the new DTO model.The new imports support the transition to ModelEndpointDTO and RBPolicyConfigDTO, along with improved error handling using JsonSyntaxException.
Also applies to: 27-28
72-80: Implementation matches previous review suggestion.The added try-catch block for JSON parsing with appropriate error handling matches the suggestion from the previous review.
82-92: LGTM! Robust endpoint selection based on API key type.The separate handling of production and sandbox endpoints with early return improves efficiency.
113-115: LGTM! Method signature updated consistently.The method signature and documentation updates align with the transition to ModelEndpointDTO.
|
Closing this PR since the same was added via [1]. |
Summary by CodeRabbit
New Features
Improvements
Stability & Data Integrity
AM_API_ENDPOINTStable to enhance data integrity.