-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
support using mariadb connector with mysql extensions #11402
support using mariadb connector with mysql extensions #11402
Conversation
@@ -57,6 +67,194 @@ public static void throwIfPropertiesAreNotAllowed( | |||
} | |||
} | |||
|
|||
public static Set<String> tryParseJdbcUriParameters(String connectionUri, boolean allowUnknown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add javadocs explaining the fallback to mariaDB?
try { | ||
return tryParseMySqlConnectionUri(connectionUri); | ||
} | ||
catch (ClassNotFoundException notFoundMysql) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these branches covered by unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not currently, trying to think of a good way to test it, i guess i need some different classloaders to model different configurations or something. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one way would be to take Class.forName()
to a different static method in a different class and use that method here instead. In tests, you could mock the other class and method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found Mockito.mockStatic
which lets me model these branches a bit by just letting me mock the tryParse{x]ConnectionUri
methods, so the coverage is a bit better now i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Few minor comments
@JacksonInject JdbcAccessSecurityConfig securityConfig | ||
) | ||
{ | ||
this.connectorConfig = connectorConfig; | ||
this.driverClassName = driverClassName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if the connectURL and driverClassName match? i.e. we can gracefully fail if we get connectURL=jdbc:mariadb://localhost:3306/test?... and driverClassName=com.mysql.jdbc.Driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this I think should be happening in ConnectionUriUtils
where if only the mysql driver is loaded then it will fail to parse the mariadb jdbc uri. I did change this though so that if driverClassName
is null, and if the uri starts with the mariadb prefix, then it will default to the mariadb driver.
Or you are you imagining a case where both drivers are loaded so that one is used for validation but a separate is used for the actual driver? I guess that could happen, and would certainly be strange, but isn't really a vulnerability I think at least because you would still need access to the Druid configuration to make a more permissive configuration for validation than the actual driver.
integration-tests/docker/Dockerfile
Outdated
RUN if [ "$MYSQL_DRIVER_CLASSNAME" = "com.mysql.jdbc.Driver" ] ; \ | ||
then wget -q "https://repo1.maven.org/maven2/mysql/mysql-connector-java/$MYSQL_VERSION/mysql-connector-java-$MYSQL_VERSION.jar" \ | ||
-O /usr/local/druid/lib/mysql-connector-java.jar; \ | ||
else wget -q "https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/$MARIA_VERSION/mariadb-java-client-$MARIA_VERSION.jar" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be else if and check that the MYSQL_DRIVER_CLASSNAME is set to Maria? But we easier to read/test when we add more driver classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to if/else
* support using mariadb connector with mysql extensions * cleanup and more tests * fix test * javadocs, more tests, etc * style and more test * more test more better * missing pom * more pom
Description
This PR adds support for using MariaDB java connector (and by extension database) using the Druid MySQL metadata extension, as well as JDBC connection string uri validation for lookups and SQL ingestion.
For the metadata connector, a new config,
druid.metadata.mysql.driver.driverClassName
has been added, which can be set toorg.mariadb.jdbc.Driver
for example to allow use of the MariaDB Connector/J.I got a bit carried away, and now JDBC connection string uri validation for all supported databases has been moved into
druid-core
and is performed with reflection, so the logic is no longer duplicated across the lookup JDBC handling and SQL firehose logic and is instead centralized inConnectionUriUtils
.The Docker integration tests have been modified to accept an additional environment variable,
MYSQL_DRIVER_CLASSNAME
, and setting it toorg.mariadb.jdbc.Driver
will kick the test container into a mode where they load the mariadb connector jar instead of mysql jar and use that instead. I duplicated the 'query' integration test to run against mariadb, which at least gives coverage of the metadata connector, but I have tested jdbc lookups and sql ingestion manually as well to ensure that the property validation stuff is functional.It looked like there are a handful of documentation changes that should be made, which I will address in a follow-up PR if that is acceptable to reviewers.
This PR has: