chore(spanner-driver): add auto-sync workflow and script to embed go-sql-spanner/spannerlib-node in spanner-driver - #9024
Conversation
1ba8ad7 to
595792a
Compare
There was a problem hiding this comment.
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)
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)
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)
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)
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>>;
}
}595792a to
18bdb14
Compare
No description provided.