Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate Row and ResultSet to Object Conversion to RowDocument #1618

Closed
wants to merge 6 commits into from
Closed

Migrate Row and ResultSet to Object Conversion to RowDocument #1618

wants to merge 6 commits into from

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Sep 19, 2023 •
edited
Loading

JDBC and R2DBC converters now consistently use RowDocument for object reading. Additionally, JDBC uses contextual reference resolution as fallback when Single Query Loading cannot be applied.

Closes #1554

TODO:

  • Rename BasicJdbcConverter to MappingJdbcConverter and introduce deprecated subclass variant of MappingJdbcConverter
  • Merge BasicRelationalConverter into MappingRelationalConverter and introduce deprecated variant

@mp911de mp911de added the type: enhancement A general enhancement label Sep 19, 2023
@mp911de mp911de requested a review from schauder September 19, 2023 14:14
@rishiraj88
Copy link

Thanks, @mp911de .

schauder
schauder reviewed Sep 22, 2023
View reviewed changes
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just one question .. see review comments.

...ain/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java
}

if (RowDocument.class.isAssignableFrom(rawType)) {
return (S) document;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't that documentAccessor.document()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might apply to other places here as well. If so, we should add tests to cover these cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

@schauder
Copy link
Contributor

Wouldn't this one be closed by this as well? #1394

@mp911de mp911de mentioned this pull request Sep 22, 2023
Closed
@mp911de
Copy link
Member Author

mp911de commented Sep 22, 2023

Good catch, I closed #1394. I think for Spring Data JDBC, we need to revisit the projection arrangement anyways as this pull request uses projections at the converter level. Other places, that rely on projections through repository proxy interceptors need to call the project method.

@rishiraj88
Copy link

Thanks, @mp911de .

@mp911de mp911de mentioned this pull request Sep 28, 2023
Open
@schauder
Copy link
Contributor

@mp911de Please take another look at this. It has test failures which before didn't break the build, but after a rebase onto main does. See #1637

@mp911de @schauder
Prepare issue branch.
632b31f
@mp911de @schauder
Add support for multi-level projections using EntityProjection.
6b8ecbe 
We now support multi-level projections by introspecting the result and the domain type and read projections directly into a DTO or a backing map for interface projections.
@mp911de @schauder
Migrate R2DBC to read RowDocument.
e070d97
@mp911de @schauder
Migrate JDBC to use RowDocument for reading aggregates.
37fd3c1
@mp911de @schauder
Deprecate Basic…Converter infrastructure.
55c3894 
Merge basic converters into Mapping…Converters and introduce deprecated variant to provide guidance for migration off the deprecated types.

Cleanup no longer required code.
schauder pushed a commit that referenced this pull request Oct 13, 2023
@mp911de @schauder
Add support for multi-level projections using EntityProjection.
41b37a6 
We now support multi-level projections by introspecting the result and the domain type and read projections directly into a DTO or a backing map for interface projections.

Original pull request #1618
Closes #1554
schauder pushed a commit that referenced this pull request Oct 13, 2023
@mp911de @schauder
Migrate R2DBC to read RowDocument.
665ae6b 
Original pull request #1618
See #1554
schauder pushed a commit that referenced this pull request Oct 13, 2023
@mp911de @schauder
Migrate JDBC to use RowDocument for reading aggregates.
4e3120e 
Original pull request #1618
See #1554
schauder pushed a commit that referenced this pull request Oct 13, 2023
@mp911de @schauder
Deprecate Basic…Converter infrastructure.
5398b94 
Merge basic converters into Mapping…Converters and introduce deprecated variant to provide guidance for migration off the deprecated types.

Cleanup no longer required code.

Original pull request #1618
See #1554
schauder pushed a commit that referenced this pull request Oct 13, 2023
@mp911de @schauder
Fix identifier propagation for no-id list elements.
5344197 
Original pull request #1618
See #1554
schauder added a commit that referenced this pull request Oct 13, 2023
@schauder
Polishing.
8ba1204 
Original pull request #1618
See #1554
@schauder
Copy link
Contributor

That's merged.

@schauder schauder closed this Oct 13, 2023
@schauder schauder added this to the 3.2 RC1 (2023.1.0) milestone Oct 13, 2023
@schauder schauder deleted the issue/1554 branch October 13, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schauder schauder schauder left review comments

Assignees

@schauder schauder

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.2 RC1 (2023.1.0)
Development

Successfully merging this pull request may close these issues.

Result mapping of projections should always use the DTO and not the domain entity
3 participants
@mp911de @rishiraj88 @schauder