-
Notifications
You must be signed in to change notification settings - Fork 747
cleanup vertex api.ts #1350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
cleanup vertex api.ts #1350
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
export class GatewayError extends Error { | ||
constructor( | ||
message: string, | ||
public status: number = 500, | ||
public cause?: Error | ||
) { | ||
super(message); | ||
this.name = 'GatewayError'; | ||
this.status = status; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -809,7 +809,7 @@ export async function tryTargetsRecursively( | |||||
message: errorMessage, | ||||||
}), | ||||||
{ | ||||||
status: 500, | ||||||
status: error instanceof GatewayError ? error.status : 500, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Hardcoded error status code
Suggested change
|
||||||
headers: { | ||||||
'content-type': 'application/json', | ||||||
// Add this header so that the fallback loop can be interrupted if its an exception. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,8 +1,9 @@ | ||||||||||||||
import { GatewayError } from '../../errors/GatewayError'; | ||||||||||||||
import { Options } from '../../types/requestBody'; | ||||||||||||||
import { endpointStrings, ProviderAPIConfig } from '../types'; | ||||||||||||||
import { getModelAndProvider, getAccessToken, getBucketAndFile } from './utils'; | ||||||||||||||
|
||||||||||||||
const getApiVersion = (provider: string, inputModel: string) => { | ||||||||||||||
const getApiVersion = (provider: string) => { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: Unused parameter
Suggested change
|
||||||||||||||
if (provider === 'meta') return 'v1beta1'; | ||||||||||||||
return 'v1'; | ||||||||||||||
}; | ||||||||||||||
|
@@ -17,12 +18,12 @@ const getProjectRoute = ( | |||||||||||||
vertexServiceAccountJson, | ||||||||||||||
} = providerOptions; | ||||||||||||||
let projectId = inputProjectId; | ||||||||||||||
if (vertexServiceAccountJson && vertexServiceAccountJson.project_id) { | ||||||||||||||
if (vertexServiceAccountJson?.project_id) { | ||||||||||||||
|
if (vertexServiceAccountJson?.project_id) { | |
if (vertexServiceAccountJson?.project_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved: Previous suggestion was for optional chaining which is not needed as the code already handles null case correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: Unused parameter in function call
Fix: Remove unused inputModel
parameter
Impact: Cleaner function call
const routeVersion = getApiVersion(provider); | |
const routeVersion = getApiVersion(provider); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: Inconsistent URL search params initialization
Fix: Use new URLSearchParams(url.search)
for consistency
Impact: More consistent code pattern
const searchParams = new URLSearchParams(url.search); | |
const searchParams = new URLSearchParams(url.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: Missing default case in switch statement
Fix: Add default case returning empty string
Impact: Prevents potential undefined returns
default: | |
return ''; | |
default: | |
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security Issue Fix
Issue: Missing model validation could lead to unexpected behavior
Fix: Add explicit model validation with GatewayError
Impact: Prevents processing requests without required model parameter
if (!inputModel) { | |
throw new GatewayError('Model is required', 400); | |
} | |
if (!inputModel) { | |
throw new GatewayError('Model is required', 400); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: Missing default return in switch statement
Fix: Add default return for rawPredict endpoint
Impact: Ensures consistent return value
return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; | |
return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: Redundant assignment of
status
property in constructorFix: Remove redundant assignment since it's already initialized in parameter
Impact: Cleaner code, no functional change