Skip to content

SQL: Refactor Tableau connector to make use of the connection properties #69169

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 15, 2021
Merged

SQL: Refactor Tableau connector to make use of the connection properties #69169

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2b31164
Refactor to make use of the connection propreties
bpintea Feb 18, 2021
caffe93
Merge branch 'master' into enh/tableau_conn_props
elasticmachine Feb 18, 2021
4190b61
Address review feedback
bpintea Feb 19, 2021
397d6c6
Merge branch 'enh/tableau_conn_props' of github.com:bpintea/elasticse…
bpintea Feb 19, 2021
af4e372
Merge branch 'master' into enh/tableau_conn_props
elasticmachine Feb 19, 2021
d6add92
Remove all logging
bpintea Mar 12, 2021
2d1790e
Merge branch 'master' into enh/tableau_conn_props
elasticmachine Mar 15, 2021
c131b3c
Add comment on AVPs delimiters
bpintea Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 1 addition & 29 deletions x-pack/plugin/sql/connectors/tableau/connector/connectionBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,7 @@
} else{
urlBuilder += "http://";
}
urlBuilder += attr["server"] + ":" + attr["port"];
urlBuilder += attr["server"] + ":" + attr["port"] + "?";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the question mark at the end always necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


var params = [];
params["user"] = attr["username"];
params["password"] = attr["password"];

var formattedParams = [];

for (var key in params) {
if (params[key]) {
var param = encodeURIComponent(params[key]);
formattedParams.push(connectionHelper.formatKeyValuePair(key, param));
}
}

if (formattedParams.length > 0) {
urlBuilder += "?" + formattedParams.join("&")
}

// logging visible in log.txt if -DLogLevel=Debug is added in Tableau command line
logging.log("ES JDBC URL before adding additional parameters: " + urlBuilder);

// TODO: wrap error-prone "free form"
var additionalConnectionParameters = attr[connectionHelper.attributeWarehouse];
if (additionalConnectionParameters != null && additionalConnectionParameters.trim().length > 0) {
urlBuilder += (formattedParams.length == 0) ? "?" : "&";
urlBuilder += additionalConnectionParameters;
}

logging.log("ES JDBC final URL: " + urlBuilder);
return [urlBuilder];
})
24 changes: 24 additions & 0 deletions x-pack/plugin/sql/connectors/tableau/connector/connectionProperties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
(function propertiesbuilder(attr) {
var props = {};

props["user"] = attr["username"];
props["password"] = attr["password"];

var extraProps = attr[connectionHelper.attributeWarehouse];
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("=");
Copy link
Contributor

@matriv matriv Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thx for explaining!

if (tokens.length != 2 || tokens[0].length == 0 || tokens[1].length == 0) {
logging.log("Malformed AVP: [" + avps[i] + "] in additional params: [" + extraProps + "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

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:......"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acronym expanded.

var errMessage = "Invalid additional settings property `" + avps[i] + "`: " +
"not conforming to the attribute=value format."
return connectionHelper.ThrowTableauException(errMessage);
} else {
props[tokens[0]] = tokens[1];
}
}
}

return props;
})
3 changes: 3 additions & 0 deletions x-pack/plugin/sql/connectors/tableau/connector/connectionResolver.tdr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,8 @@
</attribute-list>
</required-attributes>
</connection-normalizer>
<connection-properties>
<script file='connectionProperties.js'/>
</connection-properties>
</connection-resolver>
</tdr>