fix: treat a null local key as no related rows across all relation types - #1189
fix: treat a null local key as no related rows across all relation types#1189evoactivity wants to merge 3 commits into
Conversation
belongsTo filters null foreign keys out of the where-in and raises only on undefined. hasMany, hasOne, hasManyThrough and manyToMany raise on both, so one row with a null local key breaks the whole preload. These tests use the existing nullable tenant_id columns, so they need no schema changes. Four of them fail. The belongsTo test and the undefined-key test pass, pinning the behaviour the fix has to match and the diagnostic it must not swallow.
getValue raises for null and undefined alike, so a single row with a null local key made preload throw for the whole collection. The two cases mean different things: undefined means the column was never selected, which is a programmer error, while null means the row has no related rows, which is ordinary. Adds getNullableValue, which raises only on undefined, and uses it in hasMany, hasOne, hasManyThrough and manyToMany. Null keys are filtered out of the where-in for the eager branch, and the single-parent branch short-circuits to an empty where-in. belongsTo already did exactly this with a nullable foreign key, so this brings the other relation types in line rather than introducing a new behaviour.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNullable local keys now return empty related results for ChangesNullable relationship local keys
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ParentModel
participant RelationQueryBuilder
participant RelatedQuery
ParentModel->>RelationQueryBuilder: resolve local key
RelationQueryBuilder->>RelationQueryBuilder: preserve null or raise for undefined
RelationQueryBuilder->>RelatedQuery: apply whereIn with keys or an empty list
RelatedQuery-->>ParentModel: return related results
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/orm/nullable_local_key.spec.ts (1)
256-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the "unselected local key still raises" test to the other relation types.
This regression test only covers
hasMany.hasOne,hasManyThrough, andmanyToManyroute through the samegetNullableValuehelper but through separate query builders. Add the same "unselected column still raises" assertion for those three relation types to directly confirm each call site preserves the existing error path for undefined keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/orm/nullable_local_key.spec.ts` around lines 256 - 281, Extend the “an unselected local key still raises” coverage beyond the existing HasMany relation by adding equivalent assertions for HasOne, HasManyThrough, and ManyToMany. Define the corresponding relations in the test setup and verify that selecting only the primary key before preloading each relation rejects with the same undefined local-key error, exercising each relation query builder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/orm/nullable_local_key.spec.ts`:
- Around line 256-281: Extend the “an unselected local key still raises”
coverage beyond the existing HasMany relation by adding equivalent assertions
for HasOne, HasManyThrough, and ManyToMany. Define the corresponding relations
in the test setup and verify that selecting only the primary key before
preloading each relation rejects with the same undefined local-key error,
exercising each relation query builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8efe1529-a3c9-4baf-9f04-ab3fc37b27c4
📒 Files selected for processing (6)
src/orm/relations/has_many/query_builder.tssrc/orm/relations/has_many_through/query_builder.tssrc/orm/relations/has_one/query_builder.tssrc/orm/relations/many_to_many/query_builder.tssrc/utils/index.tstest/orm/nullable_local_key.spec.ts
Each relation type routes through its own query builder, so the shared helper covering one of them does not prove the other three keep the error path. Asserts all four rather than assuming.
|
Coderabbit review addressed in e80604c. The undefined-key regression test now asserts all four relation types rather than just Also added the missing docstring on the test's edit: maybe the docstring coverage check is pre-existing? Didn't make a difference adding it, it's in a test anyway so was a bit of a guess where I needed to add it. |
1 participant
🔗 Linked issue
Resolves #1188
❓ Type of change
📚 Description
getValueraises fornullandundefinedalike, so one row with a null local key made preload throw for the entire collection.undefinedmeans the column was never selected, which is a programmer error, whilenullmeans the row simply has no related rows.Adds
getNullableValue, which raises only onundefined, and uses it inhasMany,hasOne,hasManyThroughandmanyToMany. Null keys are filtered out of thewhereInon the eager branch, and the single-parent branch short-circuits to an emptywhereIn.belongsToalready behaved this way with a nullable foreign key, so this brings the other four in line rather than introducing new behaviour.Two commits, so the bug is verifiable independently of the fix. Checking out the first alone gives four failing tests:
The
belongsTotest and the undefined-key test pass either way, pinning the behaviour being matched and the diagnostic that must not be swallowed. The tests use the nullabletenant_idcolumns already present intest-helpers, so there are no schema changes.📝 Checklist
No documentation change: this aligns four relation types with the behaviour
belongsToalready exhibits.Summary by CodeRabbit
Bug Fixes
Tests