Inintended caching of EntityManager by EntityRepository from within a transactional context about mikro-orm HOT 8 CLOSED

Yeah, for sure, I will include the repro from OP.

from mikro-orm.

@ashleyww93 You can override .getRepository() with a custom EM to be able to get visibility into any occurence of this specific issue. Feel free to just log instead of a hard error.

// replace `PostgreSqlDriver` with you respective driver
export class ExtendedEntityManager extends EntityManager<PostgreSqlDriver> {
  public getRepository<Entity extends object>(entityName: EntityName<Entity>) {
    const repository = super.getRepository(entityName);
    const currentContext = this.id;
    const repoContext = repository.getEntityManager().id;
    if (currentContext !== repoContext) {
      throw new Error(
        `EntityRepository<${repository.getEntityName()}> context (${repoContext}) did not match the current EntityManager context (${currentContext})!`,
      );
    }
    return repository as any;
  }
}

defineConfig({
  // ...
  entityManager: ExtendedEntityManager,
});

Docs: https://mikro-orm.io/docs/entity-manager#extending-entitymanager

from mikro-orm.

@B4nan Awesome! Thanks.

Any chance the test suite can be amended with a case like this?

await em.transactional(async _ => {
    // first even call to a repository, em is not tx-specific
    await em.getRepository(Post).find({id: 1});
});
assert.equal(em.id, em.getRepository(Post).getEntityManager().id)

from mikro-orm.

That's funny, the commit that broke this (af87693) was trying to fix the exact same error (#3074). Will need to reread that thread, as reverting this would probably break some other case...

from mikro-orm.

It is worth noting that the aforementioned af87693 explicitly states "do not use context in em.getRepository()", however it is still used there (v6.1.12) and was added by b5b03a6#diff-a62cd78c400d99253a3c0fb5a9bf26e1e63e0deb511059d28ba51014b58242ddR146 but the change does not seem related to the commit's intention.

from mikro-orm.

Could this be causing other unintended consequences?

I’m currently trying to track down an issue(so I can make a reproduction) where entities are deleted and recreated with the same ID.

I thought it was an issue our side (and might be), but it’s seemingly random, like the identity map is incorrect on some occasions.

from mikro-orm.

@ashleyww93 If this issue is the culprit of your problem, it may appear random because it requires the first .getRepository() call in the app process to be inside a transaction. After a server restart it may be completely up to chance whether it happens this way.

from mikro-orm.

It is worth noting that the aforementioned af87693 explicitly states "do not use context in em.getRepository()", however it is still used there (v6.1.12) and was added by b5b03a6#diff-a62cd78c400d99253a3c0fb5a9bf26e1e63e0deb511059d28ba51014b58242ddR146 but the change does not seem related to the commit's intention.

Yeah correct, and reverting this bit is not making any of the existing tests fail, I guess I will go with that, worst case we will revert it, but I agree it looks unrelated to the commit. That change also makes the e2e test suite of the real-world app pass, so it feels safe.

from mikro-orm.