Fix issue with scheduled queries #7111

ezraodio1 · 2024-08-06T13:55:01Z

What type of PR is this?

Description

There's currently an issue in Redash where some queries fail to refresh on their schedule. This happens when the query has no latest_query_data, which makes retrieved_at = None, which makes it so the query never gets added to outdated_queries. This PR fixes that.

The problem can be reproduced by running the unit test that's part of this PR (without making the fix first, of course)

def test_enqueues_scheduled_query_without_latest_query_data(self):
        """
        Queries with a schedule but no latest_query_data will still be reported by Query.outdated_queries()
        """
        query = self.factory.create_query(
            schedule=self.schedule(interval="60"),
            data_source=self.factory.create_data_source(),
        )

        outdated_queries = models.Query.outdated_queries()
        self.assertEqual(query.latest_query_data, None)
        self.assertEqual(len(outdated_queries), 1)
        self.assertIn(query, outdated_queries)

How is this tested?

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

Related Tickets & Documents

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

eradman

Tested this manually, seems to check out

justinclift · 2024-09-20T16:30:23Z

@arikfr Any idea if this will interact with the scheduled queries hash issue you were looking at yesterday?

redash/models/__init__.py

arikfr · 2024-09-21T04:20:17Z

redash/models/__init__.py

+                if all(value is None for value in query.schedule.values()):
+                    continue


What's this for?

It's been a little while since I looked at this, but I think this is the answer:

The only way to check whether a query doesn't have a refresh schedule is to ensure that everything in query.schedule.values() is None. For example, interval, until, time are all keys in the query.schedule. It's possible to have a query refresh schedule when just one of these keys is not None.

If a query doesn't have a refresh schedule, then we can just continue to the next query in the loop.

When a query does not have a schedule then the schedule value is null (or empty). It's possible there is some edge case scenario that might keep schedule object with empty values, but I would assume it's not common?

To keep the code maintainable I would rather avoid this change in this PR and if you still feel it's necessary let's bring it back in a separate one and have the discussion there (and potentially add tests for this case).

I think this is worth double checking. I could be mistaken, but I remember this edge case popping up during my testing. Perhaps if a query had a schedule and then that schedule was later removed?

Sounds like we'll need to create a follow up PR to this one if/when we merge this. 😄

I found out that we have an explicit test case for this (schedule object with all values being None). I don't know if this is to cover some case that won't happen anymore, but just to be on the safe side I kept this logic + added a comment to explain.

arikfr

One last review. Please let us know if you have the bandwidth to apply the changes. Thanks!

arikfr · 2024-10-07T09:30:55Z

redash/models/__init__.py

+                if all(value is None for value in query.schedule.values()):
+                    continue


When a query does not have a schedule then the schedule value is null (or empty). It's possible there is some edge case scenario that might keep schedule object with empty values, but I would assume it's not common?

To keep the code maintainable I would rather avoid this change in this PR and if you still feel it's necessary let's bring it back in a separate one and have the discussion there (and potentially add tests for this case).

redash/models/__init__.py

ezraodio1 · 2024-10-09T00:26:15Z

I lost access to the VM that I was using over the summer for Redash dev work unfortunately, so I think it's easiest if someone else applies the changes.

justinclift · 2024-10-09T01:03:06Z

@ezraodio1 If you need a new VM for doing Redash dev work on, then I'm happy to spin one up for you. Would that be useful? 😄

ezraodio1 · 2024-10-15T20:44:45Z

I'm taking a tough course load right, so I'm pretty busy. I'd definitely be interested after this semester ends, though. I'll try to spin up a new VM for myself.

justinclift · 2024-10-15T21:21:56Z

No worries. Keep it in mind as an option if/when it'd be useful.

Good luck with your course load, hopefully it all goes really well. 😄

justinclift · 2024-10-24T23:09:15Z

@arikfr Should we merge this PR as-is, or do you want to modify it a bit first? Asking because @ezraodio1 doesn't have the bandwidth to make changes at present.

