Skip to content

chore(spanner-driver): add auto-sync workflow and script to embed go-sql-spanner/spannerlib-node in spanner-driver - #9024

Draft
surbhigarg92 wants to merge 1 commit into
mainfrom
spanner-pg-spannerlib-node-sync
Draft

chore(spanner-driver): add auto-sync workflow and script to embed go-sql-spanner/spannerlib-node in spanner-driver#9024
surbhigarg92 wants to merge 1 commit into
mainfrom
spanner-pg-spannerlib-node-sync

Conversation

@surbhigarg92

Copy link
Copy Markdown
Contributor

No description provided.

@surbhigarg92
surbhigarg92 requested review from a team as code owners July 30, 2026 10:46
@surbhigarg92
surbhigarg92 force-pushed the spanner-pg-spannerlib-node-sync branch from 1ba8ad7 to 595792a Compare July 30, 2026 10:47
@surbhigarg92 surbhigarg92 changed the title Spanner pg spannerlib node sync chore(spanner-driver): add spannerlib-node CGO auto-sync workflow and script Jul 30, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the initial scaffolding for the Google Cloud Spanner Node.js Driver, including the core Client, Pool, and Query classes, as well as a script to sync native wrappers and comprehensive unit tests. The review feedback highlights a few key issues: the sync script unconditionally deletes the go-sql-spanner directory even when cloning is skipped, which could delete local user repositories; Query.setPromise should avoid overwriting an already established promise; and both Client.query and Pool.query ignore additional values or callback arguments when a Query instance is passed directly.

I am having trouble creating individual review comments. Click here to see my feedback.

handwritten/spanner-driver/scripts/sync_spannerlib_node.sh (32)

high

If SKIP_CLONE is set to true, the script assumes a local go-sql-spanner directory exists. However, the script unconditionally deletes go-sql-spanner at the end of execution. This will delete the user's local repository/changes. The cleanup should only occur if the repository was actually cloned.

if ! [ "$SKIP_CLONE" == "true" ]; then
  rm -rf go-sql-spanner
fi

handwritten/spanner-driver/src/lib/query.ts (145-147)

medium

To prevent the outer promise (e.g., from Pool.query which manages connection acquisition and release) from being overwritten by the inner promise (from Client.query), setPromise should avoid overwriting an already established promise. This ensures consistent behavior regardless of when the query is awaited.

  public setPromise(promise: Promise<T>): void {
    if (!this.promise) {
      this.promise = promise;
    }
  }

handwritten/spanner-driver/src/lib/client.ts (127-130)

medium

When queryText is an instance of Query, any additional values or callback arguments passed to client.query are currently ignored because the existing Query instance is reused as-is without updating its values or callback properties. This can lead to queries being executed with missing or incorrect parameters if they were not specified during the Query construction but passed to client.query.

    const query = queryText instanceof Query ? queryText : new Query<QueryResult<R>>(queryText, values as unknown[], callback);
    if (queryText instanceof Query) {
      if (Array.isArray(values)) {
        query.values = values;
      }
      if (typeof callback === 'function') {
        query.callback = callback;
      } else if (typeof values === 'function') {
        query.callback = values as QueryCallback<QueryResult<R>>;
      }
    }

handwritten/spanner-driver/src/lib/pool.ts (98-101)

medium

When queryText is an instance of Query, any additional values or callback arguments passed to pool.query are currently ignored on the Query instance itself. To ensure consistency and correct parameter propagation, update the Query instance's properties if they are passed.

    const query = queryText instanceof Query ? queryText : new Query<QueryResult<R>>(queryText, values as unknown[], callback);
    if (queryText instanceof Query) {
      if (Array.isArray(values)) {
        query.values = values;
      }
      if (typeof callback === 'function') {
        query.callback = callback;
      } else if (typeof values === 'function') {
        query.callback = values as QueryCallback<QueryResult<R>>;
      }
    }

@surbhigarg92 surbhigarg92 changed the title chore(spanner-driver): add spannerlib-node CGO auto-sync workflow and script chore(spanner-driver): add auto-sync workflow and script to embed go-sql-spanner/spannerlib-node in spanner-driver Jul 30, 2026
@surbhigarg92
surbhigarg92 force-pushed the spanner-pg-spannerlib-node-sync branch from 595792a to 18bdb14 Compare July 30, 2026 11:33
@surbhigarg92
surbhigarg92 marked this pull request as draft July 30, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant