DATAJDBC-219 - add support for optimistic locking #124

thombergs · 2019-03-09T22:10:06Z

This PR adds support for optimistic locking by evaluating the @Version annotation.

Some indentations are wrong as I didn't find a code formatter configuration but I can fix indentations if you point me to your code formatter.

schauder

Hi Tom,

Thanks for tackling this.

I think we can replace the VersionsAccessor with to simple methods as in Mongo Db: https://github.com/spring-projects/spring-data-mongodb/blob/8f4db5540d70ddf4926824d050c50f73a35c6aab/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java#L672

These will also take care of handling immutable Version attributes properly.

A delete where an entity is given as a parameter should also check for optimistic locking.

I rebased and squashed your changes into a branch: https://github.com/spring-projects/spring-data-jdbc/tree/pr/datajdbc-219
Please use that for further work on this pr.
We try to avoid merge commits, since they make it hard to understand the changes made.

schauder · 2019-03-11T08:49:08Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java

+	}
+
+    String getUpdateWithVersion(Number version) {
+        return String.format("%s AND %s = %s", updateSql.get(), entity.getVersionProperty().getColumnName(), version);


The versions should be a bind parameter as all other parameters.
For the parameter name pick something that is highly unlikely to collide with any other parameter names, like ___oldOptimisticLockingVersion

@Version

The @Version annotation is now evaluated properly and an OptimisticLockingFailureException is thrown on updates when the version has been incremented in the meantime. The @Version field can be of type Long, Integer or Short (or their primitive counterparts).

thombergs · 2019-03-15T17:25:45Z

Hi @schauder,

I was wondering if there was a simpler way for accessing and modifying the version field. I simplified it and removed the VersionAccessor class. However, there is still an ugly if/else for supporting the different field types. I force pushed a cleanly squashed commit.

To support optimistic locking for the delete operation, I think we would have to modify DeleteRoot to include the entity instead of just the ID of the entity. Since this would mean a lot of changes, I'd like to hear your feedback on that first.

schauder · 2019-03-25T09:15:56Z

I force pushed a cleanly squashed commit.

Great, I'll take a look.

To support optimistic locking for the delete operation, I think we would have to modify DeleteRoot to include the entity instead of just the ID of the entity. Since this would mean a lot of changes, I'd like to hear your feedback on that first.

I don't think we need the full entity, but just the version attribute.
In any case, changes to the public API must be made in a backward compatible way, i.e. don't simply change the signature, but add a new method with the additional parameter.

schauder

I don't think this works with an immutable version attribute.

Take a look how this is handled for the Id in https://github.com/spring-projects/spring-data-jdbc/blob/ade3324e2c8f578ca256f920998d76bd62a04df2/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/AggregateChange.java#L66

schauder · 2019-03-25T09:20:31Z

...ng-data-jdbc/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java

+	}
+
+	private <T> Number getNextVersion(T instance, RelationalPersistentEntity<T> entity, ConversionService conversionService) {
+		Number version = getVersion(instance, entity, conversionService);


How does this fail if we just use always Long? I'd have thought that it would simply work.

Yes, if I use a ConvertingPropertyAccessor, it automatically converts from Long to the required type. The next push will contain a simplified version.

And no, it currently does not work with an immutable version field. I'll add a test. I see the merit in having an immutable version field, but I fail to see how the code in AggregateChange handles modifying an immutable ID field. It uses a PropertyAccessor, too, doesn't it?

The trick is that the PropertyAccessor creates a new bean with the new value for id/version and everything after that uses the new bean returned by propertyAccessor.getBean(). See the end of the method I linked to.

Ok, I get that. But in my case it fails even earlier with an UnsupportedOperationException right here: https://github.com/spring-projects/spring-data-commons/blob/master/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java#L85

BeanWrapper seems to expect a wither method. Is the existence of a wither method a valid constraint for our use case?

thombergs · 2019-03-25T15:37:35Z

@schauder as for supporting optimistic locking for delete: Wouldn't it be cleaner to pass the whole entity through from JdbcAggregateTemplate, RelationalEntityDeleteWriter, and DeleteRoot instead of passing through only the current value of the version property? It would then be symmetrical to the update operation, at least ;).

And where does the public API start? I wasn't aware that any of this code is public API.

schauder · 2019-03-25T15:42:16Z

@thombergs symmetry is a good argument (especially when talking to a physicist).
Let's go with the full entity.

Anything that is public (i.e. public methods in public classes) are considered public API by Spring.
Having to keep these compatible really encourages usage of package scope ;-)

schauder · 2019-03-25T16:53:24Z

Yes, if an attribute is immutable we require a wither.

thombergs · 2019-03-25T20:40:19Z

@schauder The "don't touch public API" policy is hard on refactorings like this one where we have to pass through the whole stack :).

Is it OK to modify the semantics of RelationalEntityDeleteWriter#write(Object, AggregateChange) so that the Object is interpreted as the root entity itself instead of as an ID? Otherwise, I guess we'd have to create another writer next to the existing one that does basically the same just with an entity as input instead of an ID. That seems awkward to me. Or there is a better way I don't see :)

schauder · 2019-03-26T06:43:34Z

Don't we need both implementations anyway since deleting by id only should still be supported?

Of course, both implementations should reuse most of the code

schauder · 2019-04-08T09:45:55Z

@thombergs Sorry, missed the question about the formatters.

See https://github.com/spring-projects/spring-data-build/tree/master/etc/ide
and for more information about style of commit messages and more:
https://github.com/spring-projects/spring-data-build/blob/master/CONTRIBUTING.adoc

fabiocvalente · 2019-08-26T17:53:47Z

Hi, I'm changing my application from Spring Data JPA to Spring Data JDBC and we use @Version for optimistic locking.
I noticed that the version column in the databse was not being set.
After some googling I found this issue that has an open PR since March, but it has conflicts.
I would like to continue using this feature with JDBC.
Do you have an idea when this wil be available in milestone and release versions?

schauder · 2019-08-27T05:41:28Z

This is certainly one of the more pressing issues but I can't really give any dates.

If someone would rebase the PR on the current master and/or address the open issues layed out above that would certainly help.

tkvangorder · 2019-08-30T21:48:41Z

@schauder I have taken the work Tom started, rebased, and I have the update/insert working. I am going to look at the delete next. I am not yet ready for a PR, but the branch is here : https://github.com/tkvangorder/spring-data-jdbc/tree/DATAJDBC-219

schauder · 2019-09-12T05:59:06Z

There is a new PR, building on this one and replacing it: #166

…ionalDatabaseClient.

Reduce Lombok usage to within tests.

Tighten nullability constraints. Consistent Javadoc.

schauder requested changes Mar 11, 2019

View reviewed changes

thombergs force-pushed the datajdbc-219-optimistic-locking branch 2 times, most recently from af1a414 to f60919b Compare March 15, 2019 16:56

thombergs force-pushed the datajdbc-219-optimistic-locking branch from f60919b to 67eeca8 Compare March 15, 2019 17:15

schauder requested changes Mar 25, 2019

View reviewed changes

schauder mentioned this pull request Apr 11, 2019

Add support for Optimistic Locking using @Version spring-projects/spring-data-r2dbc#93

Closed

schauder closed this Sep 12, 2019

orange-buffalo mentioned this pull request Feb 28, 2020

#93 - Adding support for optimistic locking based on @Version column spring-projects/spring-data-r2dbc#314

Closed

spring-projects-issues mentioned this pull request Dec 31, 2020

Support for optimistic locking [DATAJDBC-219] #445

Closed

mp911de added a commit that referenced this pull request Feb 21, 2022

#124 - Remove deprecated DatabaseClient.execute().sql(…) and Transact…

3c3277e

…ionalDatabaseClient.

mp911de added a commit that referenced this pull request Feb 21, 2022

#124 - Polishing.

5955270

Reduce Lombok usage to within tests.

mp911de added a commit that referenced this pull request Feb 21, 2022

#124 - Polishing.

e15bd31

Tighten nullability constraints. Consistent Javadoc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAJDBC-219 - add support for optimistic locking #124

DATAJDBC-219 - add support for optimistic locking #124

thombergs commented Mar 9, 2019

schauder left a comment

schauder Mar 11, 2019

thombergs Mar 15, 2019

thombergs commented Mar 15, 2019

schauder commented Mar 25, 2019

schauder left a comment

schauder Mar 25, 2019

thombergs Mar 25, 2019

schauder Mar 25, 2019

thombergs Mar 25, 2019

thombergs commented Mar 25, 2019

schauder commented Mar 25, 2019

schauder commented Mar 25, 2019

thombergs commented Mar 25, 2019 •

edited

Loading

schauder commented Mar 26, 2019

schauder commented Apr 8, 2019

fabiocvalente commented Aug 26, 2019

schauder commented Aug 27, 2019

tkvangorder commented Aug 30, 2019

schauder commented Sep 12, 2019

DATAJDBC-219 - add support for optimistic locking #124

DATAJDBC-219 - add support for optimistic locking #124

Conversation

thombergs commented Mar 9, 2019

schauder left a comment

Choose a reason for hiding this comment

schauder Mar 11, 2019

Choose a reason for hiding this comment

thombergs Mar 15, 2019

Choose a reason for hiding this comment

thombergs commented Mar 15, 2019

schauder commented Mar 25, 2019

schauder left a comment

Choose a reason for hiding this comment

schauder Mar 25, 2019

Choose a reason for hiding this comment

thombergs Mar 25, 2019

Choose a reason for hiding this comment

schauder Mar 25, 2019

Choose a reason for hiding this comment

thombergs Mar 25, 2019

Choose a reason for hiding this comment

thombergs commented Mar 25, 2019

schauder commented Mar 25, 2019

schauder commented Mar 25, 2019

thombergs commented Mar 25, 2019 • edited Loading

schauder commented Mar 26, 2019

schauder commented Apr 8, 2019

fabiocvalente commented Aug 26, 2019

schauder commented Aug 27, 2019

tkvangorder commented Aug 30, 2019

schauder commented Sep 12, 2019

thombergs commented Mar 25, 2019 •

edited

Loading