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.

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

Fix id setting for partial updates of collections of immutable types #1920

Closed
wants to merge 2 commits into from
Closed

Fix id setting for partial updates of collections of immutable types #1920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ebf325
prepare branch
schauder Oct 18, 2024
0fcf496
Fix id setting for partial updates of collections of immutables.
schauder Oct 18, 2024
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.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-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.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-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.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-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.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-SNAPSHOT</version>
</parent>

<properties>
Expand Down
63 changes: 43 additions & 20 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,16 +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.Optional;
import java.util.Set;
import java.util.*;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -241,7 +232,7 @@ private Object getIdFrom(DbAction.WithEntity<?> idOwningAction) {
RelationalPersistentEntity<?> persistentEntity = getRequiredPersistentEntity(idOwningAction.getEntityType());
Object identifier = persistentEntity.getIdentifierAccessor(idOwningAction.getEntity()).getIdentifier();

Assert.state(identifier != null,() -> "Couldn't obtain a required id value for " + persistentEntity);
Assert.state(identifier != null, () -> "Couldn't obtain a required id value for " + persistentEntity);

return identifier;
}
Expand All @@ -268,12 +259,22 @@ <T> List<T> populateIdsIfNecessary() {
}

// the id property was immutable, so we have to propagate changes up the tree
if (newEntity != action.getEntity() && action instanceof DbAction.Insert<?> insert) {
if (action instanceof DbAction.Insert<?> insert) {

Pair<?, ?> qualifier = insert.getQualifier();
Object qualifierValue = qualifier == null ? null : qualifier.getSecond();

cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(),
qualifier == null ? null : qualifier.getSecond(), newEntity);
if (newEntity != action.getEntity()) {

cascadingValues.stage(insert.getDependingOn(), insert.getPropertyPath(),
qualifierValue, newEntity);

} else if (insert.getPropertyPath().getLeafProperty().isCollectionLike()) {

cascadingValues.gather(insert.getDependingOn(), insert.getPropertyPath(),
qualifierValue, newEntity);

}
}
}

