Doctrine2 Module: Is flush() before checking the database so necessary?

[AntonTyutin]

Why do we flush managed Doctrine entities before checking the database in Doctrine2::proceedSeeInRepository()?

In functional tests the Entity Manager of application is used in the Codeception Doctrine2 Module. If we do test a request with validation rejection, and we want to be sure the database was not changed, the test result will be false negative (the request fetches and populates some entity, but didn't save changes to the database, and then dontSeeInRepository() flushes the invalid entity to the database and does fail).

[artmnv]

flush causes write operations to the database. But it's only operations from UnitOfWork. You can notify UnitOfWork to perform operations during flush with persist, remove etc. Flush doesn't cause write without persist. Since there is no persist in Doctrine2::proceedSeeInRepository() , no write operation will be initiated above that you've mark with persist in your app code. And you should not persist invalid or validation rejected with entities. You can read more about flushing and UnitOfWork in Doctrine 2 official docs.

[AntonTyutin]

Let's consider a functional test case containing following steps: 1) sending a POST request with invalid form data, 2) checking the response for validation errors, 3) checking the database record is unchanged.

The test case uses the same Doctrine Entity Manager. After the first step UoW contains a "dirty" entity, and on the third step this state will be written to the database and following check will fail.

It doesn't look correctly.

[artmnv]

You are right. That's why it's not intended to work that way. It will work the way you are described if your code is something like this:

$entity = new MyEntity($data); // UoW doesn't contains "dirty" entity
$em->persist($data); // now it does
if (isValid($entity)) {
    $em->flush(); // writing only valid entity 
}
// now test can flush even dirty entity because you put it in UoW

But you should not persist invalid entity:

$entity = new MyEntity($data); // UoW doesn't contains "dirty" entity
if (isValid($entity)) {
    $em->persist($data); // UoW will contains only valid entities
    $em->flush(); // writing only valid entity 
}
// UoW is empty and nothing will be written even after test's flush

Best practice: There shouldn't be such thing as invalid or incomplete entity. You should check data before creating it. There is a great talk from Doctrine 2 maintainer Ocramius about this. So it should be something like this:

if (isValid($data)) {
    $entity = new MyEntity($data);
    $em->persist($data); // UoW will contains only valid entities
    $em->flush(); // writing only valid entity 
} else {
    // error
}

If it doesn't help please provide some code example.

[AntonTyutin]

What if the POST request in my example above updates the exising database record? )

[AntonTyutin]

The real question is "what problem do we solve by flushing before checking the database record?"

[AntonTyutin]

Maybe we should clear() the Entity Manager instead of flush(). What do you think?

[TemirkhanN]

@artmnv an example with inconsistent entity is correct. Yet, the situation is somehow nonsense. Lets consider we have a code

echo $someEntity->getSomeProperty(); // some property value
$someEntity->setSomeProperty('another property value'); // we've changed state

$this->entityManager->persist($someEntity); // we have staged state
#$this->entityManager->flush(); // ooops, we've forgotten to flush our changes

yep, now we have functional test

$I->seeInRepository(SomeEntity::class, ['someProperty' => 'another property value']);

What do we have here? Testcase that matches wrong assert for in reality we DON'T have SomeEntity with that property value in our database. And this is the trouble.

[ThomasLandauer]

I was just about to open a new issue with the title: "Doctrine Module grabEntityFromRepository() flushes before grabbing" (just mentioning this for easier findability).

My app code looks like this (as recommended at https://symfony.com/doc/current/forms.html#handling-form-submissions ):

if ($form->isSubmitted() and $form->isValid())
{
    $em->persist($user);
    $em->flush();
}

So I'm doing the validation on the form, not on the entity. And I do have a custom callback validator on the entity (see https://symfony.com/doc/current/reference/constraints/Callback.html ). (After reading the post by @artmnv above, I'm not 100% sure anymore that this is the right/best way to do it - but it works ;-)

Anyway, as @AntonTyutin and @TemirkhanN are saying: The test produces false results, and this is a serious issue!

[artmnv]

I'm agree this flush seems to be redundant. Even if you don't persist inconsistent entities, it does nothing. But I don't know how it will affect existing tests. Maybe it should be removed in next major release?

