Default filters break references loading

[darkbasic]

Describe the bug

@Filter({ name: 'old', cond: { age: { $gt: 80 } }, default: true })
@Entity()
class Author {
  @PrimaryKey()
  id!: number;

  @Property()
  name: string;

  @Property()
  age: number;

  [...]
}

If you have a reference to an Author who is not "old" .load() it will return just its id instead of a loaded entity.
Default filters should be applied to Collections but it makes no sense to apply them to References IMO.

Reproduction

There won't be any reproduction because I've already created a failing test and the relative fix.
Since this is a prerequisite for my upcoming PR which is going to improve the Reference and Collection dataloaders this fix will be part of that PR.

What driver are you using?

@mikro-orm/sqlite

MikroORM version

git master

Node.js version

20

Operating system

Arch Linux

Validations

• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Slack.
• The provided reproduction is a minimal reproducible example of the bug.

[darkbasic]

Apparently this is expected behavior ("soft deletes"), but its current implementation is very confusing in my opinion.

• There is no way to load a reference if default filters prevent it from loading because, contrary to collections, there is no where parameter in the Reference.load method where you can disable them. Collections allow you to pass custom where filters when loading them, the same should be true for references.
• If a default filter prevents a reference from being loaded it currently returns the same uninitialized Reference (ref.isInitialized() === false), so there is absolutely no way to tell if a Reference returns no fields other than its id because of default filters or because it has not been loaded yet. Collections are marked as initialized even when default filters prevent them from returning values, the same should be true for references: they should return undefined. Since a reference can now return undefined the typings should be updated to return T | undefined (instead of just T) every time you load Reference<T> with any kind of filter attached to it.
• Since loading a reference (or a collection) might return different values depending on which filter has been used to load them they (loaded References/Collections) should carry a property listing all the filters that has been used to load them.
• This behavior is still quite confusing in my opinion and there should be an option to disable default filters on References/Collections.
• Last but not least, make sure that Collection.loadItems returns immutable arrays, otherwise loading the same collection twice with different filters will retroactively change whatever results you would have previously got.

[B4nan]

There is no way to load a reference if default filters prevent it from loading because, contrary to collections,

This behavior is still quite confusing in my opinion and there should be an option to disable default filters on References/Collections.

Indeed, both InitOptions and LoadReferenceOptions should have the filters option. Users need to be able to control filters, default filters can be disabled, non-default filters enabled, and users can also pass arguments to parametric filters.

there is no where parameter in the Reference.load method where you can disable them.

We should probably have populateWhere option in both of those methods too, as well as the recently added populateOrderBy, since they both have populate option.

If a default filter prevents a reference from being loaded it currently returns the same uninitialized Reference (ref.isInitialized() === false), so there is absolutely no way to tell if a Reference returns no fields other than its id because of default filters or because it has not been loaded yet. Collections are marked as initialized even when default filters prevent them from returning values, the same should be true for references: they should return undefined.

This is a good point, I agree we should return null (or undefined when forceUndefined option is enabled) if the entity cannot be loaded due to the enabled filters.

Since a reference can now return undefined the typings should be updated to return T | undefined (instead of just T) every time you load Reference with any kind of filter attached to it.

I am just not fully sure about this part, while it's technically correct, it feels a bit too strict. Maybe we could (in v7) change the way references work to align it with collections more - the reference instance would be always present, and you would query that, instead of it being null when the underlying FK is null. Few years ago, one user was suggesting that and now I am finally seeing why it would be a good idea.

Since loading a reference (or a collection) might return different values depending on which filter has been used to load them they (loaded References/Collections) should carry a property listing all the filters that has been used to load them.

I am not sure about that, how would you use this information? We could store it quite easily (in WrappedEntity), I don't mind, but I am not sure how it would be usable. Entities can mutate, that's the whole point of the ORM, e.g. you can load entity with default filters enabled and then reload some of its properties with them disabled.

Last but not least, make sure that Collection.loadItems returns immutable arrays, otherwise loading the same collection twice with different filters will retroactively change whatever results you would have previously got.

Same as the last point, I don't think this is something to change, it always worked this way.

[B4nan]

I am starting to think we should join the relation automatically and ensure the FK is not present when the filters disallow it. That's the only way to properly ensure things without the need for Reference wrapper (or adjusting its interface).

[B4nan]

