add Model family support #13144
add Model family support #13144
Conversation
tharindu1st
requested review from
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw,
tgtshanika,
tharikaGitHub,
senthuran16,
hisanhunais,
HiranyaKavishani,
RakhithaRR,
npamudika,
HeshanSudarshana,
ashera96,
pubudu538 and
Krishanx92
as code owners
📝 WalkthroughWalkthroughThis update restructures the handling of LLM provider models across the API management platform. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant RESTClient
participant AdminAPI
participant LlmProvidersApiServiceImpl
participant LLMProviderMappingUtil
participant ApiMgtDAO
participant Database
RESTClient->>AdminAPI: POST /llm-providers (with models array, multipleVendorSupport)
AdminAPI->>LlmProvidersApiServiceImpl: addLLMProvider(...)
LlmProvidersApiServiceImpl->>LLMProviderMappingUtil: Convert LLMModelDTO list to LLMModel list
LlmProvidersApiServiceImpl->>ApiMgtDAO: addLLMProvider(..., LLMProvider with List<LLMModel>, modelFamilySupported)
ApiMgtDAO->>Database: Insert LLMProvider, LLMModel (with MODEL_FAMILY_NAME)
ApiMgtDAO-->>LlmProvidersApiServiceImpl: LLMProvider (with model families)
LlmProvidersApiServiceImpl-->>AdminAPI: Response
RESTClient->>AdminAPI: GET /llm-providers/{id}/models
AdminAPI->>LlmProvidersApiServiceImpl: getLLMProviderModels(...)
LlmProvidersApiServiceImpl->>ApiMgtDAO: getLLMProvider(...)
ApiMgtDAO->>Database: Query LLMProvider and models (grouped by family)
ApiMgtDAO-->>LlmProvidersApiServiceImpl: LLMProvider (with List<LLMModel>)
LlmProvidersApiServiceImpl->>LLMProviderMappingUtil: Convert List<LLMModel> to List<LLMModelDTO>
LlmProvidersApiServiceImpl-->>AdminAPI: List<LLMModelDTO>
sequenceDiagram
participant RESTClient
participant PublisherAPI
participant LlmProvidersApiServiceImpl
participant ApiMgtDAO
participant Database
participant LLMProviderMappingUtil
RESTClient->>PublisherAPI: GET /llm-providers/{id}/models
PublisherAPI->>LlmProvidersApiServiceImpl: getLLMProviderModels(...)
LlmProvidersApiServiceImpl->>ApiMgtDAO: getLLMProvider(...)
ApiMgtDAO->>Database: Query LLMProvider and models (with families)
ApiMgtDAO-->>LlmProvidersApiServiceImpl: LLMProvider (List<LLMModel>)
LlmProvidersApiServiceImpl->>LLMProviderMappingUtil: fromLLMModelToLLMModelDTO(List<LLMModel>)
LlmProvidersApiServiceImpl-->>PublisherAPI: List<LLMModelDTO>
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.javacomponents/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✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR extends LLM provider support by introducing model families (vendor grouping) and a flag for multiple-vendor support.
- Introduces a new
LLMModelobject to represent vendor and its model values. - Updates request/response DTOs, OpenAPI specs, and SQL schema/DAO logic to handle model families and the
multipleVendorSupportflag. - Adjusts admin API service and mapping utilities to populate and persist the new structures.
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| components/.../LLMModelDTO.java | Adds DTO for vendor+values grouping |
| publisher-api.yaml, admin-api.yaml | Updates schema to reference new LLMModel and adds multipleVendorSupport flag |
| LLMProviderMappingUtil.java | Maps internal LLMModel to LLMModelDTO |
| LlmProvidersApiServiceImpl.java | Handles incoming DTOs, builds LLMProvider with families and flag |
| ApiMgtDAO.java, SQLConstants.java | Persists and retrieves model families in DB; adds new SQL columns and methods |
| APIAdminImpl.java | Removes legacy model-list API and delegates via DAO |
| OpenAiLLMProviderService.java, MistralAiLLMProviderService.java, AzureOpenAiLLMProviderService.java | Initializes default LLMModel lists |
Comments suppressed due to low confidence (4)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java:150
- Consider adding or updating unit tests to verify the new
multipleVendorSupportflag and model-family mapping logic inbuildLLMProvider, ensuring both single- and multi-vendor setups work as expected.
provider.setModelList(llmModels);
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/LLMProviderMappingUtil.java:73
- Missing import for java.util.stream.Collectors; add
import java.util.stream.Collectors;so.collect(Collectors.toList())compiles successfully.
.collect(Collectors.toList()));
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java:238
- The
multipleVendorSupportparameter passed intoupdateLLMProvideris never used when building the updated provider. UpdatebuildUpdatedLLMProvidersignature to accept this flag and callprovider.setModelFamilySupported(multipleVendorSupport)accordingly.
buildUpdatedLLMProvider(retrievedProvider, llmProviderId, description, configurations,
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java:15382
- The null-check is inverted:
models.get(familyName)returns null on first encounter, so the else branch (llmModel.getValues().add(...)) will NPE. Swap the condition toif (llmModel == null) { ... } else { ... }.
if (llmModel != null) {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml (1)
12774-12885: 🛠️ Refactor suggestionAdd required fields and quote example for clarity.
The new
LLMModelschema should explicitly declare which properties are mandatory and ensure thevendorexample is treated as a string literal. Consider applying this diff:LLMModel: title: LLM Model type: object + required: + - values + - vendor properties: values: type: array items: type: string example: "gpt-4o" vendor: type: string - example: OpenAI + example: "OpenAI"Optionally, add
descriptionfields to clarify usage.components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java (1)
285-315: 🛠️ Refactor suggestionUpdating clears existing models when
modelsparam is absentWhen
modelList == null, an empty list is set, wiping all previously stored models unintentionally.-List<LLMModel> llmModels = new ArrayList<>(); -if (modelList != null) { - ... -} -provider.setModelList(llmModels); +if (modelList != null) { + provider.setModelList(modelList.stream() + .map(dto -> new LLMModel(dto.getVendor(), dto.getValues())) + .collect(Collectors.toList())); +} else { + provider.setModelList(retrievedProvider.getModelList()); +}
🧹 Nitpick comments (33)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql (2)
3343-3343: Use consistent data type length for boolean flags
The newMODEL_FAMILY_SUPPORTEDcolumn is defined asVARCHAR(10), while the existingBUILT_IN_SUPPORTusesVARCHAR(5)to store'true'/'false'. Consider aligning them (e.g.,VARCHAR(5)) or switching to aCHAR(1)flag with a check constraint ('0'/'1') for stricter boolean enforcement.
3363-3363: Evaluate indexing and nullability for model family name
The addedMODEL_FAMILY_NAMEis nullable and lacks an index. If you frequently query or filter by this column, adding an index will improve performance. Also, whenMODEL_FAMILY_SUPPORTEDis true, you might enforceMODEL_FAMILY_NAMEasNOT NULLto ensure data consistency.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml (6)
1-14: Suggest defining a global security requirementAdding a root-level
securityblock ensures every path defaults to requiring the API key, reducing the risk of missing auth on future operations.Apply this diff:
openapi: 3.0.3 info: title: Amazon Bedrock Converse API +security: + - api_key: [] version: "1.0.0"🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 1-181: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
16-23: Group the/converseoperation withtagsFor better organization in generated docs, consider adding:
tags: - Bedrockimmediately under the operation’s metadata.
43-50: Group the/converse-streamoperation withtagsSimilarly, add:
tags: - Bedrockso the streaming endpoint appears under the same section in your API UI.
85-90: Enforce a maximum size on themessagesarrayBound the
messageslist to prevent excessively large payloads. For example:messages: type: array + maxItems: 100 description: List of conversation messages including prior turns.🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 85-90: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
106-111: Enforce a maximum size onadditionalModelResponseFieldPathsPrevent unbounded field lists with:
additionalModelResponseFieldPaths: type: array + maxItems: 50 items: type: string
180-180: Add a trailing newlineEnsure the file ends with a newline to satisfy YAML linting requirements.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 180-180: no new line character at the end of file
(new-line-at-end-of-file)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql (1)
2553-2553: Enforce nullability or default for model family name
MODEL_FAMILY_NAMEis nullable, but ifMODEL_FAMILY_SUPPORTEDis true the field should be populated. Consider adding aNOT NULLconstraint or aCHECKthat tiesMODEL_FAMILY_NAMEpresence toMODEL_FAMILY_SUPPORTED = TRUE, or provide a sensible default.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql (2)
3652-3652: Ensure data type length and default consistency
The newMODEL_FAMILY_SUPPORTEDcolumn is declared asVARCHAR(10)with a default of'false', whereas the existingBUILT_IN_SUPPORTflag usesVARCHAR(5). For consistency and to limit storage, consider reducing the length toVARCHAR(5)and/or introducing aCHECKconstraint to restrict values to'true'or'false'.
3670-3670: Validate nullability and indexing for model family
You’ve madeMODEL_FAMILY_NAMEnullable, which is fine generally, but ifMODEL_FAMILY_SUPPORTED = 'true'this column may become mandatory. Confirm the intended behavior and consider a conditionalNOT NULLconstraint or application-level validation. Also, if you’ll frequently filter byMODEL_FAMILY_NAME, think about adding an index or extending an existing unique constraint.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql (1)
2851-2851: Review uniqueness semantics forAM_LLM_PROVIDER_MODEL
The existing unique constraintUNIQUE (MODEL_NAME, LLM_PROVIDER_UUID)omitsMODEL_FAMILY_NAME, which may permit duplicate model names across different families under the same provider. If model names must be unique within each family, extend the constraint accordingly.- UNIQUE (MODEL_NAME, LLM_PROVIDER_UUID), + UNIQUE (MODEL_NAME, MODEL_FAMILY_NAME, LLM_PROVIDER_UUID),features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql (2)
3937-3937: Enforce valid boolean values and verify cross-dialect updates.
Add a CHECK constraint to restrict MODEL_FAMILY_SUPPORTED to 'true' or 'false', and ensure the same column is added in your MySQL/PostgreSQL scripts.Proposed DDL:
ALTER TABLE AM_LLM_PROVIDER ADD CONSTRAINT CHK_AM_LLM_PROVIDER_MODEL_FAMILY_SUPPORTED CHECK (MODEL_FAMILY_SUPPORTED IN ('true', 'false'));Verification script:
#!/bin/bash # Confirm all SQL dialect files include the new column rg --files-with-matches "CREATE TABLE.*AM_LLM_PROVIDER" -t sql | while read file; do grep -q "MODEL_FAMILY_SUPPORTED" "$file" || echo "Missing MODEL_FAMILY_SUPPORTED in $file" done
3958-3958: Consider indexing MODEL_FAMILY_NAME for query performance.
If your application filters or groups by MODEL_FAMILY_NAME frequently, an index will improve lookup speed.Example index:
CREATE INDEX IDX_AM_LLM_PROVIDER_MODEL_FAMILY_NAME ON AM_LLM_PROVIDER_MODEL(MODEL_FAMILY_NAME);features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql (1)
2937-2937: Enforce non-nullMODEL_FAMILY_NAMEwhen families are supported
IfmodelFamilySupportedisTRUE, each model must carry a family name. Consider adding aCHECKconstraint or markingMODEL_FAMILY_NAMEasNOT NULL(with an appropriate default) to prevent orphaned entries.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql (1)
2579-2579: Missing index for the newMODEL_FAMILY_NAMEcolumn
MODEL_FAMILY_NAMEwill likely be used in lookup queries when fetching models per family. Consider adding an index (or extending the existing unique index) to prevent full-table scans once the table grows.CREATE INDEX IDX_LLM_PROVIDER_MODEL_FAMILY ON AM_LLM_PROVIDER_MODEL (MODEL_FAMILY_NAME);components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java (2)
21-24: Unused wildcard import avoided – good, but remember code-style orderingAdding
java.util.ArraysandLLMModelimports is correct. Ensure your IDE keeps the project’s import-order rules to avoid future PR noise.
95-99: SetmodelFamilySupportedexplicitly for clarityYou migrated to
LLMModel, but the newmodelFamilySupportedflag onLLMProvideris left to its default value. Being explicit improves readability and protects against constructor-default changes:llmProvider.setModelList(modelList); +// OpenAI ships a single family, hence set to false explicitly +llmProvider.setModelFamilySupported(false);components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java (1)
95-99: Mirror the explicit flag setting here as wellFor consistency with other providers and to document intent:
llmProvider.setModelList(modelList); +llmProvider.setModelFamilySupported(false);components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java (1)
96-100: Immutable vs mutable nested list
Arrays.asList(...)returns a fixed-size list. If later code attempts to add/remove models to thisvalueslist, you’ll hitUnsupportedOperationException.
If mutability is desired, wrap withnew ArrayList<>(Arrays.asList(...)).components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java (1)
7-14: AddserialVersionUIDand basic value object helpers
LLMModelisSerializablebut lacks an explicitserialVersionUID, risking deserialization issues across builds.
Also consider overridingequals,hashCode, andtoString(or use Lombok) so the object behaves predictably in collections/logs.public class LLMModel implements Serializable { + private static final long serialVersionUID = 1L;features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql (2)
3938-3938: Use BOOLEAN Type for MODEL_FAMILY_SUPPORTED for Consistency
The newMODEL_FAMILY_SUPPORTEDcolumn is defined asVARCHAR(10)with'false'/'true'values, while existing feature flags use the nativeBOOLEANtype. Switching this toBOOLEAN DEFAULT FALSEwill align with other flags and simplify downstream logic.Please also verify that equivalent changes are applied to the MySQL/Postgres migration scripts.
3959-3959: Consider Constraint on Optional MODEL_FAMILY_NAME
CurrentlyMODEL_FAMILY_NAMEis nullable; however, whenMODEL_FAMILY_SUPPORTEDis enabled, a missing family name may violate business rules. Consider adding aNOT NULLor a check constraint (e.g. enforceMODEL_FAMILY_NAMEIS NOT NULL when supported) to maintain data integrity.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LLMProviderMappingUtil.java (1)
96-105: Method name & null-safety can be tightened
- The method handles lists, so a clearer name (e.g.,
fromLLMModelListToDTOList) would improve readability.- Prefer
Collections.emptyList()overnew ArrayList<>()for the null-input fast path.- Guard against
nullelements insidemodelListto avoid NPEs:.filter(Objects::nonNull)model.getValues()could benull; consider defaulting toCollections.emptyList().Example diff:
-public static List<LLMModelDTO> fromLLMModelToLLMModelDTO(List<LLMModel> modelList) { - if (modelList == null) { - return new ArrayList<>(); - } - return modelList.stream() - .map(model -> { - return new LLMModelDTO().vendor(model.getModelVendor()).values(model.getValues()); - }) - .collect(Collectors.toList()); +public static List<LLMModelDTO> fromLLMModelListToDTOList(List<LLMModel> modelList) { + if (modelList == null) { + return Collections.emptyList(); + } + return modelList.stream() + .filter(Objects::nonNull) + .map(model -> new LLMModelDTO() + .vendor(model.getModelVendor()) + .values(model.getValues() != null ? model.getValues() : Collections.emptyList())) + .collect(Collectors.toList()); }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java (2)
2804-2806: Include model family support in provider lookup by name/version
TheGET_LLM_PROVIDER_BY_NAME_AND_VERSION_SQLwas updated to selectMODEL_FAMILY_SUPPORTED. Good. Consider prefixing with the table alias (e.g.,PROVIDER.MODEL_FAMILY_SUPPORTED) for consistency and to avoid ambiguity in complex joins.
2819-2819: Retrieve model family name in provider models query
GET_LLM_PROVIDER_MODELS_SQLselects bothMODEL_NAMEandMODEL_FAMILY_NAME. This aligns with the newLLMModelstructure. For predictable ordering, consider adding anORDER BY MODEL_FAMILY_NAME, MODEL_NAME.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml (1)
12774-12785: AugmentLLMModelschema with metadata.To improve clarity and validation:
- Add a
requiredarray (values,vendor) to enforce mandatory fields.- Include
descriptionfor the component and each property.- Consider
uniqueItems: trueonvaluesif duplicates aren’t allowed.- Quote the
vendorexample ("OpenAI") for YAML consistency.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml (1)
5407-5417: EnhanceLLMModelschema with field descriptions
TheLLMModeldefinition is correct, but addingdescriptionentries forvaluesandvendorwill improve clarity in generated docs:values: type: array items: type: string description: List of supported model names (e.g., "gpt-4o"). vendor: type: string description: Name of the model vendor (e.g., "OpenAI").components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml (3)
4242-4242: Fix grammatical error in label creation description.The description "Label object that should to be added" has an incorrect "should to be". Change it to "Label object to be added."
5402-5406: Require model list parameter.The
modelsarray inLLMProviderRequestshould likely be mandatory whenmultipleVendorSupportis true. Add"models"to therequiredlist in the schema or provide guidance on its necessity.
5442-5452: Makemodelsrequired in response.In
LLMProviderResponse, themodelsarray should be listed in therequiredsection to guarantee clients always receive model information. Consider adding:LLMProviderResponse: required: - models properties: models: ...components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java (2)
67-68: Javadoc now stale – parameter description mismatches actual type
@param modelsstill says “Comma separated list of models” but the signature was migrated toList<LLMModelDTO>.
Update the comment to avoid misleading API consumers.
128-151: Minor: redundant temporaryllmModelsallocation
llmModelsis only populated whenmodelList != null; otherwise the empty list is assigned.
You can skip the extra variable and directly setCollections.emptyList()for clarity:-List<LLMModel> llmModels = new ArrayList<>(); -if (modelList != null) { - for (LLMModelDTO modelDTO : modelList) { - llmModels.add(new LLMModel(modelDTO.getVendor(), modelDTO.getValues())); - } -} -provider.setModelList(llmModels); +if (modelList != null) { + provider.setModelList( + modelList.stream() + .map(dto -> new LLMModel(dto.getVendor(), dto.getValues())) + .collect(Collectors.toList())); +} else { + provider.setModelList(Collections.emptyList()); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/LlmProvidersApi.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/LlmProvidersApiService.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMModelDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMProviderRequestDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMProviderResponseDTO.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/LLMModelDTO.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/LlmProvidersApi.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/LlmProvidersApiService.javais excluded by!**/gen/**
📒 Files selected for processing (27)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIAdmin.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMProvider.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java(10 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.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java(8 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/LLMProviderMappingUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml(5 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml(5 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LLMProviderMappingUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LlmProvidersApiServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql(2 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(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql(2 hunks)
💤 Files with no reviewable 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.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LLMProviderMappingUtil.java (3)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java (1)
LLMModel(7-34)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMProvider.java (1)
LLMProvider(30-137)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/gen/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/dto/LLMModelDTO.java (1)
LLMModelDTO(23-102)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java (2)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java (1)
LLMModel(7-34)components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMModelDTO.java (1)
LLMModelDTO(23-102)
🪛 Checkov (3.2.334)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml
[HIGH] 1-181: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[MEDIUM] 85-90: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml
[error] 180-180: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (4, group4)
- GitHub Check: run-benchmark-test
- GitHub Check: build-product (3, group3)
- GitHub Check: build-carbon
- GitHub Check: build-product (2, group2)
🔇 Additional comments (13)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql (1)
2571-2571: Boolean flag stored as VARCHAR – verify type & default compatibility
MODEL_FAMILY_SUPPORTEDis declared asVARCHAR(10) NOT NULL DEFAULT 'false'.
Storing booleans in a text column can be error-prone and wastes space. MySQL providesTINYINT(1)orENUM('true','false'), which are semantically clearer and avoid unexpected casing issues.-MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', +MODEL_FAMILY_SUPPORTED TINYINT(1) NOT NULL DEFAULT 0,If you keep the current definition, remember to quote the default value correctly for every RDBMS flavour and update the DAO layer to map it to a boolean.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java (1)
21-24: Imports look goodNecessary imports for
ArraysandLLMModelwere added correctly.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml (1)
9314-9317: Approve the schema reference update but verify consistency across endpoints.Switching from plain strings to a
$refofLLMModelcorrectly aligns with the newLLMModeldefinition. Please ensure that:
- All other endpoints (e.g., Admin API) referencing model lists are updated similarly.
- Example payloads and response examples in this spec are adjusted to show the new object shape.
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java (1)
96-100: Remember to flag provider-level “model family supported” capability
LLMProvidernow carries amodelFamilySupportedboolean. This built-in provider clearly supports families (single vendor entry with multiple models) but the flag isn’t set, so downstream UI/DAO logic may treat it as flat-model.llmProvider.setModelList(modelList); +llmProvider.setModelFamilySupported(true);Please verify the actual setter name in
LLMProvider.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java (2)
2800-2801: Select provider model family support flag
TheGET_LLM_PROVIDER_SQLnow correctly retrieves the newMODEL_FAMILY_SUPPORTEDcolumn for LLM providers. Ensure the DAO mapping andLLMProvidermodel include this field.
Please verify that the correspondingResultSetExtractororRowMapperhas been updated to map this new column.
2816-2816: Add model family name to model insert
TheINSERT_LLM_PROVIDER_MODELS_SQLnow persistsMODEL_FAMILY_NAMEalongsideMODEL_NAME. Make sure the DAO method binds parameters in the order(modelName, modelFamilyName, providerUuid).
Double-check that any existing tests for model insertion are updated to include the new parameter.components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/LLMProviderMappingUtil.java (1)
20-20: Import aligns with new DTO usage – all good.
No further action needed.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml (1)
9315-9317: Reference newLLMModelobject for response items.You’ve correctly updated the endpoint to return an array of
LLMModelobjects instead of plain strings, aligning the OpenAPI spec with the new domain model.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml (3)
5387-5393: Verify mapping ofmultipleVendorSupportto backend field
The newmultipleVendorSupportflag inLLMProviderRequestshould correspond to themodelFamilySupportedboolean in your domain model and DAO layer. Ensure the mapping util and API service correctly translate between these names.
5401-5404: Confirm renaming ofmodelListtomodelsin request schema
You’ve introducedmodelsas an array ofLLMModelin the request. Please verify that all references to the oldmodelListproperty have been removed and that code generators or DTO mappers have been updated accordingly.
5449-5452: Confirm renaming ofmodelListtomodelsin response schema
Similar to the request, ensure the oldmodelListfield is fully removed fromLLMProviderResponseand that serialization/deserialization logic aligns with this newmodelsarray.components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml (1)
4260-4260: Confirm conflict response addition.Adding the 409 Conflict response is appropriate when a label already exists. Ensure the client handles this status correctly.
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java (1)
295-296:modelFamilySupportedcannot be changed via updateYou copy the old value (
retrievedProvider.isModelFamilySupported()) without considering the newmultipleVendorSupportrequest param.
Extend the method signature and logic so clients can toggle this flag.
| @@ -2532,6 +2532,7 @@ CREATE TABLE IF NOT EXISTS AM_LLM_PROVIDER ( | |||
| DESCRIPTION VARCHAR(1023), | |||
| CONFIGURATIONS BLOB NOT NULL, | |||
| API_DEFINITION LONGBLOB NOT NULL, | |||
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a consistent data type for boolean flags
Currently MODEL_FAMILY_SUPPORTED is defined as VARCHAR(10) with default 'false'. For consistency with the existing BUILT_IN_SUPPORT column (which uses VARCHAR(5)) and H2’s native boolean support, consider one of the following refactors:
Option A: Switch to a Boolean type
- MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false',
+ MODEL_FAMILY_SUPPORTED BOOLEAN NOT NULL DEFAULT FALSE,Option B: Align string lengths
- MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false',
+ MODEL_FAMILY_SUPPORTED VARCHAR(5) NOT NULL DEFAULT 'false',Ensure your Java entity mappings are updated accordingly.
📝 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.
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |
| MODEL_FAMILY_SUPPORTED BOOLEAN NOT NULL DEFAULT FALSE, |
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql
at line 2535, the MODEL_FAMILY_SUPPORTED column is defined as VARCHAR(10) with
default 'false', which is inconsistent with the BUILT_IN_SUPPORT column using
VARCHAR(5) and H2's boolean support. To fix this, either change
MODEL_FAMILY_SUPPORTED to a BOOLEAN type with a default of FALSE or adjust its
VARCHAR length to 5 to match BUILT_IN_SUPPORT. Also, update the corresponding
Java entity mappings to reflect this data type change.
| @@ -2828,6 +2828,7 @@ CREATE TABLE AM_LLM_PROVIDER ( | |||
| DESCRIPTION VARCHAR(1023), | |||
| CONFIGURATIONS VARBINARY(MAX) NOT NULL, | |||
| API_DEFINITION VARBINARY(MAX) NOT NULL, | |||
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use BIT type for MODEL_FAMILY_SUPPORTED
Consistency: other boolean flags in this schema use BIT (or CHAR(1)) rather than a VARCHAR(10). Switch to a BIT NOT NULL DEFAULT 0 for cleaner storage and indexing.
- MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false',
+ MODEL_FAMILY_SUPPORTED BIT NOT NULL DEFAULT 0,📝 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.
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |
| MODEL_FAMILY_SUPPORTED BIT NOT NULL DEFAULT 0, |
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql
at line 2831, change the column type of MODEL_FAMILY_SUPPORTED from VARCHAR(10)
to BIT, and update the default value from 'false' to 0. This aligns with the
schema's convention for boolean flags, improving storage efficiency and
indexing.
| MODEL_NAME VARCHAR(255) NOT NULL, | ||
| MODEL_FAMILY_NAME VARCHAR(255) NULL, | ||
| LLM_PROVIDER_UUID VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
Missing migration and type consistency for model family name
Similar to above, new MODEL_FAMILY_NAME in AM_LLM_PROVIDER_MODEL won’t be applied on existing schemas. You must supply an ALTER TABLE AM_LLM_PROVIDER_MODEL ADD COLUMN MODEL_FAMILY_NAME VARCHAR(255) migration. Optionally, specify a default or enforce non-null when MODEL_FAMILY_SUPPORTED is true.
Shall I prepare the corresponding ALTER TABLE statement?
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql
around lines 2739 to 2741, the new column MODEL_FAMILY_NAME is added to the
AM_LLM_PROVIDER_MODEL table but there is no corresponding migration to update
existing schemas. To fix this, create an ALTER TABLE migration statement that
adds the MODEL_FAMILY_NAME column with type VARCHAR(255) to the
AM_LLM_PROVIDER_MODEL table. Optionally, set a default value or add a NOT NULL
constraint when MODEL_FAMILY_SUPPORTED is true to maintain type consistency.
| @@ -2719,6 +2719,7 @@ CREATE TABLE IF NOT EXISTS AM_LLM_PROVIDER ( | |||
| DESCRIPTION VARCHAR(1023), | |||
| CONFIGURATIONS BLOB NOT NULL, | |||
| API_DEFINITION LONGBLOB NOT NULL, | |||
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |||
There was a problem hiding this comment.
Existing installs won’t pick up the new column
Using CREATE TABLE IF NOT EXISTS means that for an existing AM_LLM_PROVIDER table the new MODEL_FAMILY_SUPPORTED column will never be added. You need a migration ALTER TABLE script to add this column.
Additionally, for consistency with the existing BUILT_IN_SUPPORT VARCHAR(5) flag, consider using VARCHAR(5) (or better yet BOOLEAN/TINYINT(1)) instead of VARCHAR(10).
Do you want help drafting the ALTER TABLE AM_LLM_PROVIDER ADD COLUMN MODEL_FAMILY_SUPPORTED ... migration snippet?
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql
at line 2722, the new column MODEL_FAMILY_SUPPORTED is added only in the CREATE
TABLE statement, which won't affect existing tables. To fix this, create a
separate ALTER TABLE migration script that adds the MODEL_FAMILY_SUPPORTED
column to the existing AM_LLM_PROVIDER table. Also, change the column type from
VARCHAR(10) to VARCHAR(5) or preferably BOOLEAN/TINYINT(1) to be consistent with
the BUILT_IN_SUPPORT flag. If needed, draft the ALTER TABLE statement
accordingly.
| @@ -2913,6 +2913,7 @@ CREATE TABLE IF NOT EXISTS AM_LLM_PROVIDER ( | |||
| DESCRIPTION VARCHAR(1023), | |||
| CONFIGURATIONS BYTEA NOT NULL, | |||
| API_DEFINITION BYTEA NOT NULL, | |||
| MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false', | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch MODEL_FAMILY_SUPPORTED to BOOLEAN and backfill existing rows
Using VARCHAR(10) for a boolean‐style flag can lead to invalid values and wastes storage. Replace it with PostgreSQL’s native BOOLEAN DEFAULT FALSE and include a migration backfill, for example:
ALTER TABLE AM_LLM_PROVIDER
ALTER COLUMN MODEL_FAMILY_SUPPORTED TYPE BOOLEAN USING (MODEL_FAMILY_SUPPORTED::BOOLEAN),
ALTER COLUMN MODEL_FAMILY_SUPPORTED SET DEFAULT FALSE;
UPDATE AM_LLM_PROVIDER SET MODEL_FAMILY_SUPPORTED = FALSE;I’d like to confirm where and how MODEL_FAMILY_SUPPORTED is used across the repo before recommending the change. Let’s locate all references:
🏁 Script executed:
#!/bin/bash
set -e
rg -n "MODEL_FAMILY_SUPPORTED"Length of output: 2379
Switch MODEL_FAMILY_SUPPORTED to BOOLEAN everywhere and update DAO parsing
We should convert this flag from VARCHAR(10) to native BOOLEAN in all dialect scripts and adjust the Java DAO to read it correctly:
• In each SQL script under features/apimgt/.../sql/ (mssql.sql, postgresql.sql, oracle.sql, oracle_23c.sql, oracle_rac.sql, mysql.sql, mysql_cluster.sql, h2.sql, db2.sql):
- MODEL_FAMILY_SUPPORTED VARCHAR(10) NOT NULL DEFAULT 'false',
+ MODEL_FAMILY_SUPPORTED BOOLEAN NOT NULL DEFAULT FALSE,And add a migration for existing tables, e.g. for Postgres:
ALTER TABLE AM_LLM_PROVIDER
ALTER COLUMN MODEL_FAMILY_SUPPORTED TYPE BOOLEAN USING (MODEL_FAMILY_SUPPORTED::BOOLEAN),
ALTER COLUMN MODEL_FAMILY_SUPPORTED SET DEFAULT FALSE;
UPDATE AM_LLM_PROVIDER SET MODEL_FAMILY_SUPPORTED = FALSE;(Repeat equivalent ALTER/UPDATE for each dialect.)
• In components/apimgt/.../SQLConstants.java: no change to constant strings, but note that the column is now boolean.
• In components/apimgt/.../ApiMgtDAO.java: replace
Boolean.parseBoolean(providerResultSet.getString("MODEL_FAMILY_SUPPORTED"))with
providerResultSet.getBoolean("MODEL_FAMILY_SUPPORTED")in both occurrences to avoid parsing “t”/“f” or driver‐specific boolean literals.
This ensures type safety, eliminates invalid values, saves space, and keeps the DAO in sync.
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql
at line 2916, change the MODEL_FAMILY_SUPPORTED column from VARCHAR(10) to
BOOLEAN with DEFAULT FALSE by altering the column type using a cast and setting
the default. Add an UPDATE statement to backfill existing rows with FALSE.
Repeat equivalent ALTER and UPDATE statements in all other SQL dialect scripts
under features/apimgt/.../sql/. Then, in the Java DAO (ApiMgtDAO.java), replace
all occurrences of
Boolean.parseBoolean(providerResultSet.getString("MODEL_FAMILY_SUPPORTED")) with
providerResultSet.getBoolean("MODEL_FAMILY_SUPPORTED") to correctly handle the
boolean type. No changes are needed in SQLConstants.java.
| * @param multipleVendorSupport Whether the provider supports multiple vendors (unused in logic). | ||
| * @param configurations The configurations of the provider. |
There was a problem hiding this comment.
Docstring & parameter named but still “unused” – leads to API drift
The Javadoc admits multipleVendorSupport is unused in the update path.
Updating the provider should allow toggling this flag; otherwise the REST contract is misleading.
🤖 Prompt for AI Agents
In
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java
around lines 215-216, the parameter multipleVendorSupport is documented but not
used in the update logic, causing API drift. To fix this, incorporate logic to
update the multipleVendorSupport flag in the provider update method so that
changes to this flag are reflected, ensuring the REST API contract matches the
implementation.
| public Response updateLLMProvider(String llmProviderId, String name, String apiVersion, | ||
| String description, String configurations, InputStream apiDefinitionInputStream, | ||
| Attachment apiDefinitionDetail, String modelList, MessageContext messageContext) | ||
| String description, Boolean multipleVendorSupport, String configurations, | ||
| InputStream apiDefinitionInputStream, | ||
| Attachment apiDefinitionDetail, List<LLMModelDTO> models, | ||
| MessageContext messageContext) | ||
| throws APIManagementException { |
There was a problem hiding this comment.
multipleVendorSupport ignored in update flow
updateLLMProvider(..) receives the Boolean but never propagates it to buildUpdatedLLMProvider(..).
Result: clients cannot change the flag once the provider is created.
- buildUpdatedLLMProvider(retrievedProvider, llmProviderId, description,
- configurations, apiDefinitionInputStream, models);
+ buildUpdatedLLMProvider(retrievedProvider, llmProviderId, description,
+ configurations, apiDefinitionInputStream, models,
+ multipleVendorSupport);and extend the helper method accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest.api.admin.v1.impl/LlmProvidersApiServiceImpl.java
around lines 225 to 230, the multipleVendorSupport parameter is received in
updateLLMProvider but not passed to buildUpdatedLLMProvider, preventing updates
to this flag. Modify the updateLLMProvider method to pass multipleVendorSupport
to buildUpdatedLLMProvider and update the helper method signature and
implementation to accept and handle this parameter accordingly.
.../src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java
Outdated
Show resolved
Hide resolved
...mgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java
Outdated
Show resolved
Hide resolved
| private void addLLMModels(Connection connection, LLMProvider llmProvider) throws SQLException { | ||
| try (PreparedStatement prepStmtInsertModels = connection.prepareStatement( | ||
| SQLConstants.INSERT_LLM_PROVIDER_MODELS_SQL)) { | ||
| // Insert LLM provider model | ||
| for (LLMModel model : llmProvider.getModelList()) { | ||
| for (String value : model.getValues()) { | ||
| prepStmtInsertModels.setString(1, value); | ||
| prepStmtInsertModels.setString(2, model.getModelVendor()); | ||
| prepStmtInsertModels.setString(3, llmProvider.getId()); | ||
| prepStmtInsertModels.addBatch(); | ||
| } | ||
| } | ||
| prepStmtInsertModels.executeBatch(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safety + batch insert robustness
llmProvider.getModelList() or model.getValues() may legally be null/empty when a provider is first created.
Guard the loops and skip the batch when there is nothing to persist to avoid NullPointerException.
-for (LLMModel model : llmProvider.getModelList()) {
- for (String value : model.getValues()) {
+if (llmProvider.getModelList() != null) {
+ for (LLMModel model : llmProvider.getModelList()) {
+ if (model.getValues() == null) {
+ continue;
+ }
+ for (String value : model.getValues()) {
...
}
}
}📝 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 void addLLMModels(Connection connection, LLMProvider llmProvider) throws SQLException { | |
| try (PreparedStatement prepStmtInsertModels = connection.prepareStatement( | |
| SQLConstants.INSERT_LLM_PROVIDER_MODELS_SQL)) { | |
| // Insert LLM provider model | |
| for (LLMModel model : llmProvider.getModelList()) { | |
| for (String value : model.getValues()) { | |
| prepStmtInsertModels.setString(1, value); | |
| prepStmtInsertModels.setString(2, model.getModelVendor()); | |
| prepStmtInsertModels.setString(3, llmProvider.getId()); | |
| prepStmtInsertModels.addBatch(); | |
| } | |
| } | |
| prepStmtInsertModels.executeBatch(); | |
| } | |
| if (llmProvider.getModelList() != null) { | |
| for (LLMModel model : llmProvider.getModelList()) { | |
| if (model.getValues() == null) { | |
| continue; | |
| } | |
| for (String value : model.getValues()) { | |
| // … | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java
around lines 15205 to 15218, add null checks before iterating over
llmProvider.getModelList() and model.getValues(). If either is null or empty,
skip the batch insert logic to prevent NullPointerException. This involves
checking for null or empty collections before the loops and returning early or
continuing as appropriate.
tharindu1st
force-pushed
the
model-family-support
branch
from
21d0f58 to
4a0a021
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml (2)
85-90: Define maximum items for array properties
Prevent excessively large requests by adding amaxItemsconstraint to arrays such asmessagesandadditionalModelResponseFieldPaths.Example diff for
messages:messages: type: array description: List of conversation messages including prior turns. + maxItems: 100 items: $ref: '#/components/schemas/Message'Mirror this for
additionalModelResponseFieldPaths.
180-180: Ensure newline at end of file
YAMLLint flags missing newline at EOF.Add a newline character after the last line.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/LlmProvidersApi.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/LlmProvidersApiService.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMModelDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMProviderRequestDTO.javais excluded by!**/gen/**components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/LLMProviderResponseDTO.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/LLMModelDTO.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/LlmProvidersApi.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/LlmProvidersApiService.javais excluded by!**/gen/**
📒 Files selected for processing (32)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIAdmin.java(0 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/AzureOpenAiLLMProviderService.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMProvider.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java(10 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.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java(10 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/LLMProviderMappingUtil.java(3 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml(6 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml(6 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml(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/mappings/PublisherCommonUtils.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LLMProviderMappingUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LlmProvidersApiServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml(1 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/OGG/oracle/apimgt/tables.sql(0 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/OGG/oracle/apimgt/tables_23c.sql(0 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/Postgresql/apimgt/tables.sql(0 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/SQLServer/mssql/apimgt/tables.sql(0 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql(2 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(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql(2 hunks)features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql(2 hunks)
💤 Files with no reviewable changes (5)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIAdmin.java
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/OGG/oracle/apimgt/tables.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/Postgresql/apimgt/tables.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/OGG/oracle/apimgt/tables_23c.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/multi-dc/SQLServer/mssql/apimgt/tables.sql
✅ Files skipped from review due to trivial changes (2)
- 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
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_23c.sql
🚧 Files skipped from review as they are similar to previous changes (24)
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/OpenAiLLMProviderService.java
- 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/LlmProvidersApiServiceImpl.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/MistralAiLLMProviderService.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMModel.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/LLMProviderMappingUtil.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/resources/publisher-api.yaml
- components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/LLMProviderMappingUtil.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/publisher-api.yaml
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle_rac.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql_cluster.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/db2.sql
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/constants/SQLConstants.java
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIAdminImpl.java
- components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/model/LLMProvider.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/LlmProvidersApiServiceImpl.java
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dao/ApiMgtDAO.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql
- components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml
[HIGH] 1-181: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[MEDIUM] 85-90: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml
[error] 180-180: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (11)
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml (11)
1-7: OpenAPI header and info section is valid
API version, title, and description align with Amazon Bedrock’s conversational API.
8-14: Server variable configuration is correct
Theregionvariable defaults tous-east-1with an appropriate description.
15-41: Operation-level security is applied
Each endpoint correctly includessecurity: - api_key: [].
42-67: Streaming endpoint definition looks good
/converse-streamis defined with consistent request/response structure and security.
68-74: Security scheme definition looks correct
The API key scheme undercomponents.securitySchemesis properly declared.
75-103: ConverseRequest schema is comprehensive
Covers session context, messages, inference config, and optional fields as intended.
115-128: Message schema is well-defined
Requiredroleandcontentfields use clear enums and descriptions.
129-140: InferenceConfig schema covers key parameters
maxTokens,temperature, andtopPare correctly typed and described.
142-162: ConverseResponse schema aligns with expected output
IncludessessionId,output, and ancillary info likemetricsandusage.
163-169: ConverseStreamResponse schema is appropriate
Defines astreamarray ofStreamEventfor chunked responses.
172-180: StreamEvent schema is valid
CaptureseventTypeanddatafields for streaming events.
| openapi: 3.0.3 | ||
| info: | ||
| title: Amazon Bedrock Converse API | ||
| version: "1.0.0" | ||
| description: | | ||
| OpenAPI specification for Amazon Bedrock's Converse and ConverseStream operations. | ||
|
|
There was a problem hiding this comment.
Add a global security definition to enforce API key usage
Defining security at the root reduces repetition and ensures all operations are secured.
Apply this diff:
openapi: 3.0.3
info:
title: Amazon Bedrock Converse API
version: "1.0.0"
description: |
OpenAPI specification for Amazon Bedrock's Converse and ConverseStream operations.
+security:
+ - api_key: []🧰 Tools
🪛 Checkov (3.2.334)
[HIGH] 1-181: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
🤖 Prompt for AI Agents
In
features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/api_definitions/aws_bedrock_api.yaml
at the top (lines 1 to 7), add a global security definition to enforce API key
usage by defining a security scheme under components and applying a root-level
security requirement. This will ensure all API operations require the API key
without repeating security details in each operation.
No description provided.