Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(validation): throw when trying to call em.flush() from hooks #574

Merged
merged 1 commit into from May 14, 2020

Conversation

B4nan
Copy link
Member

@B4nan B4nan commented May 14, 2020

Hooks are executed inside the commit action of unit of work, after all change
sets are computed. This means that it is not possible to create new entities as
usual from inside the hook. Calling em.flush() from hooks will result in
validation error. Calling em.persist() can result in undefined behaviour like
locking errors.

The internal instance of EntityManager accessible under wrap(this).__em is
not meant for public usage.

Closes #493

Hooks are executed inside the commit action of unit of work, after all change
sets are computed. This means that it is not possible to create new entities as
usual from inside the hook. Calling `em.flush()` from hooks will result in
validation error. Calling `em.persist()` can result in undefined behaviour like
locking errors.

> The **internal** instance of `EntityManager` accessible under `wrap(this).__em` is
> not meant for public usage.

Closes #493
@B4nan B4nan merged commit c3d0ce6 into master May 14, 2020
@B4nan B4nan deleted the gh493 branch May 14, 2020 06:41
@evanpurkhiser
Copy link

I'm receiving this exception, without using any of the lifecycle hooks.

if (this.working) {
throw ValidationError.cannotCommit();
}

I don't know much about the mikro-orm internals, but this looks like it's checking more than if you're trying to flush within a lifecycle.

In my case I'm flushing entities asynchronously (which is why I run into this)

https://github.com/EvanPurkhiser/prolink-connect/blob/0bfce80aad1cb0e4e333cf5073a60527e4cd1676/src/localdb/rekordbox.ts#L150-L173

Thought I would leave this comment here, but I'll likely work around it.

@B4nan
Copy link
Member Author

B4nan commented May 29, 2020

In my case I'm flushing entities asynchronously (which is why I run into this)

You can't flush asynchronously, not with the current implementation. If you want to do that, you need to create separate contexts via em.fork() and flush those.

@evanpurkhiser
Copy link

Thanks for the quick response @B4nan.

That makes sense. Thanks.

@B4nan
Copy link
Member Author

B4nan commented May 29, 2020

Here is an example of this: https://github.com/mikro-orm/mikro-orm/blob/master/tests/EntityManager.mysql.test.ts#L1507

const author = new Author2('name', 'email');
await orm.em.persistAndFlush(author); // we need to flush here so the entity gets inside IM

// fork EM without clearing the IM (once for each process), so author entity will be there
await Promise.all([
  orm.em.fork(false).persistAndFlush(new Book2('b1', author)),
  orm.em.fork(false).persistAndFlush(new Book2('b2', author)),
  orm.em.fork(false).persistAndFlush(new Book2('b3', author)),
]);

If we did not flush the author outside like this, it would mess up the identity map. The em.fork(false) keeps current contents of the IM in the fork, so the author entity is known - otherwise it would produce insert query for it (if the IM would be empty).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limitations of lifecycle hooks
2 participants