Omit sequence generation for non-new entities and entities with provided identifiers #2005

mipo256 · 2025-03-04T09:22:12Z

Closes #2003

mp911de

Thanks for having a look. Formatting is terribly off. Please make sure to use the formatting config (and import style) at https://github.com/spring-projects/spring-data-build/tree/main/etc/ide.

I left quite a few comments given that this is quite a sized PR touching up on many aspects. Let me know if you require more detail and whether my feedback is too short at places.

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

mp911de · 2025-03-04T13:43:59Z

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

+        Object identifier = persistentEntity.getIdentifierAccessor(aggregate).getIdentifier();
+
+        if (persistentEntity.getIdProperty().getType().isPrimitive()) {
+            return identifier instanceof Number num && num.longValue() != 0L;


Notice to myself: We should introduce an utility to determine whether a primitive is set to its initial value.

I can introduce it in separate commit in the same PR, it seems that it is natural to do it in the context of the current ticket. What do you think @mp911de ?

I was considering an utility in Spring Data Commons. Let me verify whether this is a good idea or not.

Sure, I'll wait for your response. I think I can help with this as well.

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

...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java

mp911de · 2025-03-04T13:55:28Z

...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java

+
+    @BeforeEach
+    void setUp() {
+        auditableNamedParameterJdbcTemplate.clearRecordedStatements();


When already having an integration test, it doesn't make much sense to verify against statements. Rather, we should the result.

I'm sorry, but it does not seem to be clear for me how we can verify the SELECT statements executed to query the sequence. We can create a wrapper around ID generating callback and try to utilize it. I am not sure which approach is better to be honest @mp911de

There's a blog post by Vlad on verifying and counting statements see https://vladmihalcea.com/how-to-detect-the-n-plus-one-query-problem-during-testing/

I think that one can serve as input for decorating our data sources to be tested.

mipo256 · 2025-03-04T14:02:36Z

Hey @mp911de! Thanks for review :)

Yeah, I forgot to ask about formatting, my bad, I'll fix it ASAP

mipo256 · 2025-03-07T20:06:19Z

Hey @mp911de! I've fixed almost all comments left, but several comments I've left open for discussion since it is not clear for me what needs to be done

mp911de · 2025-03-19T09:55:09Z

@mipo256 I'd like to come back to this one. I think the only remaining issue is AuditableNamedParameterJdbcTemplate. Please remove that one for the time being in favor of verifying target sequences and entities after the test run for assertions.

mipo256 · 2025-03-19T09:59:23Z

Hey @mp911de! I understand that it is coming into the RC, I'll resolve it very shortly. Thank you! I'll tag you once the remaining stuff is fixed

mipo256 · 2025-03-19T10:40:33Z

Regarding the article from Vlad you've proposed, Mark @mp911de.

The problem is that this article also utilizes the external library for proxying the DataSource. And Vlad is also using custom listeners, defined in the hypersistence project.

Let me just clarify - for now, do we want to actually bring any of those libraries into the project, or do we want to just get rid of AuditableNamedParameterJdbcTemplate and be done with it? I think we eventually need the solution for tracking the queries executed.

P.S: That comments thread remained unanswered. I can create an issue, it seems that it fits more in commons module, what do you think?

mp911de · 2025-03-19T13:07:19Z

The problem is that this article also utilizes the external library for proxying the DataSource. And Vlad is also using custom listeners, defined in the hypersistence project.

DataSource Proxy wouldn't be an issue, however, custom listeners are. So for the time being, let's verify target sequences and entities after the test run for assertions.

Re Util for primitive values: I'll handle that during the merge.

…n UPDATE queries Signed-off-by: mipo256 <[email protected]>

mipo256 · 2025-03-19T15:29:58Z

Done @mp911de, can you re-review please?

mp911de

This is much cleaner now, thank you. I'm going to merge this PR in the next few days so we can ship it with 3.5 RC1

mp911de

Some last remarks for your future pull request. I took care of these during polishing, highly recommended that you check out both polishing commits as I moved the sequence into RelationalPersistentProperty.

mp911de · 2025-04-09T07:33:43Z

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

+		} 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


What is @TargetSequence?

This was an old name of the @Sequence annotation, sorry, I've missed it out 🙈

mp911de · 2025-04-09T07:34:40Z

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

@@ -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


Actually, we do want to avoid dependency inversions, this class should ideally now know anything about AbstractJdbcConfiguration.

mp911de · 2025-04-09T07:35:17Z

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

