From ec8f7a25bf492b174328d90bbff5a11369540e55 Mon Sep 17 00:00:00 2001 From: Kshitij Tandon Date: Thu, 18 Dec 2025 12:15:46 +0530 Subject: [PATCH] Fixing bug in Tranform / Rollup Jobs UI Signed-off-by: Kshitij Tandon --- server/services/RollupService.test.ts | 477 +++++++++++++++++++++++ server/services/RollupService.ts | 21 +- server/services/TransformService.test.ts | 477 +++++++++++++++++++++++ server/services/TransformService.ts | 17 +- 4 files changed, 985 insertions(+), 7 deletions(-) create mode 100644 server/services/RollupService.test.ts diff --git a/server/services/RollupService.test.ts b/server/services/RollupService.test.ts new file mode 100644 index 000000000..c99b77401 --- /dev/null +++ b/server/services/RollupService.test.ts @@ -0,0 +1,477 @@ +import RollupService from "./RollupService"; + +describe("RollupService.getRollups", () => { + let rollupService: RollupService; + let mockContext: any; + let mockRequest: any; + let mockResponse: any; + let mockCallWithRequest: jest.Mock; + + beforeEach(() => { + // Create a new instance of RollupService for each test + rollupService = new RollupService({} as any); + + // Mock the context, request, and response objects + mockContext = {}; + mockRequest = { + query: { + from: "0", + size: "10", + search: "", + sortDirection: "asc", + sortField: "_id", + }, + }; + + mockResponse = { + custom: jest.fn((args) => args), + }; + + // Mock the getClientBasedOnDataSource method + mockCallWithRequest = jest.fn(); + rollupService.getClientBasedOnDataSource = jest.fn(() => mockCallWithRequest); + }); + + it("should process rollup job named 'error' correctly", async () => { + // Mock the getRollups API response + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 1, + rollups: [ + { + _id: "error", + _seqNo: 1, + _primaryTerm: 1, + rollup: { + rollup_id: "error", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock the explainRollup API response with "error" as a rollup ID + mockCallWithRequest.mockResolvedValueOnce({ + error: { + metadata_id: "error", + rollup_metadata: { + rollup_id: "error", + status: "finished", + stats: {}, + }, + }, + }); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(1); + expect(result.body.response.rollups[0]._id).toBe("error"); + expect(result.body.response.rollups[0].metadata).toBeDefined(); + expect(result.body.response.rollups[0].metadata.metadata_id).toBe("error"); + }); + + it("should process multiple rollup jobs including one named 'error'", async () => { + // Mock the getRollups API response with multiple rollups + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 3, + rollups: [ + { + _id: "error", + _seqNo: 1, + _primaryTerm: 1, + rollup: { + rollup_id: "error", + source_index: "source1", + target_index: "target1", + enabled: true, + }, + }, + { + _id: "my-rollup", + _seqNo: 2, + _primaryTerm: 1, + rollup: { + rollup_id: "my-rollup", + source_index: "source2", + target_index: "target2", + enabled: true, + }, + }, + { + _id: "another-rollup", + _seqNo: 3, + _primaryTerm: 1, + rollup: { + rollup_id: "another-rollup", + source_index: "source3", + target_index: "target3", + enabled: false, + }, + }, + ], + }); + + // Mock the explainRollup API response with multiple rollup IDs + mockCallWithRequest.mockResolvedValueOnce({ + error: { + metadata_id: "error", + rollup_metadata: { + rollup_id: "error", + status: "finished", + stats: {}, + }, + }, + "my-rollup": { + metadata_id: "my-rollup", + rollup_metadata: { + rollup_id: "my-rollup", + status: "running", + stats: {}, + }, + }, + "another-rollup": { + metadata_id: "another-rollup", + rollup_metadata: { + rollup_id: "another-rollup", + status: "stopped", + stats: {}, + }, + }, + }); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(3); + + // Verify all rollups are present with correct metadata + const errorRollup = result.body.response.rollups.find((r: any) => r._id === "error"); + expect(errorRollup).toBeDefined(); + expect(errorRollup.metadata.metadata_id).toBe("error"); + + const myRollup = result.body.response.rollups.find((r: any) => r._id === "my-rollup"); + expect(myRollup).toBeDefined(); + expect(myRollup.metadata.metadata_id).toBe("my-rollup"); + + const anotherRollup = result.body.response.rollups.find((r: any) => r._id === "another-rollup"); + expect(anotherRollup).toBeDefined(); + expect(anotherRollup.metadata.metadata_id).toBe("another-rollup"); + }); + + it("should process rollup job named 'ok' correctly", async () => { + // Mock the getRollups API response + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 1, + rollups: [ + { + _id: "ok", + _seqNo: 1, + _primaryTerm: 1, + rollup: { + rollup_id: "ok", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock the explainRollup API response with "ok" as a rollup ID + mockCallWithRequest.mockResolvedValueOnce({ + ok: { + metadata_id: "ok", + rollup_metadata: { + rollup_id: "ok", + status: "finished", + stats: {}, + }, + }, + }); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(1); + expect(result.body.response.rollups[0]._id).toBe("ok"); + expect(result.body.response.rollups[0].metadata).toBeDefined(); + expect(result.body.response.rollups[0].metadata.metadata_id).toBe("ok"); + }); + + it("should catch actual API errors in try-catch block", async () => { + // Mock the getRollups API to throw an error + const apiError = new Error("Connection timeout"); + (apiError as any).statusCode = 500; + mockCallWithRequest.mockRejectedValueOnce(apiError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Connection timeout"); + }); + + it("should handle 404 index_not_found_exception gracefully", async () => { + // Mock the getRollups API to throw a 404 error + const notFoundError = new Error("Index not found"); + (notFoundError as any).statusCode = 404; + (notFoundError as any).body = { + error: { + type: "index_not_found_exception", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(notFoundError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toEqual([]); + expect(result.body.response.totalRollups).toBe(0); + }); + + it("should handle rollup jobs with various reserved property names", async () => { + // Mock the getRollups API response with rollups having reserved names + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 4, + rollups: [ + { _id: "error", _seqNo: 1, _primaryTerm: 1, rollup: { rollup_id: "error" } }, + { _id: "ok", _seqNo: 2, _primaryTerm: 1, rollup: { rollup_id: "ok" } }, + { _id: "response", _seqNo: 3, _primaryTerm: 1, rollup: { rollup_id: "response" } }, + { _id: "metadata", _seqNo: 4, _primaryTerm: 1, rollup: { rollup_id: "metadata" } }, + ], + }); + + // Mock the explainRollup API response + mockCallWithRequest.mockResolvedValueOnce({ + error: { metadata_id: "error", rollup_metadata: {} }, + ok: { metadata_id: "ok", rollup_metadata: {} }, + response: { metadata_id: "response", rollup_metadata: {} }, + metadata: { metadata_id: "metadata", rollup_metadata: {} }, + }); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(4); + + // Verify all reserved names are processed correctly + ["error", "ok", "response", "metadata"].forEach((name) => { + const rollup = result.body.response.rollups.find((r: any) => r._id === name); + expect(rollup).toBeDefined(); + expect(rollup.metadata.metadata_id).toBe(name); + }); + }); + + it("should set metadata to null for rollups without explainResponse entry", async () => { + // Mock the getRollups API response + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 2, + rollups: [ + { _id: "rollup-1", _seqNo: 1, _primaryTerm: 1, rollup: { rollup_id: "rollup-1" } }, + { _id: "rollup-2", _seqNo: 2, _primaryTerm: 1, rollup: { rollup_id: "rollup-2" } }, + ], + }); + + // Mock the explainRollup API response with only one rollup + mockCallWithRequest.mockResolvedValueOnce({ + "rollup-1": { metadata_id: "rollup-1", rollup_metadata: {} }, + // rollup-2 is missing from explainResponse + }); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(2); + + const rollup1 = result.body.response.rollups.find((r: any) => r._id === "rollup-1"); + expect(rollup1.metadata).toBeDefined(); + expect(rollup1.metadata.metadata_id).toBe("rollup-1"); + + const rollup2 = result.body.response.rollups.find((r: any) => r._id === "rollup-2"); + expect(rollup2.metadata).toBeNull(); + }); + + it("should handle 500 internal server errors", async () => { + // Mock the getRollups API to throw a 500 error + const internalError = new Error("Internal Server Error"); + (internalError as any).statusCode = 500; + (internalError as any).body = { + error: { + type: "internal_server_error", + reason: "OpenSearch internal error", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(internalError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Internal Server Error"); + }); + + it("should handle network timeout errors", async () => { + // Mock the getRollups API to throw a network timeout error + const timeoutError = new Error("Request timeout"); + (timeoutError as any).statusCode = 408; + (timeoutError as any).name = "RequestTimeout"; + mockCallWithRequest.mockRejectedValueOnce(timeoutError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Request timeout"); + }); + + it("should handle connection refused errors", async () => { + // Mock the getRollups API to throw a connection error + const connectionError = new Error("ECONNREFUSED"); + (connectionError as any).code = "ECONNREFUSED"; + mockCallWithRequest.mockRejectedValueOnce(connectionError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("ECONNREFUSED"); + }); + + it("should handle authentication errors", async () => { + // Mock the getRollups API to throw an authentication error + const authError = new Error("Authentication failed"); + (authError as any).statusCode = 401; + (authError as any).body = { + error: { + type: "security_exception", + reason: "Invalid credentials", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(authError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Authentication failed"); + }); + + it("should handle authorization errors", async () => { + // Mock the getRollups API to throw an authorization error + const forbiddenError = new Error("Forbidden"); + (forbiddenError as any).statusCode = 403; + (forbiddenError as any).body = { + error: { + type: "security_exception", + reason: "Insufficient permissions", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(forbiddenError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Forbidden"); + }); + + it("should handle errors during explainRollup call", async () => { + // Mock successful getRollups but failed explainRollup + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 1, + rollups: [ + { + _id: "test-rollup", + _seqNo: 1, + _primaryTerm: 1, + rollup: { + rollup_id: "test-rollup", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock explainRollup to throw an error + const explainError = new Error("Explain API failed"); + (explainError as any).statusCode = 500; + mockCallWithRequest.mockRejectedValueOnce(explainError); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getRollups"); + expect(result.body.error).toContain("Explain API failed"); + }); + + it("should handle malformed explainResponse gracefully", async () => { + // Mock the getRollups API response + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 1, + rollups: [ + { + _id: "test-rollup", + _seqNo: 1, + _primaryTerm: 1, + rollup: { + rollup_id: "test-rollup", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock explainRollup to return null (malformed response) + mockCallWithRequest.mockResolvedValueOnce(null); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Unexpected response format from Explain API"); + }); + + it("should handle empty explainResponse object", async () => { + // Mock the getRollups API response + mockCallWithRequest.mockResolvedValueOnce({ + total_rollups: 2, + rollups: [ + { _id: "rollup-1", _seqNo: 1, _primaryTerm: 1, rollup: { rollup_id: "rollup-1" } }, + { _id: "rollup-2", _seqNo: 2, _primaryTerm: 1, rollup: { rollup_id: "rollup-2" } }, + ], + }); + + // Mock explainRollup to return empty object + mockCallWithRequest.mockResolvedValueOnce({}); + + const result = await rollupService.getRollups(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.rollups).toHaveLength(2); + + // Both rollups should have null metadata + result.body.response.rollups.forEach((rollup: any) => { + expect(rollup.metadata).toBeNull(); + }); + }); +}); diff --git a/server/services/RollupService.ts b/server/services/RollupService.ts index e7f8a0014..c3c4a5840 100644 --- a/server/services/RollupService.ts +++ b/server/services/RollupService.ts @@ -302,22 +302,35 @@ export default class RollupService extends MDSEnabledClientService { // Concat rollup job ids const ids = rollups.map((rollup: DocumentRollup) => rollup._id).join(","); const explainResponse: any = await callWithRequest("ism.explainRollup", { rollupId: ids }); - if (!explainResponse.error) { + + // Bug fix: Do NOT check for explainResponse.error property + // The Explain API returns an object where rollup IDs are keys (e.g., { "my-rollup": {...metadata...} }) + // If a rollup is named "error", the response will be { "error": {...metadata...} } + // This would incorrectly trigger error handling if we checked for explainResponse.error + // Actual API errors are caught by the try-catch block above, not returned as response properties + // Reference: https://github.com/opensearch-project/index-management-dashboards-plugin/issues/1376 + + // Validate that explainResponse is a valid object + if (explainResponse && typeof explainResponse === "object") { + // Map metadata to each rollup, setting to null if not found in explainResponse rollups.map((rollup: DocumentRollup) => { - rollup.metadata = explainResponse[rollup._id]; + rollup.metadata = explainResponse[rollup._id] || null; }); + return response.custom({ statusCode: 200, body: { ok: true, response: { rollups: rollups, totalRollups: totalRollups, metadata: explainResponse } }, }); - } else + } else { + // Unexpected response format - this should rarely happen return response.custom({ statusCode: 200, body: { ok: false, - error: explainResponse ? explainResponse.error : "An error occurred when calling getExplain API.", + error: "Unexpected response format from Explain API", }, }); + } } return response.custom({ statusCode: 200, diff --git a/server/services/TransformService.test.ts b/server/services/TransformService.test.ts index 7c7208b5f..babc3fd0b 100644 --- a/server/services/TransformService.test.ts +++ b/server/services/TransformService.test.ts @@ -1,4 +1,481 @@ import { schema } from "@osd/config-schema"; +import TransformService from "./TransformService"; + +describe("TransformService.getTransforms", () => { + let transformService: TransformService; + let mockContext: any; + let mockRequest: any; + let mockResponse: any; + let mockCallWithRequest: jest.Mock; + + beforeEach(() => { + // Create a new instance of TransformService for each test + transformService = new TransformService({} as any); + + // Mock the context, request, and response objects + mockContext = {}; + mockRequest = { + query: { + from: 0, + size: 10, + search: "", + sortDirection: "asc", + sortField: "_id", + }, + }; + + mockResponse = { + custom: jest.fn((args) => args), + }; + + // Mock the getClientBasedOnDataSource method + mockCallWithRequest = jest.fn(); + transformService.getClientBasedOnDataSource = jest.fn(() => mockCallWithRequest); + }); + + it("should process transform named 'error' correctly", async () => { + // Mock the getTransforms API response + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 1, + transforms: [ + { + _id: "error", + _seqNo: 1, + _primaryTerm: 1, + transform: { + transform_id: "error", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock the explainTransform API response with "error" as a transform ID + mockCallWithRequest.mockResolvedValueOnce({ + error: { + metadata_id: "error", + transform_metadata: { + transform_id: "error", + status: "finished", + stats: {}, + }, + }, + }); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(1); + expect(result.body.response.transforms[0]._id).toBe("error"); + expect(result.body.response.transforms[0].metadata).toBeDefined(); + expect(result.body.response.transforms[0].metadata.metadata_id).toBe("error"); + }); + + it("should process transform named 'ok' correctly", async () => { + // Mock the getTransforms API response + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 1, + transforms: [ + { + _id: "ok", + _seqNo: 1, + _primaryTerm: 1, + transform: { + transform_id: "ok", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock the explainTransform API response with "ok" as a transform ID + mockCallWithRequest.mockResolvedValueOnce({ + ok: { + metadata_id: "ok", + transform_metadata: { + transform_id: "ok", + status: "finished", + stats: {}, + }, + }, + }); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(1); + expect(result.body.response.transforms[0]._id).toBe("ok"); + expect(result.body.response.transforms[0].metadata).toBeDefined(); + expect(result.body.response.transforms[0].metadata.metadata_id).toBe("ok"); + }); + + it("should process multiple transforms including one named 'error'", async () => { + // Mock the getTransforms API response with multiple transforms + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 3, + transforms: [ + { + _id: "error", + _seqNo: 1, + _primaryTerm: 1, + transform: { + transform_id: "error", + source_index: "source1", + target_index: "target1", + enabled: true, + }, + }, + { + _id: "my-transform", + _seqNo: 2, + _primaryTerm: 1, + transform: { + transform_id: "my-transform", + source_index: "source2", + target_index: "target2", + enabled: true, + }, + }, + { + _id: "another-transform", + _seqNo: 3, + _primaryTerm: 1, + transform: { + transform_id: "another-transform", + source_index: "source3", + target_index: "target3", + enabled: false, + }, + }, + ], + }); + + // Mock the explainTransform API response with multiple transform IDs + mockCallWithRequest.mockResolvedValueOnce({ + error: { + metadata_id: "error", + transform_metadata: { + transform_id: "error", + status: "finished", + stats: {}, + }, + }, + "my-transform": { + metadata_id: "my-transform", + transform_metadata: { + transform_id: "my-transform", + status: "running", + stats: {}, + }, + }, + "another-transform": { + metadata_id: "another-transform", + transform_metadata: { + transform_id: "another-transform", + status: "stopped", + stats: {}, + }, + }, + }); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(3); + + // Verify all transforms are present with correct metadata + const errorTransform = result.body.response.transforms.find((t: any) => t._id === "error"); + expect(errorTransform).toBeDefined(); + expect(errorTransform.metadata.metadata_id).toBe("error"); + + const myTransform = result.body.response.transforms.find((t: any) => t._id === "my-transform"); + expect(myTransform).toBeDefined(); + expect(myTransform.metadata.metadata_id).toBe("my-transform"); + + const anotherTransform = result.body.response.transforms.find((t: any) => t._id === "another-transform"); + expect(anotherTransform).toBeDefined(); + expect(anotherTransform.metadata.metadata_id).toBe("another-transform"); + }); + + it("should catch actual API errors in try-catch block", async () => { + // Mock the getTransforms API to throw an error + const apiError = new Error("Connection timeout"); + (apiError as any).statusCode = 500; + mockCallWithRequest.mockRejectedValueOnce(apiError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Connection timeout"); + }); + + it("should handle 404 index_not_found_exception gracefully", async () => { + // Mock the getTransforms API to throw a 404 error + const notFoundError = new Error("Index not found"); + (notFoundError as any).statusCode = 404; + (notFoundError as any).body = { + error: { + type: "index_not_found_exception", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(notFoundError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toEqual([]); + expect(result.body.response.totalTransforms).toBe(0); + }); + + it("should handle transforms with various reserved property names", async () => { + // Mock the getTransforms API response with transforms having reserved names + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 4, + transforms: [ + { _id: "error", _seqNo: 1, _primaryTerm: 1, transform: { transform_id: "error" } }, + { _id: "ok", _seqNo: 2, _primaryTerm: 1, transform: { transform_id: "ok" } }, + { _id: "response", _seqNo: 3, _primaryTerm: 1, transform: { transform_id: "response" } }, + { _id: "metadata", _seqNo: 4, _primaryTerm: 1, transform: { transform_id: "metadata" } }, + ], + }); + + // Mock the explainTransform API response + mockCallWithRequest.mockResolvedValueOnce({ + error: { metadata_id: "error", transform_metadata: {} }, + ok: { metadata_id: "ok", transform_metadata: {} }, + response: { metadata_id: "response", transform_metadata: {} }, + metadata: { metadata_id: "metadata", transform_metadata: {} }, + }); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(4); + + // Verify all reserved names are processed correctly + ["error", "ok", "response", "metadata"].forEach((name) => { + const transform = result.body.response.transforms.find((t: any) => t._id === name); + expect(transform).toBeDefined(); + expect(transform.metadata.metadata_id).toBe(name); + }); + }); + + it("should set metadata to null for transforms without explainResponse entry", async () => { + // Mock the getTransforms API response + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 2, + transforms: [ + { _id: "transform-1", _seqNo: 1, _primaryTerm: 1, transform: { transform_id: "transform-1" } }, + { _id: "transform-2", _seqNo: 2, _primaryTerm: 1, transform: { transform_id: "transform-2" } }, + ], + }); + + // Mock the explainTransform API response with only one transform + mockCallWithRequest.mockResolvedValueOnce({ + "transform-1": { metadata_id: "transform-1", transform_metadata: {} }, + // transform-2 is missing from explainResponse + }); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(2); + + const transform1 = result.body.response.transforms.find((t: any) => t._id === "transform-1"); + expect(transform1.metadata).toBeDefined(); + expect(transform1.metadata.metadata_id).toBe("transform-1"); + + const transform2 = result.body.response.transforms.find((t: any) => t._id === "transform-2"); + expect(transform2.metadata).toBeNull(); + }); + + it("should handle 500 internal server errors", async () => { + // Mock the getTransforms API to throw a 500 error + const internalError = new Error("Internal Server Error"); + (internalError as any).statusCode = 500; + (internalError as any).body = { + error: { + type: "internal_server_error", + reason: "OpenSearch internal error", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(internalError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Internal Server Error"); + }); + + it("should handle network timeout errors", async () => { + // Mock the getTransforms API to throw a network timeout error + const timeoutError = new Error("Request timeout"); + (timeoutError as any).statusCode = 408; + (timeoutError as any).name = "RequestTimeout"; + mockCallWithRequest.mockRejectedValueOnce(timeoutError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Request timeout"); + }); + + it("should handle connection refused errors", async () => { + // Mock the getTransforms API to throw a connection error + const connectionError = new Error("ECONNREFUSED"); + (connectionError as any).code = "ECONNREFUSED"; + mockCallWithRequest.mockRejectedValueOnce(connectionError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("ECONNREFUSED"); + }); + + it("should handle authentication errors", async () => { + // Mock the getTransforms API to throw an authentication error + const authError = new Error("Authentication failed"); + (authError as any).statusCode = 401; + (authError as any).body = { + error: { + type: "security_exception", + reason: "Invalid credentials", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(authError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Authentication failed"); + }); + + it("should handle authorization errors", async () => { + // Mock the getTransforms API to throw an authorization error + const forbiddenError = new Error("Forbidden"); + (forbiddenError as any).statusCode = 403; + (forbiddenError as any).body = { + error: { + type: "security_exception", + reason: "Insufficient permissions", + }, + }; + mockCallWithRequest.mockRejectedValueOnce(forbiddenError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Forbidden"); + }); + + it("should handle errors during explainTransform call", async () => { + // Mock successful getTransforms but failed explainTransform + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 1, + transforms: [ + { + _id: "test-transform", + _seqNo: 1, + _primaryTerm: 1, + transform: { + transform_id: "test-transform", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock explainTransform to throw an error + const explainError = new Error("Explain API failed"); + (explainError as any).statusCode = 500; + mockCallWithRequest.mockRejectedValueOnce(explainError); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Error in getTransforms"); + expect(result.body.error).toContain("Explain API failed"); + }); + + it("should handle malformed explainResponse gracefully", async () => { + // Mock the getTransforms API response + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 1, + transforms: [ + { + _id: "test-transform", + _seqNo: 1, + _primaryTerm: 1, + transform: { + transform_id: "test-transform", + source_index: "source", + target_index: "target", + enabled: true, + }, + }, + ], + }); + + // Mock explainTransform to return null (malformed response) + mockCallWithRequest.mockResolvedValueOnce(null); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(false); + expect(result.body.error).toContain("Unexpected response format from Explain API"); + }); + + it("should handle empty explainResponse object", async () => { + // Mock the getTransforms API response + mockCallWithRequest.mockResolvedValueOnce({ + total_transforms: 2, + transforms: [ + { _id: "transform-1", _seqNo: 1, _primaryTerm: 1, transform: { transform_id: "transform-1" } }, + { _id: "transform-2", _seqNo: 2, _primaryTerm: 1, transform: { transform_id: "transform-2" } }, + ], + }); + + // Mock explainTransform to return empty object + mockCallWithRequest.mockResolvedValueOnce({}); + + const result = await transformService.getTransforms(mockContext, mockRequest, mockResponse); + + expect(result.statusCode).toBe(200); + expect(result.body.ok).toBe(true); + expect(result.body.response.transforms).toHaveLength(2); + + // Both transforms should have null metadata + result.body.response.transforms.forEach((transform: any) => { + expect(transform.metadata).toBeNull(); + }); + }); +}); describe("Index Name Validation", () => { const validateSchema = schema.object({ diff --git a/server/services/TransformService.ts b/server/services/TransformService.ts index c4cb8d35e..15a734c7d 100644 --- a/server/services/TransformService.ts +++ b/server/services/TransformService.ts @@ -67,9 +67,19 @@ export default class TransformService extends MDSEnabledClientService { const callWithRequest = this.getClientBasedOnDataSource(context, request); const explainResponse = (await callWithRequest("ism.explainTransform", { transformId: ids })) as any; - if (!explainResponse.error) { + + // Bug fix: Do NOT check for explainResponse.error property + // The Explain API returns an object where transform IDs are keys (e.g., { "my-transform": {...metadata...} }) + // If a transform is named "error", the response will be { "error": {...metadata...} } + // This would incorrectly trigger error handling if we checked for explainResponse.error + // Actual API errors are caught by the try-catch block above, not returned as response properties + // Reference: https://github.com/opensearch-project/index-management-dashboards-plugin/issues/1376 + + // Validate that explainResponse is a valid object + if (explainResponse && typeof explainResponse === "object") { + // Map metadata to each transform, setting to null if not found in explainResponse transforms.map((transform: DocumentTransform) => { - transform.metadata = explainResponse[transform._id]; + transform.metadata = explainResponse[transform._id] || null; }); return response.custom({ @@ -80,11 +90,12 @@ export default class TransformService extends MDSEnabledClientService { }, }); } else { + // Unexpected response format - this should rarely happen return response.custom({ statusCode: 200, body: { ok: false, - error: explainResponse ? explainResponse.error : "An error occurred when calling getExplain API.", + error: "Unexpected response format from Explain API", }, }); }