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 diff --git a/test/orm/nullable_local_key.spec.ts b/test/orm/nullable_local_key.spec.ts new file mode 100644 index 00000000..4ff6a5d8 --- /dev/null +++ b/test/orm/nullable_local_key.spec.ts @@ -0,0 +1,327 @@ +/* + * @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() + }) + + /** + * 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() + 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({ 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 + + @column() + declare tenantId: number | null + + @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) + + /** + * 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` + ) + } + }) +})