Enhance JDBC connection string parsing for multiple hosts #1082
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #1082 +/- ## ============================================ - Coverage 60.74% 60.72% -0.02% Complexity 87 87 ============================================ Files 320 320 Lines 14446 14491 +45 Branches 1988 2014 +26 ============================================ + Hits 8775 8800 +25 - Misses 5091 5102 +11 - Partials 580 589 +9
Continue to review full report at Codecov.
|
SylvainJuge left a comment
SylvainJuge left a comment There was a problem hiding this comment.
LGTM: Only few really minor things, impressive test suite.
| return new ConnectionMetaData(dbVendor, host, port, user); | ||
| } | ||
| | ||
| private HostPort parseAddressList(String connectionUrl) { |
There was a problem hiding this comment.
[minor] maybe return null when HostPort.host == null || HostPort.port < 0
There was a problem hiding this comment.
[minor] maybe naming this method with Oracle might be relevant as it's very Oracle-specific.
There was a problem hiding this comment.
maybe return
nullwhenHostPort.host == null || HostPort.port < 0
👍 for HostPort.host == null, however I am pretty sure the port is optional, so one may set a host and not the port (in which case the default will be used- there is a test for that)
maybe naming this method with Oracle might be relevant as it's very Oracle-specific.
It's within the Oracle parser (enum)
| } | ||
| default: { | ||
| if (currentValueBuffer == null) { | ||
| logger.warn("Failed to parse Oracle DB address list from: {}", connectionUrl); |
There was a problem hiding this comment.
do we really want to continue parsing in case of error ?
There was a problem hiding this comment.
Could be an error or an unknown use of the connection string. However, I only care about very specific nodes in this tree, so if the parsing yields a valid tree in which I can find the tree nodes I am looking for, I don't care.
| return ConnectionUrlParser.defaultParse(connectionUrl, dbVendor, 3306, user); | ||
| String host = "localhost"; | ||
| int port = 3306; | ||
| HostPort hostPort = parseMySqlFlavor(connectionUrl); |
There was a problem hiding this comment.
[minor] same here, having a null return value would avoid duplication of the null check and port > 0 below.
There was a problem hiding this comment.
I don't think- most (or all) make port configuration optional, each provider using a different default port if not configured.
3 participants
What does this PR do?
Enhance the JDBC connection string parser to support dedicated syntaxes for multiple hosts in Oracle, MySQL and MariaDB.
Checklist
Related issues
Closes #1031