Skip to content

fix(repository): dedupe/filter source ids before querying in hasManyThrough, hasMany, and hasOne inclusion resolvers - #11784

Open
karthikchundi-commits wants to merge 2 commits into
loopbackio:masterfrom
karthikchundi-commits:fix/has-many-through-source-id-mutation
Open

karthikchundi-commits wants to merge 2 commits into
loopbackio:masterfrom
karthikchundi-commits:fix/has-many-through-source-id-mutation

Conversation

@karthikchundi-commits

@karthikchundi-commits karthikchundi-commits commented Sep 15, 2026

Copy link
Copy Markdown

This PR was originally scoped to hasManyThrough only. It now also absorbs the hasMany/hasOne fix from #11764, which was closed in favor of this PR but never actually landed here — consolidating both so the fix isn't lost.

The bug

hasManyThrough, hasMany, and hasOne's inclusion resolvers each passed their raw, unfiltered sourceIds array by reference straight into findByForeignKeys(), which wraps it in an {inq: [...]} where clause and hands that same array to the connector without cloning it first. A connector/query layer that sanitizes an inq array in place (e.g. stripping falsy values before running the query, as the in-memory connector does) then mutates that exact array out from under the caller — shrinking the very sourceIds array each resolver still needs, unmodified, to correctly zip results back onto each original entity. That silently misaligns or truncates the returned array whenever any source entity's key was undefined (e.g. excluded by a fields filter) or duplicated another entity's.

belongsTo and referencesMany inclusion resolvers already pass a fresh, deduplicated, filtered array (never the original reference) before querying. hasManyThrough had no dedup/filter attempt at all; hasMany/hasOne didn't either.

Testing

Added a regression test to each of has-many-through-inclusion-resolver.acceptance.ts, has-many-inclusion-resolver.relation.acceptance.ts, and has-one.inclusion-resolver.acceptance.ts: a duplicate source entity plus one with an undefined key, confirming the result stays length-3 and aligned with the input. Verified each test actually catches the bug by temporarily reverting the corresponding source fix and re-running — reproduces the exact truncation (length 2 instead of 3) this fix addresses.

cd packages/repository && npm run build
cd packages/repository-tests && npm run build
npx mocha "dist/__tests__/**/*.js"

128 passing, 8 pending (pre-existing, unrelated to this change), no failures.

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide — eslint and prettier both clean on all changed files

@aaqilniz

Copy link
Copy Markdown
Contributor

Hi, @karthikchundi-commits. Thanks for the PR. Everything seems good. Just one small piece of feedback I'd like to give is that the comments in the code are too descriptive. Can we make them concise and leave the detailed explanation here in the PR description?

…hrough inclusion resolver

hasManyThrough's inclusion resolver passed its raw, unfiltered
sourceIds array by reference straight into findByForeignKeys(), which
wraps it in an {inq: [...]} where clause and hands that same array to
the connector without cloning it first. A connector/query layer that
sanitizes an inq array in place (e.g. stripping falsy values before
running the query, as the in-memory connector does) then mutates that
exact array out from under the caller - shrinking the very sourceIds
array the resolver still needs below, unmodified, to correctly zip
through-results back onto each original entity via
flattenTargetsOfOneToManyRelation(). That silently misaligns or
truncates the returned array whenever any source entity's key was
undefined (e.g. excluded by a fields filter) or duplicated another
entity's.

belongsTo and referencesMany inclusion resolvers already pass a fresh,
deduplicated, filtered array (never the original reference) before
querying; hasManyThrough had no dedup/filter attempt at all.

Added a regression test to has-many-through-inclusion-resolver.acceptance.ts:
a duplicate source entity plus one with an undefined key, confirming
the result stays length-3 and aligned with the input. Verified the
test actually catches the bug by temporarily reverting the source fix
and re-running - reproduces the exact truncation (length 2 instead of
3) this fix addresses.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Karth <karthik.chundi@gmail.com>
…and hasOne inclusion resolvers

Both resolvers had the same aliasing bug just fixed in hasManyThrough:
they passed their raw, unfiltered source-id array by reference
straight to findByForeignKeys(), which wraps it in an {inq: [...]}
where clause and hands that same array on to the connector. A
connector/query layer that sanitizes an inq array in place (e.g.
stripping falsy values before running the query, as the in-memory
connector does) then mutates that shared array out from under the
caller - shrinking the very array each resolver still needed,
unmodified, to zip results back onto each original entity. That
silently truncated and misaligned the resolver's return value whenever
any source entity's key was undefined (e.g. excluded by a fields
filter) or duplicated another entity's.

belongsTo and referencesMany already pass a fresh, deduplicated,
filtered array (never the original reference) before querying;
hasMany and hasOne now do the same.

Confirmed via a standalone script against the real compiled resolvers
and an in-memory DataSource: reverting just this fix reproduces the
truncated/misaligned result (length 2 instead of 3 for a 3-entity
batch with one undefined key); with the fix, the result is correctly
length 3 with undefined in the right position.

Includes a test fix for the same regression tests: toJSON() serializes
an undefined array element to null (JSON has no undefined), so the
memory-connector acceptance run must assert null, not undefined, for
that slot.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Karth <karthik.chundi@gmail.com>
@karthikchundi-commits
karthikchundi-commits force-pushed the fix/has-many-through-source-id-mutation branch from dcc05c8 to b579d30 Compare September 22, 2026 17:57
@karthikchundi-commits karthikchundi-commits changed the title fix(repository): dedupe/filter source ids before querying in hasManyThrough inclusion resolver fix(repository): dedupe/filter source ids before querying in hasManyThrough, hasMany, and hasOne inclusion resolvers Sep 22, 2026
@karthikchundi-commits

Copy link
Copy Markdown
Author

@dhmlau Two things:

  1. Trimmed the inline comments to a couple of lines each, per your feedback — the detailed rationale now lives only in the PR description and commit messages.
  2. Folded in the hasMany/hasOne fix from fix(repository): dedupe/filter source ids before querying in hasMany and hasOne inclusion resolvers #11764. That PR's CI was actually green by the time it was closed (the one real failure was fixed by a follow-up commit), but it never got merged here as expected — its diff only ever touched hasManyThrough. Consolidated both into this PR now so the hasMany/hasOne fix isn't lost. Title and description updated to reflect the full scope; all three resolvers now have matching regression tests, 128 passing / 8 pending (pre-existing) locally.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants