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.

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

Jump to bottom

Sanitize create acceleration queries and direct queries #1605

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Sanitize create acceleration queries and direct queries #1605

merged 1 commit into from
Mar 21, 2024

Conversation

ps48
Copy link
Member

@ps48 ps48 commented Mar 21, 2024

Description

Sanitize queries which have a potential for users to add in a \n, \t or extra white spaces

Issues Resolved

  1. Sanitize direct queries from explorer
  2. Sanitize direct queries from create acceleration flyout

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ps48
sanitize create acceleration queries and direct queries
61143d8 
Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
@codecov Codecov
Copy link

codecov bot commented Mar 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.75%. Comparing base (76d206c) to head (61143d8).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1605   +/-   ##
=======================================
  Coverage   57.75%   57.75%           
=======================================
  Files         367      367           
  Lines       13824    13825    +1     
  Branches     3627     3627           
=======================================
+ Hits         7984     7985    +1     
  Misses       5777     5777           
  Partials       63       63
Flag Coverage Δ
dashboards-observability 57.75% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ps48 ps48 added bug Something isn't working backport 2.x backport 2.13 labels Mar 21, 2024
Swiddis
Swiddis reviewed Mar 21, 2024
View reviewed changes
...nents/manage/accelerations/create_accelerations_flyout/create/create_acceleration_button.tsx
@@ -39,7 +40,7 @@ export const CreateAccelerationButton = ({

const requestPayload: DirectQueryRequest = {
lang: 'sql',
query: accelerationQueryBuilder(accelerationFormData),
query: accelerationQueryBuilder(accelerationFormData).replaceAll(SANITIZE_QUERY_REGEX, ' '),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know I have code that's subject to the same issue, but this will also replace instances of multiple spaces in query strings and prevents things like adding single-line comments to queries.

It might be beyond the scope of this PR, but if we're doing this repeatedly, I think we should make a centralized sanitizeSql method that can handle detecting spaces in strings and remove comments before it tries to normalize whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. We need also centralize direct query calls so that we keep sanitizeSql method used only in one location. Have added this hook that can be used at all places from cache loaders, integrations to https://github.com/opensearch-project/dashboards-observability/blob/76d206c18bf412d9c4c541f06c991843a113ac7d/public/framework/datasources/direct_query_hook.tsx

Issue: #1503

Copy link
Member Author

Choose a reason for hiding this comment

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

added comments to the issue we are tracking

@ps48 ps48 mentioned this pull request Mar 21, 2024
Open
4 tasks
@ps48 ps48 merged commit b4fd35e into opensearch-project:main Mar 21, 2024
20 of 26 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 21, 2024
@github-actions
sanitize create acceleration queries and direct queries (#1605)
ed229d4 
Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
(cherry picked from commit b4fd35e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Mar 21, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 21, 2024
@github-actions
sanitize create acceleration queries and direct queries (#1605)
9f1ef2a 
Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
(cherry picked from commit b4fd35e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Mar 21, 2024
Merged
ps48 pushed a commit that referenced this pull request Mar 21, 2024
@opensearch-trigger-bot @github-actions
sanitize create acceleration queries and direct queries (#1605) (#1606)
776ce9f 
(cherry picked from commit b4fd35e)

Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ps48 pushed a commit that referenced this pull request Mar 21, 2024
@opensearch-trigger-bot @github-actions
sanitize create acceleration queries and direct queries (#1605) (#1607)
443794d 
(cherry picked from commit b4fd35e)

Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
amsiglan pushed a commit to amsiglan/dashboards-observability that referenced this pull request Jun 7, 2024
@opensearch-trigger-bot
sanitize create acceleration queries and direct queries (opensearch-p…
11037b4 
…roject#1605) (opensearch-project#1607)

(cherry picked from commit b4fd35e)

Signed-off-by: Shenoy Pratik <sgguruda@amazon.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 443794d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swiddis Swiddis Swiddis left review comments

@paulstn paulstn paulstn approved these changes

@YANG-DB YANG-DB YANG-DB approved these changes

@kavithacm kavithacm Awaiting requested review from kavithacm

@derek-ho derek-ho Awaiting requested review from derek-ho

@joshuali925 joshuali925 Awaiting requested review from joshuali925

@dai-chen dai-chen Awaiting requested review from dai-chen

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@mengweieric mengweieric Awaiting requested review from mengweieric

@vamsi-amazon vamsi-amazon Awaiting requested review from vamsi-amazon

@penghuo penghuo Awaiting requested review from penghuo

@seankao-az seankao-az Awaiting requested review from seankao-az

@anirudha anirudha Awaiting requested review from anirudha

@sumukhswamy sumukhswamy Awaiting requested review from sumukhswamy

Assignees
No one assigned
Labels
backport 2.x backport 2.13 bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ps48 @paulstn @Swiddis @YANG-DB