Dataloader failure with ManyToMany relationship

[tackley]

Describe the bug

With a simple many to many relationship on an entity like this:

  @ManyToMany(() => Country)
  countries = new Collection<Country>(this);

Enabling dataloader to load this collection with e.g. await user.countries.load({dataloader: true}) fails with the following error:

TypeError: Cannot read properties of undefined (reading '__helper')

      at helper (node_modules/@mikro-orm/core/entity/wrap.js:24:19)
      at node_modules/@mikro-orm/core/utils/DataloaderUtils.js:155:42
          at Array.filter (<anonymous>)
      at node_modules/@mikro-orm/core/utils/DataloaderUtils.js:179:33
          at Array.map (<anonymous>)
      at DataLoader._batchLoadFn (node_modules/@mikro-orm/core/utils/DataloaderUtils.js:171:34)

Reproduction

https://github.com/tackley/mikro-orm-reproduction

test that illustrates the issue in https://github.com/tackley/mikro-orm-reproduction/blob/master/src/dataloader-m-to-m.test.ts

What driver are you using?

@mikro-orm/postgresql

MikroORM version

6.3.10

Node.js version

20.17.0

Operating system

Linux

Validations

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

[B4nan]

The problem is specifically a unidirectional M:N relation, if you add the inverse side it works fine.

@darkbasic could you look into this please?

[darkbasic]

The problem is specifically a unidirectional M:N relation, if you add the inverse side it works fine

I think that's exactly what's going on. I will further look into this but I'm pretty sure that I need an inverse side in order to compute the relationship and be able to reassign the results. This needs to be documented at the very least but I'm wondering if it could be possible to dynamically prohibit dataloader usage whenever the M:N relationship lacks an inverse side.

[B4nan]

Uh, why? I don't see how an inverse side should be a limiting factor for this. This is something to fix, not do document.

[B4nan]

I took a closer look, and the whole implementation of the dataloader filters feels quite duplicated, the EntityLoader already implements all those things, in the correct way (without the need for inverse sides). Namely the M:N collections are handled here:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/entity/EntityLoader.ts#L458

I tried to hack it and it seems to be working fine: #6053 (@darkbasic review welcome, I wonder why didn't you try to reuse that when implementing the dataloader support?)

The existing tests are quite sparse, and when looking at the implementation, it feels like it cannot really work correctly with the various options, namely I can't see any place where we would actually try to handle different where conditions or child populate hints, it feels like the options are internally used only for the keys and not really for what they are meant for. With that said, I don't even see how we could be using them, maybe we should validate and disallow them with the dataloader enabled.

[darkbasic]

I took a closer look, and the whole implementation of the dataloader filters feels quite duplicated, the EntityLoader already implements all those things, in the correct way (without the need for inverse sides). I wonder why didn't you try to reuse that when implementing the dataloader support?)

It was born as an out of tree implementation and it gone through several major ORM versions which broke the implementation each time. I don't remember exactly but I guess that's the reason why I ended up using the least amount of internal interfaces subject to breaking changes.

The existing tests are quite sparse

Yeah even though I noticed that the tests were originally built considering MANY-to-MANY relationships with both inverse and no inverse side: https://github.com/mikro-orm/mikro-orm/blob/master/tests/features/dataloader.test.ts#L49 Yet somehow only the collection with the inverse side ended up being actually tested: https://github.com/mikro-orm/mikro-orm/blob/master/tests/features/dataloader.test.ts#L250 At this point I'm not sure if I forgot to implement the no inverse side case or what else.

looking at the implementation, it feels like it cannot really work correctly with the various options, namely I can't see any place where we would actually try to handle different where conditions or child populate hints, it feels like the options are internally used only for the keys and not really for what they are meant for

That's expected, the initial mainlined implementation was supposed to handle just the simplest use cases. Of course the typings should be updated to reflect that.

With that said, I don't even see how we could be using them, maybe we should validate and disallow them with the dataloader enabled.

Handling of where and populate options has already been implemented in the out of tree library, but it's doesn't support the full set of the operators so it is typed accordingly. Currently the out of tree dataloader also tries to optimize as many queries/where options combinations into the smallest number of queries possible: that's the main reason why implementing some operators proved to be so tricky.

The first step should be to update the typings to reflect the current capabilities. Then I would like to merge the where/populate handling from the out of tree module without the query optimizer: that way a different query for each where/populate combination should be fired which is still good enough for most use cases. It was already in the plan, but life got in the way and never managed to do so.

I tried to hack it and it seems to be working fine: https://github.com/mikro-orm/mikro-orm/pull/6053 (@darkbasic review welcome

Sure I will have a look, thanks for doing so.