Skip to content

Fix inconsistency of JdbcAggregateTemplate#save return value. #1208

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 4 commits into from
Closed

Fix inconsistency of JdbcAggregateTemplate#save return value. #1208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44bf59c
1201-update-return-value - Prepare branch
ctailor2 Mar 18, 2022
a6cb5b4
Capture execution results for update db actions to fix root reference…
ctailor2 Mar 18, 2022
a591119
Remove unused DbAction.Update.
ctailor2 Mar 22, 2022
405b093
Pass the root from BeforeSaveCallback through to InsertRoot,UpdateRoot
ctailor2 Mar 16, 2022
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
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1201-update-return-value-SNAPSHOT</version>
<packaging>pom</packaging>

<name>Spring Data Relational Parent</name>
Expand Down
2 changes: 1 addition & 1 deletion spring-data-jdbc-distribution/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1201-update-return-value-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
4 changes: 2 additions & 2 deletions spring-data-jdbc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<modelVersion>4.0.0</modelVersion>

<artifactId>spring-data-jdbc</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1201-update-return-value-SNAPSHOT</version>

<name>Spring Data JDBC</name>
<description>Spring Data module for JDBC repositories.</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1201-update-return-value-SNAPSHOT</version>
</parent>

<properties>
Expand Down
20 changes: 10 additions & 10 deletions ...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
import org.springframework.data.jdbc.core.convert.JdbcConverter;
import org.springframework.data.relational.core.conversion.AggregateChange;
import org.springframework.data.relational.core.conversion.AggregateChangeWithRoot;
import org.springframework.data.relational.core.conversion.DbAction;
import org.springframework.data.relational.core.conversion.DbActionExecutionException;
import org.springframework.data.relational.core.conversion.MutableAggregateChange;
import org.springframework.lang.Nullable;

/**
* Executes an {@link MutableAggregateChange}.
Expand All @@ -42,20 +42,22 @@ class AggregateChangeExecutor {
this.accessStrategy = accessStrategy;
}

@Nullable
<T> T execute(AggregateChange<T> aggregateChange) {
<T> T execute(AggregateChangeWithRoot<T> aggregateChange) {

JdbcAggregateChangeExecutionContext executionContext = new JdbcAggregateChangeExecutionContext(converter,
accessStrategy);

aggregateChange.forEachAction(action -> execute(action, executionContext));

T root = executionContext.populateIdsIfNecessary();
if (root == null) {
root = aggregateChange.getEntity();
}
return executionContext.populateIdsIfNecessary();
}

return root;
<T> void execute(AggregateChange<T> aggregateChange) {

JdbcAggregateChangeExecutionContext executionContext = new JdbcAggregateChangeExecutionContext(converter,
accessStrategy);

aggregateChange.forEachAction(action -> execute(action, executionContext));
}

private void execute(DbAction<?> action, JdbcAggregateChangeExecutionContext executionContext) {
Expand All @@ -69,8 +71,6 @@ private void execute(DbAction<?> action, JdbcAggregateChangeExecutionContext exe
executionContext.executeInsertBatch((DbAction.InsertBatch<?>) action);
} else if (action instanceof DbAction.UpdateRoot) {
executionContext.executeUpdateRoot((DbAction.UpdateRoot<?>) action);
} else if (action instanceof DbAction.Update) {
executionContext.executeUpdate((DbAction.Update<?>) action);
} else if (action instanceof DbAction.Delete) {
executionContext.executeDelete((DbAction.Delete<?>) action);
} else if (action instanceof DbAction.DeleteAll) {
Expand Down
70 changes: 26 additions & 44 deletions ...src/main/java/org/springframework/data/jdbc/core/JdbcAggregateChangeExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,7 @@
*/
package org.springframework.data.jdbc.core;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.*;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;

Expand All @@ -40,6 +32,7 @@
import org.springframework.data.mapping.context.MappingContext;
import org.springframework.data.relational.core.conversion.DbAction;
import org.springframework.data.relational.core.conversion.DbActionExecutionResult;
import org.springframework.data.relational.core.conversion.IdValueSource;
import org.springframework.data.relational.core.mapping.PersistentPropertyPathExtension;
import org.springframework.data.relational.core.mapping.RelationalPersistentEntity;
import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
Expand Down Expand Up @@ -108,15 +101,7 @@ <T> void executeUpdateRoot(DbAction.UpdateRoot<T> update) {
} else {
updateWithoutVersion(update);
}
}

<T> void executeUpdate(DbAction.Update<T> update) {

if (!accessStrategy.update(update.getEntity(), update.getEntityType())) {

throw new IncorrectUpdateSemanticsDataAccessException(
String.format(UPDATE_FAILED, update.getEntity(), getIdFrom(update)));
}
add(new DbActionExecutionResult(update));
}

