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

Refactor Spring Data R2DBC on top of Spring R2DBC #412

Closed
wants to merge 4 commits into from
Closed

Refactor Spring Data R2DBC on top of Spring R2DBC #412

wants to merge 4 commits into from

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Jul 23, 2020

Spring Framework aims to include foundational R2DBC support in a similar way to Spring JDBC. We should adopt to these changes by deprecating org.springframework.data.r2dbc.core.DatabaseClient in favor of org.springframework.r2dbc.core.DatabaseClient.

Components on top of DatabaseClient should be rewritten to use Spring Framework's R2DBC variants. In particular:

  • Deprecate our DatabaseClient and all necessary infrastructure classes/interfaces
  • Deprecate ColumnMapRowMapper and SettableValue
  • Adapt AbstractR2dbcConfiguration
  • Deprecate connectionfactory package in favor of org.springframework.r2dbc.connectionfactory
  • Deprecate support package in favor of org.springframework.r2dbc.connectionfactory
  • Use Framework's DatabaseClient in R2dbcEntityTemplate and the repository support classes

@mp911de mp911de requested a review from schauder July 23, 2020 07:31
schauder
schauder reviewed Jul 24, 2020
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.

Should DatabaseClientExtension.kt get deprecation information as well?

I left a couple of comments and added a commit to the PR.

src/main/asciidoc/reference/r2dbc-upgrading.adoc Outdated
* `R2dbcDialect`
* Types in `org.springframework.data.r2dbc.query`

We recommend that you review your imports if you work with these types directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

"review" is a rather weak verb.

Ok, I looked at my imports, now what?

src/main/asciidoc/reference/r2dbc-upgrading.adoc Outdated
=== Usage of replacements provided by Spring R2DBC

To ease migration, several deprecated types are now subtypes of their replacements provided by Spring R2DBC. Spring Data R2DBC has changes several methods or introduced new methods accepting Spring R2DBC types.
Specifically the following classes are affected:
Copy link
Contributor

Choose a reason for hiding this comment

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

are affected -> changed

(prefere active voice)

src/main/asciidoc/reference/r2dbc-upgrading.adoc

To make use of Spring R2DBC, make sure to include the following dependency:

* `org.springframework:spring-r2dbc`
Copy link
Contributor

Choose a reason for hiding this comment

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

That should be done by the build tool for 99.9% of the users, shouldn't it?

I there for just mention it in the starting paragraph, that spring-data-r2dbc now depends on spring-r2dbc.

src/main/java/org/springframework/data/r2dbc/connectionfactory/init/DatabasePopulatorUtils.java Outdated
@@ -27,7 +27,9 @@
* Utility methods for executing a {@link DatabasePopulator}.
*
* @author Mark Paluch
* @deprecated since 1.2 in favor of Spring R2DBC. Use {@link org.springframework.r2dbc.connection.init} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find a matching replacement for this.
Maybe there isn't any?

src/main/java/org/springframework/data/r2dbc/connectionfactory/ConnectionHandle.java Outdated
@@ -23,8 +23,10 @@
* @author Mark Paluch
* @see SimpleConnectionHandle
* @see ConnectionHolder
* @deprecated since 1.2 in favor of Spring R2DBC. Use {@link org.springframework.r2dbc.connection} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find a replacement class. We should clarify where to find it.

src/main/java/org/springframework/data/r2dbc/connectionfactory/SmartConnectionFactory.java Outdated
@@ -27,7 +27,9 @@
*
* @author Mark Paluch
* @see ConnectionFactoryUtils#closeConnection
* @deprecated since 1.2 in favor of Spring R2DBC. Use {@link org.springframework.r2dbc.connection} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, couldn't find a replacement.

src/main/java/org/springframework/data/r2dbc/mapping/SettableValue.java
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a couple of usages of this outside deprecated classes/ interfaces, e.g. ReactiveDataAccessStrategy.getBindValue(SettableValue).
I wonder if we should add deprecations to those methods as well, in order to point to the right replacement.

Copy link
Member Author

Choose a reason for hiding this comment

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

SettableValue is used in the deprecated DatabaseClient and considered in query/update mappers for a smoother upgrade experience.

We don't want to deprecate update/query mappers but gradually remove support for SettableValue.

src/main/java/org/springframework/data/r2dbc/mapping/SettableValue.java
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some comments referring to it, wich should refer to Parameter instead. Again an example is ReactiveDataAccessStrategy.getBindValue(Parameter).

@mp911de
#368 - Prepare issue branch.
5a70a01
@mp911de
#368 - Deprecate API provided by Spring R2DBC.
5c4eee3 
This commit deprecates API that has been moved to Spring R2DBC.
@mp911de
#368 - Use Spring R2DBC.
0fab99f 
We now use Spring R2DBC DatabaseClient and utilities to implement Spring Data R2DBC functionality.
@mp911de mp911de added this to the 1.2 M2 (2020.0.0) milestone Jul 28, 2020
mp911de added a commit that referenced this pull request Aug 4, 2020
@mp911de
#368 - Deprecate API provided by Spring R2DBC.
f331572 
This commit deprecates API that has been moved to Spring R2DBC.

Original pull request: #412.
mp911de added a commit that referenced this pull request Aug 4, 2020
@mp911de
#368 - Use Spring R2DBC.
086c5a1 
We now use Spring R2DBC DatabaseClient and utilities to implement Spring Data R2DBC functionality.

Original pull request: #412.
mp911de added a commit that referenced this pull request Aug 4, 2020
@mp911de
#368 - Polishing.
3d59c63 
Original pull request: #412.
@mp911de
Copy link
Member Author

mp911de commented Aug 4, 2020

That's merged now.

@mp911de mp911de closed this Aug 4, 2020
@mp911de mp911de deleted the issue/gh-368 branch August 4, 2020 12:54
mp911de pushed a commit that referenced this pull request Aug 4, 2020
@schauder @mp911de
#368 - Polishing.
b0e03ea 
Line breaks after dots in ascii doctor file for easier reading of the source files.
Adds specific deprecation links instead of just links to the general package.
Removes an unused private method in `ConnectionFactoryUtils`

Original pull request: #412.
@skivol skivol mentioned this pull request Aug 6, 2020
Closed
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
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.2 M2 (2020.0.0)
Development

Successfully merging this pull request may close these issues.
2 participants
@mp911de @schauder