Expand Down Expand Up @@ -360,7 +361,7 @@ private static class StagedValues {
static final List<MultiValueAggregator> aggregators = Arrays.asList(SetAggregator.INSTANCE, MapAggregator.INSTANCE,
ListAggregator.INSTANCE, SingleElementAggregator.INSTANCE);

Map<DbAction, Map<PersistentPropertyPath, Object>> values = new HashMap<>();
Map<DbAction, Map<PersistentPropertyPath, StagedValue>> values = new HashMap<>();

/**
* Adds a value that needs to be set in an entity higher up in the tree of entities in the aggregate. If the
Expand All @@ -375,18 +376,26 @@ private static class StagedValues {
*/
@SuppressWarnings("unchecked")
<T> void stage(DbAction<?> action, PersistentPropertyPath path, @Nullable Object qualifier, Object value) {
gather(action, path, qualifier, value);
values.get(action).get(path).isStaged = true;
}

<T> void gather(DbAction<?> action, PersistentPropertyPath path, @Nullable Object qualifier, Object value) {

MultiValueAggregator<T> aggregator = getAggregatorFor(path);

Map<PersistentPropertyPath, Object> valuesForPath = this.values.computeIfAbsent(action,
Map<PersistentPropertyPath, StagedValue> valuesForPath = this.values.computeIfAbsent(action,
dbAction -> new HashMap<>());

T currentValue = (T) valuesForPath.computeIfAbsent(path,
persistentPropertyPath -> aggregator.createEmptyInstance());
StagedValue stagedValue = valuesForPath.computeIfAbsent(path,
persistentPropertyPath -> new StagedValue(aggregator.createEmptyInstance()));
T currentValue = (T) stagedValue.value;

Object newValue = aggregator.add(currentValue, qualifier, value);

valuesForPath.put(path, newValue);
stagedValue.value = newValue;

valuesForPath.put(path, stagedValue);
}

private MultiValueAggregator getAggregatorFor(PersistentPropertyPath path) {
Expand All @@ -408,7 +417,21 @@ private MultiValueAggregator getAggregatorFor(PersistentPropertyPath path) {
* property.
*/
void forEachPath(DbAction<?> dbAction, BiConsumer<PersistentPropertyPath, Object> action) {
values.getOrDefault(dbAction, Collections.emptyMap()).forEach(action);
values.getOrDefault(dbAction, Collections.emptyMap()).forEach((persistentPropertyPath, stagedValue) -> {
if (stagedValue.isStaged) {
action.accept(persistentPropertyPath, stagedValue.value);
}
});
}

}

private static class StagedValue {
Object value;
boolean isStaged;

public StagedValue(Object value) {
this.value = value;
}
}

Expand Down
71 changes: 22 additions & 49 deletions ...ava/org/springframework/data/jdbc/repository/JdbcRepositoryWithListsIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.data.annotation.Id;
import org.springframework.data.annotation.PersistenceCreator;
import org.springframework.data.jdbc.repository.support.JdbcRepositoryFactory;
import org.springframework.data.jdbc.testing.EnabledOnFeature;
import org.springframework.data.jdbc.testing.IntegrationTest;
Expand All @@ -55,8 +56,7 @@ public class JdbcRepositoryWithListsIntegrationTests {

private static DummyEntity createDummyEntity() {

DummyEntity entity = new DummyEntity();
entity.setName("Entity Name");
DummyEntity entity = new DummyEntity(null, "Entity Name", new ArrayList<>());
return entity;
}

Expand Down Expand Up @@ -94,7 +94,7 @@ public void saveAndLoadNonEmptyList() {
assertThat(reloaded.content) //
.isNotNull() //
.extracting(e -> e.id) //
.containsExactlyInAnyOrder(element1.id, element2.id);
.containsExactlyInAnyOrder(entity.content.get(0).id, entity.content.get(1).id);
}

@Test // GH-1159
Expand Down Expand Up @@ -147,24 +147,25 @@ public void findAllLoadsList() {
@EnabledOnFeature(SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES)
public void updateList() {

Element element1 = createElement("one");
Element element2 = createElement("two");
Element element3 = createElement("three");
Element element1 = new Element("one");
Element element2 = new Element("two");
Element element3 = new Element("three");

DummyEntity entity = createDummyEntity();
entity.content.add(element1);
entity.content.add(element2);

entity = repository.save(entity);

entity.content.remove(element1);
element2.content = "two changed";
entity.content.remove(0);
entity.content.set(0, new Element(entity.content.get(0).id, "two changed"));
entity.content.add(element3);

entity = repository.save(entity);

assertThat(entity.id).isNotNull();
assertThat(entity.content).allMatch(v -> v.id != null);
assertThat(entity.content).hasSize(2);

DummyEntity reloaded = repository.findById(entity.id).orElseThrow(AssertionFailedError::new);

Expand All @@ -175,8 +176,8 @@ public void updateList() {
assertThat(reloaded.content) //
.extracting(e -> e.id, e -> e.content) //
.containsExactly( //
tuple(element2.id, "two changed"), //
tuple(element3.id, "three") //
tuple(entity.content.get(0).id, "two changed"), //
tuple(entity.content.get(1).id, "three") //
);

Long count = template.queryForObject("SELECT count(1) FROM Element", new HashMap<>(), Long.class);
Expand All @@ -186,8 +187,8 @@ public void updateList() {
@Test // DATAJDBC-130
public void deletingWithList() {

Element element1 = createElement("one");
Element element2 = createElement("two");
Element element1 = new Element("one");
Element element2 = new Element("two");

DummyEntity entity = createDummyEntity();
entity.content.add(element1);
Expand All @@ -203,13 +204,6 @@ public void deletingWithList() {
assertThat(count).isEqualTo(0);
}

private Element createElement(String content) {

Element element = new Element();
element.content = content;
return element;
}

interface DummyEntityRepository extends CrudRepository<DummyEntity, Long> {}

interface RootRepository extends CrudRepository<Root, Long> {}
Expand All @@ -229,43 +223,22 @@ RootRepository rootRepository(JdbcRepositoryFactory factory) {
}
}

static class DummyEntity {
record DummyEntity(@Id Long id, String name, List<Element> content) {
}

String name;
List<Element> content = new ArrayList<>();
@Id private Long id;
record Element(@Id Long id, String content) {

public String getName() {
return this.name;
}
@PersistenceCreator
Element {}

public List<Element> getContent() {
return this.content;
Element() {
this(null, null);
}

public Long getId() {
return this.id;
Element(String content) {
this(null, content);
}

public void setName(String name) {
this.name = name;
}

public void setContent(List<Element> content) {
this.content = content;
}

public void setId(Long id) {
this.id = id;
}
}

static class Element {

String content;
@Id private Long id;

public Element() {}
}

static class Root {
Expand Down
4 changes: 2 additions & 2 deletions spring-data-r2dbc/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-r2dbc</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-SNAPSHOT</version>

<name>Spring Data R2DBC</name>
<description>Spring Data module for R2DBC</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-SNAPSHOT</version>
</parent>

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

<artifactId>spring-data-relational</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-SNAPSHOT</version>

<name>Spring Data Relational</name>
<description>Spring Data Relational support</description>

<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.4.0-SNAPSHOT</version>
<version>3.4.0-1907-partial-insert-immutables-SNAPSHOT</version>
</parent>

<properties>
Expand Down
13 changes: 7 additions & 6 deletions ...elational/src/main/java/org/springframework/data/relational/core/conversion/DbAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
*/
package org.springframework.data.relational.core.conversion;

import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;

import org.springframework.data.mapping.PersistentPropertyPath;
Expand Down Expand Up @@ -479,15 +482,13 @@ interface WithDependingOn<T> extends WithPropertyPath<T>, WithEntity<T> {
default Pair<PersistentPropertyPath<RelationalPersistentProperty>, Object> getQualifier() {

Map<PersistentPropertyPath<RelationalPersistentProperty>, Object> qualifiers = getQualifiers();
if (qualifiers.isEmpty())
if (qualifiers.isEmpty()) {
return null;

if (qualifiers.size() > 1) {
throw new IllegalStateException("Can't handle more then one qualifier");
}

Map.Entry<PersistentPropertyPath<RelationalPersistentProperty>, Object> entry = qualifiers.entrySet().iterator()
.next();
Set<Map.Entry<PersistentPropertyPath<RelationalPersistentProperty>, Object>> entries = qualifiers.entrySet();
Map.Entry<PersistentPropertyPath<RelationalPersistentProperty>, Object> entry = entries.stream().sorted(Comparator.comparing(e -> -e.getKey().getLength())).findFirst().get();

if (entry.getValue() == null) {
return null;
}
Expand Down
Loading