-
Notifications
You must be signed in to change notification settings - Fork 369
R2DBC @Sequence annotation support #2028
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.
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
Conversation
81535e4 to
395fee6
Compare
Thanks a lot. Let me check how the documentation fits into both modules. Maybe we can have a shared fragment for inclusion.
...dbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingEntityCallback.java
Show resolved
Hide resolved
...rc/main/java/org/springframework/data/r2dbc/core/mapping/IdGeneratingBeforeSaveCallback.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/data/r2dbc/core/mapping/IdGeneratingBeforeSaveCallback.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obtaining the IdColumn as early as possible guards the later code against potential absence. Also, there's repetative access to IdProperty. I think it would be much more aligned with a future support of embedded Id's to use the IdProperty instead of IdColumn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be much more aligned with a future support of embedded Id's to use the IdProperty instead of IdColumn.
Can you please elaborate on what you mean here? The OutboundRow contains the mapping between column names (not properties) and their repsective values. I can obtain the idProperty once (although, this operation is relatively cheap since it just returns a reference to an already computed Id PersistentProperty), but I do not see how we can use the property here intead of column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a difference between persistentEntity.getIdColumn() and PersistentProperty.getColumnName().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not understand what change needs to be done here to be honest @mp911de
This is a difference between persistentEntity.getIdColumn() and PersistentProperty.getColumnName().
The persistentEntity.getIdColumn() is implemeted internally as getRequiredIdProperty().getColumnName(), which partically the same as calling the PersistentProperty.getColumnName().
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
395fee6 to
758529e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. It should actually be withIdFromSequenceRepository and asserting id and name properties. I wonder whether you ever ran these tests because they fail upon the first run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm terribly sorry for this overlook @mp911de. Of course, I run them. As far as I understood, you've already merged this PR. Should I take a look? Or you already did
Extract SequenceEntityCallbackDelegate from IdGeneratingBeforeSaveCallback. Renameto IdGeneratingEntityCallback and move callback to convert package. Align return values and associate generated sequence value with the entity. Fix test. Add ticket references to tests. Extract documentation partials. See #1955 Original pull request: #2028
Extract SequenceEntityCallbackDelegate from IdGeneratingBeforeSaveCallback. Renameto IdGeneratingEntityCallback and move callback to convert package. Align return values and associate generated sequence value with the entity. Fix test. Add ticket references to tests. Extract documentation partials. See #1955 Original pull request: #2028
Thank you for your contribution. That's merged, polished, and forward-ported now.
Hey @mp911de! As discussed here, I've provided the corresponding PR for the Spring Data R2DBC module for
@Sequencesupport.I've also provided the adoc documentation for this feature, I think it is worth a separate small page. I can do the same for the Spring Data JDBC module, should I? @mp911de