RFC: Remove persist/flush from repository level in v6

[B4nan]

Both persist and flush are tied to the EM instance, we have the repository shortcuts but they feel confusing, as they might give users false sense that they do something only with that given entity type. Maybe we should remove them and make the em getter public instead, to preserve easy access to the functionality.

private userRepo: EntityRepository<User>;

-this.userRepo.persist(u);
+this.userRepo.em.persist(u);

This should be very easy to migrate via simple find/replace, we could provide a codemod too (especially if we will have a codemod for #3982, it could happen in a single run).

[hayksaryan]

Do you think it would be better to just require importing/injecting em along with repository?

Instead of
this.userRepo.flush(); // or this.userRepo.em.flush();
use
this.em.flush();

Because having this.userRepo.em is still somewhat confusing. For someone who's getting started with MikroORM it might seem userRepo.em is not the same as carRepo.em.

I understand that it's a handy "shortcut", but it's a bit misleading from the API standpoint.

[hayksaryan]

Another approach would be making assign context-aware, so to speak. So, throw an error on runtime, when an incorrect entity is passed as a param.

[B4nan]

I think I will end up with a public getEntityManager() method in the repository. I really feel like this is super nice shortcut, and removing it would be step backwards, it would make people mad for no reason. The em getter seems too easy to miss, but if we have this long-named method, it could serve the right purpose - to make people understand they are using the EM (and not blindly writing some shortcut).

And will add validations to the entity type to some repository methods to protect from the issue with assign discussed above etc.

[B4nan]

Validation is added here: 301bdf8

This PR #4259 is marking the methods for deprecation and adds the new repo.getEntityManager() public method.

[EvilCheetah]

Is there a plan to update the documentation to include the changes? Are there any plans to update the implementation in the Recipes section and example apps, such as express-js-example-app and nestjs-realworld-example-app, to include these changes?

[B4nan]

I am not aware of any place in docs that would use those methods on repository level, it was never suggested - but feel free to suggest where we should mention this. I will update the example apps before v6 gets out (rather sooner, did the realworld example app right now as that is the most popular one), right now it's just a deprecation, it still works as before - and to do a simple migration you could just prefix those calls with repo.getEntityManager(), that's exactly what those methods were doing - they were just shortcuts (which is actually documented).

[ps-aux]

This API change actually complicates the unit testing when the mock/stub of the repo object is provided as a dependency.

It is easier to create a mock/stub for invocation this.repo.persist() than it is for this.repo.getEntityManager().persist().

[B4nan]

How come? jest.spyOn(EntityManager.prototype, 'persist')?

[ps-aux]

I don't use Jest for stubbing/mocking so I am not sure I can infer the possible context of your example. Would it be possible to provide more code?

What I had in mind was:

import { Entity, EntityManager } from '@mikro-orm/core'
import { EntityRepository, SqlEntityManager } from '@mikro-orm/postgresql'
import td from 'testdouble'

@Entity()
class Foo {
}

export class MyService {

    constructor(private repo: EntityRepository<Foo>) {
    }

    async doSomethingOldWay() {
        const f = new Foo()
        await this.repo.persist(f)
    }

    async doSomethingUsingEm() {
        const f = new Foo()
        await this.repo.getEntityManager().persist(f)
    }
}

it('old way', () => {
    const repo = td.instance<EntityRepository<Foo>>(EntityRepository)

    const sut = new MyService(repo)
    sut.doSomethingOldWay()

    td.verify(repo.persist(td.matchers.isA(Foo)))
})

it('with exposing EM', () => {

    const em: SqlEntityManager = td.instance(SqlEntityManager)
    // The extra setup here
    const repo = td.instance<EntityRepository<Foo>>(EntityRepository)
    td.when(repo.getEntityManager()).thenReturn(em)

    const sut = new MyService(repo)
    sut.doSomethingUsingEm()

    td.verify(em.persist(td.matchers.isA(Foo)))
})

Also what I see as another design disadvantage is that the client objects are now coupled to 2 different APIs - the higher level repo one and the lower level EntityManager one. Sure, there was shortcut to get EM reference from a repo before but it was optional.

[B4nan]

Never used testdouble so can't help with that, spyOn is a pretty standard API these days, it's not just a jest thing (vitest and bun support that too afaik). I am not a fan of mocking in general, especially when it comes to ORMs, so if you ask me, my answer is don't do that in the first place, it will only add more tight coupling to tests. Using spyOn is a pretty nice way to mock only the bit you care about and keep the rest as is, it's the only sane way to mock complex things IMO.

Also persist is a sync method, not sure why you are awaiting it in your example, it's not doing any database calls.

Also what I see as another design disadvantage is that the client objects are now coupled to 2 different APIs - the higher level repo one and the lower level EntityManager one. Sure, there was shortcut to get EM reference from a repo before but it was optional.

That's exactly the motivation for this change, you were always coupled to both, you just didn't care. Repositories don't do anything on their own, they just forward the calls to the entity manager, and that is what you should be mocking.

[B4nan]

FWIW you can still have those methods in your base repository if you really need them (you don't, but let's put that aside).

https://mikro-orm.io/docs/next/repositories#removed-methods-from-entityrepository-interface

[ps-aux]

If repositories are not meant as separate abstraction but more of a helper to EntityManager layer than the case is different.

Generally I see repos as a layer on top of concrete persistence solution - the way it is defined by DDD (or outside of DDD known as DAOs). It is common in Java stack (and I originally thought Spring Data repositories was an inspiration for the MikroORM repositories) but now I understand this is probably not the idea for MikroORM.

Regarding the testing - I am not a mockist myself but I believe mocking has its place in testing. However testing approach is really matter of preference. I personally don't think the Jest approach is ideal even though it might be standard in TS/JS world.

Regarding the persist invocation - you are right. My example originally contained persistAndFlush but I changed it to persist and forgot to remove the await.

[B4nan]

One more RFC for this here: #4944