GH-2003 ID generation via Sequence should be omitted in UPDATE queries

Signed-off-by: mipo256 <[email protected]>

Author: mipo256

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

@@ -557,7 +557,6 @@ private <T> RootAggregateChange<T> createInsertChange(T instance) {
 	}
 
 	private <T> RootAggregateChange<T> createUpdateChange(EntityAndPreviousVersion<T> entityAndVersion) {
-
 		RootAggregateChange<T> aggregateChange = MutableAggregateChange.forSave(entityAndVersion.entity,
 				entityAndVersion.version);
 		new RelationalEntityUpdateWriter<T>(context).write(entityAndVersion.entity, aggregateChange);

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

@@ -2,6 +2,7 @@
 
 import java.util.Map;
 import java.util.Optional;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.data.jdbc.repository.config.AbstractJdbcConfiguration;
@@ -16,59 +17,73 @@
 import org.springframework.util.Assert;
 
 /**
- * Callback for generating ID via the database sequence. By default, it is registered as a
- * bean in {@link AbstractJdbcConfiguration}
+ * Callback for generating ID via the database sequence. By default, it is registered as a bean in
+ * {@link AbstractJdbcConfiguration}
  *
  * @author Mikhail Polivakha
  */
 public class IdGeneratingBeforeSaveCallback implements BeforeSaveCallback<Object> {
 
-    private static final Log LOG = LogFactory.getLog(IdGeneratingBeforeSaveCallback.class);
-
-    private final RelationalMappingContext relationalMappingContext;
-    private final Dialect dialect;
-    private final NamedParameterJdbcOperations operations;
-
-    public IdGeneratingBeforeSaveCallback(
-      RelationalMappingContext relationalMappingContext,
-      Dialect dialect,
-      NamedParameterJdbcOperations namedParameterJdbcOperations
-    ) {
-        this.relationalMappingContext = relationalMappingContext;
-        this.dialect = dialect;
-        this.operations = namedParameterJdbcOperations;
-    }
-
-    @Override
-    public Object onBeforeSave(Object aggregate, MutableAggregateChange<Object> aggregateChange) {
-
-        Assert.notNull(aggregate, "The aggregate cannot be null at this point");
-
-        RelationalPersistentEntity<?> persistentEntity = relationalMappingContext.getPersistentEntity(aggregate.getClass());
-        Optional<SqlIdentifier> idSequence = persistentEntity.getIdSequence();
-
-        if (dialect.getIdGeneration().sequencesSupported()) {
-
-            if (persistentEntity.getIdProperty() != null) {
-                idSequence
-                  .map(s -> dialect.getIdGeneration().createSequenceQuery(s))
-                  .ifPresent(sql -> {
-                      Long idValue = operations.queryForObject(sql, Map.of(), (rs, rowNum) -> rs.getLong(1));
-                      PersistentPropertyAccessor<Object> propertyAccessor = persistentEntity.getPropertyAccessor(aggregate);
-                      propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), idValue);
-                  });
-            }
-        } else {
-            if (idSequence.isPresent()) {
-                LOG.warn("""
-                        It seems you're trying to insert an aggregate of type '%s' annotated with @TargetSequence, but the problem is RDBMS you're
-                        working with does not support sequences as such. Falling back to identity columns
-                        """
-                    .formatted(aggregate.getClass().getName())
-                );
-            }
-        }
-
-        return aggregate;
-    }
+	private static final Log LOG = LogFactory.getLog(IdGeneratingBeforeSaveCallback.class);
+
+	private final RelationalMappingContext relationalMappingContext;
+	private final Dialect dialect;
+	private final NamedParameterJdbcOperations operations;
+
+	public IdGeneratingBeforeSaveCallback(RelationalMappingContext relationalMappingContext, Dialect dialect,
+			NamedParameterJdbcOperations namedParameterJdbcOperations) {
+		this.relationalMappingContext = relationalMappingContext;
+		this.dialect = dialect;
+		this.operations = namedParameterJdbcOperations;
+	}
+
+	@Override
+	public Object onBeforeSave(Object aggregate, MutableAggregateChange<Object> aggregateChange) {
+
+		Assert.notNull(aggregate, "The aggregate cannot be null at this point");
+
+		RelationalPersistentEntity<?> persistentEntity = relationalMappingContext.getPersistentEntity(aggregate.getClass());
+
+		if (persistentEntity.hasIdProperty()) {
+			return aggregate;
+		}
+
+		// we're doing INSERT and ID property value is not set explicitly by client
+		if (persistentEntity.isNew(aggregate) && !hasIdentifierValue(aggregate, persistentEntity)) {
+			return potentiallyFetchIdFromSequence(aggregate, persistentEntity);
+		} else {
+			return aggregate;
+		}
+	}
+
+	private boolean hasIdentifierValue(Object aggregate, RelationalPersistentEntity<?> persistentEntity) {
+		Object identifier = persistentEntity.getIdentifierAccessor(aggregate).getIdentifier();
+
+		if (persistentEntity.getIdProperty().getType().isPrimitive()) {
+			return identifier instanceof Number num && num.longValue() != 0L;
+		} else {
+			return identifier != null;
+		}
+	}
+
+	private Object potentiallyFetchIdFromSequence(Object aggregate, RelationalPersistentEntity<?> persistentEntity) {
+		Optional<SqlIdentifier> idSequence = persistentEntity.getIdSequence();
+
+		if (dialect.getIdGeneration().sequencesSupported()) {
+			idSequence.map(s -> dialect.getIdGeneration().createSequenceQuery(s)).ifPresent(sql -> {
+				Long idValue = operations.queryForObject(sql, Map.of(), (rs, rowNum) -> rs.getLong(1));
+				PersistentPropertyAccessor<Object> propertyAccessor = persistentEntity.getPropertyAccessor(aggregate);
+				propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), idValue);
+			});
+		} else {
+			if (idSequence.isPresent()) {
+				LOG.warn("""
+						It seems you're trying to insert an aggregate of type '%s' annotated with @TargetSequence, but the problem is RDBMS you're
+						working with does not support sequences as such. Falling back to identity columns
+						""".formatted(aggregate.getClass().getName()));
+			}
+		}
+
+		return aggregate;
+	}
 }

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

@@ -28,6 +28,7 @@
 import org.springframework.data.jdbc.testing.EnabledOnDatabase;
 import org.springframework.data.relational.core.mapping.RelationalMappingContext;
 import org.springframework.data.repository.CrudRepository;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations;
 import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
 
 /**
@@ -40,7 +41,7 @@
 abstract class AbstractJdbcRepositoryLookUpStrategyTests {
 
 	@Autowired protected OnesRepository onesRepository;
-	@Autowired NamedParameterJdbcTemplate template;
+	@Autowired NamedParameterJdbcOperations template;
 	@Autowired RelationalMappingContext context;
 
 	void insertTestInstances() {