#93 - Adding support for optimistic locking based on @Version column

[orange-buffalo]

This PR is a proposal for implementation of optimistic locking (#93). Suggested solution was heavily inspired by spring-projects/spring-data-relational#124.

Current tests are failing, and I need an advice from the project members what would be the best way to resolve it.
The thing is that typically OptimisticLockingException is expected by a user if update failed due to version mismatch (expected result in the tests). But previous implementation already used ID criteria during update query execution and now fails with TransientDataAccessResourceException if query did not update any rows (regardless if version or ID did not match). I do not see any way to distinguish between ID/version mismatches but executing an additional query in the failure case to throw a proper exception. But this obviously has a cost. At the same time using TransientDataAccessResourceException for optimistic locking does not seem to be a good design. And TransientDataAccessResourceException is already a part of API contract, just replacing it with OptimisticLockingException would mean an incompatible contract change. So all available options seems to have downsides to me..

[orange-buffalo]

@mp911de, are there any plans to consider this PR or resolve #93 in any other way?
I've rebased the branch on top of your changes in Criteria class, now it is mergeable again. I am open to implement the required changes, if there is any feedback.

[mp911de]

Thanks for your pull request. We definitively want to add support for optimistic locking. We just didn't had the time yet to work ourselves into this matter in the context of R2DBC. The design of the R2DBC module evolves and we realized that we have to treat DatabaseClient more like JdbcTemplate and not so much an all-in-one abstraction, that is why we introduced the R2dbcEntityTemplate.

After quickly looking at the implementation, please make note that primitive version properties (long) are initialized with 1 while wrapper type versions (Long) are initialized with zero. See the MongoDB implementation for reference.

[orange-buffalo]

Thank you for the feedback, @mp911de.

Regarding the initial version handling, could you please comment on the motivation for this decision? Me as a user expect to have the versioning consistent for all Numbers and always start with the same initial value. As far as I can see, spring-data-jdbc always starts from 1 and Hibernate starts from 0 for all Numbers.

Btw, I've noticed that there is no AbstractSimpleR2dbcRepositoryIntegrationTests implementation for MariaDB, so optimistic locking is not tested for this database. Should I add one in scope of this PR?

[mp911de]

The rationale for the difference is that an uninitialized Long renders to null while an uninitialized long renders to 0 (zero). Looking at Spring Data JDBC, we should align it as well with how other Spring Data modules handle optimistic locking.

Regarding the SimpleRepositoryTests: Let's keep it out of this PR for the time being. The simple repository is indirectly tested with the full repository integration test suites.

[schauder]

I created an issue in Spring Data JDBC for changing the version initialisation. https://jira.spring.io/browse/DATAJDBC-507

[orange-buffalo]

Thanks, guys, I've update the PR to follow Spring Data versioning policy.

Coming back to the initial question about OptimisticLockingException vs TransientDataAccessResourceException, what would be your suggestion?

[mp911de]

We previously had no support for optimistic locking so we used TransientDataAccessResourceException. We should add another code path for optimistic locking and emit OptimisticLockingException if the update fails.

[orange-buffalo]

Done.

[orange-buffalo]

Hi guys, is there anything I can do for this PR to get accepted?

[mp911de]

No, there's nothing else required from your side. Given the current situation, we're tied more to prioritization and some contributions may take longer to get merged. Thanks for your understanding.

[schauder]

This is polished and merged.

Thanks for the work.