@darkbasic wdyt about the above? unless you are strongly opposed, I'll give that a shot today, it's pretty much the last thing I want to finalize before another RC (and technically before the stable release too, I might just ship things tomorrow if there won't be any roadblocks in this one.

We are probably also missing the filters option in the Collection.load and Reference.load, but that feels like a super small fix, I think it should just work once we add the option.

[darkbasic]

I am starting to think we should join the relation automatically and ensure the FK is not present when the filters disallow it. That's the only way to properly ensure things without the need for Reference wrapper (or adjusting its interface).

@B4nan that makes sense and would kind of remove (more on that later) the need for the reference instance to be always present instead of it being null when the underlying FK is null. One downside that I see is that you are always going to make an additional join even when you are not interested in the value of such property. Also, while that would solve the issue for default filters, that won't fix it for custom filters that you might want to apply manually to the reference via load() options.

While I think that this is nice to have, it should probably be configurable if not even opt-in and in v7 we should still aim to change how references work to align them with collections. I'm also a bit skeptical of how this eagerly-join-and-remove-the-FK would interact with a user that later wants to apply a different filter to the reference, unless the FK won't be some kind of wrapper instead of null.

[B4nan]

One downside that I see is that you are always going to make an additional join even when you are not interested in the value of such property.

Well, you have a filter that affects the FK, without the join you don't get the true value respecting the filters. Keep in mind you can always disable the filter locally (and in the end, you've opted into that by having a default filter in the first place). You can also use partial loading if you are not interested in the value at all (which reminds me I should also finally add excludes option to v6 to allow partial loading of everything except a few columns).

Another argument for this is that technically, you are always interested in the value if you select it, it will end up in serialized entity even if you don't populate the relation, and speaking of the soft deletes, I doubt you would want that.

Also, while that would solve the issue for default filters, that won't fix it for custom filters that you might want to apply manually to the reference via load() options.

That feels like even minor issue imo, I don't want to make 99% cases less ergononic because of that (aka I'd keep the Ref.load() return a value instead of allowing null all the time). But we could maybe have an overload which will add the null to the return type only when there is where or filters in the load() options, that sounds reasonable (ideally this would also remove the null in case of filters: false). Let's see if it won't break the Loaded type :D

While I think that this is nice to have, it should probably be configurable if not even opt-in and

As I said above, I prefer correctness here, that's why I want to have it enabled by default, but will be happy to make this configurable for others who might not like it.

in v7 we should still aim to change how references work to align them with collections

I am still not sure about that, if we have the join, the problem is technically solved. I still like the way it works now more than having the ref instance there all the time, but open to discussion. We could run a poll in the discussions over time and see what others think, and maybe even consider having both ways (but that would complicate the types I guess, and could be a chore to maintain).

[darkbasic]

which reminds me I should also finally add excludes option to v6

Good, an exclude option should be enough to prevent unnecessary joins.

That feels like even minor issue imo, I don't want to make 99% cases less ergononic because of that (aka I'd keep the Ref.load() return a value instead of allowing null all the time). But we could maybe have an overload which will add the null to the return type only when there is where or filters in the load() options, that sounds reasonable (ideally this would also remove the null in case of filters: false)

I don't think this issue would be solved by overloading the load() option. Let's say you have a default filter on Book.author which shows only authors that are still alive. If you run const book = em.findOne(Book, {name: "Divine Comedy"}]) you end up with a very old book written by the long since dead Dante Alighieri and since it eagerly joined book.author will be null. At this point point how are you supposed to load the reference with different filters? book.author is null and null.load([...]) won't do the trick...

I am still not sure about that, if we have the join, the problem is technically solved

See the previous point.

[B4nan]

I don't think this issue would be solved by overloading the load() option. Let's say you have a default filter on Book.author which shows only authors that are still alive. If you run const book = em.findOne(Book, {name: "Divine Comedy"}]) you end up with a very old book written by the long since dead Dante Alighieri and since it eagerly joined book.author will be null.

Sounds like the internal join here should be inner join so the book wouldn't be selected, that kinda makes sense to me. Probably based on the nullability, e.g. for nullable columns we'd make left join, for not null it would be inner join.

At this point point how are you supposed to load the reference with different filters? book.author is null and null.load([...]) won't do the trick...

You would disable filters when loading the Book if that's what you are after.

Also, I don't think it would be any better if we had the Ref type present all the time as with Collection, it still represents the FK even without loading, this would need to be hidden under the load method, effectively making this feature useless, turning everything into async or preloaded. It would be kinda the same as making the property lazy: true (so not selected unless populated). We would still need the join I think.

[darkbasic]

Probably based on the nullability, e.g. for nullable columns we'd make left join, for not null it would be inner join

Sounds good to me.

[B4nan]

Btw I've started with the exclude option and it's a major chore to support this on the type level once again :D

[darkbasic]

Yeah I would have bet on that: the most time consuming part are always the typings :]

[B4nan]

Looking at this again, and I actually think the problem you highlighted with Ref.load() and filters is valid for BaseEntity.init() (or wrap().init()) as well, and even if we forget about filters - those methods can be executed after the target entity was removed and should return null in those cases (just like em.refresh is already doing).

I feel like we could have something like ensure: true (or maybe fail: true) in the options which would make it behave like findOneOrFail, so throw in case of the entity is not found. Now the question is whether this should be the default or not, but the null in the return type could work based on that easily.

(with that said, I still believe we should have the auto joining in place for enabled filters - note that I say enabled, it is really not just about the default filters)

Here is the exclude option #5024 btw.

edit: I guess I will go with loadOrFail to be consistent with findOneOrFail as well as the newly added refreshOrFail