Node wrapper poc

[surbhigarg92]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces three different proof-of-concept (POC) implementations for a Node.js wrapper for the Go Spanner library: an IPC-based gRPC wrapper, a Koffi-based FFI wrapper, and an N-API C++ wrapper. The changes include build scripts, setup utilities, and test suites for each approach. My feedback highlights several critical and high-severity issues, including hardcoded infrastructure paths and credentials, missing cross-platform support in build configurations, and the need for better error handling and code modularity across the POCs.

[gemini-code-assist]

The script contains hardcoded paths to protoc plugins, which makes it non-portable and specific to a single user's machine (/Users/loite/...). This will cause the script to fail for other developers or in CI/CD environments.

To improve this, consider one of the following approaches:

1. Rely on PATH: Require developers to have the protoc-gen-* executables in their PATH.
2. Use Environment Variables: Allow developers to specify the path to the plugins via environment variables.
3. Local Installation: Add a step to download and install the required plugins locally within the project or use a package manager that handles this.

[gemini-code-assist]

The projectId and instanceId are hardcoded. This makes the setup script not portable and tightly coupled to a specific GCP project and Spanner instance. It's better to read these values from environment variables to allow other developers to run the tests against their own infrastructure.

For example:

const projectId = process.env.GCP_PROJECT_ID || 'span-cloud-testing';
const instanceId = process.env.SPANNER_INSTANCE_ID || 'gargsurbhi-testing';

This provides a default but allows for easy overriding.

Suggested change

	const projectId = 'span-cloud-testing';
	const instanceId = 'gargsurbhi-testing';
	const projectId = process.env.GCP_PROJECT_ID || 'span-cloud-testing';
	const instanceId = process.env.SPANNER_INSTANCE_ID || 'gargsurbhi-testing';

[gemini-code-assist]

There are two issues here:

1. The projectId and instanceId are hardcoded. This makes the setup script not portable. These should be read from environment variables.
2. The databaseId is generated with the prefix test-db-koffi-. This seems like a copy-paste error from the koffi POC. It should be test-db-napi- to be consistent with this N-API POC.

Suggested change

	const projectId = 'span-cloud-testing';
	const instanceId = 'gargsurbhi-testing';
	const databaseId = `test-db-koffi-${Math.floor(Math.random() * 1000)}`;
	const projectId = process.env.GCP_PROJECT_ID || 'span-cloud-testing';
	const instanceId = process.env.SPANNER_INSTANCE_ID || 'gargsurbhi-testing';
	const databaseId = `test-db-napi-${Math.floor(Math.random() * 1000)}`;

[gemini-code-assist]

The projectId and instanceId are hardcoded. This makes the setup script not portable and tightly coupled to a specific GCP project and Spanner instance. It's better to read these values from environment variables to allow other developers to run the tests against their own infrastructure.

For example:

const projectId = process.env.GCP_PROJECT_ID || 'span-cloud-testing';
const instanceId = process.env.SPANNER_INSTANCE_ID || 'gargsurbhi-testing';

This provides a default but allows for easy overriding.

Suggested change

	const projectId = 'span-cloud-testing';
	const instanceId = 'gargsurbhi-testing';
	const projectId = process.env.GCP_PROJECT_ID || 'span-cloud-testing';
	const instanceId = process.env.SPANNER_INSTANCE_ID || 'gargsurbhi-testing';

[gemini-code-assist]

The build configuration is missing a condition for Windows (OS=='win'). This will cause the build to fail on Windows platforms. You should add a condition for Windows to specify the correct library name (libspanner.dll) and linker settings.

[gemini-code-assist]

The path to the shared library is hardcoded to use the .so extension, which is specific to Linux. This will fail on other operating systems like macOS (.dylib) or Windows (.dll). To make this cross-platform, you should determine the correct extension based on process.platform.

For example:

const getLibraryPath = () => {
  const root = path.resolve(__dirname, '../../../../../shared');
  switch (process.platform) {
    case 'darwin':
      return path.join(root, 'libspanner.dylib');
    case 'win32':
      return path.join(root, 'libspanner.dll');
    default:
      return path.join(root, 'libspanner.so');
  }
};
const libPath = getLibraryPath();

[gemini-code-assist]

The gRPC server address is hardcoded to localhost:50051. This limits flexibility. It's better to make this configurable via an environment variable, so the client can connect to a server running on a different host or port.

For example:

const GRPC_SERVER_ADDRESS = process.env.GRPC_SERVER_ADDRESS || 'localhost:50051';
const client = new SpannerLib(GRPC_SERVER_ADDRESS, grpc.credentials.createInsecure());

Suggested change

	const client = new SpannerLib('localhost:50051', grpc.credentials.createInsecure());
	const GRPC_SERVER_ADDRESS = process.env.GRPC_SERVER_ADDRESS || 'localhost:50051';
	const client = new SpannerLib(GRPC_SERVER_ADDRESS, grpc.credentials.createInsecure());

[gemini-code-assist]

The catch block is empty, which swallows any errors that might occur when reading test-db.txt. This can make debugging difficult. It's better to log a message indicating that the file was not found and a default is being used.

For example:

} catch (e) {
  console.log('test-db.txt not found, using default. Run `npm run setup` to create a new test database.');
}

Suggested change

	} catch (e) { }
	} catch (e) { console.log("test-db.txt not found, using default. Run `npm run setup` to create a new test database."); }

[gemini-code-assist]

The catch block is empty, which swallows any errors that might occur when reading test-db.txt. This can make debugging difficult. It's better to log a message indicating that the file was not found and a default is being used.

For example:

} catch (e) {
  console.log('test-db.txt not found, using default. Run `npm run setup` to create a new test database.');
}

Suggested change

	} catch (e) { }
	} catch (e) { console.log("test-db.txt not found, using default. Run `npm run setup` to create a new test database."); }