Skip to content

fix: deserialize protobuf longs as numbers (#844) - #845

Open
Tsuyoshi Ushio (TsuyoshiUshio) wants to merge 2 commits into
v3.xfrom
fix/issue-844-protobuf-longs
Open

fix: deserialize protobuf longs as numbers (#844)#845
Tsuyoshi Ushio (TsuyoshiUshio) wants to merge 2 commits into
v3.xfrom
fix/issue-844-protobuf-longs

Conversation

@TsuyoshiUshio

Copy link
Copy Markdown
Contributor

Summary

Explicitly decode protobuf 64-bit integer fields as JavaScript numbers so Worker bundle dependency resolution cannot change the public trigger metadata shape.

Issue

Fixes #844

Changes

  • Set longs: Number in the worker's @grpc/proto-loader options.
  • Add a wire-level regression test covering both scalar TypedData.int and CollectionSInt64 trigger metadata.
  • Verify the fix using the real serializer/deserializer generated by the production GrpcClient path.

Breaking Changes

None. This restores the established JavaScript number behavior from Worker 3.14.1 and earlier.

Testing

  • TDD RED: the new test failed because scalar int decoded to { low, high, unsigned }.
  • TDD GREEN: focused GrpcClient tests pass, 6 passing.
  • npm run build: pass.
  • npm test: pass, 150 passing and 8 pending.
  • npm run lint: pass; existing TypeScript parser compatibility warning only.
  • npm run webpack: pass; production worker bundle compiled successfully.
  • VS Code diagnostics for changed files: no errors.
  • npm audit --audit-level=high: reports the existing dependency baseline (4 high, 1 moderate, 1 low); this PR does not modify dependency manifests or the lockfile.

Self-Review

  • Only src/GrpcClient.ts and test/GrpcClient.test.ts are changed.
  • No generated files, intermediate files, debug statements, credentials, or customer-specific incident data are included.

Fixes #844

Co-authored-by: Dobby <dobby@microsoft.com>
Comment thread src/GrpcClient.ts
objects: true,
defaults: true,
oneofs: true,
longs: Number,

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.

longs: Number is the correct backward-compatible fix, but we should explicitly define all supported proto-loader options instead of relying on defaults.
The protobufjs update made Long available to webpack, changing decoded values from numbers to { low, high, unsigned } objects.
Add wire-level tests for TypedData.int, CollectionSInt64, shared-memory offsets/counts, and timestamps.
Run these tests against the production webpack bundle, since bundling caused the regression.
Document that values above Number.MAX_SAFE_INTEGER require a future string or BigInt contract.

const rpcLoaderOptions: grpcloader.Options = {
    keepCase: false,
    longs: Number,
    defaults: true,
    arrays: true,
    objects: true,
    oneofs: true,
    json: false,
};

const packageDef = grpcloader.fromJSON(
    jsonModule as protobuf.INamespace,
    rpcLoaderOptions
);```

@TsuyoshiUshio

Copy link
Copy Markdown
Contributor Author

Follow-up compatibility audit: protobufjs 7.5.6 to 7.6.0

I reviewed the published package diff and the relevant transitive dependency changes to check for other breaking behavior beyond #844.

Findings

  • Confirmed breaking behavior: Long discovery. protobufjs replaced the bundler-hidden util.inquire("long") lookup with a static require("long"). This made Long visible to webpack and changed the default decoded shape of 64-bit fields. It can also change constructor identity in a Node dependency graph containing multiple Long versions: in an isolated test, 7.5.6 adopted proto-loader's Long 4.x, while 7.6.0 adopted protobufjs's own Long 5.x. Explicit longs: Number in this PR removes that dependency-resolution ambiguity from the worker contract.
  • Additive: BigInt conversion. toObject({ longs: BigInt }) was added. It does not affect the worker unless explicitly selected.
  • Conditional compatibility risk: recursion limits. JSON reflection and .proto parsing now enforce util.recursionLimit (100). The Functions RPC reflection tree has a maximum depth of 3, leaving 97 levels of headroom.
  • Conditional compatibility risk: namespace lookup order. Recursive namespace lookup now stops at the first match. The current Functions RPC descriptor does not produce a behavioral difference.
  • Conditional bundler/browser risk: fs and Buffer discovery. fs now uses a guarded bundler-aware require and package browser mapping; Buffer discovery now uses global.Buffer. The worker passes a generated JSON descriptor to fromJSON and does not use protobufjs file loading, so these paths do not affect it.
  • Type-only change in @protobufjs/fetch. Its exported option type was renamed from FetchOptions to IFetchOptions. The worker does not import this API or type.
  • Transitive Long update. The minimum Long range moved from ^5.0.0 to ^5.3.2, adding BigInt and package-export changes. With longs: Number, these details no longer determine the public decoded value shape.

Production descriptor comparison

I loaded the same generated Functions RPC descriptor in isolated environments pinned to protobufjs 7.5.6 and 7.6.0, using the production proto-loader options, and compared:

  • all package-definition entries and message descriptors
  • service paths and streaming flags
  • representative serializer output

The normalized results were identical: both were 8,048 bytes with SHA-256 0E46B3C68DCEB922D21E49FFF99FBBB4ABEFC460816D015CAE2EA11B97E14607. The descriptor resolved to 53 messages, 3 namespaces, 1 service, and 11 enums. The focused GrpcClient test file also passed (5/5) in the audit worktree.

Conclusion and follow-up

I did not find another protobufjs 7.6.0 change that breaks the current Node.js worker contract. The Long default was the material regression, and this PR fixes it at the correct boundary by making the expected representation explicit.

For future updates, I recommend risk-based compatibility comparison rather than exhaustive comparison for every minor dependency update. Dependencies affecting wire formats, serialization, generated descriptors, public value shapes, or bundling should be compared before and after using the same production inputs and bundle configuration. I opened #846 to track that policy, a dependency-update checklist, future AGENTS.md / architecture.md guidance, and additional contract coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Regression] Event Hubs batch sequenceNumberArray exposed as Long-like objects with Worker 3.16.0

2 participants