Skip to content

Handle named placeholder conversion for Postgres mirror#2

Open
stritt wants to merge 1 commit intomainfrom
codex/extend-sql-placeholder-conversion-and-testing
Open

Handle named placeholder conversion for Postgres mirror#2
stritt wants to merge 1 commit intomainfrom
codex/extend-sql-placeholder-conversion-and-testing

Conversation

@stritt
Copy link
Contributor

@stritt stritt commented Sep 18, 2025

Summary

  • add sql-utils helpers to deterministically remap SQLite placeholders and build Postgres parameter arrays
  • update the auto-mirror binding logic to flatten positional, numbered, and named bindings using the shared mapping
  • switch the pg router to the shared conversion helper and add tests covering positional, numbered, and named binds

Testing

  • npm test

https://chatgpt.com/codex/tasks/task_e_68cc931f07f08324a7f26d9f5f4e6844

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

Comment on lines +185 to +203
export function buildPostgresParameterArray(
mapping: PlaceholderMappingEntry[],
positional: unknown[],
named: Map<string, unknown> = new Map(),
): unknown[] {
if (mapping.length === 0) {
return [];
}

return mapping.map(entry => {
switch (entry.kind) {
case 'positional':
case 'numbered':
return positional[entry.index];
case 'named':
if (named.has(entry.token)) {
return named.get(entry.token);
}
return named.get(entry.name);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Handle positional binding for named placeholders

The parameter remapping assumes that every named placeholder will be supplied through the named map. When callers bind values positionally (e.g. stmt.bind('Alice', 42) for INSERT INTO t VALUES (:name, :age)), normalizeParams drops both values into the positional array, but buildPostgresParameterArray never falls back to positional entries for named mappings and therefore returns [undefined, undefined]. Mirroring and the Postgres router will enqueue SQL that writes NULLs even though the original D1 execution inserted real values. Because SQLite/D1 permit positional binding for named placeholders, this will silently desynchronize data whenever clients rely on that behaviour. Consider consuming positional arguments in mapping order when a named value is missing from the map.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant