Skip to content

Unified RowMapper infrastructure #2000

Closed
mipo256 wants to merge 2 commits into
spring-projects:mainfrom
mipo256:extend-visibility
Closed

Unified RowMapper infrastructure #2000
mipo256 wants to merge 2 commits into
spring-projects:mainfrom
mipo256:extend-visibility

Conversation

@mipo256

@mipo256 mipo256 commented Feb 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #1998

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 21, 2025
@mipo256 mipo256 marked this pull request as draft February 21, 2025 13:18
@mipo256 mipo256 changed the title Extend visibility Unified RowMapper infrastructure Feb 21, 2025
@mipo256 mipo256 mentioned this pull request Feb 21, 2025
Closed
@mp911de mp911de assigned schauder and mp911de and unassigned schauder Feb 24, 2025
@mipo256 mipo256 force-pushed the extend-visibility branch from 93d0b53 to a8044cb Compare March 3, 2025 19:26
schauder
schauder requested changes Mar 13, 2025
View reviewed changes

@schauder schauder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks mostly good to me.

I left some in line comments.
Plus the dependency test fails, so I guess some classes need to get shuffled around.

Comment thread .../src/main/java/org/springframework/data/jdbc/repository/support/JdbcQueryLookupStrategy.java Outdated
Comment thread ...data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/RowMapperFactory.java
Comment thread ...ata-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/AbstractJdbcQuery.java
Comment thread ...data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/RowMapperFactory.java Outdated
@schauder schauder added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 13, 2025
@mipo256 mipo256 force-pushed the extend-visibility branch 2 times, most recently from e87abbb to 1d65e9c Compare March 15, 2025 19:53
@mipo256

mipo256 commented Mar 16, 2025

Copy link
Copy Markdown
Contributor Author

@schauder What DependencyTests are failing exactly? I've run in all three modules relational, jdbc and r2dbc, and all seems to be fine.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 16, 2025
@mipo256 mipo256 force-pushed the extend-visibility branch from 1d65e9c to 0227022 Compare March 16, 2025 11:47
mipo256
mipo256 commented Mar 16, 2025
View reviewed changes

@mipo256 mipo256 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @schauder. I've addressed all the comments. I left a couple of discussion pending, just to either let you know some details or to discuss with you other possibilities.

Comment thread .../src/main/java/org/springframework/data/jdbc/repository/support/JdbcQueryLookupStrategy.java
Comment thread .../src/main/java/org/springframework/data/jdbc/repository/support/JdbcQueryLookupStrategy.java
Comment thread ...ata-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/AbstractJdbcQuery.java
@mipo256 mipo256 requested a review from schauder March 16, 2025 11:54
@mipo256 mipo256 force-pushed the extend-visibility branch 2 times, most recently from 4cb07a6 to 4bc2fd6 Compare March 17, 2025 18:09
@schauder

schauder commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

Could you please sign off on your commit for DCO.

@schauder

schauder commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

The PR is marked as draft. Is there anything you don't consider ready yet?

@mipo256

mipo256 commented Mar 18, 2025

Copy link
Copy Markdown
Contributor Author

@schauder The PR was inspired by the following issue in the YDB dialect on top of spring data jdbc. Most likely, we'll also need to allow for the extension of JdbcQueryLookupStrategy.

I'll convert it as ready very shortly, as soon as I'll understand that the dialect is capable to extend everything that it needs.

Or, alternatively, we can merge this PR and other possible changes would happen in the upcoming PRs. What are your thoughts on this?

@schauder

schauder commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

Ok, I'll wait with further reviewing until we have something that is complete from your perspective.

@mipo256 mipo256 mentioned this pull request Mar 21, 2025
Draft
@mipo256

mipo256 commented Mar 21, 2025

Copy link
Copy Markdown
Contributor Author

Hey @schauder! Thank you for the wait, we came up with the draft:

I've pushed the remaining changes in the separate commit, because they do not directly related to unification of RowMapper infrastructure. To summarize what was altered:

  1. Split a public constructor of the JdbcParameters into two, retaining the backward compatibility. That is required in order for custom implementations to supply their own Function<MethodParameter, RelationalParameter> parameterFactory. That is not a breaking change.
  2. The JdbcParameter was moved to the upper level. That is a breaking change. I want to point out that this is not really necessary, and I can revert it. It is the only breaking change that happens in the commit.
  3. Made JdbcQueryLookupStrategy public in order to be able to extend it (it is abstract after all). Also made the constructor of this class protected. Additionally, exposed some fields like JdbcConverter etc. to the subclasses of JdbcQueryLookupStrategy. That is not a breaking change.
  4. Made fields of JdbcRepositoryFactoryBean/JdbcRepositoryFactory protected, in order to allow subclasses to access them. The subclasses, of course, when invoking the super() constructor can save these fields themselves, but that is an overhead since these fields are already present in the parent, so I think it makes sense to expose them to the children only.

@mipo256 mipo256 marked this pull request as ready for review March 21, 2025 15:07
@mipo256 mipo256 force-pushed the extend-visibility branch from e1477fd to 0cfc5d9 Compare March 21, 2025 15:07
mipo256 added 2 commits March 21, 2025 18:10
@mipo256
Polishing: JdbcQueryLookupStrategy
2bb0c57 
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
@mipo256
rowmappers extraction
8032c0f 
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>
@mipo256 mipo256 force-pushed the extend-visibility branch from 0cfc5d9 to 3f56f75 Compare March 21, 2025 15:10
@mipo256 mipo256 mentioned this pull request Mar 25, 2025
Open
@mipo256

mipo256 commented Apr 5, 2025

Copy link
Copy Markdown
Contributor Author

Hey @schauder!

At the end, after discussion with @mp911de here, the unification of RowMappers is the only change in this PR, so yeah, I rolled back other changes.

The PR is complete and ready for final review now.

@mipo256 mipo256 force-pushed the extend-visibility branch from 3f56f75 to 8032c0f Compare April 5, 2025 15:44
@mp911de mp911de force-pushed the main branch 2 times, most recently from 571fd96 to 1f2e694 Compare April 9, 2025 13:29
schauder pushed a commit that referenced this pull request May 13, 2025
@mipo256 @schauder
Polishing.
98ce382 
Uses new switch syntax.

Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>

Commit message changed by Jens Schauder

Original pull request #2000
See #1998
schauder pushed a commit that referenced this pull request May 13, 2025
@mipo256 @schauder
Extract RowMapper into top level classes.
6dab06a 
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>

Commit message changed by Jens Schauder

Original pull request #2000
Closes #1998
schauder added a commit that referenced this pull request May 13, 2025
@schauder
Polishing.
ed4edba 
Formatting.

Original pull request #2000
See #1998
@schauder schauder added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels May 13, 2025
@schauder schauder added this to the 4.0 M3 (2025.1.0) milestone May 13, 2025
@schauder

schauder commented May 13, 2025

Copy link
Copy Markdown
Contributor

That is merged. Thanks.

@schauder schauder closed this May 13, 2025
mp911de pushed a commit that referenced this pull request May 16, 2025
@mipo256 @mp911de
Polishing.
cb86d91 
Uses new switch syntax.

Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>

Commit message changed by Jens Schauder

Original pull request #2000
See #1998
mp911de pushed a commit that referenced this pull request May 16, 2025
@mipo256 @mp911de
Extract RowMapper into top level classes.
1ef4182 
Signed-off-by: mipo256 <mikhailpolivakha@gmail.com>

Commit message changed by Jens Schauder

Original pull request #2000
Closes #1998
mp911de pushed a commit that referenced this pull request May 16, 2025
@schauder @mp911de
Polishing.
5d3d38e 
Formatting.

Original pull request #2000
See #1998
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schauder schauder Awaiting requested review from schauder

Assignees

@schauder schauder

@mp911de mp911de

Labels

type: enhancement A general enhancement

Projects

None yet

Milestone

4.0 M3 (2025.1.0)

Development

Successfully merging this pull request may close these issues.

Allow for the extension of the JdbcQueryLookupStrategy

4 participants

@mipo256 @schauder @mp911de @spring-projects-issues