Skip to content

Query schedule may not always contain an "until" key#6771

Merged
justinclift merged 2 commits intogetredash:masterfrom
robinedwards:fix/key-error-empty-schedules
Mar 14, 2024
Merged

Query schedule may not always contain an "until" key#6771
justinclift merged 2 commits intogetredash:masterfrom
robinedwards:fix/key-error-empty-schedules

Conversation

@robinedwards
Copy link
Copy Markdown
Contributor

What type of PR is this?

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

Description

Noticed this error occurring from redash in our Sentry:

image

How is this tested?

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

Sentry error stopped occurring

Related Tickets & Documents

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

@robinedwards robinedwards force-pushed the fix/key-error-empty-schedules branch from 35d31b7 to aa0d3db Compare February 21, 2024 08:25
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.43%. Comparing base (d554136) to head (46aacf8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6771   +/-   ##
=======================================
  Coverage   63.43%   63.43%           
=======================================
  Files         163      163           
  Lines       13200    13200           
  Branches     1822     1822           
=======================================
  Hits         8373     8373           
  Misses       4530     4530           
  Partials      297      297           
Files Coverage Δ
redash/models/__init__.py 91.95% <ø> (ø)

@eradman
Copy link
Copy Markdown
Collaborator

eradman commented Feb 26, 2024

I can't figure out how schedule->until can end up missing, unless the queries table was modified manually. Are there any other keys missing? Try running this:

SELECT id,array_agg(keys) AS key
FROM queries, jsonb_object_keys(schedule::jsonb) keys
GROUP BY id ORDER BY id;

@robinedwards
Copy link
Copy Markdown
Contributor Author

robinedwards commented Feb 28, 2024

I've not modified it manually, there was a new migration added:

https://github.com/getredash/redash/blob/master/migrations/versions/640888ce445d_.py

Which touches the column.

Running your query:

redash=> SELECT id,array_agg(keys) AS key
FROM queries, jsonb_object_keys(schedule::jsonb) keys
GROUP BY id ORDER BY id;
ERROR:  cannot call jsonb_object_keys on a scalar

Looking a bit closer I have several rows where the schedule is null:

redash=> select count(*) from queries where schedule is null;
 count 
-------
   255
(1 row)

Then picking out a few examples where until is null:

redash=> select schedule from queries where schedule is not null limit 10;
                                   schedule                                    
-------------------------------------------------------------------------------
 {"time": "08:00", "until": null, "interval": 604800, "day_of_week": "Monday"}
 {"time": "23:15", "until": null, "interval": 86400, "day_of_week": null}
 {"time": "08:00", "until": null, "interval": 604800, "day_of_week": "Monday"}
 {"time": "00:15", "until": null, "interval": 86400, "day_of_week": null}
 null
 {"time": "09:00", "until": null, "interval": 604800, "day_of_week": "Monday"}
 {"time": "01:15", "until": null, "interval": 604800, "day_of_week": "Sunday"}
 {"time": null, "until": null, "interval": 3600, "day_of_week": null}
 {"time": null, "until": null, "interval": 3600, "day_of_week": null}
 {"disabled": true}
(10 rows)```

@robinedwards
Copy link
Copy Markdown
Contributor Author

@justinclift would it be possible to get a review

@justinclift
Copy link
Copy Markdown
Member

@guidopetri Do you have time to look at this? It seems super simple. 😄

Copy link
Copy Markdown
Collaborator

@eradman eradman left a comment

Choose a reason for hiding this comment

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

This record is wrong

 {"disabled": true}

But I can't figure out how this value was set. Seems like it must have been a failed migration. Approving since we're not making progress. After this query is updated I expect the schedule to be formatted with standard fields.

@justinclift justinclift merged commit 7d0d242 into getredash:master Mar 14, 2024
@justinclift
Copy link
Copy Markdown
Member

Approving since we're not making progress.

Thanks. Just merged it. 😄

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
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