justinclift · 2024-10-27T22:29:57Z

@arikfr Should this be merged now? 😄

codecov · 2024-10-29T10:13:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.14%. Comparing base (a2611b8) to head (5dd129c).
Report is 34 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7111      +/-   ##
==========================================
- Coverage   64.15%   64.14%   -0.01%     
==========================================
  Files         162      162              
  Lines       13295    13341      +46     
  Branches     1883     1893      +10     
==========================================
+ Hits         8529     8558      +29     
- Misses       4432     4450      +18     
+ Partials      334      333       -1

Files with missing lines	Coverage Δ
redash/models/__init__.py	`91.93% <100.00%> (+0.08%)`	⬆️

... and 3 files with indirect coverage changes

justinclift

Eric already approved the previous revision of this. 😄

eradman previously approved these changes Sep 20, 2024

View reviewed changes

Add scheduled queries with no latest_query_data to outdated_queries

f1b6be0

eradman force-pushed the fix-query-schedule branch from 0d863b8 to f1b6be0 Compare September 20, 2024 14:24

arikfr reviewed Sep 21, 2024

View reviewed changes

arikfr mentioned this pull request Oct 7, 2024

Fixes #6767: correctly rehash queries in a migration #7184

Merged

2 tasks

arikfr reviewed Oct 7, 2024

View reviewed changes

Apply code review comments

49fa2b1

arikfr dismissed eradman’s stale review via 49fa2b1 October 27, 2024 13:11

Merge branch 'master' into fix-query-schedule

271de55

Merge branch 'master' into fix-query-schedule

5dd129c

justinclift approved these changes Oct 29, 2024

View reviewed changes

justinclift enabled auto-merge (squash) October 29, 2024 10:17

justinclift merged commit 673ba76 into getredash:master Oct 29, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with scheduled queries #7111

Fix issue with scheduled queries #7111

ezraodio1 commented Aug 6, 2024 •

edited

Loading

eradman left a comment

justinclift commented Sep 20, 2024

arikfr Sep 21, 2024

ezraodio1 Sep 21, 2024

arikfr Oct 7, 2024

ezraodio1 Oct 9, 2024

justinclift Oct 18, 2024

arikfr Oct 27, 2024

arikfr left a comment

arikfr Oct 7, 2024

ezraodio1 commented Oct 9, 2024

justinclift commented Oct 9, 2024 •

edited

Loading

ezraodio1 commented Oct 15, 2024

justinclift commented Oct 15, 2024

justinclift commented Oct 24, 2024

justinclift commented Oct 27, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

justinclift left a comment

		if all(value is None for value in query.schedule.values()):
		continue

Fix issue with scheduled queries #7111

Fix issue with scheduled queries #7111

Conversation

ezraodio1 commented Aug 6, 2024 • edited Loading

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

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

eradman left a comment

Choose a reason for hiding this comment

justinclift commented Sep 20, 2024

arikfr Sep 21, 2024

Choose a reason for hiding this comment

ezraodio1 Sep 21, 2024

Choose a reason for hiding this comment

arikfr Oct 7, 2024

Choose a reason for hiding this comment

ezraodio1 Oct 9, 2024

Choose a reason for hiding this comment

justinclift Oct 18, 2024

Choose a reason for hiding this comment

arikfr Oct 27, 2024

Choose a reason for hiding this comment

arikfr left a comment

Choose a reason for hiding this comment

arikfr Oct 7, 2024

Choose a reason for hiding this comment

ezraodio1 commented Oct 9, 2024

justinclift commented Oct 9, 2024 • edited Loading

ezraodio1 commented Oct 15, 2024

justinclift commented Oct 15, 2024

justinclift commented Oct 24, 2024

justinclift commented Oct 27, 2024

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

justinclift left a comment

Choose a reason for hiding this comment

ezraodio1 commented Aug 6, 2024 •

edited

Loading

justinclift commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading