From f5edf8cf49f688e97ca857881fca86fb9b11c42c Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Tue, 7 Jul 2026 09:46:22 -0500 Subject: [PATCH] fix(mybatis): quote/comment robustness, iBatis coverage, dup-id collision (#1182) (#1204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps in the MyBatis mapper extractor, all reported and reproduced by @ESPINS in #1182 and verified against main: 1. Single-quoted attribute values (namespace/id/refid/resultType/parameterType) were dropped — the regexes hardcoded double quotes. Now accept either quote via a backreference. 2. Tags inside produced phantom statement/include symbols. A length-preserving, CDATA-aware pre-pass blanks comments before scanning, keeping offsets/line numbers intact. 3. Legacy iBatis 2 files had zero statement coverage (the root finder gated on a root). It now also recognizes (namespaced and namespace-less DAO.method ids) and iBatis's extra / verbs — closing the gap with no new dependency (option (c) from the issue; the batis-xml parser route is declined). 4. Two statements sharing a qualifiedName AND a start line (a vendor-split databaseId pair on one line) collided on the node id, so INSERT OR REPLACE silently dropped one. The id-hash now folds in the statement's byte offset; the stored qualifiedName/startLine are unchanged so the Java<->XML bridge is untouched. Gaps 1 and 2 follow @ESPINS's fix-mybatis-quotes-comments branch. Tests add extractor-level coverage for all four gaps plus a DB-level e2e that proves iBatis statements land and both vendor-split nodes survive a real indexAll. Co-authored-by: Jimin Lee Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 + __tests__/frameworks-integration.test.ts | 64 +++++ .../mybatis-extractor-robustness.test.ts | 218 ++++++++++++++++++ src/extraction/mybatis-extractor.ts | 186 ++++++++++++--- 4 files changed, 433 insertions(+), 37 deletions(-) create mode 100644 __tests__/mybatis-extractor-robustness.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 58b4a87..2ecab07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,9 +31,11 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - The Claude Code context hook now recognizes prompts that describe code in plain words — in any language — by checking the prompt's words against the symbol names actually in your project's index. Asking about "the state machine des commandes" finds `OrderStateMachine` with no keyword involved. Confidence decides how much gets injected: structural questions and prompts naming a real symbol still get full context up front; a plain-words match gets a short pointer to the matching symbols so the agent queries them itself; everything else stays silent, exactly as before. - Anonymous usage telemetry now counts how often the context hook injected context, offered a hint, or stayed silent — fixed counter names only; the prompt's content is never stored or sent. This makes the hook's accuracy measurable instead of guessed. The counters record what actually happened, not what was attempted: a lookup that errors or comes back empty counts as a distinct silent outcome, never as delivered context (#1143, thanks @inth3shadows). - Metal shader files (`.metal`) are now indexed. Metal Shading Language is close enough to C++ that vertex/fragment/kernel functions, structs, type aliases, and the calls between them all land in the graph — so shader pipelines in Apple-platform projects show up in impact analysis and flow traces instead of being silently skipped. Metal's `[[buffer(0)]]`-style attribute annotations are handled so they can't corrupt what gets extracted. Thanks @FluxKo for the report. (#1121) +- CodeGraph now indexes legacy **iBatis 2** SQL maps (``), not just MyBatis 3 `` files. `SELECT FROM account WHERE id = #id#\n" + + " INSERT INTO account (id) VALUES (#id#)\n" + + ' \n' + + '\n' + ); + // Namespace-less sqlMap whose ids carry the qualifier as `Map.statement`. + fs.writeFileSync( + path.join(xmlDir, 'LegacyDao.xml'), + '\n' + + ' \n' + + '\n' + ); + // MyBatis mapper with a vendor-split databaseId pair written on ONE line — + // same qualifiedName + same start line. Before the id-hash fold both nodes + // hashed identically and INSERT OR REPLACE dropped one. + fs.writeFileSync( + path.join(xmlDir, 'VendorMapper.xml'), + '\n' + + '\n' + + '\n' + ); + + const cg = CodeGraph.initSync(tmpDir); + await cg.indexAll(); + + const xmlMethods = cg.getNodesByKind('method').filter((n) => n.language === 'xml'); + const qnames = xmlMethods.map((n) => n.qualifiedName); + + // iBatis statements now land in the graph (was zero coverage before #1182). + expect(qnames).toContain('Account::getById'); + expect(qnames).toContain('Account::insert'); + expect(qnames).toContain('Account::cols'); + expect(qnames).toContain('LegacyDao::findAll'); + // The commented-out statement produced no node. + expect(qnames).not.toContain('Account::disabled'); + + // resolves to the fragment in the same map. + const getById = xmlMethods.find((n) => n.qualifiedName === 'Account::getById'); + const cols = xmlMethods.find((n) => n.qualifiedName === 'Account::cols'); + expect(getById).toBeDefined(); + expect(cols).toBeDefined(); + const incEdge = cg.getOutgoingEdges(getById!.id).find((e) => e.target === cols!.id); + expect(incEdge, "iBatis should reach the fragment").toBeDefined(); + + // Both vendor-split statements survive the DB write (the collision fix). + const findUser = xmlMethods.filter((n) => n.name === 'findUser'); + expect(findUser, 'both databaseId variants of findUser should survive').toHaveLength(2); + expect(new Set(findUser.map((n) => n.id)).size).toBe(2); + + cg.close(); + }); + it('binds @Value / @ConfigurationProperties to YAML + .properties keys (incl. relaxed binding)', async () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-spring-config-')); const javaDir = path.join(tmpDir, 'src/main/java/com/example'); diff --git a/__tests__/mybatis-extractor-robustness.test.ts b/__tests__/mybatis-extractor-robustness.test.ts new file mode 100644 index 0000000..9598075 --- /dev/null +++ b/__tests__/mybatis-extractor-robustness.test.ts @@ -0,0 +1,218 @@ +import { describe, it, expect } from 'vitest'; +import { extractFromSource } from '../src/extraction/tree-sitter'; + +// Robustness of the MyBatis / iBatis mapper extractor. Four shapes the regex +// scanner previously mishandled, all reported and diagnosed by @ESPINS in #1182: +// 1. single-quoted attribute values, +// 2. tags that live inside XML comments, +// 3. iBatis 2 `` files (zero statement coverage before), +// 4. two statements that share a qualifiedName *and* a start line colliding +// on the node id (silent statement loss at the DB layer). +// The quoting and comment suites below follow @ESPINS's fix-mybatis-quotes-comments +// branch; the iBatis and collision suites cover the regex-only path taken here +// (no parser dependency). + +const methodNodes = (xml: string, file = 'FooMapper.xml') => + extractFromSource(file, xml).nodes.filter((n) => n.kind === 'method'); + +const methodNames = (xml: string, file = 'FooMapper.xml') => + methodNodes(xml, file).map((n) => n.qualifiedName); + +describe('MyBatis extractor — attribute quoting', () => { + it('accepts a single-quoted namespace', () => { + const xml = + "" + + ''; + expect(methodNames(xml)).toContain('com.example.FooMapper::getById'); + }); + + it('accepts a single-quoted statement id', () => { + const xml = + '' + + ""; + expect(methodNames(xml)).toContain('com.example.FooMapper::getById'); + }); + + it('accepts a single-quoted ', () => { + const xml = + '' + + 'id, name' + + "" + + ''; + const refs = extractFromSource('FooMapper.xml', xml).unresolvedReferences.map( + (r) => r.referenceName + ); + expect(refs).toContain('com.example.FooMapper::cols'); + }); + + it('reads single-quoted resultType / parameterType into the signature', () => { + const xml = + "" + + "" + + ''; + const sig = methodNodes(xml).find((n) => n.name === 'getById')?.signature; + expect(sig).toContain('result=User'); + expect(sig).toContain('param=int'); + }); + + it('handles mixed single- and double-quoted attributes in one file', () => { + const xml = + "" + + "" + + 'UPDATE t SET x=1' + + ''; + expect(methodNames(xml)).toEqual([ + 'com.example.FooMapper::getById', + 'com.example.FooMapper::touch', + ]); + }); + + it('still accepts double-quoted attributes (regression guard)', () => { + const xml = + '' + + ''; + expect(methodNames(xml)).toContain('com.example.FooMapper::getById'); + }); +}); + +describe('MyBatis extractor — XML comments', () => { + const result = (xml: string) => extractFromSource('FooMapper.xml', xml); + + it('does not emit a node for a statement inside a comment', () => { + const xml = + '' + + '' + + ''; + const names = result(xml) + .nodes.filter((n) => n.kind === 'method') + .map((n) => n.name); + expect(names).toContain('live'); + expect(names).not.toContain('dead'); + }); + + it('does not follow an inside a comment', () => { + const xml = + '' + + '' + + ''; + const refs = result(xml).unresolvedReferences.map((r) => r.referenceName); + expect(refs).not.toContain('com.example.FooMapper::cols'); + }); + + it('keeps the correct startLine for a statement after a multi-line comment', () => { + const xml = + '\n' + + '\n' + + '\n' + + '\n'; + const stmt = result(xml).nodes.find((n) => n.name === 'getById'); + expect(stmt).toBeDefined(); + // The SELECT 1' + + ']]>' + + ''; + const names = result(xml) + .nodes.filter((n) => n.kind === 'method') + .map((n) => n.name); + expect(names).toContain('live'); + }); + + it('does not crash on an unterminated comment (blanks to end of file)', () => { + const xml = + '' + + '' + + '` are + * ignored (see the constructor's comment-stripping pre-pass). * * Non-mapper XML (Maven `pom.xml`, Spring beans XML, `web.xml`, log4j config, - * etc.) is detected by the absence of a `` root and - * returns just a file node — we still need the file row so the watcher can - * track it, but we emit no symbols. + * etc.) is detected by the absence of a `` / + * `` root and returns just a file node — we still need the file row so + * the watcher can track it, but we emit no symbols. */ export class MyBatisExtractor { private filePath: string; @@ -35,19 +41,50 @@ export class MyBatisExtractor { constructor(filePath: string, source: string) { this.filePath = filePath; - this.source = source; + // Blank out XML comments up front so commented-out statements and includes + // aren't matched by the scans below (a `` + // block must not produce a phantom node). Length-preserving — comment bytes + // become spaces, newlines are kept — so the offsets and line numbers + // computed afterwards still map to the original source. Text inside + // `` is left intact: a literal `', i + 4); + const stop = end >= 0 ? end + 3 : n; + for (let j = i; j < stop; j++) { + if (source.charCodeAt(j) !== 10) out[j] = ' '; + } + i = stop; + continue; + } + i++; + } + return out.join(''); + } + extract(): ExtractionResult { const startTime = Date.now(); const fileNode = this.createFileNode(); try { - const mapperMatch = this.findMapperRoot(); - if (mapperMatch) { - this.extractMapper(fileNode.id, mapperMatch.namespace, mapperMatch.bodyStart, mapperMatch.bodyEnd); + const root = this.findMapperRoot(); + if (root) { + this.extractMapper(fileNode.id, root.namespace, root.dialect, root.bodyStart, root.bodyEnd); } } catch (error) { this.errors.push({ @@ -87,47 +124,98 @@ export class MyBatisExtractor { } /** - * Find the `` opening tag. Returns the namespace and - * the byte offsets of the body (between the opening and closing tag) so - * statement extraction can be scoped to mapper contents. + * Find the mapper root and its dialect. Two shapes are recognized: + * - MyBatis 3: `` — namespace required. + * - iBatis 2: ``, or a namespace-less + * `` whose statement ids carry the qualifier as `Map.statement`. + * Returns the namespace, the dialect, and the byte offsets of the body + * (between the opening and closing tag) so statement extraction is scoped to + * the root's contents. Either quote style is accepted for the namespace + * (`namespace='X'` is legal XML and common in older mappers). */ - private findMapperRoot(): { namespace: string; bodyStart: number; bodyEnd: number } | null { - const open = /]*)>/.exec(this.source); - if (!open) return null; - const attrs = open[1] ?? ''; - const nsMatch = /\bnamespace\s*=\s*"([^"]+)"/.exec(attrs); - if (!nsMatch) return null; - const bodyStart = open.index + open[0].length; - const closeIdx = this.source.indexOf('', bodyStart); - const bodyEnd = closeIdx >= 0 ? closeIdx : this.source.length; - return { namespace: nsMatch[1]!, bodyStart, bodyEnd }; + private findMapperRoot(): + | { namespace: string; dialect: 'mybatis' | 'ibatis'; bodyStart: number; bodyEnd: number } + | null { + const mapper = /]*)>/.exec(this.source); + if (mapper) { + const nsMatch = /\bnamespace\s*=\s*(["'])([^"']+)\1/.exec(mapper[1] ?? ''); + if (nsMatch) { + const bodyStart = mapper.index + mapper[0].length; + const closeIdx = this.source.indexOf('', bodyStart); + return { + namespace: nsMatch[2]!, + dialect: 'mybatis', + bodyStart, + bodyEnd: closeIdx >= 0 ? closeIdx : this.source.length, + }; + } + } + // iBatis 2 SqlMap. `\b` keeps `` (the iBatis config root, + // which holds no statements) from matching here. namespace is optional. + const sqlMap = /]*)>/.exec(this.source); + if (sqlMap) { + const nsMatch = /\bnamespace\s*=\s*(["'])([^"']+)\1/.exec(sqlMap[1] ?? ''); + const bodyStart = sqlMap.index + sqlMap[0].length; + const closeIdx = this.source.indexOf('', bodyStart); + return { + namespace: nsMatch?.[2] ?? '', + dialect: 'ibatis', + bodyStart, + bodyEnd: closeIdx >= 0 ? closeIdx : this.source.length, + }; + } + return null; } - private extractMapper(fileNodeId: string, namespace: string, bodyStart: number, bodyEnd: number): void { + private extractMapper( + fileNodeId: string, + namespace: string, + dialect: 'mybatis' | 'ibatis', + bodyStart: number, + bodyEnd: number + ): void { const body = this.source.slice(bodyStart, bodyEnd); // Match each top-level statement-shaped element. The body may have nested // tags (``, ``, ``), so we scan with a regex that // pairs an opening tag to its matching close — the simple form below works - // because MyBatis statement elements are not themselves nested. - const stmtRegex = /<(select|insert|update|delete|sql)\b([^>]*)>([\s\S]*?)<\/\1>/g; + // because MyBatis/iBatis statement elements are not themselves nested. + // iBatis 2 adds the generic `` and `` on top of the + // MyBatis 3 verbs; gating by dialect keeps MyBatis extraction unchanged. + const verbs = + dialect === 'ibatis' + ? 'select|insert|update|delete|sql|statement|procedure' + : 'select|insert|update|delete|sql'; + const stmtRegex = new RegExp(`<(${verbs})\\b([^>]*)>([\\s\\S]*?)`, 'g'); let m: RegExpExecArray | null; while ((m = stmtRegex.exec(body)) !== null) { const elemType = m[1]!; const attrs = m[2] ?? ''; const elemBody = m[3] ?? ''; - const idMatch = /\bid\s*=\s*"([^"]+)"/.exec(attrs); + // Accept either quote style (`(["'])…\1`). The identifier-shaped MyBatis + // attributes matched here and below (namespace/id/refid/resultType/ + // parameterType) are Java FQNs, method names, or type aliases and never + // contain a quote character, so excluding both quotes from the value is safe. + const idMatch = /\bid\s*=\s*(["'])([^"']+)\1/.exec(attrs); if (!idMatch) continue; - const id = idMatch[1]!; + const id = idMatch[2]!; const absoluteIndex = bodyStart + m.index; const startLine = this.getLineNumber(absoluteIndex); const endLine = this.getLineNumber(absoluteIndex + m[0].length); - const qualified = `${namespace}::${id}`; + const { qualifiedName: qualified, name } = this.qualifyStatement(namespace, id); const isSqlFragment = elemType === 'sql'; - const nodeId = generateNodeId(this.filePath, 'method', qualified, startLine); + // The id-hash folds in the statement's byte offset (unique per statement + // in the file), not just the start line: two statements sharing a + // qualifiedName AND a start line — e.g. a vendor-split `databaseId` pair + // (`