From 8669ab9ff1689dce3b8bb3875ebba43383659d1c Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Wed, 6 Aug 2025 16:52:01 -0700 Subject: [PATCH] refactor: simplify XML parser error handling by removing unnecessary MAX_FAILURES logic - Remove circuit breaker pattern with MAX_FAILURES counter - Use immediate fallback for any parse error, not just addChild errors - Simplify error handling logic as retrying the same XML parse multiple times is unnecessary - Update tests to reflect the simplified error handling approach --- .../__tests__/multiApplyDiffTool.test.ts | 40 +++-------- src/utils/xml.ts | 67 +++++-------------- 2 files changed, 27 insertions(+), 80 deletions(-) diff --git a/src/core/tools/__tests__/multiApplyDiffTool.test.ts b/src/core/tools/__tests__/multiApplyDiffTool.test.ts index 68d2c5f1a1..5c8c6d899f 100644 --- a/src/core/tools/__tests__/multiApplyDiffTool.test.ts +++ b/src/core/tools/__tests__/multiApplyDiffTool.test.ts @@ -155,7 +155,7 @@ describe("multiApplyDiffTool", () => { }) describe("Fallback parsing", () => { - it("should use fallback parser after repeated failures", () => { + it("should use fallback parser immediately on any failure", () => { const xml = ` test.txt @@ -166,14 +166,9 @@ describe("multiApplyDiffTool", () => { ` - // Simulate multiple failures to trigger fallback - let callCount = 0 + // Mock parseXml to simulate immediate fallback on any error vi.mocked(parseXml).mockImplementation(() => { - callCount++ - if (callCount <= 3) { - throw new Error("Cannot read properties of undefined (reading 'addChild')") - } - // After 3 failures, the fallback should be used + // Fallback should be used immediately return { file: { path: "test.txt", @@ -185,12 +180,7 @@ describe("multiApplyDiffTool", () => { } }) - // First 3 calls should fail - for (let i = 0; i < 3; i++) { - expect(() => parseXml(xml)).toThrow() - } - - // Fourth call should succeed with fallback + // First call should succeed with fallback const result = parseXml(xml) as any expect(result).toBeDefined() expect(result.file.path).toBe("test.txt") @@ -246,28 +236,16 @@ describe("multiApplyDiffTool", () => { } }) - it("should track parse failure count for circuit breaker", () => { - // This tests the circuit breaker pattern - let failureCount = 0 - const MAX_FAILURES = 3 - + it("should immediately use fallback on parse failure", () => { + // This tests the immediate fallback pattern const simulateParseFailure = () => { - failureCount++ - if (failureCount >= MAX_FAILURES) { - // Should trigger fallback - return "fallback_result" - } - throw new Error("Parse failed") + // Should immediately trigger fallback on any error + return "fallback_result" } - // First two failures - expect(() => simulateParseFailure()).toThrow() - expect(() => simulateParseFailure()).toThrow() - - // Third failure triggers fallback + // First failure immediately triggers fallback const result = simulateParseFailure() expect(result).toBe("fallback_result") - expect(failureCount).toBe(3) }) }) diff --git a/src/utils/xml.ts b/src/utils/xml.ts index 6078d3d35b..de57a0df8b 100644 --- a/src/utils/xml.ts +++ b/src/utils/xml.ts @@ -1,20 +1,14 @@ import { XMLParser } from "fast-xml-parser" /** - * Encapsulated XML parser with circuit breaker pattern + * Encapsulated XML parser with fallback mechanism * * This dual-parser system handles interference from external XML parsers (like xml2js) * that may be loaded globally by other VSCode extensions. When the primary parser * (fast-xml-parser) fails due to external interference, it automatically falls back * to a regex-based parser. - * - * Note: This parser instance should not be used concurrently as parseFailureCount - * is not thread-safe. However, this is not an issue in practice since JavaScript - * is single-threaded. */ class XmlParserWithFallback { - private parseFailureCount = 0 - private readonly MAX_FAILURES = 3 private readonly MAX_XML_SIZE = 10 * 1024 * 1024 // 10MB limit for fallback parser /** @@ -118,14 +112,7 @@ class XmlParserWithFallback { stopNodes: _stopNodes, }) - const result = parser.parse(xmlString) - - // Reset failure count on success - if (this.parseFailureCount > 0) { - this.parseFailureCount = 0 - } - - return result + return parser.parse(xmlString) } catch (error) { // Enhance error message for better debugging // Handle cases where error might not be an Error instance (e.g., strings, objects) @@ -140,41 +127,23 @@ class XmlParserWithFallback { errorMessage = "Unknown error" } - // Check for xml2js specific error patterns - IMMEDIATELY use fallback - if (errorMessage.includes("addChild")) { - // Don't wait for multiple failures - use fallback immediately for addChild errors - try { - const result = this.fallbackXmlParse(xmlString) - return result - } catch (fallbackError) { - const fallbackErrorMsg = fallbackError instanceof Error ? fallbackError.message : "Unknown error" - // Still throw the error but make it clear we tried the fallback - throw new Error( - `XML parsing failed with fallback parser. Fallback parser also failed: ${fallbackErrorMsg}`, - ) - } + // Try fallback parser for any parsing error + // This handles both xml2js interference (addChild errors) and other parse failures + try { + const result = this.fallbackXmlParse(xmlString) + return result + } catch (fallbackError) { + const fallbackErrorMsg = fallbackError instanceof Error ? fallbackError.message : "Unknown error" + // Provide context about which error was from xml2js interference + const isXml2jsError = errorMessage.includes("addChild") + const errorContext = isXml2jsError + ? "XML parsing failed due to external parser interference (xml2js)." + : "XML parsing failed." + + throw new Error( + `${errorContext} Fallback parser also failed. Original: ${errorMessage}, Fallback: ${fallbackErrorMsg}`, + ) } - - // For other errors, also consider using fallback after repeated failures - this.parseFailureCount++ - - if (this.parseFailureCount >= this.MAX_FAILURES) { - try { - const result = this.fallbackXmlParse(xmlString) - // Reset counter on successful fallback - this.parseFailureCount = 0 - return result - } catch (fallbackError) { - // Reset counter after fallback attempt - this.parseFailureCount = 0 - const fallbackErrorMsg = fallbackError instanceof Error ? fallbackError.message : "Unknown error" - throw new Error( - `XML parsing failed with both parsers. Original: ${errorMessage}, Fallback: ${fallbackErrorMsg}`, - ) - } - } - - throw new Error(`Failed to parse XML: ${errorMessage}`) } } }