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

Attempt at fixing scheduled parameterized queries #6063

Closed
wants to merge 5 commits into from

Conversation

remil1000
Copy link

@remil1000 remil1000 commented May 25, 2023

What type of PR is this?

  • Bug Fix

Description

  • generating query_hash with query expanded with default parameters values to match query_result with queries hash
  • added default parameters related properties in Query model class

How is this tested?

  • Manually

Tested queries with parameters and without, through dashboard and not and with changing parameter for ad-hoc query run/refresh

Extra info

The query hash is generated by removing any SQL comment and striping spaces then getting the md5 of the query text. This PR does the same only replacing parameters in braces {{ }} by their default value

Example below with a simple remote JSON API and parameter being {{ n }}, the participants number - the other is a static/no param JSON API example

>>> import hashlib
>>> hashlib.md5("url:http://www.boredapi.com/api/activity?participants=1".encode("utf-8")).hexdigest()
'93f7d98a4395f7cdbe1cea43947b6ff0'
>>> hashlib.md5("url:http://www.boredapi.com/api/activity?participants=2".encode("utf-8")).hexdigest()
'91716cdd4ca05899dbcf92a44828c536'

postgres=# select query, query_hash from queries; -- n = 1
                             query                              |            query_hash
----------------------------------------------------------------+----------------------------------
 url: https://api.chucknorris.io/jokes/random                   | 0616dfb448c39dd5608419df498da332
 url: http://www.boredapi.com/api/activity?participants={{ n }} | 93f7d98a4395f7cdbe1cea43947b6ff0
(2 rows)

postgres=# select query, query_hash from queries; -- n = 2
                             query                              |            query_hash
----------------------------------------------------------------+----------------------------------
 url: https://api.chucknorris.io/jokes/random                   | 0616dfb448c39dd5608419df498da332
 url: http://www.boredapi.com/api/activity?participants={{ n }} | 91716cdd4ca05899dbcf92a44828c536
(2 rows)

Related Tickets & Documents

#4182

@Avey777

This comment was marked as outdated.

@guidopetri
Copy link
Collaborator

@myonlylonely would you be willing to test this PR and see if the changes fix the original issue (from 2019! 😬 )?

@remil1000 , would you mind merging master into this branch/resolving conflicts + fixing any formatting issues with make format?

@myonlylonely
Copy link
Contributor

@guidopetri
I tried the PR, it still does not work. I create 2 queries, one without parameters and one with parameter. Only the one without parameters works. Maybe I could test it again after this PR resolved conflicts with master.
image

@remil1000
Copy link
Author

Greetings,

I tried to first rebase but finally updated the whole MR content, description and added some explanation

Tested locally with a regular query and a dashboard - the Last Executed At field is updated in the Queries list and last execution time in dashboard including this query is also updated as is the response content

Please let me know if you can't reproduce

@myonlylonely
Copy link
Contributor

myonlylonely commented Oct 30, 2023

I tested the latest PR, queries with or without parameters both works! Great job! 👍

@guidopetri
Copy link
Collaborator

@myonlylonely thanks for testing!

@remil1000 unfortunately it looks like quite a few of our backend unit tests are failing - would you mind taking a look?

@guidopetri
Copy link
Collaborator

@eradman 😅

@eradman
Copy link
Collaborator

eradman commented Nov 3, 2023

I'll aim to test this change out early next week

Rémi Laurent added 3 commits November 6, 2023 13:56
- generating query_hash with query expanded with default parameters
  values to match query_result with queries hash
- added default parameters related properties in Query model class
@eradman
Copy link
Collaborator

eradman commented Nov 7, 2023

@remil1000 I rebased this change and the backend unit tests are failing. Can you investiate?

@eradman
Copy link
Collaborator

eradman commented Dec 28, 2023

Tested this change; it does seem to work! Queries with parameters are refreshed

query-refresh

Logs

[2023-12-28 22:27:09,221][PID:114][INFO][rq.job.redash.tasks.queries.maintenance] job.func_name=redash.tasks.queries.maintenance.refresh_queries job.id=305c0ca
e0a196ae96915fd2b6f81001c435aad65 Done refreshing queries: {'started_at': 1703802429.0425797, 'outdated_queries_count': 2, 'last_refresh_at': 1703802429.220878
8, 'query_ids': '[1, 2]'}

I'll take a closer look at the test failures next week

@@ -810,6 +810,18 @@ def lowercase_name(cls):
def parameters(self):
return self.options.get("parameters", [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests require that we handle the case where options is NULL

@eradman
Copy link
Collaborator

eradman commented Jan 10, 2024

This issue was resolved in #6683

Closing, but we would be interested to know how this works for others.

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

Successfully merging this pull request may close these issues.

None yet

5 participants