`QueryBinder.bindIdSet` throws on `undefined`/`null` entries in the id array

[anmolshres98]

Summary

QueryBinder.bindIdSet throws when the id array contains undefined/null entries, e.g. bindIdSet(1, ["0x1", undefined, "0x2"]). the legacy ECSqlStatement.bindIdSet (native binder) silently drops such entries instead, so code migrating from withPreparedStatement/ECSqlStatement to withQueryReader/QueryBinder (see #9463) can crash on inputs that used to work fine.

Repro

const qb = new QueryBinder();
qb.bindIdSet(1, ["0x1", undefined, "0x2"]);

throws:

TypeError: Cannot read properties of undefined (reading 'length')
    at Object.isId64 (core/bentley/.../Id.js)
    at Object.isValidId64 (core/bentley/.../Id.js)
    at compressIds (core/bentley/.../CompressedId64Set.js)
    at compressArray
    at sortAndCompress
    at QueryBinder.bindIdSet (core/common/.../ConcurrentQuery.js)

Same crash reachable via withQueryReader/InVirtualSet using an id array bound through QueryBinder.

Root cause

QueryBinder.bindIdSet (core/common/src/ConcurrentQuery.ts) passes the raw array directly to CompressedId64Set.sortAndCompress, which calls Id64.isValidId64 on every entry. isValidId64 reads .length on the value, which throws for undefined/null instead of returning false.

Verified against the native implementation (iTwin/imodel-native, iModelJsNodeAddon/IModelJsNative.cpp, NativeECSqlBinder::BindIdSet): it uses the REQUIRE_ARGUMENT_STRING_ARRAY macro, which silently skips any non-string array entry before building the native IdSet. That's why the same input works through ECSqlStatement.bindIdSet but crashes through QueryBinder.bindIdSet.

-- created w/ help from AnmolDroid 🤖

[anmolshres98]

question: would this be considered a breaking change? since this would change existing behavior of a public api?

the fix only changes bindIdSet behavior for undefined/null array entries (throws today, would silently skip). signature and .api.md stay the same.

checked for anything relying on the throw:

• no test in this repo asserts a throw for nullish entries
• bindIdSet already silently ignores invalid id strings like "50", so tolerant handling is the existing pattern here
• no docs claim the throw is intentional
• searched the iTwin org for try/catch around bindIdSet, found nothing relying on it

leaning toward: bug fix, not breaking. want a second opinion before the fix PR.

cc: @khanaffan @rschili

[khanaffan]

There are two possible approaches:

1. Return a clear error message explaining the invalid input
   • Pro: If a developer sees the error, they can identify and fix the issue quickly.
   • Con: If the error reaches an end user, the operation simply fails, resulting in a poor user experience.
2. Add validation for each Id64 value. Validate every entry to ensure it is a valid Id64 string. If an entry is invalid, skip it, log a warning, and document the behavior.
   • Pro: Developers can see the log message and correct the issue.
   • Pro: End users are not exposed to runtime errors.
   • Con: Ignoring invalid entries could potentially lead to unexpected or incorrect application behavior.

In my opinion, option 2 is the better approach. Supplying an invalid Id64 is most likely an oversight rather than a critical error, and it should not cause a runtime failure. Gracefully handling the invalid value, logging the issue, and continuing execution provides a better user experience while still giving developers the information they need to diagnose and fix the problem.

[anmolshres98]

fix is up in #9535: pre-filters invalid entries in bindIdSet with a logged warning, per option 2 above.

audited the rest of the surface for the same class of bug. the root cause (Id64.isValidId64 throws on non-string instead of returning false) also affects other id-collection APIs (CompressedId64Set.compress*, PlanarClipMaskSettings.create, and more). split that out to #9537 since fixing it means changing core-bentley primitives. no other QueryBinder method has this legacy-tolerant/modern-throws mismatch (verified empirically against the legacy binder).

-- created w/ help from AnmolDroid 🤖

[anmolshres98]

after discussion in the linked PR, opting for the implementation that was proposed as "option 1" above

after discussion on PR thread itself, decided to switch to option 2 instead