Omit sequence generation for non-new entities and entities with provided identifiers #2005
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| Object identifier = persistentEntity.getIdentifierAccessor(aggregate).getIdentifier(); | ||
|
|
||
| if (persistentEntity.getIdProperty().getType().isPrimitive()) { | ||
| return identifier instanceof Number num && num.longValue() != 0L; |
There was a problem hiding this comment.
Notice to myself: We should introduce an utility to determine whether a primitive is set to its initial value.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I was considering an utility in Spring Data Commons. Let me verify whether this is a good idea or not.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/data/jdbc/repository/JdbcRepositoryIdGenerationIntegrationTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| auditableNamedParameterJdbcTemplate.clearRecordedStatements(); |
There was a problem hiding this comment.
When already having an integration test, it doesn't make much sense to verify against statements. Rather, we should the result.
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
mp911de
changed the title
mp911de
added
type: bug
|
Hey @mp911de! Thanks for review :) Yeah, I forgot to ask about formatting, my bad, I'll fix it ASAP |
|
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 |
|
@mipo256 I'd like to come back to this one. I think the only remaining issue is |
|
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 |
|
Regarding the article from Vlad you've proposed, Mark @mp911de. The problem is that this article also utilizes the external library for proxying the 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 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? |
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]>
|
Done @mp911de, can you re-review please? |
Closes #2003