Copilot commented on code in PR #3529:
URL: https://github.com/apache/thrift/pull/3529#discussion_r3366031221
##########
lib/nodejs/lib/thrift/browser.js:
##########
@@ -37,6 +37,39 @@ exports.createClient = require("./create_client");
exports.Int64 = require("node-int64");
+const bigIntCompat = require("./bigint_compat");
+
+/**
+ * Convert a `node-int64` Int64 to a native `bigint`. Used by code generated
+ * with `js:bigint=true`. Feature-detects `Buffer#readBigInt64BE` (Node 12+
+ * and modern buffer polyfills) and falls back to a `readInt32BE` /
+ * `readUInt32BE` + BigInt composition when the native method is absent.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return bigIntCompat.readBigInt64BE(i64.buffer, i64.offset || 0);
+};
+
+/**
+ * Convert a native `bigint` to a `node-int64` Int64 for `writeI64`. Values
+ * outside the signed 64-bit range are wrapped (`BigInt.asIntN(64, ...)`).
+ * Also accepts a decimal-string or `number` so generated `map<i64, …>`
+ * serialization works (map keys reach this function as object-key strings).
+ * Uses the same feature-detected fallback as `toBigInt`.
+ *
+ * @param {bigint | string | number} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ const big = typeof value === "bigint" ? value : BigInt(value);
+ bigIntCompat.writeBigInt64BE(buf, big, 0);
+ return new Int64(buf);
+};
Review Comment:
Same issue as the Node entrypoint: `fromBigInt` accepts `number`, but does
`BigInt(value)` without guarding `Number.isSafeInteger(value)`. Unsafe integers
will be rounded before conversion, yielding an incorrect i64 with no error.
##########
lib/nodejs/lib/thrift/index.js:
##########
@@ -57,6 +57,50 @@ exports.createWebServer = web_server.createWebServer;
exports.Int64 = require("node-int64");
+const bigIntCompat = require("./bigint_compat");
+
+/**
+ * Convert a `node-int64` Int64 (as returned by `TBinaryProtocol.readI64`,
+ * `TCompactProtocol.readI64`, etc.) to a native `bigint`. Used by code
+ * generated with `js:bigint=true` to surface int64 values as BigInt without
+ * a protocol-layer toggle.
+ *
+ * Uses `Buffer#readBigInt64BE` when available (Node >= 12), and a
+ * `readInt32BE` / `readUInt32BE` + BigInt fallback otherwise — so the
+ * helpers work across the full `engines: >= 10.18.0` support range.
+ *
+ * @param {Int64} i64
+ * @returns {bigint}
+ */
+exports.toBigInt = function (i64) {
+ return bigIntCompat.readBigInt64BE(i64.buffer, i64.offset || 0);
+};
+
+/**
+ * Convert a native `bigint` to a `node-int64` Int64 suitable for passing to
+ * `writeI64`. Values outside the signed 64-bit range are wrapped to fit
+ * (`BigInt.asIntN(64, ...)`).
+ *
+ * Also accepts a decimal-string or `number` for callers that don't hold the
+ * value as a `bigint` — notably generated code serializing `map<i64, …>`,
+ * where the map key is iterated as the JS object-key string (`for (k in
obj)`).
+ * Strings are parsed via `BigInt(string)` (decimal); numbers must be in the
+ * safe integer range.
+ *
+ * Uses `Buffer#writeBigInt64BE` when available, with a 32-bit-pair fallback
+ * for older runtimes (see `bigint_compat.js`).
+ *
+ * @param {bigint | string | number} value
+ * @returns {Int64}
+ */
+exports.fromBigInt = function (value) {
+ const Int64 = exports.Int64;
+ const buf = Buffer.allocUnsafe(8);
+ const big = typeof value === "bigint" ? value : BigInt(value);
+ bigIntCompat.writeBigInt64BE(buf, big, 0);
+ return new Int64(buf);
+};
Review Comment:
`fromBigInt` claims number inputs must be within the safe integer range, but
it currently does `BigInt(value)` without validating
`Number.isSafeInteger(value)`. Passing an unsafe integer (e.g.
`9007199254740993`) would silently round in JS before conversion, producing the
wrong i64. It’s safer to reject non-safe integers explicitly to avoid silent
data corruption.
##########
compiler/cpp/src/thrift/generate/t_js_generator.cc:
##########
@@ -551,10 +578,14 @@ string t_js_generator::js_includes() {
}
}
if (gen_esm_) {
- result += "import Int64 from 'node-int64';\n";
+ if (!gen_bigint_) {
+ result += "import Int64 from 'node-int64';\n";
+ }
result += "import { v4 as uuid } from 'uuid';";
Review Comment:
When `gen_bigint_` is enabled, the generator emits `thrift.toBigInt(...)` /
`thrift.fromBigInt(...)` in the generated code, but the ESM include path
currently only imports `{ Thrift }` (no `thrift` module namespace/default
import). As a result, `--gen js:node,es6,esm` with the default `bigint=true`
will reference an undefined `thrift` identifier at runtime.
##########
lib/nodejs/README.md:
##########
@@ -65,6 +66,48 @@ client.get_slice("Keyspace", "key", new
ttypes.ColumnParent({column_family: "Exa
Since JavaScript represents all numbers as doubles, int64 values cannot be
accurately represented naturally. To solve this, int64 values in responses will
be wrapped with Thrift.Int64 objects. The Int64 implementation used is
[broofa/node-int64](https://github.com/broofa/node-int64).
Review Comment:
The Int64 section still states that “int64 values in responses will be
wrapped with Thrift.Int64 objects”, but the generator now defaults to
`js:bigint=true`, where generated structs surface i64 fields as native
`bigint`. This sentence should be clarified to distinguish protocol-level
`readI64()` (still `Thrift.Int64`) from generated-code field types (now
`bigint` by default).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
