Convert unexpected parameter validation errors to InvalidParameterError

[rauchy]

What type of PR is this? (check all applicable)

• Refactor

Description

A quick follow up to #4072

[arikfr]

Looks fine, but I wonder if we won't swallow some edge cases that we should handle differently this way? Maybe worth keeping existing error handling and expend it over time as we discover new cases?

This way we can add logging for exceptions in the case of really unexpected ones (and properly handle them if needed).

Wdyt?

[guidopetri]

@rauchy , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

[codecov]

Codecov Report

Merging #4084 (490d9a6) into master (710dd8c) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4084      +/-   ##
==========================================
- Coverage   60.87%   60.86%   -0.02%     
==========================================
  Files         155      155              
  Lines       12718    12714       -4     
  Branches     1729     1730       +1     
==========================================
- Hits         7742     7738       -4     
  Misses       4747     4747              
  Partials      229      229              

Files Changed	Coverage Δ
redash/models/parameterized_query.py	97.56% <100.00%> (-0.08%)	⬇️

[justinclift]

This one kind of looks like it could have more test coverage added, to get the Codecov pieces to pass?

[guidopetri]

Yeah, it was passing before I rebased, heh

[guidopetri]

Alright, that's the best I'm getting this PR. 😅 there was a coverage change because all error types were being converted to InvalidParameterError, when in reality there was a specific kind of error (QueryDetachedFromDataSourceError) that needed to be raised separately. (@arikfr had a lot of foresight back when he made his original comment, heh)

Fixing that, there's no indirect coverage changes, but the coverage is going down anyway because we are removing lines that were fully covered - i.e. the percentage of fully covered lines goes down because the absolute number of fully covered lines is going down.

[justinclift]

Cool, good stuff @guidopetri. 😁

[guidopetri]

It took me like two hours to figure out the fix lol. Lesson learned: don't use bare exceptions