Fallback to native entity if no revision found for properties

[soullivaneuh]

If an audited entity has relational properties, the reader will try to find them as the same version, or previous.

But if the targeted class does not has any revision at all, this produces a NoRevisionFound exception.

This should be catched and should fallback to a classic doctrine find query.

Concrete error with stack trace:

No revision of class 'AppBundle\Entity\User' (1) was found at revision 7 or before. The entity did not exist at the specified revision yet.

in vendor/simplethings/entity-audit-bundle/src/SimpleThings/EntityAudit/AuditReader.php at line 322   + 
at AuditReader ->find ('AppBundle\Entity\User', array('id' => '1'), '7', array('threatDeletionsAsExceptions' => true)) 
in vendor/simplethings/entity-audit-bundle/src/SimpleThings/EntityAudit/AuditReader.php at line 443   + 
at AuditReader ->createEntity ('AppBundle\Entity\Ticket', array('id' => 'id', 'title' => 'title', 'addedAt' => 'added_at', 'updatedAt' => 'updated_at', 'assignedAt' => 'assigned_at', 'closedAt' => 'closed_at', 'priority' => 'priority', 'state' => 'state', 'awaitingAnswerReminded' => 'awaiting_answer_reminded', 'feedbackScore' => 'feedback_score', 'feedbackComment' => 'feedback_comment', 'feedbackDoneAt' => 'feedback_done_at', 'awaitingFeedbackReminded' => 'awaiting_feedback_reminded', 'category_id' => 'category_id', 'author_id' => 'author_id', 'assigned_id' => 'assigned_id', 'organization_id' => 'organization_id', 'server_id' => 'server_id'), array('id' => '526', 'title' => 'Ticket de test Behat', 'addedAt' => '2016-10-01 14:45:00', 'updatedAt' => '2016-10-04 11:33:45', 'assignedAt' => null, 'closedAt' => null, 'priority' => '2', 'state' => '0', 'awaitingAnswerReminded' => '0', 'feedbackScore' => null, 'feedbackComment' => null, 'feedbackDoneAt' => null, 'awaitingFeedbackReminded' => '0', 'category_id' => '1', 'author_id' => '1', 'assigned_id' => null, 'organization_id' => null, 'server_id' => '1'), '7') 
in vendor/simplethings/entity-audit-bundle/src/SimpleThings/EntityAudit/AuditReader.php at line 331   + 
at AuditReader ->find ('AppBundle\Entity\Ticket', array('id' => '526'), '7') 
in src/AppBundle/Manager/TicketManager.php at line 250   + 
at TicketManager ->getTimeLine (object(Ticket)) 
in src/AppBundle/Controller/TicketController.php at line 259   + 
at TicketController ->showAction (object(Request), object(Ticket)) 
at call_user_func_array (array(object(TicketController), 'showAction'), array(object(Request), object(Ticket))) 
in vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php at line 144   + 
at HttpKernel ->handleRaw (object(Request), '1') 
in vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php at line 64   + 
at HttpKernel ->handle (object(Request), '1', true) 
in vendor/symfony/symfony/src/Symfony/Component/HttpKernel/DependencyInjection/ContainerAwareHttpKernel.php at line 69   + 
at ContainerAwareHttpKernel ->handle (object(Request), '1', true) 
in vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php at line 185   + 
at Kernel ->handle (object(Request)) 
in web/app_dev.php at line 34   +

Basically, the reader try to find a User instance of a specified revision because it's a property of the Ticket instance.

But after a fresh deploy, existing User instances does not has any revision yet.

Could you please push a new patch release after merging this? This is quite critical.

Regards.

fix #157

[tolry]

@soullivaneuh would you mind writing a test that covers this case?

quick question: what happens, if the find-query doesn't succeed either? Wouldn't this throw a 'new' exception (which might need conversion to NoRevisionFoundException)?

[soullivaneuh]

would you mind writing a test that covers this case?

I'll look at it.

quick question: what happens, if the find-query doesn't succeed either? Wouldn't this throw a 'new' exception (which might need conversion to NoRevisionFoundException)?

You are right. I'll throw the exception again if no result is found.

[soullivaneuh]

@tolry After reflection, I'm not sure this way is the good way.

Indeed, this method is here to find an entity at a specific revision.

This make no sense to return another entity if the revision does not exist.

Two possible solution:

• Do nothing. The end user must catch the NoRevisionFoundException and do the native query instead, or not.
• Add a fallbackToNativeEntity parameter. If true, fallback to native find query like I did, if false, just throw the NoRevisionFoundException.

What do you think?

[soullivaneuh]

Ok now I remember why I did this. This is for relation.

Indeed, on my code, I list all revision of an entity before finding them:

$ticketRevisions = array_reverse($this->auditReader->findRevisions(Ticket::class, $ticket->getId()));
foreach ($ticketRevisions as $revision) {
    $currentTicketVersion = $this->auditReader->find(Ticket::class, $ticket->getId(), $revision->getRev());

So there is no way to have a NoRevisionFoundException.

But the reader also tries to find related entities at the same revision. And at this case, it fails.

So we should add a doctrineFallback flag, default to false and set it to true on internal calls.

[soullivaneuh]

Ok so just forget all my comments. The PR is right because the fallback is done only for properties here.

This happen when you didn't update a PR from a long time... 😛

I'm looking for testing.

[soullivaneuh]

@tolry Tests added. Failing Travis seems to not be related to this PR.

Something looks wrong with pgsql...

[soullivaneuh]

I have an issue with MySQL: https://travis-ci.org/simplethings/EntityAudit/jobs/188847686#L305

I can't delete the related object of article to test the exception re-throw.

Actually, the find method of the manager returns null if no entity is found. Maybe would it be better to let the doctrine behavior for this case?

[DavidBadura]

Actually, the find method of the manager returns null if no entity is found. Maybe would it be better to let the doctrine behavior for this case?

👍 yes, i think so too.

[soullivaneuh]

@DavidBadura will do.

I saw the 1.0.0 release was done with the related branch.

As it's a bugfix, should I change the target branch for 1.0?

[DavidBadura]

done

[DavidBadura]

👍 LGTM

[soullivaneuh]

@DavidBadura I just fixed the tests, it should be OK now.

[DavidBadura]

thanks!

[soullivaneuh]

Thanks you too! Any chance to have a patch release for this? This is related to a quite critical issue.

[soullivaneuh]

ping @DavidBadura? :-)

[DavidBadura]

I'll make it this week

[soullivaneuh]

@DavidBadura Please wait a little before make a new release, I found a bug with reverse relations.

I'm working on a patch.