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.

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

Migrate Row and ResultSet to Object Conversion to RowDocument #1618

Closed
wants to merge 6 commits into from

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Sep 19, 2023

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

@rishiraj88
Copy link

Thanks, @mp911de .

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.

}

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

@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

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.
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
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
schauder pushed a commit that referenced this pull request Oct 13, 2023
schauder pushed a commit that referenced this pull request Oct 13, 2023
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
schauder added a commit that referenced this pull request Oct 13, 2023
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
Labels
type: enhancement A general enhancement
Projects
None yet
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