[database] Fix, tables endpoint #9144
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9144 +/- ##
=======================================
Coverage 59.06% 59.06%
=======================================
Files 372 372
Lines 11922 11922
Branches 2919 2919
=======================================
Hits 7042 7042
Misses 4698 4698
Partials 182 182
Continue to review full report at Codecov.
|
dpgaspar
requested review from
villebro,
john-bodley and
etr2460
and removed request for
villebro and
john-bodley
superset/views/database/api.py
Outdated
| @protect() | ||
| @safe | ||
| @event_logger.log_this | ||
| def tables( # pylint: disable=invalid-name,too-many-locals |
There was a problem hiding this comment.
I'm starting to wonder if we want to do this migration now as part of the security fix PR? This looks like we've pulled the method definition over almost as-is, but as pylint is showing us, this really could use a refactor. Would it be better to have one PR that adds the security fix, then a second that refactors the method into the new API?
Another approach here could be to "strangle" the current implementation by proxying to the current implementation, then refactoring it over time.
There was a problem hiding this comment.
Agreed I'll break this into 2 PR's
dpgaspar
changed the title
dpgaspar
changed the title
There was a problem hiding this comment.
I agree with @willbarrett , much better to have the original PR split up as separate PRs.
| database = query.filter_by(id=db_id).one_or_none() | ||
| if not database: | ||
| return json_error_response("Not found", 404) |
There was a problem hiding this comment.
Did we settle for try-except with .one() or .one_or_none() with if not? I'm fine either way, just prefer to have it as uniform as possible in the codebase.
There was a problem hiding this comment.
There is a majority of one_or_none() on core.py
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
Adds database filter to
/superset/tables. Took the chance to add typing, improved parameter validation and testsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@willbarrett