-                      propertyAccessor.setProperty(persistentEntity.getRequiredIdProperty(), idValue);
-                  });
-            }
-        } else {


There are other files using space formatting , such as IdGeneratingBeforeSaveCallbackTest. As regular contributor I bet you can do better.

Sorry, I've imported the spring data code formatter into Intelij. I'll double-check my formatting, thanks

mp911de · 2025-04-09T07:51:41Z

...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java


-	@Test // DATAJDBC-98
+	@Autowired
+	SimpleSeqRepository simpleSeqRepository;


After revising these tests, we could have gone for testing through JdbcAggregateOperations, we don't really need repositories for identifier testing. But it is now what it is.

…he entity is not new. Signed-off-by: mipo256 <[email protected]> Closes #2003 Original pull request: #2005

Refine assignment flow and use early returns where possible. Cache empty MapSqlParameterSource. Reduce dependency on RelationalMappingContext using a lower-level abstraction signature. Simplify names. Use default value check from Commons. Fix log warning message. Add missing since tags. Remove superfluous annotations and redundant code. Tweak documentation wording. Closes #2003 Original pull request: #2005

Sequence details are now maintained on the property level instead of using the entity level. This is a more accurate representation of the underlying model and that properties are annotated and not entities. It also allows future extension of expanding sequence support to general properties. Extract abstract support class for sequence generation. Move types to org.springframework.data.jdbc.core.convert to resolve package cycles. See #2003 Original pull request: #2005

mp911de · 2025-04-09T08:33:38Z

Thank you for your contribution. That's merged and polished now.

Sequence details are now maintained on the property level instead of using the entity level. This is a more accurate representation of the underlying model and that properties are annotated and not entities. It also allows future extension of expanding sequence support to general properties. Extract delegate for sequence generation. Move types to org.springframework.data.jdbc.core.convert to resolve package cycles. See #2003 Original pull request: #2005

See #2003 Original pull request: #2005

mipo256 force-pushed the GH-2003 branch from dd42db6 to 786ff58 Compare March 4, 2025 09:22

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 4, 2025

mp911de requested changes Mar 4, 2025

View reviewed changes

mp911de changed the title ~~GH-2003 ID generation via Sequence should be omitted in UPDATE queries~~ Omit sequence generation for non-new entities and entities with provided identifiers Mar 4, 2025

mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 4, 2025

mp911de self-assigned this Mar 4, 2025

mipo256 force-pushed the GH-2003 branch from 786ff58 to 64fca82 Compare March 7, 2025 20:01

mipo256 requested a review from mp911de March 7, 2025 20:05

mp911de added this to the 3.5 RC1 (2025.0.0) milestone Mar 19, 2025

spring-projectsGH-2003 ID generation via Sequence should be omitted i…

9cb8cf3

…n UPDATE queries Signed-off-by: mipo256 <[email protected]>

mipo256 force-pushed the GH-2003 branch from 64fca82 to 9cb8cf3 Compare March 19, 2025 15:29

mp911de approved these changes Apr 8, 2025

View reviewed changes

mp911de reviewed Apr 9, 2025

View reviewed changes

mp911de pushed a commit that referenced this pull request Apr 9, 2025

Omit Sequence value generation when identifier value is provided or t…

0fc3187

…he entity is not new. Signed-off-by: mipo256 <[email protected]> Closes #2003 Original pull request: #2005

mp911de closed this Apr 9, 2025

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix typo in sequence selection.

a1ed51e

See #2003 Original pull request: #2005

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix typo in sequence selection.

e27a058

See #2003 Original pull request: #2005

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix integration tests.

14444a7

See #2003 Original pull request: #2005

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix integration tests.

ff120ba

See #2003 Original pull request: #2005

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix integration tests.

571fd96

See #2003 Original pull request: #2005

mp911de added a commit that referenced this pull request Apr 9, 2025

Fix integration tests.

1f2e694

See #2003 Original pull request: #2005

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit sequence generation for non-new entities and entities with provided identifiers #2005

Omit sequence generation for non-new entities and entities with provided identifiers #2005

mipo256 commented Mar 4, 2025

mp911de left a comment

mp911de Mar 4, 2025

mipo256 Mar 6, 2025

mp911de Mar 6, 2025

mipo256 Mar 6, 2025

mp911de Mar 4, 2025

mipo256 Mar 6, 2025

mp911de Mar 10, 2025

mipo256 commented Mar 4, 2025

mipo256 commented Mar 7, 2025

mp911de commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mp911de commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mp911de left a comment

mp911de left a comment

mp911de Apr 9, 2025

mipo256 Apr 9, 2025

mp911de Apr 9, 2025

mp911de Apr 9, 2025

mipo256 Apr 9, 2025

mp911de Apr 9, 2025

mp911de commented Apr 9, 2025

Omit sequence generation for non-new entities and entities with provided identifiers #2005

Omit sequence generation for non-new entities and entities with provided identifiers #2005

Conversation

mipo256 commented Mar 4, 2025

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mipo256 commented Mar 4, 2025

mipo256 commented Mar 7, 2025

mp911de commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mp911de commented Mar 19, 2025

mipo256 commented Mar 19, 2025

mp911de left a comment

Choose a reason for hiding this comment

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented Apr 9, 2025