Skip to content

Commit

Permalink
ui: fix errors thrown by 0.toNumber()
Browse files Browse the repository at this point in the history
When generating JS protobuf bindings, protobufjs's `pbjs` CLI accepts a
--strict-long (or the more modern --force-long) flag with the following
description:

  > Enfores the use of 'Long' for s-/u-/int64 and s-/fixed64 fields.

This uses the `long` package on NPM[^1], which it expects to be present
when one of those clients is used at runtime. If `long` isn't
available when `pbjs` is executed, it falls back to generating
number-based fields where possible. Long is available in all consumers
of `crdb-protobuf-client`, and `protobufjs` declares `long` as one
of its few build-time and run-time dependencies, so nothing looked
alarming.

For each field with a type that might be represented by a Long (a
heuristic influenced by --strict-long), that field is marked as "long"
if and only if the protobufjs-internal util.Long is non-null[^2]. That
util.Long value is set via the use of the `@protobufjs/inquire` package
instead of a standard `require`[^3] though, since the former silently
returns `null` instead of throwing an error[^4]. This, again, is not
alarming on its own because `protobufjs` depends directly on `long`.

The complication comes in with rules_js' use of the pnpm visibility
rules, which are very strict when it comes to JS package dependencies.
Put simply: packages only have access to the dependencies they directly
declare, not transitive dependencies ("phantom dependencies")[^5]. Since
`@protobufjs/inquire` is a separate package with no direct dependencies
of its own[^6], Bazel lays out a node_modules tree where
`@protobufjs/inquire` has no access to the `long` package. Since
inaccessible packages are indistinguishable from nonexistent packages
and `inquire` is intended to be silent, it returns `null`. The `pbjs`
CLI then assumes Long isn't installed and falls back to number-based
fields where a Long isn't strictly required.

When combined with protobuf's zero-value, this caused several cases of
'0.toNumber is not a function' JS errors at runtime[^7]. We requested a
Long, we support Longs, but a client was silently generated without
respecting --strict-long.

Like with most other phantom dependencies, use pnpm.packageOverrides to
declare that @protobufjs/inquire does in fact depend on Long.

[^1]: https://www.npmjs.com/package/long
[^2]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/src/field.js#L153-L157
[^3]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/src/util/minimal.js#L163-L167
[^4]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/lib/inquire/index.js#L4-L17
[^5]: https://docs.aspect.build/rules/aspect_rules_js/docs/pnpm/#hoisting
[^6]: https://github.com/protobufjs/protobuf.js/blob/918ff014efe19f3eb43195ae3d71f7aeb3fcdd73/lib/inquire/package.json
[^7]: cockroachdb#106318

Fixes: cockroachdb#106318
Release note: Observability pages no longer crash when they encounter
zeros (e.g. a session with no memory allocated).
  • Loading branch information
sjbarag committed Aug 15, 2023
1 parent dc2c52d commit a656a81
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 5 deletions.
13 changes: 13 additions & 0 deletions pkg/ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,19 @@
"tmp": "0.0.33",
"uglify-js": "^2.8.29"
}
},
"// @protobufjs/inquire hacks": [
"@protobufjs/inquire (used by protobufjs@6) uses eval() to require",
"packages while hiding them from bundlers. Since inquire is a separate",
"package from protobufjs and doesn't declare all of protobufjs'",
"direct dependencies, any call to inquire() for packages not provided",
"by the node stdlib is a 'phantom' dependency. Currently only 'long'",
"fits into this category."
],
"@protobufjs/[email protected]": {
"dependencies": {
"long": "4.0.0"
}
}
},
"overrides": {
Expand Down
4 changes: 3 additions & 1 deletion pkg/ui/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,10 @@ describe("SQLDetailsStats sagas", () => {
}),
type: "request",
};
// Don't include /start and /end, since they're not included in the above payload.
// /start and /end aren't included in the above payload, but the default value
// when missing (0 for both) appears anyway.
const key =
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname";
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname/0/0";
const SQLDetailsStatsResponse =
new cockroach.server.serverpb.StatementDetailsResponse({
statement: {
Expand Down Expand Up @@ -674,7 +675,7 @@ describe("SQLDetailsStats sagas", () => {
.withReducer(reducer)
.hasFinalState<SQLDetailsStatsReducerState>({
cachedData: {
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname":
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname/0/0":
{
data: SQLDetailsStatsResponse,
lastError: null,
Expand All @@ -700,7 +701,7 @@ describe("SQLDetailsStats sagas", () => {
.withReducer(reducer)
.hasFinalState<SQLDetailsStatsReducerState>({
cachedData: {
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname":
"SELECT * FROM crdb_internal.node_build_info/$ cockroach sql,newname/0/0":
{
data: null,
lastError: error,
Expand Down

0 comments on commit a656a81

Please sign in to comment.