fix: allow inference profile application when uploading file #1323
Conversation
|
DescriptionSummary By MatterAI
🔄 What ChangedThis pull request modifies 🔍 Impact of the ChangeThe primary impact is enhanced flexibility for users uploading files to Bedrock, allowing them to leverage inference profiles for model selection. This improves the integration with AWS Bedrock's advanced features and simplifies model management for users who rely on ARNs. The change ensures that the correct foundation model is identified and used for file uploads, even when provided as an ARN, preventing potential errors or misconfigurations. 📁 Total Files Changed
🧪 Test AddedN/A - No explicit tests mentioned in the PR data. 🔒Security VulnerabilitiesN/A - No new security vulnerabilities were identified in the provided changes. The ARN parsing logic should be robust to prevent unexpected behavior. MotivationThe motivation is to fix a limitation where inference profiles could not be applied during file uploads, thereby enhancing the utility and flexibility of the Bedrock file upload functionality. Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as BedrockUploadFileHandler
participant Utils as BedrockUtils
participant AWSAPI as AWS Bedrock API
participant S3 as AWS S3
Client->>+Handler: POST /bedrock/uploadFile (file, providerOptions{awsBedrockModel: ARN/slug})
Note over Handler: Extract providerOptions
Handler->>Handler: Check awsAuthType
alt awsAuthType === 'assumedRole'
Handler->>Utils: providerAssumedRoleCredentials(c, providerOptions)
Utils-->>Handler: Credentials set
end
Handler->>Handler: Extract awsBedrockModel (modelParam)
alt modelParam includes 'arn:aws'
Handler->>+Utils: getFoundationModelFromInferenceProfile(c, modelParam, providerOptions)
Note over Utils: Resolve ARN to foundation model
Utils->>AWSAPI: DescribeFoundationModel(ARN) OR ListFoundationModels()
AWSAPI-->>-Utils: FoundationModelId
Utils-->>-Handler: resolvedFoundationModelId
Handler->>Handler: Set awsBedrockModel = resolvedFoundationModelId
end
Handler->>Handler: Validate awsS3Bucket and awsBedrockModel
alt Validation Failed
Handler-->>-Client: 400 Bad Request (Missing bucket/model)
else Validation Success
Handler->>+S3: AwsMultipartUploadHandler.upload(awsRegion, awsS3Bucket, awsS3ObjectKey, file)
S3-->>-Handler: Upload Success
Handler-->>-Client: 200 OK (Upload Status)
end
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| if (awsBedrockModel.includes('arn:aws')) { | ||
| const foundationModel = awsBedrockModel.includes('foundation-model/') | ||
| ? awsBedrockModel.split('/').pop() | ||
| : await getFoundationModelFromInferenceProfile( | ||
| c, | ||
| awsBedrockModel, | ||
| providerOptions | ||
| ); | ||
| if (foundationModel) { | ||
| awsBedrockModel = foundationModel; | ||
| } | ||
| } |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Logic error in ARN parsing - split('/').pop() will return undefined for ARNs ending with '/' or empty segments
Fix: Add null check and fallback handling for edge cases
Impact: Prevents runtime crashes when processing malformed ARNs
| if (awsBedrockModel.includes('arn:aws')) { | |
| const foundationModel = awsBedrockModel.includes('foundation-model/') | |
| ? awsBedrockModel.split('/').pop() | |
| : await getFoundationModelFromInferenceProfile( | |
| c, | |
| awsBedrockModel, | |
| providerOptions | |
| ); | |
| if (foundationModel) { | |
| awsBedrockModel = foundationModel; | |
| } | |
| } | |
| if (awsBedrockModel.includes('arn:aws')) { | |
| const foundationModel = awsBedrockModel.includes('foundation-model/') | |
| ? awsBedrockModel.split('/').pop() || awsBedrockModel | |
| : await getFoundationModelFromInferenceProfile( | |
| c, | |
| awsBedrockModel, | |
| providerOptions | |
| ); | |
| if (foundationModel) { | |
| awsBedrockModel = foundationModel; | |
| } | |
| } |
| const { | ||
| awsRegion, | ||
| awsS3Bucket, | ||
| awsBedrockModel: modelParam, | ||
| } = providerOptions; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Inconsistent variable naming - modelParam is used temporarily but awsBedrockModel is the actual working variable
Fix: Use more descriptive names to clarify the transformation flow
Impact: Improves code readability and reduces confusion about variable purpose
| const { | |
| awsRegion, | |
| awsS3Bucket, | |
| awsBedrockModel: modelParam, | |
| } = providerOptions; | |
| const { | |
| awsRegion, | |
| awsS3Bucket, | |
| awsBedrockModel: originalModelParam, | |
| } = providerOptions; |
| providerOptions.awsS3ObjectKey || crypto.randomUUID() + '.jsonl'; | ||
|
|
||
| if (!awsS3Bucket || !awsBedrockModel) { | ||
| if (!awsS3Bucket || !modelParam) { |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Variable reference should match the renamed parameter for consistency
Fix: Update validation to use the renamed parameter
Impact: Maintains consistency with variable naming convention
| if (!awsS3Bucket || !modelParam) { | |
| if (!awsS3Bucket || !originalModelParam) { |
| ); | ||
| } | ||
|
|
||
| let awsBedrockModel = modelParam; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Variable initialization should reference the renamed parameter for consistency
Fix: Update initialization to use the renamed parameter
Impact: Maintains consistency with variable naming convention
| let awsBedrockModel = modelParam; | |
| let awsBedrockModel = originalModelParam; |
3 participants
Description
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues