Skip to content

DATAJDBC-219 - add support for optimistic locking #124

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

DATAJDBC-219 - add support for optimistic locking #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion ...data-jdbc/src/main/java/org/springframework/data/jdbc/core/DefaultDataAccessStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,20 @@
import java.util.Map;
import java.util.function.Predicate;

import org.springframework.core.convert.ConversionService;
import org.springframework.dao.DataRetrievalFailureException;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.dao.InvalidDataAccessApiUsageException;
import org.springframework.dao.OptimisticLockingFailureException;
import org.springframework.data.jdbc.core.convert.JdbcConverter;
import org.springframework.data.jdbc.core.convert.JdbcValue;
import org.springframework.data.jdbc.support.JdbcUtil;
import org.springframework.data.mapping.PersistentProperty;
import org.springframework.data.mapping.PersistentPropertyAccessor;
import org.springframework.data.mapping.PersistentPropertyPath;
import org.springframework.data.mapping.PropertyHandler;
import org.springframework.data.mapping.model.ConvertingPropertyAccessor;
import org.springframework.data.relational.core.conversion.RelationalConverter;
import org.springframework.data.relational.core.mapping.RelationalMappingContext;
import org.springframework.data.relational.core.mapping.RelationalPersistentEntity;
import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
Expand All @@ -48,6 +52,7 @@
import org.springframework.jdbc.support.KeyHolder;
import org.springframework.lang.Nullable;
import org.springframework.util.Assert;
import static org.springframework.data.jdbc.core.SqlGenerator.*;

