[Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider #15153

mind-echo · 2023-11-10T08:05:17Z

fix #15152 : When using PostgreSQL, MyBatis did not set databaseId correctly, causing the workflow to fail to start

Purpose of the pull request

fix bug #15152

Brief change log

add bean of VendorDatabaseIdProvider
change org.apache.dolphinscheduler.dao.mapper.TriggerRelationMapper#upsert.databaseId from pg to PostgreSQL

Verify this pull request

fuchanghai

LGTM
BTW, do we need to do some mapping ? as following :

@Bean
public DatabaseIdProvider databaseIdProvider() {
    VendorDatabaseIdProvider databaseIdProvider = new VendorDatabaseIdProvider();
    Properties properties = new Properties();
    properties.put("Oracle","oracle");
    properties.put("MySQL","mysql");
    properties.put("PostgreSQL","pg");
    databaseIdProvider.setProperties(properties);
    return databaseIdProvider;
}

cc @EricGao888

mind-echo · 2023-11-13T06:19:23Z

LGTM BTW, do we need to do some mapping ? as following :

@Bean
public DatabaseIdProvider databaseIdProvider() {
    VendorDatabaseIdProvider databaseIdProvider = new VendorDatabaseIdProvider();
    Properties properties = new Properties();
    properties.put("Oracle","oracle");
    properties.put("MySQL","mysql");
    properties.put("PostgreSQL","pg");
    databaseIdProvider.setProperties(properties);
    return databaseIdProvider;
}

cc @EricGao888

Thank you for your review. I have made changes to the current code

fuchanghai · 2023-11-13T08:49:59Z

hi @destroydestiny Is it possible to write an enumeration class to reduce the magic value?

mind-echo · 2023-11-13T09:51:21Z

hi @destroydestiny Is it possible to write an enumeration class to reduce the magic value?

I added an enum, but I'm not sure if I added it in the right package

SbloodyS · 2023-11-14T02:34:45Z

dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/enums/DatabaseId.java

+public enum DatabaseId {
+
+    H2("H2", "h2"),
+    ORACLE("Oracle", "oracle"),


We do not support oracle database as meta database.

ruanwenjun · 2023-11-15T15:54:08Z

dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/enums/DatabaseId.java

+public enum DatabaseId {
+
+    H2("H2", "h2"),
+    MYSQL("MySQL", "mysql"),
+    POSTGRESQL("PostgreSQL", "pg");
+
+    private final String productName;
+    private final String databaseId;
+}


Can we move this into ado-plugin https://github.com/apache/dolphinscheduler/tree/dev/dolphinscheduler-dao-plugin, and add method to get databaseId mapping at https://github.com/apache/dolphinscheduler/blob/dev/dolphinscheduler-dao-plugin/dolphinscheduler-dao-api/src/main/java/org/apache/dolphinscheduler/dao/plugin/api/DaoPluginConfiguration.java

ruanwenjun

LGTM

ruanwenjun · 2023-11-16T14:27:48Z

dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/DaoConfiguration.java

@@ -61,6 +63,11 @@ public MybatisPlusInterceptor paginationInterceptor(DbType dbType) {
        return interceptor;
    }

+    @Bean
+    public DatabaseIdProvider databaseIdProvider() {
+        return new FixedDatabaseIdProvider(daoPluginConfiguration.databaseId());


Can we directly use daoPluginConfiguration.dbType().name() as databaseId?
I find the databaseId only exist in TriggerRelationMapper.xml if we modify the name to POSTGRE_SQL this should work.

that's what I thought at first, use DatabaseProductName as databaseId. view commit 19d8510

If so, we can remove databaseId, and use the default VendorDatabaseIdProvider without add properties after change the datasourceId to PostgreSQL in mapper? .

Please remove databaseId in DaoPlugin, and use this way.

Can I close this PR and recreate it? Too many commits are not clean enough

You can rebase the commit, and I will use squash merge to edit the commit message when merged. So this is not a problem, don't worry.

…correctly, causing the workflow to fail to start

…n the productName and the databaseId

…guration that returns a databaseId to create a FixedDatabaseIdProvider.

ruanwenjun

LGTM

SbloodyS

+1

codecov-commenter · 2023-11-17T08:53:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6175f3) 38.23% compared to head (571cebf) 38.23%.

❗ Current head 571cebf differs from pull request most recent head 4ee2941. Consider uploading reports for the commit 4ee2941 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##                dev   #15153      +/-   ##
============================================
- Coverage     38.23%   38.23%   -0.01%     
  Complexity     4695     4695              
============================================
  Files          1285     1285              
  Lines         45453    45454       +1     
  Branches       4951     4951              
============================================
  Hits          17381    17381              
  Misses        26181    26181              
- Partials       1891     1892       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarcloud · 2023-11-17T08:59:05Z

Please retry analysis of this Pull-Request directly on SonarCloud.

ruanwenjun · 2023-11-17T13:33:28Z

Merged, thanks for you pr. BTW, we don't only exist UT under h2, are you interesting add test for PG @destroydestiny

mind-echo requested review from caishunfeng and SbloodyS as code owners November 10, 2023 08:05

github-actions bot added the backend label Nov 10, 2023

fuchanghai approved these changes Nov 13, 2023

View reviewed changes

SbloodyS requested changes Nov 14, 2023

View reviewed changes

SbloodyS assigned mind-echo Nov 14, 2023

SbloodyS added bug Something isn't working 3.2.1 labels Nov 14, 2023

SbloodyS added this to the 3.2.1 milestone Nov 14, 2023

SbloodyS added the first time contributor First-time contributor label Nov 14, 2023

mind-echo requested a review from SbloodyS November 15, 2023 08:27

ruanwenjun requested changes Nov 15, 2023

View reviewed changes

ruanwenjun previously approved these changes Nov 16, 2023

View reviewed changes

ruanwenjun reviewed Nov 16, 2023

View reviewed changes

xieyuliang added 6 commits November 16, 2023 22:28

fix bug 15152: When using PostgreSQL, MyBatis did not set databaseId …

19d8510

…correctly, causing the workflow to fail to start

fix bug 15152: add databaseId mapping

74448bc

fix bug 15152: add the DatabaseId enum to describe the mapping betwee…

6485864

…n the productName and the databaseId

fix bug 15152: remove DatabaseId that are not needed

8449a42

fix bug 15152: optimize code style

f02c8bd

fix bug 15152: Remove DatabaseId enum. Add a method to DaoPluginConfi…

b2e8f47

…guration that returns a databaseId to create a FixedDatabaseIdProvider.

ruanwenjun force-pushed the fix_15152 branch from 55da505 to b2e8f47 Compare November 16, 2023 14:28

fix bug 15152: use DatabaseProductName as databaseId

4ee2941

mind-echo dismissed ruanwenjun’s stale review via 4ee2941 November 17, 2023 06:40

ruanwenjun approved these changes Nov 17, 2023

View reviewed changes

ruanwenjun changed the title ~~fix bug 15152: When using PostgreSQL, MyBatis did not set databaseId …~~ [Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider Nov 17, 2023

SbloodyS approved these changes Nov 17, 2023

View reviewed changes

SbloodyS added the ready-to-merge label Nov 17, 2023

ruanwenjun merged commit 36f7205 into apache:dev Nov 17, 2023
52 of 53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider #15153

[Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider #15153

mind-echo commented Nov 10, 2023 •

edited by SbloodyS

Loading

fuchanghai left a comment

mind-echo commented Nov 13, 2023

fuchanghai commented Nov 13, 2023

mind-echo commented Nov 13, 2023

SbloodyS Nov 14, 2023

ruanwenjun Nov 15, 2023

ruanwenjun left a comment

ruanwenjun Nov 16, 2023

mind-echo Nov 16, 2023

ruanwenjun Nov 17, 2023 •

edited

Loading

mind-echo Nov 17, 2023

ruanwenjun Nov 17, 2023

mind-echo Nov 17, 2023

ruanwenjun Nov 17, 2023 •

edited

Loading

ruanwenjun left a comment

SbloodyS left a comment

codecov-commenter commented Nov 17, 2023

sonarcloud bot commented Nov 17, 2023

ruanwenjun commented Nov 17, 2023

[Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider #15153

[Bug] Fix TriggerRelationMapper cannot work due to miss DatabaseIdProvider #15153

Conversation

mind-echo commented Nov 10, 2023 • edited by SbloodyS Loading

Purpose of the pull request

Brief change log

Verify this pull request

fuchanghai left a comment

Choose a reason for hiding this comment

mind-echo commented Nov 13, 2023

fuchanghai commented Nov 13, 2023

mind-echo commented Nov 13, 2023

SbloodyS Nov 14, 2023

Choose a reason for hiding this comment

ruanwenjun Nov 15, 2023

Choose a reason for hiding this comment

ruanwenjun left a comment

Choose a reason for hiding this comment

ruanwenjun Nov 16, 2023

Choose a reason for hiding this comment

mind-echo Nov 16, 2023

Choose a reason for hiding this comment

ruanwenjun Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

mind-echo Nov 17, 2023

Choose a reason for hiding this comment

ruanwenjun Nov 17, 2023

Choose a reason for hiding this comment

mind-echo Nov 17, 2023

Choose a reason for hiding this comment

ruanwenjun Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

ruanwenjun left a comment

Choose a reason for hiding this comment

SbloodyS left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2023

Codecov Report

sonarcloud bot commented Nov 17, 2023

ruanwenjun commented Nov 17, 2023

mind-echo commented Nov 10, 2023 •

edited by SbloodyS

Loading

ruanwenjun Nov 17, 2023 •

edited

Loading

ruanwenjun Nov 17, 2023 •

edited

Loading