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

Detect Kingbase JDBC driver #44710

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zbw1231
Copy link

@zbw1231 zbw1231 commented Mar 13, 2025

I add support for Kingabse(A new database)for v3.4.0.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 13, 2025
@@ -1,4 +1,4 @@
version=3.4.0-SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

3.4.0 was released last year and cannot be changed. Please revert.

@nosan
Copy link
Contributor

nosan commented Mar 13, 2025

Hi @zbw1231

I add support for Kingbase(A new database)

Typically, such changes are classified as enhancements, which means they would be
merged into the main branch and included in the upcoming 3.5.X release.

Unfortunately, your PR cannot be merged in its current state. It needs some updates to be considered as a potential candidate for merging:

  • Your branch should be rebased on top of the main branch
  • Your PR does not include any tests. It would be helpful to add some tests to DatabaseDriverTests.java to verify your changes.
  • I also noticed that you did not use the spring-javaformat formatter, so there are likely checkstyle errors.
  • Your commit must be signed to pass the DCO check.

I highly recommend reading CONTRIBUTING.adoc as it contains very useful information for contributors.

Additionally, the changes in your PR are very similar to this, so feel free to use it as a potential guide.

/**
* Kingbase.
*/
KINGBASE("KingbaseES","com.kingbase8.Driver","com.kingbase8.xa.KBXADataSource","SELECT 1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to add URL prefix.

	@Override
		protected Collection<String> getUrlPrefixes() {
			return Collections.singleton("kingbase8");
		}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your advice,I added test and URL prefix for kingbase like the example you provided.Please review.

@snicoll snicoll changed the title Spring boot 3.4.0 changes Detect Kingbase JDBC driver Mar 14, 2025
@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Mar 14, 2025
@zbw1231 zbw1231 force-pushed the spring-boot-3.4.0-changes branch from 1896a69 to f7bb14f Compare March 16, 2025 07:56
wilkinsona and others added 3 commits March 16, 2025 17:53
@zbw1231 zbw1231 force-pushed the spring-boot-3.4.0-changes branch from 027573f to 631643d Compare March 16, 2025 09:53
Copy link
Contributor

@nosan nosan left a comment

Choose a reason for hiding this comment

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

Thank you, @zbw1231

I've left some comments for you consideration.

@@ -1,4 +1,4 @@
version=3.5.0-SNAPSHOT
version=3.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert.

@@ -83,6 +83,19 @@ public enum DatabaseDriver {
*/
POSTGRESQL("PostgreSQL", "org.postgresql.Driver", "org.postgresql.xa.PGXADataSource", "SELECT 1"),

/**
* Kingbase.
* @since 3.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

@since 3.5.0

@@ -99,6 +100,8 @@ void databaseJdbcUrlLookups() {
.isEqualTo(DatabaseDriver.ORACLE);
assertThat(DatabaseDriver.fromJdbcUrl("jdbc:postgresql://127.0.0.1:5432/sample"))
.isEqualTo(DatabaseDriver.POSTGRESQL);
assertThat(DatabaseDriver.fromJdbcUrl("jdbc:kingbase8://localhost:54321/test"))
Copy link
Contributor

@nosan nosan Mar 16, 2025

Choose a reason for hiding this comment

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

This test fails.

Please run ./gradlew build to verify your changes

* Kingbase.
* @since 3.4.0
*/
KINGBASE("KingbaseES","com.kingbase8.Driver","com.kingbase8.xa.KBXADataSource","SELECT 1") {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still an issue with the formatting. Please refer to the Spring Boot Contributing Guidelines.

Please use ./gradlew format to reformat your code.

@@ -72,6 +72,7 @@ void databaseProductNameLookups() {
assertThat(DatabaseDriver.fromProductName("MariaDB")).isEqualTo(DatabaseDriver.MARIADB);
assertThat(DatabaseDriver.fromProductName("Oracle")).isEqualTo(DatabaseDriver.ORACLE);
assertThat(DatabaseDriver.fromProductName("PostgreSQL")).isEqualTo(DatabaseDriver.POSTGRESQL);
assertThat(DatabaseDriver.fromProductName("KingbaseES")).isEqualTo(DatabaseDriver.KingbaseES);
Copy link
Contributor

@nosan nosan Mar 16, 2025

Choose a reason for hiding this comment

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

Compile error:

/Users/dmytronosan/IdeaProjects/spring-boot/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/jdbc/DatabaseDriverTests.java:75: error: cannot find symbol
assertThat(DatabaseDriver.fromProductName("KingbaseES")).isEqualTo(DatabaseDriver.KingbaseES);
^
symbol: variable KingbaseES
location: class DatabaseDriver

Please run ./gradlew build to ensure that your changes do not break the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants