adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.07k stars 191 forks source link

Preloading a `hasMany` relationship breaks if foreign key value is null #946

Closed evoactivity closed 9 months ago

evoactivity commented 1 year ago

If you are loading a collection, and any of the items in that collection have the foreign key value set to null, if you try to preload a relationship that uses that key, an exception is thrown instead of returning null or an empty array for that item.

This was acknowledged by @thetutlage in discord last year

Possibly related to #941

Package version

18.4.0

Node.js and npm version

Node 18.13.0 pnpm 8.5.1

Sample Code (to reproduce the issue)

import { BaseModel, HasMany, hasMany, column } from "@ioc:Adonis/Lucid/Orm";
import Programme from "./Programme";

export default class Channel extends BaseModel {
  public static namingStrategy = new CamelCaseNamingStrategy();

  @column({ isPrimary: true })
  public id: number;

  @column()
  public epgId?: string | null;

  @hasMany(() => Programme, {
    localKey: 'epgId',
    foreignKey: 'channelEpgId',
  })
  public programmes: HasMany<typeof Programme>;
}
import { BaseModel, BelongsTo, belongsTo, column } from '@ioc:Adonis/Lucid/Orm';
import Channel from './Channel';

export default class Programme extends BaseModel {
  public static namingStrategy = new CamelCaseNamingStrategy();

  @column({ isPrimary: true })
  public id: number;

  @column()
  public channelEpgId: string;

  @belongsTo(() => Channel, {
    localKey: 'epgId',
    foreignKey: 'channelEpgId',
  })
  public channel: BelongsTo<typeof Channel>;
}
import Channel from './Channel';

const channels = await Channel.query().preload('programmes'); 
// exception thrown if any channel doesn't have an epgId

const channels = await Channel.query().has('programmes').preload('programmes'); 
// no channels are returned if any of them don't have an epgId

BONUS (a sample repo to reproduce the issue)

I will come back with a repro repo asap.

thetutlage commented 9 months ago

It is breaking because the localKey value is null and not the foreign key. In your case epgId is a local key, which is needed to make any relationship work. Ideally, it should be primary key from the Channel model.

evoactivity commented 9 months ago

In discord you said that should be fine and the ORM should handle it instead of raising an exception. So what changed between then and now? I made this issue because you seemed to agree the behaviour described should work.

Yes, ideally, relationships would be based on primary keys but that's not always the right choice for all relationships.

In my case EPG xml files will not have any information about the database id's of the channels when they are imported to my app, they are often provided by third parties and the user may need to edit the EPG ID for a given channel and we don't want them editing the primary key of the channel. EPG ID's tend to follow an agreed format so different platforms can interoperate but sometimes they are entirely custom so we need to let the user decide what the EPG ID is for their channels. An EPG ID may also be shared by multiple channels.

When I reported this, I changed my app so my import process looks up the channel to relate with by the epg id and then uses it's primary key, but it'd be nice to not have extra queries and complexities in that process just to make a relation.