Btw you can extend Doctrine2 Module and add custom method without flush like this:

1. Create file _support/Helper/MyDoctrine2.php:

namespace Tests\Helper;

use Codeception\Module\Doctrine2;

class MyDoctrine2 extends Doctrine2
{
    public function grabEntityFromRepositoryWithoutFlush($entity, $params = [])
    {
        $data = $this->em->getClassMetadata($entity);
        $qb = $this->em->getRepository($entity)->createQueryBuilder('s');
        $qb->select('s');
        $this->buildAssociationQuery($qb, $entity, 's', $params);
        $this->debug($qb->getDQL());

        return $qb->getQuery()->getSingleResult();
    }
}

2. Replace 'Doctrine2' with '\Tests\Helper\MyDoctrine2' in codeception.yml and all getModule() calls.

[Naktibalda]

If you want to remove something in 2.6.0, please do it quickly.

[artmnv]

@Naktibalda I can do PR and tests. But I'm not confident at this decision. What do you think about this issue?

[artmnv]

This issue is more complex than it seams. I'm agree with your false-positive and false-negative examples. And we can remove flush from SeeInRepository method but it will add nothing to your code quality. That is because of Doctrine's Unit of Work model. Your code should not rely on flush presence or lack of it. Flush can appear somewhere in other class/method of your code. Imagine we add feature to save last user action time or log some data into DB on each/some requests. It will perform flush and your code will suddenly stop working as intended. It will save not valid entities to DB. Sounds terrible.

I think in @ThomasLandauer example where Form module makes your entity invalid before form validation you should explicitly discard invalid entity (or stop using module with such bad design)

if ($form->isSubmitted() and $form->isValid()) {
    $em->persist($user);
    $em->flush();
} else {
    $em->refresh($user);
}

In the @TemirkhanN example of false-positive test if we remove flush it still can be flushed from another place. I think the only way to test that entities is flushing in your code is to mock EM and check flush calls. For example you can add add method to your repository and cover it with flush-test. You'll be sure that flush is called in your code and not somewhere in the vendor module.

So this flush removing can't save you from errors. But should we remove flush for sake of making app quality a little better? Will it prevent at least some errors? I think it will. But it can open your code for other more dangerous ones. If we'll remove flush there will be no false-negative result when we have invalid not-flushed entity. But it is false-negative from the start. And it's good. You should not rely on this test results and you will not. There is really mistake in your code that can become very dangerous vulnerability like password change without permission.

I think we should not remove flush from proceedSeeInRepository().

[TemirkhanN]

@artmnv

if we remove flush it still can be flushed from another place. I think the only way to test that entities is flushing in your code is to mock EM and check flush calls. For example you can add add method to your repository and cover it with flush-test. You'll be sure that flush is called in your code and not somewhere in the vendor module.

even if developer has such complicated implicit flush that looks and works awful it still works correct for it is the part of an application(even if it is third party like this example). But we are talking about testing framework and it has side effect on application.
For example we have following code

$someService->doSomeStuff(); // This place changes states of some pool of entities
$anotherService->doAnotherStuff(); // And here developer implicitly calls flush to save previous service actions

an example above demonstrates bad practice/architecture yet code works as expected and shall STOP working if we switch those two lines. And here we go again to unexpected actions from testing framework that hides such issues giving same result(for example above) no matter in which order services have been called.

I think the only way to test that entities is flushing in your code is to mock EM and check flush calls.
Also I am not sure that this way is somehow reachable. Flush can be called anywhere as you've said so it can be on upper layer or lower layer or on both at same time. That is why functional tests (or acceptance tests?) care only about "what we do and what result we expect" leaving space for "how we do it". And that is why I argue against side effects from QA tools.

To be honest I don't use codeception anymore and don't really care what happens with this issue. So if nobody else cares than I have nothing against leaving things to be.

[ThomasLandauer]

Because of this flush() problem, I added a recommendation for Symfony users to use the "built-in" repository, instead of this module's grab...() methods: https://codeception.com/docs/modules/Doctrine2#Grabbing-Entities-with-Symfony
This is at least a step forward for most users...