From a656a81ee415622f99f94df7c3319265b9f04f3c Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Mon, 14 Aug 2023 16:09:56 -0700 Subject: [PATCH] ui: fix errors thrown by 0.toNumber() 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]: https://github.com/cockroachdb/cockroach/issues/106318 Fixes: #106318 Release note: Observability pages no longer crash when they encounter zeros (e.g. a session with no memory allocated). --- pkg/ui/package.json | 13 +++++++++++++ pkg/ui/pnpm-lock.yaml | 4 +++- .../statementDetails/statementDetails.sagas.spec.ts | 9 +++++---- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/pkg/ui/package.json b/pkg/ui/package.json index 715e4d018a43..7d04b5df4872 100644 --- a/pkg/ui/package.json +++ b/pkg/ui/package.json @@ -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/inquire@1.x": { + "dependencies": { + "long": "4.0.0" + } } }, "overrides": { diff --git a/pkg/ui/pnpm-lock.yaml b/pkg/ui/pnpm-lock.yaml index b177a624fdc1..06603decda85 100644 --- a/pkg/ui/pnpm-lock.yaml +++ b/pkg/ui/pnpm-lock.yaml @@ -39,7 +39,7 @@ overrides: '@cockroachlabs/cluster-ui>antd': 3.26.20 analytics-node>axios-retry: 3.1.9 -packageExtensionsChecksum: c8401d95bcfa63cade05749ae746f3fe +packageExtensionsChecksum: 8b8b370d73a39eaff506edcfedb27e22 patchedDependencies: topojson@3.0.2: @@ -6178,6 +6178,8 @@ packages: /@protobufjs/inquire@1.1.0: resolution: {integrity: sha512-kdSefcPdruJiFMVSbn801t4vFK7KB/5gd2fYvrxhuJYg8ILrmn9SKSX2tZdV6V+ksulWqS7aXjBcRXl3wHoD9Q==} + dependencies: + long: 4.0.0 /@protobufjs/path@1.1.2: resolution: {integrity: sha512-6JOcJ5Tm08dOHAbdR3GrvP+yUUfkjG5ePsHYczMFLq3ZmMkAD98cDgcT2iA1lJ9NVwFd4tH/iSSoe44YWkltEA==} diff --git a/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts b/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts index 7daffae9bfdb..ec18b3b9b6d1 100644 --- a/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts @@ -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: { @@ -674,7 +675,7 @@ describe("SQLDetailsStats sagas", () => { .withReducer(reducer) .hasFinalState({ 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, @@ -700,7 +701,7 @@ describe("SQLDetailsStats sagas", () => { .withReducer(reducer) .hasFinalState({ 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,