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

Fix: Query results in public dashboard not loading when query ends with semicolon #6351

Merged
merged 2 commits into from
Aug 6, 2023

Conversation

manikBS
Copy link
Contributor

@manikBS manikBS commented Aug 6, 2023

What type of PR is this?

This PR fixes the formatting flake8 error resulting in the build failure and also includes the cherry-picked commit of the below PR:
#5809

Dashboards accessed using the public url use the cached query result mechanism to fetch results after a query job is finished.
When using parameterized queries, the frontend requests the data usually 2 times: one time after changing the parameter, and one time after the job is finished. The second request relies on the caching mechanism to find the cached result of the first request. The second request does not request auto_limit, which causes a lack of splitting and merging of sql statements. Because of that, the two sql queries differ in whitespacing and a trailing semicolon, which causes the query_hashes of both requests to differ.

That results in another job invocation and an empty query result in the second response in the frontend, which causes an endless loading spinner in the frontend and no fetched data.

By splitting the statements each time, you prevent these differences in the 2 sql queries.

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Splitting and merging statements should/can not break anything, otherwise the auto limiting would be buggy. Ran pre-commit to make sure no flake8 errors

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #6351 (e75f875) into master (113146e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6351      +/-   ##
==========================================
- Coverage   60.73%   60.73%   -0.01%     
==========================================
  Files         153      153              
  Lines       12514    12513       -1     
  Branches     1695     1695              
==========================================
- Hits         7601     7600       -1     
  Misses       4687     4687              
  Partials      226      226              
Files Changed Coverage Δ
redash/query_runner/__init__.py 75.47% <100.00%> (-0.08%) ⬇️

Copy link
Member

@justinclift justinclift left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks heaps @manikBS. 😄

@justinclift
Copy link
Member

@manikBS Just sent you a GitHub invite to the project as well, so you have access to make changes in the project that seem useful. 😄

@justinclift justinclift merged commit f4a930d into getredash:master Aug 6, 2023
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.

3 participants