Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: API to allow importing old exports (JSON/YAML) #13444

Merged
merged 4 commits into from
Mar 5, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Mar 3, 2021

SUMMARY

The new APIs for importing dashboards and datasets only accept ZIP files, even though the logic to import the old format (JSON and YAML, respectively) is in the backend. This PR makes the API accept the old format.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Added tests, and also tested manually.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@betodealmeida betodealmeida changed the title fix: fix API to allow importing old exports (JSON/YAML) fix API to allow importing old exports (JSON/YAML) Mar 3, 2021
@betodealmeida betodealmeida changed the title fix API to allow importing old exports (JSON/YAML) fix: API to allow importing old exports (JSON/YAML) Mar 3, 2021
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #13444 (9a7c3dc) into master (66a7318) will decrease coverage by 5.96%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13444      +/-   ##
==========================================
- Coverage   77.45%   71.49%   -5.97%     
==========================================
  Files         903      804      -99     
  Lines       45640    40616    -5024     
  Branches     5500     4157    -1343     
==========================================
- Hits        35349    29037    -6312     
- Misses      10165    11579    +1414     
+ Partials      126        0     -126     
Flag Coverage Δ
cypress 57.94% <ø> (-0.07%) ⬇️
hive ?
javascript ?
mysql 80.33% <100.00%> (+0.01%) ⬆️
postgres 80.36% <100.00%> (+0.02%) ⬆️
presto ?
python 80.45% <100.00%> (-0.39%) ⬇️
sqlite 79.99% <100.00%> (?)

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

Impacted Files Coverage Δ
superset/charts/api.py 81.30% <ø> (ø)
superset/databases/api.py 91.93% <ø> (ø)
superset/dashboards/api.py 87.55% <100.00%> (+0.15%) ⬆️
superset/dashboards/commands/importers/v0.py 92.85% <100.00%> (ø)
superset/datasets/api.py 91.44% <100.00%> (+0.11%) ⬆️
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/components/Pagination/Ellipsis.tsx 0.00% <0.00%> (-100.00%) ⬇️
...-frontend/src/components/OmniContainer/Omnibar.tsx 0.00% <0.00%> (-100.00%) ⬇️
...frontend/src/components/ErrorMessage/IssueCode.tsx 0.00% <0.00%> (-100.00%) ⬇️
... and 502 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66a7318...9a7c3dc. Read the comment docs.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks good, left a comment

def __init__(self, contents: Dict[str, str], database_id: Optional[int] = None):
# pylint: disable=unused-argument
def __init__(
self, contents: Dict[str, str], database_id: Optional[int] = None, **kwargs: Any
Copy link
Member

Choose a reason for hiding this comment

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

is contents a flat Dict? also why the kwargs here, they receive passwords and overwrite but nothing is done with them, I'm probably missing something.

It would be nice also to add a description for the fields on the API spec:

                schema:
                  type: object
                  properties:
                    formData:
                      type: string
                      format: binary
                    passwords:
                      type: string
                    overwrite:
                      type: bool

Copy link
Member Author

Choose a reason for hiding this comment

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

contents is a flat dict, with filename => file_contents.

*kwargs is needed because different version (v0 vs v1) take different arguments, so they just discard what they don't need.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add the description to the API, I thought I had added it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I mean a description for the passwords, overwrite, formData fields

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, a description! Will do! :)

@betodealmeida betodealmeida merged commit 9fc03f0 into apache:master Mar 5, 2021
@mistercrunch mistercrunch added the rush! Requires immediate attention label Mar 5, 2021
@junlincc junlincc removed the rush! Requires immediate attention label Mar 18, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* fix: fix API to allow importing old exports (JSON/YAML)

* Fix test

* Fix lint

* Add description to API schema
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants