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
This commit is contained in:
hannesrudolph 2025-08-06 16:52:01 -07:00
parent 6f73937e39
commit 8669ab9ff1
2 changed files with 27 additions and 80 deletions

View file

@ -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 = `<args>
<file>
<path>test.txt</path>
@ -166,14 +166,9 @@ describe("multiApplyDiffTool", () => {
</file>
</args>`
// 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)
})
})

View file

@ -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}`)
}
}
}