Skip to content

Commit 0ae8e65

Browse files
committed
fix: address PR review comments for OpenAI compatible embedder
- Add HttpError interface for better type safety - Cache isFullEndpointUrl result for performance optimization - Implement smart URL detection with regex patterns for known providers - Add comprehensive tests for URL detection edge cases
1 parent 49e775a commit 0ae8e65

File tree

2 files changed

+98
-13
lines changed

2 files changed

+98
-13
lines changed

src/services/code-index/embedders/__tests__/openai-compatible.spec.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,4 +771,68 @@ describe("OpenAICompatibleEmbedder", () => {
771771
})
772772
})
773773
})
774+
775+
describe("URL detection", () => {
776+
it("should detect Azure deployment URLs as full endpoints", async () => {
777+
const embedder = new OpenAICompatibleEmbedder(
778+
"https://myinstance.openai.azure.com/openai/deployments/my-deployment/embeddings?api-version=2023-05-15",
779+
"test-key",
780+
)
781+
782+
// The private method is tested indirectly through the createEmbeddings behavior
783+
// If it's detected as a full URL, it will make a direct HTTP request
784+
const mockFetch = vitest.fn().mockResolvedValue({
785+
ok: true,
786+
json: async () => ({
787+
data: [{ embedding: [0.1, 0.2] }],
788+
usage: { prompt_tokens: 10, total_tokens: 15 },
789+
}),
790+
})
791+
global.fetch = mockFetch
792+
793+
await embedder.createEmbeddings(["test"])
794+
795+
// Should make direct HTTP request to the full URL
796+
expect(mockFetch).toHaveBeenCalledWith(
797+
"https://myinstance.openai.azure.com/openai/deployments/my-deployment/embeddings?api-version=2023-05-15",
798+
expect.any(Object),
799+
)
800+
})
801+
802+
it("should detect /embed endpoints as full URLs", async () => {
803+
const embedder = new OpenAICompatibleEmbedder("https://api.example.com/v1/embed", "test-key")
804+
805+
const mockFetch = vitest.fn().mockResolvedValue({
806+
ok: true,
807+
json: async () => ({
808+
data: [{ embedding: [0.1, 0.2] }],
809+
usage: { prompt_tokens: 10, total_tokens: 15 },
810+
}),
811+
})
812+
global.fetch = mockFetch
813+
814+
await embedder.createEmbeddings(["test"])
815+
816+
// Should make direct HTTP request to the full URL
817+
expect(mockFetch).toHaveBeenCalledWith("https://api.example.com/v1/embed", expect.any(Object))
818+
})
819+
820+
it("should treat base URLs without endpoint patterns as SDK URLs", async () => {
821+
const embedder = new OpenAICompatibleEmbedder("https://api.openai.com/v1", "test-key")
822+
823+
// Mock the OpenAI SDK's embeddings.create method
824+
const mockCreate = vitest.fn().mockResolvedValue({
825+
data: [{ embedding: [0.1, 0.2] }],
826+
usage: { prompt_tokens: 10, total_tokens: 15 },
827+
})
828+
embedder["embeddingsClient"].embeddings = {
829+
create: mockCreate,
830+
} as any
831+
832+
await embedder.createEmbeddings(["test"])
833+
834+
// Should use SDK which will append /embeddings
835+
expect(mockCreate).toHaveBeenCalled()
836+
})
837+
})
774838
})

src/services/code-index/embedders/openai-compatible.ts

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,19 @@ interface OpenAIEmbeddingResponse {
2626
* OpenAI Compatible implementation of the embedder interface with batching and rate limiting.
2727
* This embedder allows using any OpenAI-compatible API endpoint by specifying a custom baseURL.
2828
*/
29+
interface HttpError extends Error {
30+
status?: number
31+
response?: {
32+
status?: number
33+
}
34+
}
35+
2936
export class OpenAICompatibleEmbedder implements IEmbedder {
3037
private embeddingsClient: OpenAI
3138
private readonly defaultModelId: string
3239
private readonly baseUrl: string
3340
private readonly apiKey: string
41+
private readonly isFullUrl: boolean
3442

3543
/**
3644
* Creates a new OpenAI Compatible embedder
@@ -53,6 +61,8 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
5361
apiKey: apiKey,
5462
})
5563
this.defaultModelId = modelId || getDefaultModelId("openai-compatible")
64+
// Cache the URL type check for performance
65+
this.isFullUrl = this.isFullEndpointUrl(baseUrl)
5666
}
5767

5868
/**
@@ -114,14 +124,23 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
114124
}
115125

116126
/**
117-
* Determines if the provided URL is a full endpoint URL (contains /embeddings or /deployments/)
118-
* or a base URL that needs the endpoint appended by the SDK
127+
* Determines if the provided URL is a full endpoint URL or a base URL that needs the endpoint appended by the SDK.
128+
* Uses smart pattern matching for known providers while accepting we can't cover all possible patterns.
119129
* @param url The URL to check
120130
* @returns true if it's a full endpoint URL, false if it's a base URL
121131
*/
122132
private isFullEndpointUrl(url: string): boolean {
123-
// Check if the URL contains common embedding endpoint patterns
124-
return url.includes("/embeddings") || url.includes("/deployments/")
133+
// Known patterns for major providers
134+
const patterns = [
135+
// Azure OpenAI: /deployments/{deployment-name}/embeddings
136+
/\/deployments\/[^\/]+\/embeddings(\?|$)/,
137+
// Direct endpoints: ends with /embeddings (before query params)
138+
/\/embeddings(\?|$)/,
139+
// Some providers use /embed instead of /embeddings
140+
/\/embed(\?|$)/,
141+
]
142+
143+
return patterns.some((pattern) => pattern.test(url))
125144
}
126145

127146
/**
@@ -155,7 +174,7 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
155174

156175
if (!response.ok) {
157176
const errorText = await response.text()
158-
const error: any = new Error(`HTTP ${response.status}: ${errorText}`)
177+
const error = new Error(`HTTP ${response.status}: ${errorText}`) as HttpError
159178
error.status = response.status
160179
throw error
161180
}
@@ -173,7 +192,8 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
173192
batchTexts: string[],
174193
model: string,
175194
): Promise<{ embeddings: number[][]; usage: { promptTokens: number; totalTokens: number } }> {
176-
const isFullUrl = this.isFullEndpointUrl(this.baseUrl)
195+
// Use cached value for performance
196+
const isFullUrl = this.isFullUrl
177197

178198
for (let attempts = 0; attempts < MAX_RETRIES; attempts++) {
179199
try {
@@ -222,8 +242,9 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
222242
totalTokens: response.usage?.total_tokens || 0,
223243
},
224244
}
225-
} catch (error: any) {
226-
const isRateLimitError = error?.status === 429
245+
} catch (error) {
246+
const httpError = error as HttpError
247+
const isRateLimitError = httpError?.status === 429
227248
const hasMoreAttempts = attempts < MAX_RETRIES - 1
228249

229250
if (isRateLimitError && hasMoreAttempts) {
@@ -244,19 +265,19 @@ export class OpenAICompatibleEmbedder implements IEmbedder {
244265

245266
// Provide more context in the error message using robust error extraction
246267
let errorMessage = t("embeddings:unknownError")
247-
if (error?.message) {
248-
errorMessage = error.message
268+
if (httpError?.message) {
269+
errorMessage = httpError.message
249270
} else if (typeof error === "string") {
250271
errorMessage = error
251-
} else if (error && typeof error.toString === "function") {
272+
} else if (error && typeof error === "object" && "toString" in error) {
252273
try {
253-
errorMessage = error.toString()
274+
errorMessage = String(error)
254275
} catch {
255276
errorMessage = t("embeddings:unknownError")
256277
}
257278
}
258279

259-
const statusCode = error?.status || error?.response?.status
280+
const statusCode = httpError?.status || httpError?.response?.status
260281

261282
if (statusCode === 401) {
262283
throw new Error(t("embeddings:authenticationFailed"))

0 commit comments

Comments
 (0)