fix(#338): Don't instantiate OpenAI client with missing API key
- Skip client initialization when OPENAI_API_KEY not configured - Set openai property to null instead of creating with dummy key - Methods return gracefully when embeddings not available - Updated tests to verify client is not instantiated without key Refs #338 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,12 +1,28 @@
|
|||||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, vi, afterEach } from "vitest";
|
||||||
import { EmbeddingService } from "./embedding.service";
|
import { EmbeddingService } from "./embedding.service";
|
||||||
import { PrismaService } from "../../prisma/prisma.service";
|
import { PrismaService } from "../../prisma/prisma.service";
|
||||||
|
|
||||||
|
// Mock OpenAI with a proper class
|
||||||
|
const mockEmbeddingsCreate = vi.fn();
|
||||||
|
vi.mock("openai", () => {
|
||||||
|
return {
|
||||||
|
default: class MockOpenAI {
|
||||||
|
embeddings = {
|
||||||
|
create: mockEmbeddingsCreate,
|
||||||
|
};
|
||||||
|
},
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
describe("EmbeddingService", () => {
|
describe("EmbeddingService", () => {
|
||||||
let service: EmbeddingService;
|
let service: EmbeddingService;
|
||||||
let prismaService: PrismaService;
|
let prismaService: PrismaService;
|
||||||
|
let originalEnv: string | undefined;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
|
// Store original env
|
||||||
|
originalEnv = process.env.OPENAI_API_KEY;
|
||||||
|
|
||||||
prismaService = {
|
prismaService = {
|
||||||
$executeRaw: vi.fn(),
|
$executeRaw: vi.fn(),
|
||||||
knowledgeEmbedding: {
|
knowledgeEmbedding: {
|
||||||
@@ -14,36 +30,65 @@ describe("EmbeddingService", () => {
|
|||||||
},
|
},
|
||||||
} as unknown as PrismaService;
|
} as unknown as PrismaService;
|
||||||
|
|
||||||
service = new EmbeddingService(prismaService);
|
// Clear mock call history
|
||||||
|
vi.clearAllMocks();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
// Restore original env
|
||||||
|
if (originalEnv) {
|
||||||
|
process.env.OPENAI_API_KEY = originalEnv;
|
||||||
|
} else {
|
||||||
|
delete process.env.OPENAI_API_KEY;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("constructor", () => {
|
||||||
|
it("should not instantiate OpenAI client when API key is missing", () => {
|
||||||
|
delete process.env.OPENAI_API_KEY;
|
||||||
|
|
||||||
|
service = new EmbeddingService(prismaService);
|
||||||
|
|
||||||
|
// Verify service is not configured (client is null)
|
||||||
|
expect(service.isConfigured()).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should instantiate OpenAI client when API key is provided", () => {
|
||||||
|
process.env.OPENAI_API_KEY = "test-api-key";
|
||||||
|
|
||||||
|
service = new EmbeddingService(prismaService);
|
||||||
|
|
||||||
|
// Verify service is configured (client is not null)
|
||||||
|
expect(service.isConfigured()).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// Default service setup (without API key) for remaining tests
|
||||||
|
function createServiceWithoutKey(): EmbeddingService {
|
||||||
|
delete process.env.OPENAI_API_KEY;
|
||||||
|
return new EmbeddingService(prismaService);
|
||||||
|
}
|
||||||
|
|
||||||
describe("isConfigured", () => {
|
describe("isConfigured", () => {
|
||||||
it("should return false when OPENAI_API_KEY is not set", () => {
|
it("should return false when OPENAI_API_KEY is not set", () => {
|
||||||
const originalEnv = process.env["OPENAI_API_KEY"];
|
service = createServiceWithoutKey();
|
||||||
delete process.env["OPENAI_API_KEY"];
|
|
||||||
|
|
||||||
expect(service.isConfigured()).toBe(false);
|
expect(service.isConfigured()).toBe(false);
|
||||||
|
|
||||||
if (originalEnv) {
|
|
||||||
process.env["OPENAI_API_KEY"] = originalEnv;
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should return true when OPENAI_API_KEY is set", () => {
|
it("should return true when OPENAI_API_KEY is set", () => {
|
||||||
const originalEnv = process.env["OPENAI_API_KEY"];
|
process.env.OPENAI_API_KEY = "test-key";
|
||||||
process.env["OPENAI_API_KEY"] = "test-key";
|
service = new EmbeddingService(prismaService);
|
||||||
|
|
||||||
expect(service.isConfigured()).toBe(true);
|
expect(service.isConfigured()).toBe(true);
|
||||||
|
|
||||||
if (originalEnv) {
|
|
||||||
process.env["OPENAI_API_KEY"] = originalEnv;
|
|
||||||
} else {
|
|
||||||
delete process.env["OPENAI_API_KEY"];
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("prepareContentForEmbedding", () => {
|
describe("prepareContentForEmbedding", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
service = createServiceWithoutKey();
|
||||||
|
});
|
||||||
|
|
||||||
it("should combine title and content with title weighting", () => {
|
it("should combine title and content with title weighting", () => {
|
||||||
const title = "Test Title";
|
const title = "Test Title";
|
||||||
const content = "Test content goes here";
|
const content = "Test content goes here";
|
||||||
@@ -68,20 +113,19 @@ describe("EmbeddingService", () => {
|
|||||||
|
|
||||||
describe("generateAndStoreEmbedding", () => {
|
describe("generateAndStoreEmbedding", () => {
|
||||||
it("should skip generation when not configured", async () => {
|
it("should skip generation when not configured", async () => {
|
||||||
const originalEnv = process.env["OPENAI_API_KEY"];
|
service = createServiceWithoutKey();
|
||||||
delete process.env["OPENAI_API_KEY"];
|
|
||||||
|
|
||||||
await service.generateAndStoreEmbedding("test-id", "test content");
|
await service.generateAndStoreEmbedding("test-id", "test content");
|
||||||
|
|
||||||
expect(prismaService.$executeRaw).not.toHaveBeenCalled();
|
expect(prismaService.$executeRaw).not.toHaveBeenCalled();
|
||||||
|
|
||||||
if (originalEnv) {
|
|
||||||
process.env["OPENAI_API_KEY"] = originalEnv;
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("deleteEmbedding", () => {
|
describe("deleteEmbedding", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
service = createServiceWithoutKey();
|
||||||
|
});
|
||||||
|
|
||||||
it("should delete embedding for entry", async () => {
|
it("should delete embedding for entry", async () => {
|
||||||
const entryId = "test-entry-id";
|
const entryId = "test-entry-id";
|
||||||
|
|
||||||
@@ -95,8 +139,7 @@ describe("EmbeddingService", () => {
|
|||||||
|
|
||||||
describe("batchGenerateEmbeddings", () => {
|
describe("batchGenerateEmbeddings", () => {
|
||||||
it("should return 0 when not configured", async () => {
|
it("should return 0 when not configured", async () => {
|
||||||
const originalEnv = process.env["OPENAI_API_KEY"];
|
service = createServiceWithoutKey();
|
||||||
delete process.env["OPENAI_API_KEY"];
|
|
||||||
|
|
||||||
const entries = [
|
const entries = [
|
||||||
{ id: "1", content: "content 1" },
|
{ id: "1", content: "content 1" },
|
||||||
@@ -106,10 +149,16 @@ describe("EmbeddingService", () => {
|
|||||||
const result = await service.batchGenerateEmbeddings(entries);
|
const result = await service.batchGenerateEmbeddings(entries);
|
||||||
|
|
||||||
expect(result).toBe(0);
|
expect(result).toBe(0);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
if (originalEnv) {
|
describe("generateEmbedding", () => {
|
||||||
process.env["OPENAI_API_KEY"] = originalEnv;
|
it("should throw error when not configured", async () => {
|
||||||
}
|
service = createServiceWithoutKey();
|
||||||
|
|
||||||
|
await expect(service.generateEmbedding("test text")).rejects.toThrow(
|
||||||
|
"OPENAI_API_KEY not configured"
|
||||||
|
);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -20,7 +20,7 @@ export interface EmbeddingOptions {
|
|||||||
@Injectable()
|
@Injectable()
|
||||||
export class EmbeddingService {
|
export class EmbeddingService {
|
||||||
private readonly logger = new Logger(EmbeddingService.name);
|
private readonly logger = new Logger(EmbeddingService.name);
|
||||||
private readonly openai: OpenAI;
|
private readonly openai: OpenAI | null;
|
||||||
private readonly defaultModel = "text-embedding-3-small";
|
private readonly defaultModel = "text-embedding-3-small";
|
||||||
|
|
||||||
constructor(private readonly prisma: PrismaService) {
|
constructor(private readonly prisma: PrismaService) {
|
||||||
@@ -28,18 +28,17 @@ export class EmbeddingService {
|
|||||||
|
|
||||||
if (!apiKey) {
|
if (!apiKey) {
|
||||||
this.logger.warn("OPENAI_API_KEY not configured - embedding generation will be disabled");
|
this.logger.warn("OPENAI_API_KEY not configured - embedding generation will be disabled");
|
||||||
|
this.openai = null;
|
||||||
|
} else {
|
||||||
|
this.openai = new OpenAI({ apiKey });
|
||||||
}
|
}
|
||||||
|
|
||||||
this.openai = new OpenAI({
|
|
||||||
apiKey: apiKey ?? "dummy-key", // Provide dummy key to allow instantiation
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if the service is properly configured
|
* Check if the service is properly configured
|
||||||
*/
|
*/
|
||||||
isConfigured(): boolean {
|
isConfigured(): boolean {
|
||||||
return !!process.env.OPENAI_API_KEY;
|
return this.openai !== null;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -51,7 +50,7 @@ export class EmbeddingService {
|
|||||||
* @throws Error if OpenAI API key is not configured
|
* @throws Error if OpenAI API key is not configured
|
||||||
*/
|
*/
|
||||||
async generateEmbedding(text: string, options: EmbeddingOptions = {}): Promise<number[]> {
|
async generateEmbedding(text: string, options: EmbeddingOptions = {}): Promise<number[]> {
|
||||||
if (!this.isConfigured()) {
|
if (!this.openai) {
|
||||||
throw new Error("OPENAI_API_KEY not configured");
|
throw new Error("OPENAI_API_KEY not configured");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user