Skip to content

Commit dd42db6

Browse files
mipo256mipo256
committed
GH-2003 ID generation via Sequence should be omitted in UPDATE queries
1 parent 8c017fc commit dd42db6

14 files changed

+539
-16
lines changed

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

-1
Original file line numberDiff line numberDiff line change
@@ -557,7 +557,6 @@ private <T> RootAggregateChange<T> createInsertChange(T instance) {
557557
}
558558

559559
private <T> RootAggregateChange<T> createUpdateChange(EntityAndPreviousVersion<T> entityAndVersion) {
560-
561560
RootAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(entityAndVersion.entity,
562561
entityAndVersion.version);
563562
new RelationalEntityUpdateWriter<T>(context).write(entityAndVersion.entity, aggregateChange);

‎spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/mapping/IdGeneratingBeforeSaveCallback.java

+29-8
Original file line numberDiff line numberDiff line change
@@ -45,26 +45,47 @@ public Object onBeforeSave(Object aggregate, MutableAggregateChange<Object> aggr
4545
Assert.notNull(aggregate, "The aggregate cannot be null at this point");
4646

4747
RelationalPersistentEntity<?> persistentEntity = relationalMappingContext.getPersistentEntity(aggregate.getClass());
48+
49+
if (persistentEntity.getIdProperty() == null) {
50+
return aggregate;
51+
}
52+
53+
// we're doing INSERT and ID property value is not set explicitly by client
54+
if (persistentEntity.isNew(aggregate) && !idIsProvidedByClient(aggregate, persistentEntity)) {
55+
return potentiallyFetchIdFromSequence(aggregate, persistentEntity);
56+
} else {
57+
return aggregate;
58+
}
59+
}
60+
61+
private boolean idIsProvidedByClient(Object aggregate, RelationalPersistentEntity<?> persistentEntity) {
62+
Object identifier = persistentEntity.getIdentifierAccessor(aggregate).getIdentifier();
63+
64+
if (persistentEntity.getIdProperty().getType().isPrimitive()) {
65+
return identifier instanceof Number num && num.longValue() != 0L;
66+
} else {
67+
return identifier != null;
68+
}
69+
}
70+
71+
private Object potentiallyFetchIdFromSequence(Object aggregate, RelationalPersistentEntity<?> persistentEntity) {
4872
Optional<SqlIdentifier> idSequence = persistentEntity.getIdSequence();
4973

5074
if (dialect.getIdGeneration().sequencesSupported()) {
51-
52-
if (persistentEntity.getIdProperty() != null) {
53-
idSequence
75+
idSequence
5476
.map(s -> dialect.getIdGeneration().createSequenceQuery(s))
5577
.ifPresent(sql -> {
5678
Long idValue = operations.queryForObject(sql, Map.of(), (rs, rowNum) -> rs.getLong(1));
5779
PersistentPropertyAccessor<Object> propertyAccessor = persistentEntity.getPropertyAccessor(aggregate);
5880
propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), idValue);
5981
});
60-
}
6182
} else {
6283
if (idSequence.isPresent()) {
6384
LOG.warn("""
64-
It seems you're trying to insert an aggregate of type '%s' annotated with @TargetSequence, but the problem is RDBMS you're
65-
working with does not support sequences as such. Falling back to identity columns
66-
"""
67-
.formatted(aggregate.getClass().getName())
85+
It seems you're trying to insert an aggregate of type '%s' annotated with @TargetSequence, but the problem is RDBMS you're
86+
working with does not support sequences as such. Falling back to identity columns
87+
"""
88+
.formatted(aggregate.getClass().getName())
6889
);
6990
}
7091
}

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/AbstractJdbcRepositoryLookUpStrategyTests.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.springframework.data.jdbc.testing.EnabledOnDatabase;
2929
import org.springframework.data.relational.core.mapping.RelationalMappingContext;
3030
import org.springframework.data.repository.CrudRepository;
31+
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
3132
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
3233

3334
/**
@@ -40,7 +41,7 @@
4041
abstract class AbstractJdbcRepositoryLookUpStrategyTests {
4142

4243
@Autowired protected OnesRepository onesRepository;
43-
@Autowired NamedParameterJdbcTemplate template;
44+
@Autowired NamedParameterJdbcOperations template;
4445
@Autowired RelationalMappingContext context;
4546

4647
void insertTestInstances() {

‎spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java

+138-3
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,15 @@
1616
package org.springframework.data.jdbc.repository;
1717

1818
import static org.assertj.core.api.Assertions.*;
19+
import static org.springframework.data.jdbc.testing.AuditableNamedParameterJdbcTemplate.QueryType.*;
1920

2021
import java.util.List;
2122
import java.util.Objects;
23+
import java.util.Optional;
2224
import java.util.concurrent.atomic.AtomicLong;
2325

26+
import org.assertj.core.api.Assertions;
27+
import org.junit.jupiter.api.BeforeEach;
2428
import org.junit.jupiter.api.Test;
2529
import org.springframework.beans.factory.annotation.Autowired;
2630
import org.springframework.context.annotation.Bean;
@@ -29,31 +33,51 @@
2933
import org.springframework.context.annotation.FilterType;
3034
import org.springframework.context.annotation.Import;
3135
import org.springframework.data.annotation.Id;
36+
import org.springframework.data.annotation.PersistenceCreator;
37+
import org.springframework.data.annotation.Transient;
38+
import org.springframework.data.domain.Persistable;
3239
import org.springframework.data.jdbc.repository.config.EnableJdbcRepositories;
3340
import org.springframework.data.jdbc.repository.support.SimpleJdbcRepository;
41+
import org.springframework.data.jdbc.testing.AuditableNamedParameterJdbcTemplate;
3442
import org.springframework.data.jdbc.testing.IntegrationTest;
3543
import org.springframework.data.jdbc.testing.TestConfiguration;
3644
import org.springframework.data.relational.core.mapping.NamingStrategy;
45+
import org.springframework.data.relational.core.mapping.Sequence;
3746
import org.springframework.data.relational.core.mapping.event.BeforeConvertCallback;
3847
import org.springframework.data.repository.CrudRepository;
3948
import org.springframework.data.repository.ListCrudRepository;
4049
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
50+
import org.springframework.test.context.jdbc.Sql;
51+
import org.testcontainers.shaded.org.checkerframework.checker.index.qual.SameLen;
4152

4253
/**
4354
* Testing special cases for id generation with {@link SimpleJdbcRepository}.
4455
*
4556
* @author Jens Schauder
4657
* @author Greg Turnquist
58+
* @author Mikhail Polivakha
4759
*/
4860
@IntegrationTest
4961
class JdbcRepositoryIdGenerationIntegrationTests {
5062

51-
@Autowired NamedParameterJdbcTemplate template;
52-
@Autowired ReadOnlyIdEntityRepository readOnlyIdRepository;
63+
@Autowired ReadOnlyIdEntityRepository readOnlyIdRepository;
5364
@Autowired PrimitiveIdEntityRepository primitiveIdRepository;
5465
@Autowired ImmutableWithManualIdEntityRepository immutableWithManualIdEntityRepository;
5566

56-
@Test // DATAJDBC-98
67+
@Autowired EntityWithSeqRepository entityWithSeqRepository;
68+
69+
@Autowired PersistableEntityWithSeqRepository persistableEntityWithSeqRepository;
70+
71+
@Autowired PrimitiveIdEntityWithSeqRepository primitiveIdEntityWithSeqRepository;
72+
73+
@Autowired AuditableNamedParameterJdbcTemplate auditableNamedParameterJdbcTemplate;
74+
75+
@BeforeEach
76+
void setUp() {
77+
auditableNamedParameterJdbcTemplate.clearRecordedStatements();
78+
}
79+
80+
@Test // DATAJDBC-98
5781
void idWithoutSetterGetsSet() {
5882

5983
ReadOnlyIdEntity entity = readOnlyIdRepository.save(new ReadOnlyIdEntity(null, "Entity Name"));
@@ -107,15 +131,126 @@ void manuallyGeneratedIdForSaveAll() {
107131
assertThat(immutableWithManualIdEntityRepository.findAll()).hasSize(2);
108132
}
109133

134+
@Test // DATAJDBC-2003
135+
@Sql(statements = "INSERT INTO EntityWithSeq(id, name) VALUES(1, 'initial value');")
136+
void testUpdateAggregateWithSequence() {
137+
138+
// given.
139+
EntityWithSeq entityWithSeq = new EntityWithSeq();
140+
entityWithSeq.id = 1L;
141+
entityWithSeq.name = "New name";
142+
143+
// when.
144+
EntityWithSeq updated = entityWithSeqRepository.save(entityWithSeq);
145+
146+
// then.
147+
auditableNamedParameterJdbcTemplate.assertSequenceOfStatements(UPDATE);
148+
Assertions.assertThat(updated.id).isEqualTo(1L);
149+
}
150+
151+
@Test // DATAJDBC-2003
152+
void whenInsertPersistableAggregateWithSequence_thenClientDefinedIdIsFavored() {
153+
154+
// given.
155+
long initialId = 1L;
156+
PersistableEntityWithSeq entityWithSeq = PersistableEntityWithSeq.createNew(initialId, "name");
157+
158+
// when.
159+
PersistableEntityWithSeq saved = persistableEntityWithSeqRepository.save(entityWithSeq);
160+
161+
// then.
162+
Assertions.assertThat(saved.getId()).isEqualTo(initialId);
163+
164+
// We do not expect the SELECT next value from sequence in case we're doing an INSERT with ID provided by the client
165+
auditableNamedParameterJdbcTemplate.assertSequenceOfStatements(INSERT);
166+
}
167+
168+
@Test // DATAJDBC-2003
169+
void whenInsertAggregateWithSequenceAndPrimitiveIdUnset_thenSequenceIsQueried() {
170+
171+
// given.
172+
PrimitiveIdEntityWithSeq entity = new PrimitiveIdEntityWithSeq();
173+
entity.name = "some name";
174+
175+
// when.
176+
PrimitiveIdEntityWithSeq saved = primitiveIdEntityWithSeqRepository.save(entity);
177+
178+
// then.
179+
Assertions.assertThat(saved.id).isEqualTo(1L); // sequence starts with 1
180+
181+
// 1. Select from sequence
182+
// 2. Actual INSERT
183+
auditableNamedParameterJdbcTemplate.assertSequenceOfStatements(SELECT, INSERT);
184+
}
185+
110186
private interface PrimitiveIdEntityRepository extends ListCrudRepository<PrimitiveIdEntity, Long> {}
111187

112188
private interface ReadOnlyIdEntityRepository extends ListCrudRepository<ReadOnlyIdEntity, Long> {}
113189

114190
private interface ImmutableWithManualIdEntityRepository extends ListCrudRepository<ImmutableWithManualIdEntity, Long> {}
115191

192+
private interface EntityWithSeqRepository extends ListCrudRepository<EntityWithSeq, Long> {}
193+
private interface PersistableEntityWithSeqRepository extends ListCrudRepository<PersistableEntityWithSeq, Long> {}
194+
private interface PrimitiveIdEntityWithSeqRepository extends ListCrudRepository<PrimitiveIdEntityWithSeq, Long> {}
195+
116196
record ReadOnlyIdEntity(@Id Long id, String name) {
117197
}
118198

199+
static class EntityWithSeq {
200+
201+
@Id
202+
@Sequence(value = "entity_with_seq_seq")
203+
private Long id;
204+
205+
private String name;
206+
}
207+
208+
static class PersistableEntityWithSeq implements Persistable<Long> {
209+
210+
@Id
211+
@Sequence(value = "persistable_entity_with_seq_seq")
212+
private Long id;
213+
214+
private String name;
215+
216+
@Transient
217+
private boolean isNew;
218+
219+
@PersistenceCreator
220+
public PersistableEntityWithSeq() {
221+
}
222+
223+
public PersistableEntityWithSeq(Long id, String name, boolean isNew) {
224+
this.id = id;
225+
this.name = name;
226+
this.isNew = isNew;
227+
}
228+
229+
static PersistableEntityWithSeq createNew(Long id, String name) {
230+
return new PersistableEntityWithSeq(id, name, true);
231+
}
232+
233+
@Override
234+
public Long getId() {
235+
return id;
236+
}
237+
238+
@Override
239+
public boolean isNew() {
240+
return isNew;
241+
}
242+
}
243+
244+
static class PrimitiveIdEntityWithSeq {
245+
246+
@Id
247+
@Sequence(value = "primitive_entity_with_seq_seq")
248+
private long id;
249+
250+
private String name;
251+
252+
}
253+
119254
static class PrimitiveIdEntity {
120255

121256
@Id private long id;

0 commit comments

Comments
 (0)