feat: add support for Node wrapper with TypeScript and dual-publishing (ESM/CJS)

[surbhigarg92]

This PR introduces a Node.js wrapper (spannerlib-node) for the Spanner shared library. It uses TypeScript and Node-API to provide a dual-published package (ESM and CommonJS) .

Key Highlights

1. Dual Publishing: Successfully ships targeted builds inside build/esm/ and build/cjs/ mapped through an exports map.
2. executeSql Support: Implemented the core method to execute SQL statements against the database in Node wrapper. Transactions, sessions, and other administrative methods will be built out in a follow-up PR.
3. Raw Protobuf Rows: rows.next() returns raw ListValue objects, matching Java and .NET behaviors.
4. Automated Native Build: Added a custom shell script to handle Go binary compilation on the fly, chained directly into the standard npm run build lifecycle.

Current Limitations
Windows CI: The Windows CI currently fails on compile-time static linking. We have deferred Windows support and platform-specific packaging to a dedicated task after this merges.

[gemini-code-assist]

Code Review

This pull request introduces a new Node.js wrapper for the Spanner shared library, providing three implementation approaches (N-API, Koffi, and IPC) along with a benchmarking suite. The review identified several critical issues, including hardcoded paths in build scripts, synchronous execution of potentially blocking operations, missing memory management for pinned objects, and incomplete type handling in the result parser. I recommend addressing these portability and performance concerns before merging.

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

spannerlib/grpc-server/build-protos.sh (31)

Hardcoded absolute path to the Java gRPC plugin. This will fail on any machine other than the author's. Consider using an environment variable or ensuring the plugin is available in the system PATH.

spannerlib/grpc-server/build-protos.sh (40)

Hardcoded absolute path to the C# gRPC plugin. This breaks the portability of the build script and will cause CI/CD pipelines to fail.

spannerlib/wrappers/spannerlib-node/src/cpp/addon.cc (303-321)

CloseRowsWrapper is implemented synchronously. Closing rows in a database driver often involves network communication to signal the end of a stream or release server-side resources. Calling this on the Node.js main thread can block the event loop, leading to performance degradation. It should be implemented using Napi::AsyncWorker.

spannerlib/wrappers/spannerlib-node/src/ffi/utils.ts (15-43)

The invokeAsync function accepts constructor1 and constructor2 (which should be renamed to refInstance and pinManager) but never uses them to register the pinner ID (result.r0) with the memory manager. This will lead to memory leaks in the Go shared library because pinned objects are never released when JavaScript objects are garbage collected.

spannerlib/wrappers/spannerlib-node/src/lib/connection.ts (43)

The session ID is hardcoded to "poc/dummy". Cloud Spanner requires a valid session ID for executing SQL. This will cause errors when interacting with a real Spanner instance. The session should be dynamically obtained from the pool.

spannerlib/wrappers/spannerlib-node/src/lib/rows.ts (24-39)

The parseRowToObject function only handles a subset of Spanner types. It is missing support for bytesValue, structValue, and listValue. This could lead to data loss or undefined values being returned without warning for those types.

spannerlib/wrappers/spannerlib-node/src/lib/rows.ts (72)

Fetching rows one by one (numRows: 1) is highly inefficient as it incurs a network round-trip for every single row. Consider using a larger batch size or making it configurable to improve throughput.

spannerlib/wrappers/spannerlib-nodejs-poc/benchmark/benchmark.js (31)

The database path is hardcoded to a specific testing instance. This should be configurable via environment variables to allow running benchmarks in different environments.

const dbPath = process.env.SPANNER_DB_PATH || `projects/span-cloud-testing/instances/gargsurbhi-testing/databases/${dbName}`;

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing an object-oriented TypeScript interface for managing database pools, connections, and row iterators. The implementation includes a C++ bridge and a memory management system using a FinalizationRegistry. However, several critical issues were identified regarding memory management and performance. Specifically, memory leaks occur in Rows.next() and CloseRowsWrapper because native pinner IDs are not released, and the Pool, Connection, and Rows classes are not correctly registered with the cleanup registry. Additionally, CloseRowsWrapper should be implemented asynchronously to avoid blocking the Node.js event loop, and hardcoded session values in SQL execution must be addressed for production use.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, featuring a C++ addon to bridge Node.js and Go, along with TypeScript classes for managing pools, connections, and row iteration. The implementation includes automatic memory management via a FinalizationRegistry and comprehensive build scripts. Feedback highlights a critical race condition in the C++ addon where memory might be released before being copied, as well as potential issues with promises hanging due to insufficient argument validation. Other suggestions include improving type safety in the TypeScript layer and addressing hardcoded session identifiers.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a high-performance bridge between Node.js and Go. It includes a C++ addon for handling native calls, TypeScript classes for managing pools, connections, and row iteration, and a memory management system using a FinalizationRegistry. The review identifies several critical memory management issues, specifically regarding the failure to release pinner IDs in error paths and after data copying in the C++ layer. Additionally, the FinalizationRegistry currently only releases pinner IDs without cleaning up the underlying Go objects. Other feedback suggests adding argument length checks in the C++ wrappers for better stability and replacing hardcoded session IDs with dynamic management.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a bridge between Node.js and the Go-based library. Feedback focuses on critical memory management issues, including the incorrect use of FinalizationRegistry for short-lived identifiers and missing or redundant pinner ID releases in the native and TypeScript layers. Additionally, improvements are needed for C++ argument validation and the removal of hardcoded session identifiers to ensure compatibility with real Spanner environments.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, featuring C++ bindings and TypeScript abstractions for database management. The review identifies several high-severity issues, including native memory leaks in the C++ worker classes and an architectural flaw where Go objects are unpinned but not properly closed. Additional feedback suggests improving safety through argument validation, removing unused parameters, and avoiding tight coupling to internal package paths.

[surbhigarg92]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a Node-API wrapper for the Spanner shared library, providing a high-performance bridge between Node.js and the Go-based library. The implementation includes a C++ addon, TypeScript classes for managing database resources, and a comprehensive build and test suite. However, the review highlights several critical issues: multiple argument mismatches between the TypeScript FFI layer and the C++ wrapper functions will likely cause runtime crashes. Furthermore, native memory leaks were identified in the C++ workers due to unreleased pinner IDs, and the FinalizationRegistry logic for automatic resource cleanup is incorrectly implemented. Finally, the use of internal package paths for protobuf imports should be addressed to ensure long-term stability.

[gemini-code-assist]

There is a critical argument mismatch between the TypeScript call and the C++ wrapper. CreatePoolWrapper in addon.cc expects 2 string arguments (userAgent, connStr) followed by a callback. However, this call passes 4 arguments (p, spannerLib, userAgentSuffix, connectionString). This will cause the C++ side to attempt to cast the Pool instance (p) to a string, resulting in a crash or exception.

    const handled = await ffi.invokeAsync(
      'CreatePool',
      userAgentSuffix,
      connectionString
    );

[gemini-code-assist]

Argument mismatch: CreateConnectionWrapper in C++ expects 1 number argument (poolId) and a callback. The TypeScript code is passing 3 arguments (c, spannerLib, pool.oid). The first argument c (the Connection instance) will be incorrectly interpreted as the poolId number.

    const handled = await ffi.invokeAsync(
      'CreateConnection',
      pool.oid
    );

[gemini-code-assist]

Argument mismatch: ExecuteWrapper in C++ expects 3 arguments (poolId, connId, payload) and a callback. The TypeScript code passes 5 arguments, including two leading null values. This will cause info[0] and info[1] to be null instead of the expected IDs, and info[2] to be a number instead of a Buffer, leading to a crash.

    const rowsResult = await ffi.invokeAsync(
      'Execute',
      this.pool.oid,
      this.oid,
      serializedPb
    );

[gemini-code-assist]

Argument mismatch: NextWrapper in C++ expects 5 arguments (poolId, connId, rowsId, numRows, encodeOtp) and a callback. The TypeScript code passes 7 arguments, including two leading null values. This will shift all arguments and cause type errors in the native layer.

    const handled = await ffi.invokeAsync(
      'Next',
      this.connection.pool.oid,
      this.connection.oid,
      this.oid,
      1,
      ENCODING_PROTOBUF
    );

[gemini-code-assist]

The pinner ID (result_.r0) returned by the Go library for the response message is not being released in CreatePoolWorker. This will lead to a native memory leak as the Go runtime will keep the memory pinned indefinitely. It should be released immediately after the buffer is copied into the Node.js environment.

        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        // Release the pinner ID of the response message to prevent native leak!
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});

[gemini-code-assist]

Similar to CreatePoolWorker, CreateConnectionWorker is missing the call to ::Release(result_.r0). This causes a native memory leak for every connection created.

        if (result_.r4 != nullptr && result_.r3 > 0) {
            obj.Set("r4", Napi::Buffer<uint8_t>::Copy(env, (uint8_t*)result_.r4, result_.r3));
        } else {
            obj.Set("r4", env.Null());
        }
        // Release the pinner ID of the response message to prevent native leak!
        if (result_.r0 > 0) {
            ::Release(result_.r0);
        }
        Callback().Call({env.Null(), obj});

[gemini-code-assist]

The FinalizationRegistry logic is fundamentally flawed. It is registering the pinnerId of the creation response message. As noted in other comments, this ID should be released immediately after the creation call finishes. Furthermore, releasing this pinner ID does NOT close the underlying Go object (Pool/Connection/Rows). To prevent native resource leaks, the registry should track the objectId and trigger the appropriate Close method (e.g., ClosePool), although this is complex because those methods are asynchronous and the registry callback is synchronous.

[gemini-code-assist]

Importing from an internal path (/build/protos/protos.js) of a dependency is fragile and discouraged as it can break with any minor update of the @google-cloud/spanner package. Consider using a more stable way to access these protobuf definitions if available.