refactor: upload data unification, less permissions and less endpoints

[dpgaspar]

SUMMARY

This pull request unifies csv, excel and columnar upload endpoints and permissions into a single endpoint and permission type.

This is a breaking change related with Apache Superset 5.0
Removes the following endpoints:

• POST /api/v1/database/csv_metadata

• POST /api/v1/database/excel_metadata

• POST /api/v1/database/columnar_metadata

• POST /api/v1/database/<id>/csv_upload

• POST /api/v1/database/<id>/excel_upload

• POST /api/v1/database/<id>/columnar_upload

And adds the following generic endpoints:

• POST /api/v1/database/upload_metadata
• POST /api/v1/database/<id>/upload

Removes the following permissions:
can csv upload on Database
can excel upload on Database
can columnar upload on Database

And adds the following permission: can upload on Database

Includes a db migration that should run fast, depending on the number of roles that include the can <file-type> upload on Database permission.
Upgrade and downgrade were tested, but be aware that we loose granularity when downgrading, since we are converging 3 permissions into 1, the downgrade will always expand the final permission into 3 permissions.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

[codecov]

Codecov Report

Attention: Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.32%. Comparing base (76d897e) to head (c2f6c16).
Report is 1371 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/databases/api.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #31959       +/-   ##
===========================================
+ Coverage   60.48%   83.32%   +22.83%     
===========================================
  Files        1931      544     -1387     
  Lines       76236    38916    -37320     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32425    -13689     
+ Misses      28017     6491    -21526     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.50% <52.77%> (-0.67%)	⬇️
javascript	?
mysql	75.85% <52.77%> (?)
postgres	75.91% <52.77%> (?)
presto	53.05% <52.77%> (-0.75%)	⬇️
python	83.32% <94.44%> (+19.83%)	⬆️
sqlite	75.43% <52.77%> (?)
unit	60.77% <94.44%> (+3.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[korbit-ai]

Korbit doesn't automatically review large (500+ lines changed) pull requests such as this one. If you want me to review anyway, use /korbit-review.

[mistercrunch]

Did a fairly thorough review, paying special attention to the db migration and LGTM!

[eschutho]

@dpgaspar I know that we haven't put up the SIP yet (still on my todo list) but I think we should consider api semver as a separate thing from application semver. In other words, we should only remove api endpoints when the entire v1 api is deprecated and removed.

[dpgaspar]

@dpgaspar I know that we haven't put up the SIP yet (still on my todo list) but I think we should consider api semver as a separate thing from application semver. In other words, we should only remove api endpoints when the entire v1 api is deprecated and removed.

Sounds very good for the future. We have been applying semver at the application level only, and taking the opportunity to replace APIs and change specs. This change was approved here: https://github.com/orgs/apache/projects/345?pane=issue&itemId=61955191&issue=apache%7Csuperset%7C28352.
Most probably when we start an API v2 we don't need to remove APIs, both can live side by side, the downside is that we won't be able to remove API permissions immediately, only when we remove v1 for example.
We should also use alpha and beta first for new versions to get the specs stabilised.

Alternatively I can add the endpoints back in (and permissions) and add the deprecated decorator for 6.0?

[eschutho]

@dpgaspar and I talked about the impact of this change, and it's not a commonly-used endpoint, so we'll go with the current flow which is that breaking api changes are permissible during an application breaking change release, and I'll plan on putting up a SIP soon so that we can discuss a new policy for having a separate semver for the api.