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

Add support for AWS Advanced JDBC Wrapper #43812

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Jan 14, 2025

This commit adds entry for AWS Advanced JDBC Wrapper to DatabaseDriver enum.

See gh-31995

This commit adds entry for AWS Advanced JDBC Wrapper to `DatabaseDriver`
enum.

See spring-projectsgh-31995

Signed-off-by: Vedran Pavic <[email protected]>

@Override
protected Collection<String> getUrlPrefixes() {
return List.of("aws-wrapper");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used List#of as it seems to be the simplest option, but note that implementations of #getUrlPrefixes in this enum are quite inconsistent - there are usages of Arrays#asList and Collections#singleton.

Perhaps this could be aligned in a separate issue to use List#of consistently (first-timers-only candidate?).

@wilkinsona
Copy link
Member

Thanks, @vpavic. In isolation, I wonder how useful this will be. Looking at how DatabaseDriver is used, being able to detect AWS's wrapper may not be enough in some cases. Take Flyway's auto-configuration for example. As you may remember as I think you contributed it, it uses DatabaseDriver to figure out which vendor-specific migrations to use. In that situation, I think we'd need to detect the driver that's being wrapped rather than the wrapper. I think the same applies to the basic script-based initialization and the like as well.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 15, 2025
@vpavic
Copy link
Contributor Author

vpavic commented Jan 15, 2025

Thanks for the feedback.

Motivation here is to simplify usage of AWS Advanced JDBC Wrapper by not having to specify spring.datasource.driver-class-name property. Depending on the environment (and the exact database used), sometimes we need to use plain PostgreSQL JDBC driver and other times AWS wrapper - it would be very useful to do that switch only by changing a single configuration property.

I understand your concerns, and have refreshed my memory a bit on support for vendor-specific Flyway migrations (yes, I contributed that quite some while ago 🙂) and I believe your comment aims at this usage of DatabaseDriver:

String url = JdbcUtils.extractDatabaseMetaData(this.dataSource, DatabaseMetaData::getURL);
return DatabaseDriver.fromJdbcUrl(url);

This indeed doesn't play nice with wrapper drivers but such use cases could be updated to rely on database product name instead. I can contribute that if you agree with that direction.

I can see that such approach is already used in PlatformPlaceholderDatabaseDriverResolver:

String productName = JdbcUtils.commonDatabaseName(
JdbcUtils.extractDatabaseMetaData(dataSource, DatabaseMetaData::getDatabaseProductName));
return DatabaseDriver.fromProductName(productName);

In any case, I think these concerns are not new since Testcontainers JDBC driver also is of similar nature as it wraps the underlying JDBC driver and requires inserting its prefix into JDBC URL.

@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 Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants