SQL: Refactor Tableau connector to make use of the connection properties #69169
Conversation
This refactors the way the connection URL is being built, to make use of frameworks' ability to construct the URL based on a set of connection propreties. This also ensures that the URI attributes are properly escaped.
elasticmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/bwc |
| for (var i = 0; i < avps.length; i++) { | ||
| var tokens = avps[i].split("="); | ||
| if (tokens.length != 2 || tokens[0].length == 0 || tokens[1].length == 0) { | ||
| logging.log("Malformed AVP: [" + avps[i] + "] in additional params: [" + extraProps + "]"); |
There was a problem hiding this comment.
I guess what AVP means, but I am not sure. Especially this being a logged message, I'd give it a meaningful name. Something like "Malformed attribute-value pair:......"
| @@ -6,35 +6,7 @@ | |||
| } else{ | |||
| urlBuilder += "http://"; | |||
| } | |||
| urlBuilder += attr["server"] + ":" + attr["port"]; | |||
| urlBuilder += attr["server"] + ":" + attr["port"] + "?"; | |||
There was a problem hiding this comment.
Is the question mark at the end always necessary?
There was a problem hiding this comment.
Yes: the "user" is a mandatory config option (can't submit the form if not filled), so there will always be a property set; so adding the ? is safe.
Rephrase log message.
…arch into enh/tableau_conn_props
|
@elasticmachine update branch |
Remove logging to prevent sensitive info being logged, inline with Tableau's best practices recommendations.
|
@elasticmachine run elasticsearch-ci/bwc |
| if (extraProps != null && extraProps.trim().length > 0) { | ||
| var avps = extraProps.trim().split(/[\s&]/); | ||
| for (var i = 0; i < avps.length; i++) { | ||
| var tokens = avps[i].split("="); |
There was a problem hiding this comment.
Question, to be more lenient shouldn't we trim the split tokens, to treat something like:
" param1 = value1 "
(used double quotes to denote surrounding spaces)
There was a problem hiding this comment.
Space is accepted as a delimiter between the AVPs, as a friendlier alternative to &; i.e. both forms a=b&c=d and a=b c=d are permitted. Spaces would normally need to be encoded in a URI.
That's why the initial AVP splitting is done by .split(/[\s&]/).
I've added a comment.
|
@elasticmachine update branch |
- both `&` and spaces are allowed.
…ies (#69169) * Refactor to make use of the connection propreties This refactors the way the connection URL is being built, to make use of frameworks' ability to construct the URL based on a set of connection propreties. This also ensures that the URI attributes are properly escaped. (cherry picked from commit 3c1e0a9)
…ies (#69169) * Refactor to make use of the connection propreties This refactors the way the connection URL is being built, to make use of frameworks' ability to construct the URL based on a set of connection propreties. This also ensures that the URI attributes are properly escaped. (cherry picked from commit 3c1e0a9)
…ies (#69169) * Refactor to make use of the connection propreties This refactors the way the connection URL is being built, to make use of frameworks' ability to construct the URL based on a set of connection propreties. This also ensures that the URI attributes are properly escaped. (cherry picked from commit 3c1e0a9)
This refactors the way the connection URL is being built, to make use of
frameworks' ability to construct the URL based on a set of connection
properties.
This comes with the benefit that the URI attributes escaping is taken
care for us.
The PR also removes the URI "manual" logging, which is already done
by the framework, with proper masking of the password.