<T> void executeDeleteRoot(DbAction.DeleteRoot<T> delete) {
Expand Down Expand Up @@ -203,11 +188,12 @@ private DbAction.WithEntity<?> getIdOwningAction(DbAction.WithEntity<?> action,

private Object getPotentialGeneratedIdFrom(DbAction.WithEntity<?> idOwningAction) {

if (idOwningAction instanceof DbAction.WithGeneratedId) {
if (IdValueSource.GENERATED.equals(idOwningAction.getIdValueSource())) {

Object generatedId;
DbActionExecutionResult dbActionExecutionResult = results.get(idOwningAction);
generatedId = dbActionExecutionResult == null ? null : dbActionExecutionResult.getId();
Object generatedId = Optional.ofNullable(dbActionExecutionResult) //
.map(DbActionExecutionResult::getGeneratedId) //
.orElse(null);

if (generatedId != null) {
return generatedId;
Expand All @@ -227,12 +213,8 @@ private Object getIdFrom(DbAction.WithEntity<?> idOwningAction) {
return identifier;
}

@SuppressWarnings("unchecked")
@Nullable
<T> T populateIdsIfNecessary() {

T newRoot = null;

// have the results so that the inserts on the leaves come first.
List<DbActionExecutionResult> reverseResults = new ArrayList<>(results.values());
Collections.reverse(reverseResults);
Expand All @@ -241,33 +223,29 @@ <T> T populateIdsIfNecessary() {

for (DbActionExecutionResult result : reverseResults) {

DbAction<?> action = result.getAction();
DbAction.WithEntity<?> action = result.getAction();

if (!(action instanceof DbAction.WithGeneratedId)) {
continue;
Object newEntity = setIdAndCascadingProperties(action, result.getGeneratedId(), cascadingValues);

if (action instanceof DbAction.InsertRoot || action instanceof DbAction.UpdateRoot) {
//noinspection unchecked
return (T) newEntity;
}

DbAction.WithEntity<?> withEntity = (DbAction.WithGeneratedId<?>) action;
Object newEntity = setIdAndCascadingProperties(withEntity, result.getId(), cascadingValues);

// the id property was immutable so we have to propagate changes up the tree
if (newEntity != withEntity.getEntity()) {

if (action instanceof DbAction.Insert) {
DbAction.Insert<?> insert = (DbAction.Insert<?>) action;

Pair<?, ?> qualifier = insert.getQualifier();
if (newEntity != action.getEntity() && action instanceof DbAction.Insert) {
DbAction.Insert<?> insert = (DbAction.Insert<?>) action;

cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(),
qualifier == null ? null : qualifier.getSecond(), newEntity);
Pair<?, ?> qualifier = insert.getQualifier();

} else if (action instanceof DbAction.InsertRoot) {
newRoot = (T) newEntity;
}
cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(),
qualifier == null ? null : qualifier.getSecond(), newEntity);
}
}

return newRoot;
throw new IllegalStateException(
String.format("Cannot retrieve the resulting instance unless a %s or %s action was successfully executed.",
DbAction.InsertRoot.class.getName(), DbAction.UpdateRoot.class.getName()));
}

private <S> Object setIdAndCascadingProperties(DbAction.WithEntity<S> action, @Nullable Object generatedId,
Expand All @@ -279,7 +257,7 @@ private <S> Object setIdAndCascadingProperties(DbAction.WithEntity<S> action, @N
.getRequiredPersistentEntity(action.getEntityType());
PersistentPropertyAccessor<S> propertyAccessor = converter.getPropertyAccessor(persistentEntity, originalEntity);

if (generatedId != null && persistentEntity.hasIdProperty()) {
if (IdValueSource.GENERATED.equals(action.getIdValueSource())) {
propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), generatedId);
}

Expand All @@ -301,6 +279,10 @@ private PersistentPropertyPath<?> getRelativePath(DbAction<?> action, Persistent
return pathToValue;
}

if (action instanceof DbAction.UpdateRoot) {
return pathToValue;
}

throw new IllegalArgumentException(String.format("DbAction of type %s is not supported.", action.getClass()));
}

Expand Down
34 changes: 14 additions & 20 deletions spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.springframework.data.mapping.IdentifierAccessor;
import org.springframework.data.mapping.callback.EntityCallbacks;
import org.springframework.data.relational.core.conversion.AggregateChange;
import org.springframework.data.relational.core.conversion.AggregateChangeWithRoot;
import org.springframework.data.relational.core.conversion.MutableAggregateChange;
import org.springframework.data.relational.core.conversion.RelationalEntityDeleteWriter;
import org.springframework.data.relational.core.conversion.RelationalEntityInsertWriter;
Expand Down Expand Up @@ -61,8 +62,6 @@ public class JdbcAggregateTemplate implements JdbcAggregateOperations {
private final RelationalMappingContext context;

private final RelationalEntityDeleteWriter jdbcEntityDeleteWriter;
private final RelationalEntityInsertWriter jdbcEntityInsertWriter;
private final RelationalEntityUpdateWriter jdbcEntityUpdateWriter;

private final DataAccessStrategy accessStrategy;
private final AggregateChangeExecutor executor;
Expand Down Expand Up @@ -92,8 +91,6 @@ public JdbcAggregateTemplate(ApplicationContext publisher, RelationalMappingCont
this.accessStrategy = dataAccessStrategy;
this.converter = converter;

this.jdbcEntityInsertWriter = new RelationalEntityInsertWriter(context);
this.jdbcEntityUpdateWriter = new RelationalEntityUpdateWriter(context);
this.jdbcEntityDeleteWriter = new RelationalEntityDeleteWriter(context);

this.executor = new AggregateChangeExecutor(converter, accessStrategy);
Expand Down Expand Up @@ -122,8 +119,6 @@ public JdbcAggregateTemplate(ApplicationEventPublisher publisher, RelationalMapp
this.accessStrategy = dataAccessStrategy;
this.converter = converter;

this.jdbcEntityInsertWriter = new RelationalEntityInsertWriter(context);
this.jdbcEntityUpdateWriter = new RelationalEntityUpdateWriter(context);
this.jdbcEntityDeleteWriter = new RelationalEntityDeleteWriter(context);
this.executor = new AggregateChangeExecutor(converter, accessStrategy);
}
Expand All @@ -146,7 +141,7 @@ public <T> T save(T instance) {

RelationalPersistentEntity<?> persistentEntity = context.getRequiredPersistentEntity(instance.getClass());

Function<T, MutableAggregateChange<T>> changeCreator = persistentEntity.isNew(instance)
Function<T, AggregateChangeWithRoot<T>> changeCreator = persistentEntity.isNew(instance)
? entity -> createInsertChange(prepareVersionForInsert(entity))
: entity -> createUpdateChange(prepareVersionForUpdate(entity));

Expand Down Expand Up @@ -286,18 +281,18 @@ public void deleteAll(Class<?> domainType) {
executor.execute(change);
}

private <T> T store(T aggregateRoot, Function<T, MutableAggregateChange<T>> changeCreator,
private <T> T store(T aggregateRoot, Function<T, AggregateChangeWithRoot<T>> changeCreator,
RelationalPersistentEntity<?> persistentEntity) {

Assert.notNull(aggregateRoot, "Aggregate instance must not be null!");

aggregateRoot = triggerBeforeConvert(aggregateRoot);

MutableAggregateChange<T> change = changeCreator.apply(aggregateRoot);
AggregateChangeWithRoot<T> change = changeCreator.apply(aggregateRoot);

aggregateRoot = triggerBeforeSave(change.getEntity(), change);
aggregateRoot = triggerBeforeSave(change.getRoot(), change);

change.setEntity(aggregateRoot);
change.setRoot(aggregateRoot);

T entityAfterExecution = executor.execute(change);

Expand All @@ -313,25 +308,24 @@ private <T> void deleteTree(Object id, @Nullable T entity, Class<T> domainType)
MutableAggregateChange<T> change = createDeletingChange(id, entity, domainType);

entity = triggerBeforeDelete(entity, id, change);
change.setEntity(entity);

executor.execute(change);

triggerAfterDelete(entity, id, change);
}

private <T> MutableAggregateChange<T> createInsertChange(T instance) {
private <T> AggregateChangeWithRoot<T> createInsertChange(T instance) {

MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(instance);
jdbcEntityInsertWriter.write(instance, aggregateChange);
AggregateChangeWithRoot<T> aggregateChange = MutableAggregateChange.forSave(instance);
new RelationalEntityInsertWriter<T>(context).write(instance, aggregateChange);
return aggregateChange;
}

private <T> MutableAggregateChange<T> createUpdateChange(EntityAndPreviousVersion<T> entityAndVersion) {
private <T> AggregateChangeWithRoot<T> createUpdateChange(EntityAndPreviousVersion<T> entityAndVersion) {

MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(entityAndVersion.entity,
AggregateChangeWithRoot<T> aggregateChange = MutableAggregateChange.forSave(entityAndVersion.entity,
entityAndVersion.version);
jdbcEntityUpdateWriter.write(entityAndVersion.entity, aggregateChange);
new RelationalEntityUpdateWriter<T>(context).write(entityAndVersion.entity, aggregateChange);
return aggregateChange;
}

Expand Down Expand Up @@ -383,14 +377,14 @@ private <T> MutableAggregateChange<T> createDeletingChange(Object id, @Nullable
previousVersion = RelationalEntityVersionUtils.getVersionNumberFromEntity(entity, persistentEntity, converter);
}
}
MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forDelete(domainType, entity, previousVersion);
MutableAggregateChange<T> aggregateChange = MutableAggregateChange.forDelete(domainType, previousVersion);
jdbcEntityDeleteWriter.write(id, aggregateChange);
return aggregateChange;
}

private MutableAggregateChange<?> createDeletingChange(Class<?> domainType) {

MutableAggregateChange<?> aggregateChange = MutableAggregateChange.forDelete(domainType, null);
MutableAggregateChange<?> aggregateChange = MutableAggregateChange.forDelete(domainType);
jdbcEntityDeleteWriter.write(null, aggregateChange);
return aggregateChange;
}
Expand Down
Loading