/**
* The default {@link DataAccessStrategy} is to generate SQL statements based on meta data from the entity.
Expand All @@ -56,6 +61,7 @@
* @author Mark Paluch
* @author Thomas Lang
* @author Bastian Wilhelm
* @author Tom Hombergs
*/
public class DefaultDataAccessStrategy implements DataAccessStrategy {

Expand Down Expand Up @@ -124,6 +130,12 @@ public <T> Object insert(T instance, Class<T> domainType, Identifier identifier)
KeyHolder holder = new GeneratedKeyHolder();
RelationalPersistentEntity<T> persistentEntity = getRequiredPersistentEntity(domainType);

if (persistentEntity.hasVersionProperty()) {

Number newVersion = getNextVersion(instance, persistentEntity, converter.getConversionService());
setVersion(instance, persistentEntity, newVersion);
}

MapSqlParameterSource parameterSource = getParameterSource(instance, persistentEntity, "",
PersistentProperty::isIdProperty);

Expand Down Expand Up @@ -151,13 +163,46 @@ public <T> Object insert(T instance, Class<T> domainType, Identifier identifier)
*/
@Override
public <S> boolean update(S instance, Class<S> domainType) {
RelationalPersistentEntity<S> persistentEntity = getRequiredPersistentEntity(domainType);

if (persistentEntity.hasVersionProperty()) {
return updateWithVersion(instance, domainType);
} else {
return updateWithoutVersion(instance, domainType);
}
}

private <S> boolean updateWithoutVersion(S instance, Class<S> domainType) {

RelationalPersistentEntity<S> persistentEntity = getRequiredPersistentEntity(domainType);

return operations.update(sql(domainType).getUpdate(),
getParameterSource(instance, persistentEntity, "", Predicates.includeAll())) != 0;
}

private <S> boolean updateWithVersion(S instance, Class<S> domainType) {

RelationalPersistentEntity<S> persistentEntity = getRequiredPersistentEntity(domainType);

Number oldVersion = getVersion(instance, persistentEntity, converter.getConversionService());
Number newVersion = getNextVersion(instance, persistentEntity, converter.getConversionService());
setVersion(instance, persistentEntity, newVersion);

MapSqlParameterSource parameterSource = getParameterSource(instance, persistentEntity, "", Predicates.includeAll());
parameterSource.addValue(VERSION_PARAMETER, oldVersion);
int affectedRows = operations.update(sql(domainType).getUpdateWithVersion(),
parameterSource);

if (affectedRows == 0) {
// reverting version update on entity
setVersion(instance, persistentEntity, oldVersion);
throw new OptimisticLockingFailureException(
String.format("Optimistic lock exception on saving entity of type %s.", persistentEntity.getName()));
}

return true;
}

/*
* (non-Javadoc)
* @see org.springframework.data.jdbc.core.DataAccessStrategy#delete(java.lang.Object, java.lang.Class)
Expand Down Expand Up @@ -354,7 +399,7 @@ private <S, ID> ID getIdValueOrNull(S instance, RelationalPersistentEntity<S> pe
}

private static <S, ID> boolean isIdPropertyNullOrScalarZero(@Nullable ID idValue,
RelationalPersistentEntity<S> persistentEntity) {
RelationalPersistentEntity<S> persistentEntity) {

RelationalPersistentProperty idProperty = persistentEntity.getIdProperty();
return idValue == null //
Expand Down Expand Up @@ -481,4 +526,31 @@ static Predicate<RelationalPersistentProperty> includeAll() {
return it -> false;
}
}
@Nullable
private <T> Number getVersion(T instance, RelationalPersistentEntity<T> entity, ConversionService conversionService) {
RelationalPersistentProperty versionProperty = entity.getRequiredVersionProperty();
PersistentPropertyAccessor<T> propertyAccessor = entity.getPropertyAccessor(instance);
ConvertingPropertyAccessor<T> convertingPropertyAccessor = new ConvertingPropertyAccessor<>(propertyAccessor, conversionService);
return convertingPropertyAccessor.getProperty(versionProperty, Number.class);
}

private <T> Number getNextVersion(T instance, RelationalPersistentEntity<T> entity, ConversionService conversionService) {
Number version = getVersion(instance, entity, conversionService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Class<?> versionType = entity.getRequiredVersionProperty().getType();
if (versionType == Integer.class || versionType == int.class) {
return version == null ? 1 : version.intValue() + 1;
} else if (versionType == Long.class || versionType == long.class) {
return version == null ? 1L : version.longValue() + 1;
} else if (versionType == Short.class || versionType == short.class) {
return version == null ? (short) 1 : (short) (version.shortValue() + 1);
}
throw new IllegalStateException(String.format("Entity '%s' has version property of invalid type '%s'.", entity.getType().getName(), entity.getVersionProperty().getType().getName()));
}

private <T> void setVersion(T instance, RelationalPersistentEntity<T> entity, Number newVersion) {
RelationalPersistentProperty versionProperty = entity.getRequiredVersionProperty();
PersistentPropertyAccessor<T> accessor = versionProperty.getOwner().getPropertyAccessor(instance);
accessor.setProperty(versionProperty, newVersion);
}

}
26 changes: 23 additions & 3 deletions spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/SqlGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,12 @@
* @author Yoichi Imai
* @author Bastian Wilhelm
* @author Oleksandr Kucher
* @author Tom Hombergs
*/
class SqlGenerator {

static final String VERSION_PARAMETER = "___oldOptimisticLockingVersion";

private final RelationalPersistentEntity<?> entity;
private final RelationalMappingContext context;
private final List<String> columnNames = new ArrayList<>();
Expand All @@ -62,6 +65,7 @@ class SqlGenerator {
private final Lazy<String> countSql = Lazy.of(this::createCountSql);

private final Lazy<String> updateSql = Lazy.of(this::createUpdateSql);
private final Lazy<String> updateWithVersionSql = Lazy.of(this::createUpdateWithVersionSql);

private final Lazy<String> deleteByIdSql = Lazy.of(this::createDeleteSql);
private final Lazy<String> deleteByListSql = Lazy.of(this::createDeleteByListSql);
Expand Down Expand Up @@ -176,6 +180,10 @@ String getUpdate() {
return updateSql.get();
}

String getUpdateWithVersion() {
return updateWithVersionSql.get();
}

String getCount() {
return countSql.get();
}
Expand Down Expand Up @@ -343,8 +351,8 @@ private String createInsertSql(Set<String> additionalColumns) {
String tableColumns = String.join(", ", columnNamesForInsert);

String parameterNames = columnNamesForInsert.stream()//
.map(this::columnNameToParameterName)
.map(n -> String.format(":%s", n))//
.map(this::columnNameToParameterName) //
.map(n -> String.format(":%s", n)) //
.collect(Collectors.joining(", "));

return String.format(insertTemplate, entity.getTableName(), tableColumns, parameterNames);
Expand All @@ -369,6 +377,18 @@ private String createUpdateSql() {
);
}

private String createUpdateWithVersionSql() {
String whereConditionTemplate = " AND %s = :%s";

String whereCondition = String.format( //
whereConditionTemplate, //
entity.getVersionProperty().getColumnName(), //
VERSION_PARAMETER //
);

return createUpdateSql() + whereCondition;
}

private String createDeleteSql() {
return String.format("DELETE FROM %s WHERE %s = :id", entity.getTableName(), entity.getIdColumn());
}
Expand Down Expand Up @@ -458,7 +478,7 @@ private String cascadeConditions(String innerCondition, PersistentPropertyPath<R
);
}

private String columnNameToParameterName(String columnName){
private String columnNameToParameterName(String columnName) {
return parameterPattern.matcher(columnName).replaceAll("");
}
}
155 changes: 155 additions & 0 deletions ...c/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Function;

import org.assertj.core.api.SoftAssertions;
import org.junit.Assume;
Expand All @@ -37,7 +38,9 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.dao.OptimisticLockingFailureException;
import org.springframework.data.annotation.Id;
import org.springframework.data.annotation.Version;
import org.springframework.data.jdbc.testing.DatabaseProfileValueSource;
import org.springframework.data.jdbc.testing.TestConfiguration;
import org.springframework.data.relational.core.conversion.RelationalConverter;
Expand All @@ -58,6 +61,7 @@
* @author Jens Schauder
* @author Thomas Lang
* @author Mark Paluch
* @author Tom Hombergs
*/
@ContextConfiguration
@Transactional
Expand Down Expand Up @@ -434,6 +438,60 @@ public void saveAndLoadAnEntityWithByteArray() {
assertThat(reloaded.binaryData).isEqualTo(new byte[] { 1, 23, 42 });
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithLongVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithLongVersion(), Number::longValue);
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithPrimitiveLongVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithPrimitiveLongVersion(), Number::longValue);
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithIntegerVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithIntegerVersion(), Number::intValue);
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithPrimitiveIntegerVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithPrimitiveIntegerVersion(), Number::intValue);
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithShortVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithShortVersion(), Number::shortValue);
}

@Test // DATAJDBC-219
public void saveAndUpdateAggregateWithPrimitiveShortVersion() {
saveAndUpdateAggregateWithVersion(new AggregateWithPrimitiveShortVersion(), Number::shortValue);
}

private <T extends Number> void saveAndUpdateAggregateWithVersion(VersionedAggregate aggregate, Function<Number, T> toConcreteNumber) {

template.save(aggregate);

VersionedAggregate reloadedAggregate = template.findById(aggregate.getId(), aggregate.getClass());
assertThat(reloadedAggregate.getVersion()).isEqualTo(toConcreteNumber.apply(1))
.withFailMessage("version field should initially have the value 1");
template.save(reloadedAggregate);

VersionedAggregate updatedAggregate = template.findById(aggregate.getId(), aggregate.getClass());
assertThat(updatedAggregate.getVersion()).isEqualTo(toConcreteNumber.apply(2))
.withFailMessage("version field should increment by one with each save");

reloadedAggregate.setVersion(toConcreteNumber.apply(1));
assertThatThrownBy(() -> template.save(reloadedAggregate))
.hasRootCauseInstanceOf(OptimisticLockingFailureException.class)
.withFailMessage("saving an aggregate with an outdated version should raise an exception");

reloadedAggregate.setVersion(toConcreteNumber.apply(3));
assertThatThrownBy(() -> template.save(reloadedAggregate))
.hasRootCauseInstanceOf(OptimisticLockingFailureException.class)
.withFailMessage("saving an aggregate with a future version should raise an exception");
}

private static void assumeNot(String dbProfileName) {

Assume.assumeTrue("true"
Expand Down Expand Up @@ -522,6 +580,103 @@ static class ElementNoId {
private String content;
}

@Data
static abstract class VersionedAggregate {

@Id private Long id;

abstract Number getVersion();

abstract void setVersion(Number newVersion);
}

@Data
@Table("VERSIONED_AGGREGATE")
static class AggregateWithLongVersion extends VersionedAggregate {

@Version private Long version;

@Override
void setVersion(Number newVersion) {
this.version = (Long) newVersion;
}
}

@Table("VERSIONED_AGGREGATE")
static class AggregateWithPrimitiveLongVersion extends VersionedAggregate {

@Version private long version;

@Override
void setVersion(Number newVersion) {
this.version = (long) newVersion;
}

@Override
Number getVersion(){
return this.version;
}
}

@Data
@Table("VERSIONED_AGGREGATE")
static class AggregateWithIntegerVersion extends VersionedAggregate {

@Version private Integer version;

@Override
void setVersion(Number newVersion) {
this.version = (Integer) newVersion;
}
}

@Table("VERSIONED_AGGREGATE")
static class AggregateWithPrimitiveIntegerVersion extends VersionedAggregate {

@Version private int version;

@Override
void setVersion(Number newVersion) {
this.version = (int) newVersion;
}

@Override
Number getVersion(){
return this.version;
}
}

@Data
@Table("VERSIONED_AGGREGATE")
static class AggregateWithShortVersion extends VersionedAggregate {

@Version private Short version;

@Override
void setVersion(Number newVersion) {
this.version = (Short) newVersion;
}
}

@Table("VERSIONED_AGGREGATE")
static class AggregateWithPrimitiveShortVersion extends VersionedAggregate {

@Version
private short version;

@Override
void setVersion(Number newVersion) {
this.version = (short) newVersion;
}

@Override
Number getVersion(){
return this.version;
}

}


@Configuration
@Import(TestConfiguration.class)
static class Config {
Expand Down
Loading