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

Optimistic lock not working when used with DateTime objects. #8499

Closed
diego-ninja opened this issue Feb 22, 2021 · 2 comments · Fixed by #8508
Closed

Optimistic lock not working when used with DateTime objects. #8499

diego-ninja opened this issue Feb 22, 2021 · 2 comments · Fixed by #8508

Comments

@diego-ninja
Copy link
Contributor

I will try to be as concrete and specific as possible, I think there is a bug when comparing the DateTime objects used in the optimistic lock.

In the UnitOfWork (Line 2497) you are using a !== comparison that only resolves to true when the two instances of the compared object are the same. This way it is totally impossible for $entityVersion and $lockVersion to be the same at any time since they are different objects.

    if ($entityVersion !== $lockVersion) {
        throw OptimisticLockException::lockFailedVersionMismatch($entity, $lockVersion, $entityVersion);
    }

Debugging to that point I get the following pair of objects, that should pass the validation but they don't:

Debug variables

I have checked the tests that cover the lock functionality and the only thing that is checked is that the appropriate exceptions are thrown, which is always the case since the result of the comparison is always false. There is no test that covers the success of the lock operation.

In older versions of the code the comparison was performed using the != operator with which the functionality works as expected.

Possible solutions could be to revert to using the != operator or to directly compare the timestamps of DateTime objects using the !== operator.

I can send a PR with the proposed fix.

Thanks in advance.

@Warxcell
Copy link
Contributor

Warxcell commented Feb 22, 2021

Btw there is similar problem with Composite @Id that are made of \DateTimeInterface|object

@diego-ninja
Copy link
Contributor Author

Just created a new PR with fix and tests, this time to 2.8.x branch. All checks have passed.

greg0ire pushed a commit to greg0ire/doctrine-orm that referenced this issue Feb 24, 2021
…ace objects directly.

When using optimistic lock with DateTimeInterface based version field a
bug appears due to the use of the === operator for comparing the lock
version and the entity version. This comparison always resolves to false
because the === operator when comparing objects is only true when both
sides are the exact same instance of the object.

To fix the issue I have decided to compare timestamps instead the
DateTimeInterface based objects directly, calling getTimestamp() method
and doing a strict comparison.

Modified OptimisticLockException to use DateTimeInterface instead of
DateTime class.

Fixes doctrine#8499
beberlei pushed a commit that referenced this issue Feb 27, 2021
…ace objects directly. (#8508)

When using optimistic lock with DateTimeInterface based version field a bug appears due to the use of the === operator for comparing the lock version and the entity version. This comparison always resolves to false because the === operator when comparing objects is only true when both sides are the exact same instance of the object.

To fix the issue I have decided to compare timestamps instead the DateTimeInterface based objects directly, calling getTimestamp() method and doing a strict comparison.

Modified OptimisticLockException to use DateTimeInterface instead of DateTime class.

Added test suite to cover case.

Fixes #8499
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 a pull request may close this issue.

2 participants