Skip to content
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

[feature] Add support for adhoc filters. (#103) #154

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

Olian04
Copy link

@Olian04 Olian04 commented Apr 3, 2023

This PR resolves issue #103 by adding support for adhoc filters to both SQL queries and Native queries.

This PR implements support for adhoc filters for the following query types:

As per the druid documentation, this PR does NOT implement adhoc filters for the following query types:

@Olian04
Copy link
Author

Olian04 commented Apr 4, 2023

@jbonofre pinging you since you're the one person I've seen accepting PRs.

@jbguerraz
Copy link
Contributor

Hello @Olian04 !
Thank you a lot for such great contribution 👍
I've seen some typos, but will do a proper review ASAP (matter of days). I've already questioned myself if autocomplete queries shouldn't live on the golang side of the plugin so they can be re-used later on for query building autocomplete.
Also, I had visited a bit the "autocomplete" feature and at that time seen that druid itself provided a toolkit/parser https://www.npmjs.com/package/druid-query-toolkit which may be a better fit than the postgres one.
Other than that, at first glance, it looks really cool :) thx!

Comment on lines +36 to +46
`SELECT "TABLE_NAME" FROM INFORMATION_SCHEMA.COLUMNS WHERE "TABLE_SCHEMA" = 'druid' AND "COLUMN_NAME" = '${options.key}'`
)
).map((it) => it.value);

const completions = (
await Promise.all(
tableNames.map(async (tableName) =>
this._postSqlQuery(
this.settingsData.adhoc?.shouldNotLimitAutocompleteValue
? `SELECT DISTINCT "${options.key}" FROM ${tableName}`
: `SELECT "${options.key}" FROM ${tableName} GROUP BY "${options.key}" ORDER BY COUNT("${options.key}") DESC LIMIT 1000`

Choose a reason for hiding this comment

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

Just text templating in values into SQL queries leaves them vulnerable to SQL injections attacks. Prepared statements ought to be preferred if at all possible, even if there are attempts to sanitise the templated values elsewhere.

...templatedQuery,
builder: {
...templatedQuery.builder,
query: SQL.stringify(query),

Choose a reason for hiding this comment

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

Do we know that this stringification is safe and can't lead to possible SQL injection attacks?

this._postSqlQuery(
this.settingsData.adhoc?.shouldNotLimitAutocompleteValue
? `SELECT DISTINCT "${options.key}" FROM ${tableName}`
: `SELECT "${options.key}" FROM ${tableName} GROUP BY "${options.key}" ORDER BY COUNT("${options.key}") DESC LIMIT 1000`

Choose a reason for hiding this comment

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

I appreciate the effort to limit the number of items the UI has to render, it would also be nice to limit the amount of data Druid has to work through.

@harrythebot
Copy link

What are the next steps required to merge this PR? Do we need to address all the comments from @MrLarssonJr ? I would like to get feedback from @jbguerraz

@Jhors2
Copy link

Jhors2 commented Oct 4, 2024

Any update here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants