fix: QueryBinder.bindIdSet skips invalid id entries instead of throwing

[anmolshres98]

Closes #9530

bindIdSet used to throw an uncaught TypeError on undefined/null (or otherwise invalid) entries in the id array, e.g. when collecting ids from a nullable column via ECSqlReader. An earlier version of this PR matched the legacy ECSqlStatement.bindIdSet behavior by silently skipping invalid entries with a logged warning.

Per discussion on this PR (see comments below), silently dropping entries hides real bugs in consumer code rather than surfacing them, so this now throws instead. The throw is a descriptive ITwinError (scope "itwin-QueryBinder", key "invalid-arguments").

Also removed a stray no-op uniqueIterator call.

[Copilot]

Pull request overview

This PR updates QueryBinder.bindIdSet in core-common to match legacy native binder behavior by tolerating nullish/invalid entries in bound id sets, preventing TypeError crashes during compression and improving migration compatibility for callers moving from ECSqlStatement to QueryBinder.

Changes:

• Filter bindIdSet inputs to skip non-Id64String / invalid id entries and log a warning with the skipped count.
• Add unit tests to confirm invalid/nullish entries are skipped and that ids are sorted/deduplicated.
• Add a Rush change file documenting the behavioral fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
core/common/src/ConcurrentQuery.ts	Filters invalid id entries before compression and emits a warning when entries are skipped.
core/common/src/test/ConcurrentQuery.test.ts	Adds regression tests for skipping invalid entries and for sort/dedupe behavior.
common/changes/@itwin/core-common/issue-9530-bindidset-null_2026-07-22-19-25.json	Records the change as a non-breaking fix in core-common.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

core/common/src/ConcurrentQuery.ts:504

• CompressedId64Set.sortAndCompress(ids) will allocate a second array via Array.from(ids) even though ids is already a mutable array. Since this method already materializes an array, you can avoid the extra allocation by sorting in-place and calling CompressedId64Set.compressArray(ids).

    Object.defineProperty(this._args, name, {
      enumerable: true, value: {
        type: QueryParamType.IdSet,
        value: CompressedId64Set.sortAndCompress(ids),
      },

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[grigasp]

Similar to what @pmconne wrote in #9537 (comment) - I don't get how this is a problem. We're using typescript, not javascript.

Update: invalid Id64String like "" or "0" are maybe worth doing something about. But, instead of silently skipping them with a warning that nobody will see, I think we should throw.

[anmolshres98]

I think the "we're using typescript" framing assumes the compiler is in the loop, and for id arrays flowing into bindIdSet it often isn't. ECSqlReader's own row types erase it: QueryRowProxy's index signature is [prop: string]: any and toArray() returns any[]. any assigns to Id64String[] under full strict mode with zero casts. So a fairly common pattern, collect ids from one query and bind them into the next, can pass type checking without the compiler ever verifying the ids:

// compiles clean under strict: true, no casts, no `!`, no `any` written by the caller
const reader = iModel.createQueryReader("SELECT Parent.Id AS parentId FROM bis.Element");
const parentIds: Id64String[] = [];
for await (const row of reader)
  parentIds.push(row.parentId); // row.parentId is `any` (QueryRowProxy), so this type-checks

// one root element with NULL Parent.Id -> parentIds contains `undefined`
new QueryBinder().bindIdSet(1, parentIds); // TypeError before this fix

I verified this against a real iModel: a NULL id column comes back from the reader as undefined, lands in a Id64String[] that compiles clean, and crashed bindIdSet before this fix. Some existing code that looks like variations of this shape:

• imodel-transformer's markParentModelsAsUpdated collects row.parentId from a recursive CTE over ParentModel.Id into a Set<Id64String> that later feeds back into bindIdSet. As far as I can tell, what prevents a crash there is the hand-written where parentId is not null in the SQL string, not the types.
• Core's own ViewStore.ts does JSON.parse(row.json) as { settings: DisplayStyle3dSettingsProps ... }, one cast at a persistence boundary. DisplayStyleSettings then feeds the persisted excludedElements array into CompressedId64Set.compressIds. That json was written by whatever connector or app version stored it, so the type describes what was expected, not necessarily what's there.
• saved-views has a similar chain: callITwinApi returns response.json() as unknown, displayStyleExtractor.ts maps the service json into DisplayStyleSettingsProps, and createViewState.ts feeds it into SpatialViewState.createFromProps, reaching the same compressIds sink. Interestingly it doesn't rely on its own excludedElements?: string[] type either: extractStringOrArray runtime-checks value.every((inner) => typeof inner === "string") on every entry.
• There's also precedent in core for guarding beyond the types: RenderSchedule.ts guards addElementTimeline with the comment "It's far too easy to accidentally pass a single Id (compiler can't help)", and CompressedId64Set.sortAndCompress has the same runtime string guard.
• OS+ appears to follow the same pattern in its query helpers: id sets it binds originate from earlier query results, including nullable Parent.Id columns.

On why this hasn't surfaced before: my best guess is it was masked. The legacy native ECSqlStatement.bindIdSet silently drops non-string entries, so a call site with a latent nullish id would have worked without anyone noticing. With ECSqlStatement deprecated and code moving to QueryBinder (core's own migration in #9463, apps following), those call sites can start crashing as they migrate, which after speaking to @diegoalexdiaz, is similar to how he ran into this on OS+ and I ended up filing #9530

On throw vs skip: throwing was option 1 in #9530. @khanaffan recommended skip+warn (option 2) for parity with legacy bindIdSet and I do agree with Affan's assessment. If a user is doing a one to one migration, having to deal with a new unexpected throw when the parallel API didn't throw seems more painful imo.

-- created w/ help from AnmolDroid 🤖

[saskliutas]

// compiles clean under strict: true, no casts, no `!`, no `any` written by the caller
const reader = iModel.createQueryReader("SELECT Parent.Id AS parentId FROM bis.Element");
const parentIds: Id64String[] = [];
for await (const row of reader)
  parentIds.push(row.parentId); // row.parentId is `any` (QueryRowProxy), so this type-checks

// one root element with NULL Parent.Id -> parentIds contains `undefined`
new QueryBinder().bindIdSet(1, parentIds); // TypeError before this fix

In my opinion this is a problem in consumer code and not in QueryBinder. Result of previous query should be validated before using it in other APIs. I understand that previous API was kind of doing that for consumer and everything worked. However, this already existing API that is consumed by others without any problems.

I understand that the goal of this PR is to make migration from deprecated ECSqlStatement.bindIdSet just a simple rename but as this is different API it's an opportunity to fix issues in consumer code. However, I doubt that logging warning will achieve that.

Ultimately I think the problem is with QueryRowProxy. It should be typed as [prop: string]: unknown and toArray(): unknown[] instead of any. This would encourage consumers to validated results before using them further.

EDIT: if we decide that QueryBinder.bindIdSet should support undefined and other non string values we should considering updating API signature from OrderedId64Iterable to iterable<unknown> or iterable<Id64 | undefined>

[anmolshres98]

After reading @grigasp and @saskliutas points, I would say I am swayed and agree with them. It is true that I opted to skip to mirror the deprecated api. It is however, imo, also true that that behavior masks bad data by the consumer. Like @saskliutas said, migration to a different api is the perfect time to force the consumers to also clean up their calls.

I have pushed updates so that now on invalid data, an iTwinError is thrown. This shouldn't break existing consumers either (I think) because today they would already encounter an error in such case too (it would just be a TypeError with an unhelpful stack trace). resetting all approvals so far since the implementation has drastically changed.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mergify]

This pull request is now in conflicts. Could you fix it @anmolshres98? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/