From 3a7abc2d1cecef016c6da66f1f439cab68692b2f Mon Sep 17 00:00:00 2001 From: Liam Potter Date: Sun, 2 Aug 2026 15:11:56 +0100 Subject: [PATCH 1/3] test: nullable local key raises on preload for all but belongsTo 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. --- test/orm/nullable_local_key.spec.ts | 282 ++++++++++++++++++++++++++++ 1 file changed, 282 insertions(+) create mode 100644 test/orm/nullable_local_key.spec.ts diff --git a/test/orm/nullable_local_key.spec.ts b/test/orm/nullable_local_key.spec.ts new file mode 100644 index 00000000..e49e23b5 --- /dev/null +++ b/test/orm/nullable_local_key.spec.ts @@ -0,0 +1,282 @@ +/* + * @adonisjs/lucid + * + * (c) Harminder Virk + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +import { test } from '@japa/runner' +import type { + BelongsTo, + HasMany, + HasManyThrough, + HasOne, + ManyToMany, +} from '../../src/types/relations.js' + +import { + belongsTo, + column, + hasMany, + hasManyThrough, + hasOne, + manyToMany, +} from '../../src/orm/decorators/index.js' + +import { + setup, + getDb, + cleanup, + ormAdapter, + resetTables, + getBaseModel, +} from '../../test-helpers/index.js' +import { AppFactory } from '@adonisjs/core/factories/app' + +/** + * A nullable local key is a legitimate value: the row simply has no related + * rows. Only an undefined key is a programmer error, where the column was + * never selected. + * + * belongsTo already draws that distinction. These tests assert the other + * relation types behave the same way. + */ +test.group('Nullable local key', (group) => { + group.setup(async () => { + await setup() + }) + + group.teardown(async () => { + await cleanup() + }) + + group.each.teardown(async () => { + await resetTables() + }) + + async function boot(fs: any) { + const app = new AppFactory().create(fs.baseUrl, () => {}) + await app.init() + const db = getDb() + return { db, BaseModel: getBaseModel(ormAdapter(db)) } + } + + /** + * Two users, one with a tenant and one without, and a post belonging to + * the first user's tenant. + */ + async function seed(db: any) { + await db + .insertQuery() + .table('users') + .insert([ + { username: 'virk', tenant_id: 1 }, + { username: 'nikk', tenant_id: null }, + ]) + + await db + .insertQuery() + .table('posts') + .insert([{ title: 'Adonis 101', tenant_id: 1, user_id: 1 }]) + } + + test('belongsTo tolerates a null foreign key (existing behaviour)', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Tenant extends BaseModel { + static table = 'posts' + + @column({ isPrimary: true }) + declare id: number + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @belongsTo(() => Tenant, { foreignKey: 'tenantId' }) + declare tenant: BelongsTo + } + + await seed(db) + + const users = await User.query().orderBy('id', 'asc').preload('tenant') + assert.lengthOf(users, 2) + assert.isNull(users[1].tenant) + }) + + test('hasMany tolerates a null local key', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Post extends BaseModel { + @column() + declare tenantId: number | null + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @hasMany(() => Post, { foreignKey: 'tenantId', localKey: 'tenantId' }) + declare posts: HasMany + } + + await seed(db) + + const users = await User.query().orderBy('id', 'asc').preload('posts') + assert.lengthOf(users, 2) + assert.lengthOf(users[0].posts, 1) + assert.lengthOf(users[1].posts, 0) + }) + + test('hasOne tolerates a null local key', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Post extends BaseModel { + @column() + declare tenantId: number | null + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @hasOne(() => Post, { foreignKey: 'tenantId', localKey: 'tenantId' }) + declare post: HasOne + } + + await seed(db) + + const users = await User.query().orderBy('id', 'asc').preload('post') + assert.lengthOf(users, 2) + assert.isNotNull(users[0].post) + assert.isNull(users[1].post) + }) + + test('manyToMany tolerates a null local key', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Skill extends BaseModel { + @column({ isPrimary: true }) + declare id: number + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @manyToMany(() => Skill, { + localKey: 'tenantId', + pivotForeignKey: 'user_id', + pivotTable: 'skill_user', + }) + declare skills: ManyToMany + } + + await seed(db) + await db + .insertQuery() + .table('skills') + .insert([{ name: 'Programming' }]) + await db + .insertQuery() + .table('skill_user') + .insert([{ user_id: 1, skill_id: 1 }]) + + const users = await User.query().orderBy('id', 'asc').preload('skills') + assert.lengthOf(users, 2) + assert.lengthOf(users[0].skills, 1) + assert.lengthOf(users[1].skills, 0) + }) + + test('hasManyThrough tolerates a null local key', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Post extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare userId: number + + @column() + declare tenantId: number | null + } + + class Comment extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare postId: number + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @hasManyThrough([() => Comment, () => Post], { + localKey: 'tenantId', + foreignKey: 'tenantId', + throughLocalKey: 'id', + throughForeignKey: 'postId', + }) + declare comments: HasManyThrough + } + + await seed(db) + await db + .insertQuery() + .table('comments') + .insert([{ post_id: 1, body: 'nice' }]) + + const users = await User.query().orderBy('id', 'asc').preload('comments') + assert.lengthOf(users, 2) + assert.lengthOf(users[0].comments, 1) + assert.lengthOf(users[1].comments, 0) + }) + + test('an unselected local key still raises', async ({ fs, assert }) => { + const { db, BaseModel } = await boot(fs) + + class Post extends BaseModel { + @column() + declare tenantId: number | null + } + + class User extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare tenantId: number | null + + @hasMany(() => Post, { foreignKey: 'tenantId', localKey: 'tenantId' }) + declare posts: HasMany + } + + await seed(db) + + await assert.rejects( + () => User.query().select('id').preload('posts'), + 'Cannot preload "posts", value of "User.tenantId" is undefined' + ) + }) +}) From d9e7d98e8852152424785d3aa5abffd5b3e7daa9 Mon Sep 17 00:00:00 2001 From: Liam Potter Date: Sun, 2 Aug 2026 15:20:35 +0100 Subject: [PATCH 2/3] fix: treat a null local key as no related rows, not an error 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. --- src/orm/relations/has_many/query_builder.ts | 17 ++++++++++---- .../has_many_through/query_builder.ts | 17 ++++++++++---- src/orm/relations/has_one/query_builder.ts | 18 ++++++++++----- .../relations/many_to_many/query_builder.ts | 17 ++++++++++---- src/utils/index.ts | 23 +++++++++++++++++++ 5 files changed, 71 insertions(+), 21 deletions(-) diff --git a/src/orm/relations/has_many/query_builder.ts b/src/orm/relations/has_many/query_builder.ts index 5057a89e..eff44344 100644 --- a/src/orm/relations/has_many/query_builder.ts +++ b/src/orm/relations/has_many/query_builder.ts @@ -13,7 +13,7 @@ import { type LucidRow, type LucidModel } from '../../../types/model.js' import { type HasManyQueryBuilderContract } from '../../../types/relations.js' import { type HasMany } from './index.js' -import { getValue, unique } from '../../../utils/index.js' +import { getNullableValue, unique } from '../../../utils/index.js' import { BaseQueryBuilder } from '../base/query_builder.js' /** @@ -101,9 +101,11 @@ export class HasManyQueryBuilder this.wrapExisting().whereIn( this.relation.foreignKey, unique( - this.parent.map((model) => { - return getValue(model, this.relation.localKey, this.relation, queryAction) - }) + this.parent + .map((model) => { + return getNullableValue(model, this.relation.localKey, this.relation, queryAction) + }) + .filter((value) => value !== null) ) ) return @@ -112,7 +114,12 @@ export class HasManyQueryBuilder /** * Query constraints */ - const value = getValue(this.parent, this.relation.localKey, this.relation, queryAction) + const value = getNullableValue(this.parent, this.relation.localKey, this.relation, queryAction) + if (value === null) { + this.wrapExisting().whereIn(this.relation.foreignKey, []) + return + } + this.wrapExisting().where(this.relation.foreignKey, value) } diff --git a/src/orm/relations/has_many_through/query_builder.ts b/src/orm/relations/has_many_through/query_builder.ts index f97dcd44..66faf013 100644 --- a/src/orm/relations/has_many_through/query_builder.ts +++ b/src/orm/relations/has_many_through/query_builder.ts @@ -13,7 +13,7 @@ import { type LucidRow, type LucidModel } from '../../../types/model.js' import { type HasManyThroughQueryBuilderContract } from '../../../types/relations.js' import { type HasManyThrough } from './index.js' -import { getValue, unique } from '../../../utils/index.js' +import { getNullableValue, unique } from '../../../utils/index.js' import { BaseQueryBuilder } from '../base/query_builder.js' /** @@ -81,9 +81,11 @@ export class HasManyThroughQueryBuilder builder.whereIn( this.prefixThroughTable(this.relation.foreignKeyColumnName), unique( - this.parent.map((model) => { - return getValue(model, this.relation.localKey, this.relation, queryAction) - }) + this.parent + .map((model) => { + return getNullableValue(model, this.relation.localKey, this.relation, queryAction) + }) + .filter((value) => value !== null) ) ) return @@ -92,7 +94,12 @@ export class HasManyThroughQueryBuilder /** * Query constraints */ - const value = getValue(this.parent, this.relation.localKey, this.relation, queryAction) + const value = getNullableValue(this.parent, this.relation.localKey, this.relation, queryAction) + if (value === null) { + builder.whereIn(this.prefixThroughTable(this.relation.foreignKeyColumnName), []) + return + } + builder.where(this.prefixThroughTable(this.relation.foreignKeyColumnName), value) } diff --git a/src/orm/relations/has_one/query_builder.ts b/src/orm/relations/has_one/query_builder.ts index f3e628be..59ba2b57 100644 --- a/src/orm/relations/has_one/query_builder.ts +++ b/src/orm/relations/has_one/query_builder.ts @@ -12,7 +12,7 @@ import { type LucidRow } from '../../../types/model.js' import { type QueryClientContract } from '../../../types/database.js' import { type HasOne } from './index.js' -import { getValue, unique } from '../../../utils/index.js' +import { getNullableValue, unique } from '../../../utils/index.js' import { BaseQueryBuilder } from '../base/query_builder.js' /** @@ -96,9 +96,11 @@ export class HasOneQueryBuilder extends BaseQueryBuilder { this.wrapExisting().whereIn( this.relation.foreignKey, unique( - this.parent.map((model) => { - return getValue(model, this.relation.localKey, this.relation, queryAction) - }) + this.parent + .map((model) => { + return getNullableValue(model, this.relation.localKey, this.relation, queryAction) + }) + .filter((value) => value !== null) ) ) return @@ -107,8 +109,12 @@ export class HasOneQueryBuilder extends BaseQueryBuilder { /** * Query constraints */ - const value = getValue(this.parent, this.relation.localKey, this.relation, queryAction) - this.wrapExisting().where(this.relation.foreignKey, value) + const value = getNullableValue(this.parent, this.relation.localKey, this.relation, queryAction) + if (value === null) { + this.wrapExisting().whereIn(this.relation.foreignKey, []) + } else { + this.wrapExisting().where(this.relation.foreignKey, value) + } /** * Do not add limit when updating or deleting diff --git a/src/orm/relations/many_to_many/query_builder.ts b/src/orm/relations/many_to_many/query_builder.ts index 522661b5..7b3f2ecc 100644 --- a/src/orm/relations/many_to_many/query_builder.ts +++ b/src/orm/relations/many_to_many/query_builder.ts @@ -15,7 +15,7 @@ import { type ManyToManyQueryBuilderContract } from '../../../types/relations.js import { type ManyToMany } from './index.js' import { PivotHelpers } from './pivot_helpers.js' -import { getValue, unique } from '../../../utils/index.js' +import { getNullableValue, unique } from '../../../utils/index.js' import { BaseQueryBuilder } from '../base/query_builder.js' /** @@ -115,9 +115,11 @@ export class ManyToManyQueryBuilder this.wrapExisting().whereInPivot( this.relation.pivotForeignKey, unique( - this.parent.map((model) => { - return getValue(model, this.relation.localKey, this.relation, queryAction) - }) + this.parent + .map((model) => { + return getNullableValue(model, this.relation.localKey, this.relation, queryAction) + }) + .filter((value) => value !== null) ) ) return @@ -126,7 +128,12 @@ export class ManyToManyQueryBuilder /** * Query constraints */ - const value = getValue(this.parent, this.relation.localKey, this.relation, queryAction) + const value = getNullableValue(this.parent, this.relation.localKey, this.relation, queryAction) + if (value === null) { + this.wrapExisting().whereInPivot(this.relation.pivotForeignKey, []) + return + } + this.wrapExisting().wherePivot(this.relation.pivotForeignKey, value) } diff --git a/src/utils/index.ts b/src/utils/index.ts index e57d5c36..8062eefa 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -124,6 +124,29 @@ export function getValue( }) } +/** + * Same as "getValue", but returns null instead of raising when the key + * value is null. + * + * A null key is a legitimate value: the row simply has no related rows. + * Only undefined is a programmer error, where the column was never + * selected. This mirrors how belongsTo already treats a nullable foreign + * key. + */ +export function getNullableValue( + model: LucidRow | ModelObject, + key: string, + relation: RelationshipsContract, + action = 'preload' +) { + const value = (model as ModelObject)[key] + if (value === undefined) { + return getValue(model, key, relation, action) + } + + return value +} + /** * Helper to find if value is a valid Object or * not From e80604cebd43c586afabb339cf27447098326036 Mon Sep 17 00:00:00 2001 From: Liam Potter Date: Sun, 2 Aug 2026 15:46:06 +0100 Subject: [PATCH 3/3] test: assert the undefined key still raises for every relation type 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. --- test/orm/nullable_local_key.spec.ts | 53 ++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/test/orm/nullable_local_key.spec.ts b/test/orm/nullable_local_key.spec.ts index e49e23b5..4ff6a5d8 100644 --- a/test/orm/nullable_local_key.spec.ts +++ b/test/orm/nullable_local_key.spec.ts @@ -56,6 +56,10 @@ test.group('Nullable local key', (group) => { await resetTables() }) + /** + * Boots an app and returns the db alongside a fresh base model, so each + * test can declare its own models against the same connection. + */ async function boot(fs: any) { const app = new AppFactory().create(fs.baseUrl, () => {}) await app.init() @@ -257,10 +261,26 @@ test.group('Nullable local key', (group) => { const { db, BaseModel } = await boot(fs) class Post extends BaseModel { + @column({ isPrimary: true }) + declare id: number + @column() declare tenantId: number | null } + class Comment extends BaseModel { + @column({ isPrimary: true }) + declare id: number + + @column() + declare postId: number + } + + class Skill extends BaseModel { + @column({ isPrimary: true }) + declare id: number + } + class User extends BaseModel { @column({ isPrimary: true }) declare id: number @@ -270,13 +290,38 @@ test.group('Nullable local key', (group) => { @hasMany(() => Post, { foreignKey: 'tenantId', localKey: 'tenantId' }) declare posts: HasMany + + @hasOne(() => Post, { foreignKey: 'tenantId', localKey: 'tenantId' }) + declare post: HasOne + + @hasManyThrough([() => Comment, () => Post], { + localKey: 'tenantId', + foreignKey: 'tenantId', + throughLocalKey: 'id', + throughForeignKey: 'postId', + }) + declare comments: HasManyThrough + + @manyToMany(() => Skill, { + localKey: 'tenantId', + pivotForeignKey: 'user_id', + pivotTable: 'skill_user', + }) + declare skills: ManyToMany } await seed(db) - await assert.rejects( - () => User.query().select('id').preload('posts'), - 'Cannot preload "posts", value of "User.tenantId" is undefined' - ) + /** + * Every relation type routes through its own query builder, so each + * call site is asserted rather than assuming the shared helper covers + * them all. + */ + for (const relation of ['posts', 'post', 'comments', 'skills'] as const) { + await assert.rejects( + () => User.query().select('id').preload(relation), + `Cannot preload "${relation}", value of "User.tenantId" is undefined` + ) + } }) })