Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772

mpeddada1 · 2021-12-07T22:23:56Z

Part 1 of #205
This PR adds two starters for R2DBC support in Cloud SQL (one for mySQL and another for postgreSQL). It also includes 2 samples, associated integrations tests and documentation. The starters bring in spring-boot-starter-data-r2dbc which takes care of the creation of the ConnectionFactory bean.

Currently, authentication can only be done through the use of Application Default Credentials. The feature to pass credentials through spring.cloud.gcp.credentials or spring.cloud.gcp.sql.credentials will be in the next PR(#775).

meltsufin

I mostly commented just on the MySQL side as the comments equally apply to Postgres.

spring-cloud-gcp-samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/README.adoc

...amples/spring-cloud-gcp-sql-mysql-r2dbc-sample/src/main/java/com/example/SqlApplication.java

...samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/src/main/java/com/example/WebController.java

spring-cloud-gcp-samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/pom.xml

meltsufin · 2021-12-09T15:07:56Z

...l-r2dbc-sample/src/test/java/com/example/SqlR2dbcMySqlSampleApplicationIntegrationTests.java

+ *
+ */
+@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT, classes = { SqlApplication.class }, properties = {
+		"spring.r2dbc.url=r2dbc:gcp:mysql://spring-cloud-gcp-ci:us-central1:testmysql/code_samples_r2dbc_db",


Can we put this configuration in under the spring-cloud-gcp-ci-it profile like we did for the non-R2DBC sample?

The integration tests use different properties based on the database they're talking to. This might make it harder for them to be under the same profile? The non-R2DBC samples seem to use a hybrid approach where the mySQL test uses the profile while the postgreSQL test configures properties in the test. However, moving all the setup to the test classes could potentially help with readability. I could also be missing some maven-related knowledge here so let me know what you think.

spring-cloud-gcp-samples/spring-cloud-gcp-sql-postgres-r2dbc-sample/README.adoc

spring-cloud-gcp-starters/spring-cloud-gcp-starter-sql-r2dbc/pom.xml

…rovided

…n up samples

...in/java/com/google/cloud/spring/autoconfigure/sql/R2dbcCloudSqlEnvironmentPostProcessor.java

...autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/sql/R2dbcDatabaseType.java

...cp-samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/src/main/resources/application.properties

…p into autoconfigure-r2dbc

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

...autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/sql/R2dbcDatabaseType.java

meltsufin · 2021-12-15T20:38:46Z

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java

@@ -52,8 +53,7 @@
 * @author Eddú Meléndez
 */
 public class CloudSqlEnvironmentPostProcessor implements EnvironmentPostProcessor {


Seeing now how handling both JDBC and R2DBC in one class here is not really reusing that much code, and making it less readable, maybe a separate EventPostProcessor is a better design after all. You can still re-use some code bt introducing a common AbstractCloudSqlEnvironmentPostProcessor that both would extend.

This reverts commit dee8706.

elefeint

I ran into a self-created issue (provided wrong format for instance), but as part of troubleshooting noticed that Spring Data R2DBC configuration has a concept of ConnectionFactoryOptionsBuilderCustomizer. This may be a more natural way to prepare options for the socket factory than the more generic environment post-processor. Could you try to replace R2dbcCloudSqlEnvironmentPostProcessor with a new autoconfiguration class? It will need to be listed in spring.factories under EnableAutoConfiguration.

https://docs.spring.io/spring-boot/docs/2.5.7/reference/html/features.html#features.sql.r2dbc

spring-cloud-gcp-autoconfigure/pom.xml

...-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/sql/DatabaseType.java

elefeint · 2021-12-16T18:44:36Z

...in/java/com/google/cloud/spring/autoconfigure/sql/R2dbcCloudSqlEnvironmentPostProcessor.java

+	}
+
+	DatabaseType getEnabledDatabaseType(ConfigurableEnvironment environment) {
+		if (Boolean.parseBoolean(environment.getProperty("spring.cloud.gcp.sql.enabled", "true"))


Make sure that R2dbc post-processing is not triggered if the user just wants plain old JDBC>

Done. This method now returns null if the dependency that is used to enable Spring R2DBC auto-configuration is not present.

What if the user has the R2DBC dependency, but doesn't want our auto-configuration to kick in? I think we might need something like spring.cloud.gcp.sql.r2dbc.enabled.

That would be instead of spring.cloud.gcp.sql.enabled for Cloud SQL R2DBC autoconfig being on, not in addition to, right?
I guess theoretically it's possible that an app wants spring.cloud.gcp.sql.enabled for blocking connection to Cloud SQL, and then an R2DBC driver to another type of database. It would be weird for a reactive app to have blocking components, but not impossible.

Ideally, spring.cloud.gcp.sql.enabled=false would disable all of our Cloud SQL starters.
Maybe we need to add two properties: spring.cloud.gcp.sql.r2dbc.enabled and spring.cloud.gcp.sql.jdbc.enabled.

So for this PR, the check would be that both spring.cloud.gcp.sql.enabled and spring.cloud.gcp.sql.r2dbc.enabled are true. That sounds good.

Yep. Also, consider introducing spring.cloud.gcp.sql.jdbc.enabled.
Eventually, we might want to deprecate spring.cloud.gcp.sql.enabled.

This is something that I did not think of before. Since it is possible for an app to expect a blocking connection while having the r2dbc driver on the classpath, introducing spring.cloud.gcp.sql.r2dbc.enabled sounds like a good idea. I'll add spring.cloud.gcp.sql.jdbc.enabled in a separate PR since it might break existing behavior for current cloud sql customers.

@meltsufin @elefeint Added spring.cloud.gcp.sql.r2dbc.enabled which will be true by default.

...cp-samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/src/main/resources/application.properties

spring-cloud-gcp-samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/README.adoc

mpeddada1 · 2021-12-20T17:42:15Z

I ran into a self-created issue (provided wrong format for instance), but as part of troubleshooting noticed that Spring Data R2DBC configuration has a concept of ConnectionFactoryOptionsBuilderCustomizer. This may be a more natural way to prepare options for the socket factory than the more generic environment post-processor. Could you try to replace R2dbcCloudSqlEnvironmentPostProcessor with a new autoconfiguration class? It will need to be listed in spring.factories under EnableAutoConfiguration.

https://docs.spring.io/spring-boot/docs/2.5.7/reference/html/features.html#features.sql.r2dbc

That is a great discovery! I agree with you that ConnectionFactoryOptionsBuilderCustomizer is the most natural way to customize the ConnectionFactory. However, it may come down to the kind of user experience we want to provide. The r2dbc url is expected if we go with this particular workflow. If we don't provide a url then we run into the following exception at the connection factory bean creation step:
Error creating bean with name 'connectionFactory' defined in class path resource [org/springframework/boot/autoconfigure/r2dbc/ConnectionFactoryConfigurations$Pool.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [io.r2dbc.pool.ConnectionPool]: Factory method 'connectionFactory' threw exception; nested exception is org.springframework.boot.autoconfigure.r2dbc.ConnectionFactoryOptionsInitializer$ConnectionFactoryBeanCreationException: Failed to determine a suitable R2DBC Connection URL

If we want to only require the database name and the instance connection name to be provided without the need for the url (similar to what we have for the jdbc workflow) then the environment post processor will still be needed. Let me know what you think?

elefeint · 2021-12-20T18:57:59Z

@mpeddada1 That makes sense, thank you for checking!

…ean up enum; add comment in pom.xml

elefeint

Looks good, only a couple of minor changes.

...ava/com/google/cloud/spring/autoconfigure/sql/R2dbcCloudSqlEnvironmentPostProcessorTest.java

...samples/spring-cloud-gcp-sql-mysql-r2dbc-sample/src/main/java/com/example/WebController.java

...l-r2dbc-sample/src/test/java/com/example/SqlR2dbcMySqlSampleApplicationIntegrationTests.java

...ples/spring-cloud-gcp-sql-postgres-r2dbc-sample/src/main/java/com/example/WebController.java

...-gcp-autoconfigure/src/main/java/com/google/cloud/spring/autoconfigure/sql/DatabaseType.java

meltsufin · 2021-12-21T15:46:35Z

...in/java/com/google/cloud/spring/autoconfigure/sql/R2dbcCloudSqlEnvironmentPostProcessor.java

+	private static final Log LOGGER = LogFactory.getLog(R2dbcCloudSqlEnvironmentPostProcessor.class);
+
+	@Override
+	public void postProcessEnvironment(ConfigurableEnvironment environment, SpringApplication application) {


It's a minor thing, but I think there is even more code here that can be shared by adding some extra parameters to a helper method. It could make sense, unless it makes the code less readable.

Hm that's a good point. One potential area I can think of refactoring is CloudJdbcInfoProvider and DefaultCloudSqlJdbcProvider. We used this interface in the past because we also had AppEngineCloudSqlJdbcInfoProvider but we may not need this interface anymore -- this can help reduce added hierarchy in the code. Additionally some of the code that is used to create a url can be shared between the r2dbc and jdbc workflow. I can do this refactoring in a follow-up PR if you think this is a good idea.

…ysql test

…p into autoconfigure-r2dbc

meltsufin

LGTM, but note one comment on spring.cloud.gcp.sql.r2dbc.enabled.
@elefeint WDYT?

...toconfigure/src/main/java/com/google/cloud/spring/autoconfigure/sql/PropertiesRetriever.java

meltsufin · 2022-01-19T16:46:31Z

...in/java/com/google/cloud/spring/autoconfigure/sql/R2dbcCloudSqlEnvironmentPostProcessor.java

+	}
+
+	DatabaseType getEnabledDatabaseType(ConfigurableEnvironment environment) {
+		if (Boolean.parseBoolean(environment.getProperty("spring.cloud.gcp.sql.enabled", "true"))


What if the user has the R2DBC dependency, but doesn't want our auto-configuration to kick in? I think we might need something like spring.cloud.gcp.sql.r2dbc.enabled.

…p into autoconfigure-r2dbc

sonarcloud · 2022-01-21T20:56:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.6% Coverage
0.0% Duplication

elefeint

Looks great.

Could you add a paragraph to refdoc in cloud-sql.adoc? Can be a followup PR.

mpeddada1 · 2022-01-24T16:30:04Z

Thank you! Yup, I'll do this in a follow-up PR.

This PR adds two starters for R2DBC support in Cloud SQL (one for mySQL and another for postgreSQL). It also includes 2 samples, associated integrations tests and documentation. The starters bring in spring-boot-starter-data-r2dbc which takes care of the creation of the ConnectionFactory bean.

…ogleCloudPlatform#772) This PR adds two starters for R2DBC support in Cloud SQL (one for mySQL and another for postgreSQL). It also includes 2 samples, associated integrations tests and documentation. The starters bring in spring-boot-starter-data-r2dbc which takes care of the creation of the ConnectionFactory bean.

mpeddada1 added 2 commits December 7, 2021 16:56

include starter and samples for R2DBC; undo renaming of class

31d7ed8

improve documentation and cleanup samples

7711250

mpeddada1 requested a review from a team December 7, 2021 22:24

mpeddada1 mentioned this pull request Dec 8, 2021

R2DBC: Allow external credentials to be provided to Cloud SQL #775

Merged

meltsufin requested changes Dec 9, 2021

View reviewed changes

mpeddada1 added 4 commits December 10, 2021 15:53

Build r2dbc url when database name and instance connection name are p…

92ca251

…rovided

fix README; use webflux in samples

eeacab9

improve check for postgreSQL and mySQL driver in post processor; clea…

37ee37c

…n up samples

more clean up

92b23e0

mpeddada1 changed the title ~~Create starter and samples for R2dbc support in Cloud Sql~~ Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql Dec 13, 2021

mpeddada1 added 2 commits December 13, 2021 12:40

fix comment in application.properties for postgreSQL sample

139fb92

remove public modifier from test class

99202b3

meltsufin reviewed Dec 13, 2021

View reviewed changes

mpeddada1 added 3 commits December 14, 2021 17:59

Combine R2dbc logic into CloudSqlEnvironmentPostProcessor

dee8706

Merge branch 'main' of github.com:GoogleCloudPlatform/spring-cloud-gc…

5796bdf

…p into autoconfigure-r2dbc

remove redundant else if condition

8da6245

mpeddada1 commented Dec 14, 2021

View reviewed changes

...rc/main/java/com/google/cloud/spring/autoconfigure/sql/CloudSqlEnvironmentPostProcessor.java Outdated Show resolved Hide resolved

meltsufin reviewed Dec 15, 2021

View reviewed changes

mpeddada1 added 3 commits December 15, 2021 17:00

Revert "Combine R2dbc logic into CloudSqlEnvironmentPostProcessor"

2975a77

This reverts commit dee8706.

Introduce helper to gather properties

2394ad6

remove unused class

a428256

elefeint suggested changes Dec 16, 2021

View reviewed changes

mpeddada1 added 4 commits December 20, 2021 14:12

check if ConnectionFactory is present in getEnabledDatabaseType(); cl…

8bd52bf

…ean up enum; add comment in pom.xml

add more instructions to sample

ac2334b

switch order in comment

c8497b8

add test for extra condition

1fdf4c8

mpeddada1 requested a review from elefeint December 20, 2021 19:29

elefeint approved these changes Dec 21, 2021

View reviewed changes

meltsufin reviewed Dec 21, 2021

View reviewed changes

mpeddada1 added 3 commits December 28, 2021 10:49

use Flux; only 2 constants in enum; use properties from pom.xml for m…

a494df6

…ysql test

fix pom.xml

c05e025

Merge branch 'main' of github.com:GoogleCloudPlatform/spring-cloud-gc…

1171bec

…p into autoconfigure-r2dbc

mpeddada1 requested review from elefeint and meltsufin December 28, 2021 20:11

mpeddada1 mentioned this pull request Dec 28, 2021

R2DBC in Cloud SQL : Enable support for IAM database authentication #821

Closed

meltsufin approved these changes Jan 19, 2022

View reviewed changes

mpeddada1 added 6 commits January 21, 2022 11:39

apply google java formatter

e9bab44

resolve merge conficts

29d1540

Merge branch 'main' of github.com:GoogleCloudPlatform/spring-cloud-gc…

c44ca56

…p into autoconfigure-r2dbc

more google formatting

31e7ddf

Add property to enable r2dbc

cf7d15c

fix test

f4b758d

elefeint approved these changes Jan 24, 2022

View reviewed changes

mpeddada1 mentioned this pull request Jan 24, 2022

Introduce property to enable JDBC support for Cloud SQL #901

Closed

mpeddada1 merged commit 7b69799 into main Jan 24, 2022

mpeddada1 deleted the autoconfigure-r2dbc branch January 24, 2022 16:30

elefeint mentioned this pull request Jan 28, 2022

Autoconfigure R2DBC support in Cloud Sql (backport) #923

Closed

ttosta-google mentioned this pull request Apr 23, 2024

feat: Spring Cloud AlloyDB integration (#2787) #2788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772

Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772

mpeddada1 commented Dec 7, 2021 •

edited

meltsufin left a comment

meltsufin Dec 9, 2021

mpeddada1 Dec 15, 2021

meltsufin Dec 15, 2021

elefeint left a comment

elefeint Dec 16, 2021

mpeddada1 Dec 20, 2021

meltsufin Jan 19, 2022

elefeint Jan 19, 2022

meltsufin Jan 19, 2022

elefeint Jan 19, 2022

meltsufin Jan 19, 2022

mpeddada1 Jan 19, 2022

mpeddada1 Jan 21, 2022

mpeddada1 commented Dec 20, 2021

elefeint commented Dec 20, 2021

elefeint left a comment

meltsufin Dec 21, 2021

mpeddada1 Dec 28, 2021

meltsufin left a comment

meltsufin Jan 19, 2022

sonarcloud bot commented Jan 21, 2022

elefeint left a comment

mpeddada1 commented Jan 24, 2022

Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772

Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772

Conversation

mpeddada1 commented Dec 7, 2021 • edited

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpeddada1 commented Dec 20, 2021

elefeint commented Dec 20, 2021

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 21, 2022

elefeint left a comment

Choose a reason for hiding this comment

mpeddada1 commented Jan 24, 2022

mpeddada1 commented Dec 7, 2021 •

edited