Repository navigation
feat(v17): build libpg-query@17 with the full API, like v18 - #176
pyramation wants to merge 4 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high · 🟡 4 medium 💬 Inline comments (5)
🧹 Nitpicks (1) — 🟢 1 low
The change converts the version packages to exact-commit pinning (
Reviewed commit: 9e294a4 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR pins every version package to an exact upstream commit, regenerates all Makefiles from templates, and rewrites the PG17 bindings with a full parser API (fingerprint, normalize, scan, PL/pgSQL) plus a rewritten C WASM wrapper.
Key findings
- 🟠 Error detection by string prefix returns errors as results — index.ts:279
- 🟡 Restore the dropped
contextfield on parse errors — index.ts:214 - 🟡 Escape all control characters in scan JSON output — wasm_wrapper.c:322
- 🟡 Validate
x-publishfields infetch-protos— fetch-protos.js:74 - 🟡 Restore exported
SqlErrorFormatOptionsand duck-typed guard — index.ts:45
| if (resultStr.startsWith('syntax error') || resultStr.startsWith('deparse error') || resultStr.startsWith('ERROR')) { | ||
| throw new Error(resultStr); | ||
| } |
There was a problem hiding this comment.
🟠 bug · high
Error detection by string prefix returns errors as results
fingerprint, normalize, scan and their Sync twins detect failures only by resultStr.startsWith('syntax error' | 'deparse error' | 'ERROR') (versions/17/src/index.ts:279). The C wrapper returns error strings that match none of these prefixes: validate_input failure returns "Invalid input: query cannot be null or empty" (versions/17/src/wasm_wrapper.c:376) and unpack/allocation failures return "Failed to unpack scan result" / "Memory allocation failed". Consequently fingerprint("") resolves successfully with the literal string "Invalid input: query cannot be null or empty" as the fingerprint — which callers building cache keys silently persist — and scanSync("") surfaces a cryptic JSON.parse SyntaxError instead of a query error.
📋 Prompt for AI Agents
In versions/17/src/index.ts (fingerprint at 271-290, normalize at 292-311, scan at 450-469, and fingerprintSync/normalizeSync/scanSync at 402-448/471-493; mirror in versions/18/src/index.ts and templates/full/index.ts), replace the resultStr.startsWith('syntax error') ... heuristic with a structural error check. Preferred: in versions/17/src/wasm_wrapper.c have wasm_fingerprint/wasm_normalize_query/wasm_scan return a distinguishable envelope (e.g. {"error":"..."}) on failure and in index.ts detect that envelope and throw. Minimal TypeScript-only alternative: also treat an empty result and results starting with 'Invalid input', 'Failed to unpack', or 'Memory allocation failed' as errors, and reject empty queries up front in these six wrappers, because fingerprint("") currently returns the C-side error message as a successful fingerprint.
| if (errorPtr) { | ||
| // Read PgQueryError struct fields | ||
| // struct { char* message; char* funcname; char* filename; int lineno; int cursorpos; char* context; } | ||
| const messagePtr = wasmModule.getValue(errorPtr, 'i32'); // offset 0 | ||
| const funcnamePtr = wasmModule.getValue(errorPtr + 4, 'i32'); // offset 4 | ||
| const filenamePtr = wasmModule.getValue(errorPtr + 8, 'i32'); // offset 8 | ||
| const lineno = wasmModule.getValue(errorPtr + 12, 'i32'); // offset 12 | ||
| const cursorpos = wasmModule.getValue(errorPtr + 16, 'i32'); // offset 16 | ||
| const contextPtr = wasmModule.getValue(errorPtr + 20, 'i32'); // offset 20 | ||
| // Read PgQueryError struct | ||
| const messagePtr = wasmModule.getValue(errorPtr, 'i32'); | ||
| const funcnamePtr = wasmModule.getValue(errorPtr + 4, 'i32'); | ||
| const filenamePtr = wasmModule.getValue(errorPtr + 8, 'i32'); | ||
| const lineno = wasmModule.getValue(errorPtr + 12, 'i32'); | ||
| const cursorpos = wasmModule.getValue(errorPtr + 16, 'i32'); | ||
|
|
||
| const message = messagePtr ? wasmModule.UTF8ToString(messagePtr) : 'Unknown error'; | ||
| const filename = filenamePtr ? wasmModule.UTF8ToString(filenamePtr) : null; | ||
| const funcname = funcnamePtr ? wasmModule.UTF8ToString(funcnamePtr) : undefined; | ||
| const filename = filenamePtr ? wasmModule.UTF8ToString(filenamePtr) : undefined; | ||
|
|
||
| const errorDetails: SqlErrorDetails = { | ||
| message: message, | ||
| throw new SqlError(message, { | ||
| message, | ||
| cursorPosition: cursorpos > 0 ? cursorpos - 1 : 0, // Convert to 0-based | ||
| fileName: filename || undefined, | ||
| functionName: funcnamePtr ? wasmModule.UTF8ToString(funcnamePtr) : undefined, | ||
| lineNumber: lineno > 0 ? lineno : undefined, | ||
| context: contextPtr ? wasmModule.UTF8ToString(contextPtr) : undefined | ||
| }; | ||
|
|
||
| throw new SqlError(message, errorDetails); | ||
| fileName: filename, | ||
| functionName: funcname, | ||
| lineNumber: lineno > 0 ? lineno : undefined | ||
| }); |
There was a problem hiding this comment.
🟡 bug · medium
Restore the dropped context field on parse errors
The rewrite of parse/parseSync stopped reading the context pointer at offset 20 of the PgQueryError struct and no longer sets context in the thrown SqlError (versions/17/src/index.ts:214-232). The C wrapper still populates PgQueryError.context (versions/17/src/wasm_wrapper.c:207-213) and the exported SqlErrorDetails interface still declares context?: string, so consumers relying on the surrounding SQL fragment Postgres reports silently receive undefined for every parse error.
📋 Prompt for AI Agents
In versions/17/src/index.ts, in the error branches of parse (lines 214-232) and parseSync (lines 342-360), re-add const contextPtr = wasmModule.getValue(errorPtr + 20, 'i32'); after the cursorpos read and include context: contextPtr ? wasmModule.UTF8ToString(contextPtr) : undefined in the SqlError details object. The struct is {message, funcname, filename, lineno, cursorpos, context}, so offset 20 holds the context char* on 4-byte-pointer wasm builds. Apply the same change in versions/18/src/index.ts and templates/full/index.ts.
| for (int j = 0; j < token_length; j++) { | ||
| char c = token_text[j]; | ||
| if (c == '"' || c == '\\') { | ||
| escaped_text[escaped_pos++] = '\\'; | ||
| escaped_text[escaped_pos++] = c; | ||
| } else if (c == '\n') { | ||
| escaped_text[escaped_pos++] = '\\'; | ||
| escaped_text[escaped_pos++] = 'n'; | ||
| } else if (c == '\r') { | ||
| escaped_text[escaped_pos++] = '\\'; | ||
| escaped_text[escaped_pos++] = 'r'; | ||
| } else if (c == '\t') { | ||
| escaped_text[escaped_pos++] = '\\'; | ||
| escaped_text[escaped_pos++] = 't'; | ||
| } else { | ||
| escaped_text[escaped_pos++] = c; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
Escape all control characters in scan JSON output
The JSON escaping loop in build_scan_json only escapes ", \, , \r, and \t; every other C0 control character such as \b, \f, or \x01 is copied verbatim into the JSON string (versions/17/src/wasm_wrapper.c:322-339). Token text is attacker-controlled SQL fed to wasm_scan, and JSON.parse in scan/scanSync (versions/17/src/index.ts:462) rejects raw control characters in string literals. Any query containing, e.g., a form feed inside a dollar-quoted string or block comment makes scanSync throw SyntaxError: Bad control character in string literal instead of returning tokens.
📋 Prompt for AI Agents
In versions/17/src/wasm_wrapper.c, in the character-escaping loop of build_scan_json (lines 322-339, and the same code in templates/full/wasm_wrapper.c), after the existing cases for '"', '\', '\n', '\r', '\t' add a final branch for any character c < 0x20 that writes a six-character \u00XX escape (e.g. snprintf(&escaped_text[escaped_pos], 7, "\\u%04x", (unsigned char)c) and advance escaped_pos by 6). Since the escaped buffer is sized token_length * 2 + 1, raise it to token_length * 6 + 1 (and the matching estimated_size allowance) so worst-case escaping cannot overflow. This keeps scan/scanSync from failing JSON.parse on SQL containing control characters.
| // The same repo and commit the WASM is built from | ||
| const repoPath = new URL(libpgQueryRepo).pathname.replace(/^\/|\.git$/g, ''); | ||
| const url = `https://raw.githubusercontent.com/${repoPath}/${libpgQueryCommit}/protobuf/pg_query.proto`; | ||
| const destPath = path.join(versionDir, 'pg_query.proto'); | ||
|
|
||
| console.log(`Fetching protobuf for PostgreSQL ${pgVersion} with tag ${libpgQueryTag}...`); | ||
| console.log(`Fetching protobuf for PostgreSQL ${pgVersion} from ${repoPath} ${libpgQueryTag} @ ${libpgQueryCommit.slice(0, 7)}...`); |
There was a problem hiding this comment.
🟡 security · medium
Validate x-publish fields in fetch-protos
fetchProtos consumes libpgQueryRepo/libpgQueryCommit straight from each package's x-publish with no validation (scripts/fetch-protos.js:75-79), while copy-templates.js:39-41 enforces a 40-hex commit for the same data. new URL(libpgQueryRepo).pathname and libpgQueryCommit.slice also run outside the per-version try block, so a missing or malformed field throws a TypeError that escapes the loop and aborts the fetch for all remaining versions. Impact: a proto can be fetched from any repo or ref the config names — silently decoupling the shipped proto from the pinned WASM commit — and one bad package.json field crashes the whole script with an unhelpful stack instead of a per-version error.
📋 Prompt for AI Agents
In scripts/fetch-protos.js, mirror the validation in scripts/copy-templates.js lines 39-41: in getVersionMappings (lines 16-24) reject mappings whose libpgQueryCommit does not match /^[0-9a-f]{40}$/ or whose libpgQueryRepo is missing/not an https URL on the allowlisted hosts (github.com paths under pganalyze or constructive-io), throwing with the package path in the message. Additionally move the new URL(libpgQueryRepo) construction and libpgQueryCommit.slice(0, 7) logging (lines 75-79) inside the existing per-version try/catch (lines 81-89) so a single malformed version logs a failure and sets process.exitCode = 1 instead of aborting all remaining versions with a raw TypeError.
| export function hasSqlDetails(error: unknown): error is SqlError { | ||
| return error instanceof SqlError && error.sqlDetails !== undefined; | ||
| } | ||
|
|
||
| // Format SQL error with visual position indicator | ||
| export function formatSqlError( | ||
| error: Error & { sqlDetails?: SqlErrorDetails }, | ||
| error: SqlError, | ||
| query: string, | ||
| options: SqlErrorFormatOptions = {} | ||
| options: { | ||
| showPosition?: boolean; | ||
| showQuery?: boolean; | ||
| color?: boolean; | ||
| maxQueryLength?: number; | ||
| } = {} | ||
| ): string { |
There was a problem hiding this comment.
🟡 bug · medium
Restore exported SqlErrorFormatOptions and duck-typed guard
The rewrite deletes the exported SqlErrorFormatOptions interface (previously the third parameter of formatSqlError) and changes hasSqlDetails from a duck-typed error is Error & { sqlDetails: SqlErrorDetails } guard to error is SqlError (versions/17/src/index.ts:45-58). Sibling version packages still export the interface, so TypeScript consumers importing it from v17 now fail to compile; and errors deserialized across a process boundary that lost their prototype silently fail the instanceof SqlError check, changing formatting behavior at runtime.
📋 Prompt for AI Agents
In versions/17/src/index.ts lines 45-58, restore backward-compatible signatures matching the other version packages: export function hasSqlDetails(error: unknown): error is Error & { sqlDetails: SqlErrorDetails } using a duck-typed check for a sqlDetails object, and re-export the SqlErrorFormatOptions interface with formatSqlError(error: Error & { sqlDetails?: SqlErrorDetails }, query: string, options: SqlErrorFormatOptions = {}). If the breaking change is intentional, gate it behind a package major bump and document it.
# Conflicts: # versions/17/package.json
# Conflicts: # versions/17/package.json # versions/17/src/index.ts
Summary
Builds
versions/17the wayversions/18is built: one flag inx-publish, everything else is generated bycopy:templatesfromtemplates/full/."x-publish": { "libpgQueryRepo": "https://github.com/constructive-io/libpg_query.git", "libpgQueryTag": "17-constructive", - "libpgQueryCommit": "1ade880f…" + "libpgQueryCommit": "1ade880f…", + "fullApi": true }libpg-query@17therefore gainsparsePlPgSQL,scan,fingerprint,normalize(+ sync variants). That replaces the retired@libpg-query/parser@17(full/) build path, so v17 no longer needs a separate package or release branch. Adding exports makes this a minor release (17.8.0).Stacked on #174. This branch includes #174's commits, so merge #174 first. The only commit of its own is
feat(v17): …. It conflicts with #175 only in the generatedversions/17/src/index.ts; whichever merges second fixes that by rerunningpnpm copy:templates.What doesn't change
17-constructive@1ade880), same emsdk 6.0.3, same memory flags. No C, fork, proto or@pgsql/typeschanges (@pgsql/typesstays^17.6.2).parse/parseSyncstill go through_wasm_parse_query_raw, so the output is the same. I compared npm[email protected]with this build on all 293 SQL files in pgsql-parser's corpus (deparse, deparse-depesz, postgres_regress, plpgsql_regress), split into 50,866 statements: 50,866 byte-identical results, including the 5,910 error results (message +sqlDetails).Cost
Tests
fingerprint,normalize,plpgsql,scantests are copied toversions/17/testand added topnpm test. Local: 86/86 pass on a freshpnpm run build.Link to Devin session: https://app.devin.ai/sessions/bcd8b2f791164247acb7a659b481efc2
Open in Devin Desktop: https://app.devin.ai/desktop/session/bcd8b2f791164247acb7a659b481efc2?variant=devin
Requested by: @pyramation