test(core): load items error

[jbmikk]

For now this is just a failing test for an issue I found.

It happened to me while adding request level transactions to an Apollo application. Several resolvers started failing after running queries which contained nested transactions.

It looks like collections remain attached to the transaction that has already finished and loadItems fails.

I tried calling getContext() in the getEntityManager method for the Collection class but it didn't work. I'm not sure what's happening yet. Still working on it.

I tested this both with v4.5.9 and master and it failed in both.

@B4nan when we solve this, would it be possible to have a v4.5.10 release?

[B4nan]

Either way, please don't PR the 4.x branch, the fix needs to go to master first (and if possible, we will cherry pick, otherwise do a separate/unrelated commit to 4.x).

[jbmikk]

@B4nan done.

Do you have any clues why this could be happening?
This is the error that happens on init:

[DriverException: Transaction query already complete, run with DEBUG=knex:tx for more info]

[B4nan]

I guess its because of mixing the two approaches for transactions (em.begin vs em.transactional), will need to test it myself to be sure.

[jbmikk]

You are right. If I wrap two transactional calls it works.
If I omit the external transaction and just call loadItems after the inner one it also fails.

[jbmikk]

The reason I need to make the begin and commit calls this way is because I wrote an apollo plugin that has lifecycle events, so I need to make these separate calls in the plugin. It's just not possible to use a transactional call there.

[B4nan]

So the problem here is that when you run em.transactional(), all currently loaded entities are merged into the inner EM fork. But when that happens, their internal EM instance is updated to the fork, and it stays like that after you exit the transactional handler. So by calling the loadItems() (or any other method that works with the internal EM instance, e.g. wrap(e).init()), you are trying to use the inner fork, that should no longer exists (and its transaction that is finished).

I will respect the existing EM instance when merging the entities, but I not fully sure if we should backport this to v4, it feels somehow breaking (it does not break any existing tests tho, maybe I am just too cautious).

[jbmikk]

@B4nan Does that mean on v5 this will work? Has this been fixed already on master?
I haven't had time to keep working on this.
The main use case for this is having begin and commit working nicely with transactional so that scoped TypeDI containers with default transactions and Apollo can work well together and have fully transactional isolated requests. This is not just useful for Apollo but for any form of framework integration that requires scoped containers and default transactions.

[jbmikk]

@B4nan maybe a way of avoiding breaking changes is having a parameter for default behaviors in cases like this. Although I'm not sure it is worth the effort and complexity doing this if you don't really care about supporting the old way.

[B4nan]

Yes, this is closed by a commit that fixed the issue (and yes, it is a oneliner).