Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mipo256
Copy link
Contributor

@mipo256 mipo256 commented Apr 14, 2025

Hey @mp911de! As discussed here, I've provided the corresponding PR for the Spring Data R2DBC module for @Sequence support.

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

Signed-off-by: mipo256 <[email protected]>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 14, 2025
@mp911de mp911de self-assigned this Apr 15, 2025
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 15, 2025
@mp911de mp911de added this to the 3.5 RC1 (2025.0.0) milestone Apr 15, 2025
@mp911de
Copy link
Member

mp911de commented Apr 15, 2025

Thanks a lot. Let me check how the documentation fits into both modules. Maybe we can have a shared fragment for inclusion.

.map((r, rowMetadata) -> r.get(0)) //
.one() //
.doOnNext(fetchedId -> { //
row.put(persistentEntity.getIdColumn().toSql(dialect.getIdentifierProcessing()), //
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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().

Copy link
Contributor Author

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().

@mipo256 mipo256 requested a review from mp911de April 16, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants