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: CSV upload with overprescribed table/schema #18758

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Feb 16, 2022

SUMMARY

This PR fixes a couple of issues related to CSV upload:

  1. If the schema was overprescribed (both explicitly and in the table name) the flashed message would fail (resulting in a 500 error) due to invalid field names passed to the message generator. This code path was not covered by unit tests.
  2. The schema was not correctly extracted when specified in the table name.

I debated how best to tackle this and initially went down the route of redefining the server-side form post logic (with unit tests), but opted for hopefully an easier/cleaner route by adding form validators.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen Shot 2022-02-18 at 6 42 17 PM

TESTING INSTRUCTIONS

Tested locally.

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

@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch from d323067 to 44154f5 Compare February 16, 2022 21:32
@john-bodley john-bodley marked this pull request as ready for review February 16, 2022 21:33
@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch 5 times, most recently from fa8907e to 08e94bd Compare February 17, 2022 00:22
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #18758 (644bca6) into master (cf8b57e) will decrease coverage by 0.15%.
The diff coverage is 70.96%.

❗ Current head 644bca6 differs from pull request most recent head a424b6d. Consider uploading reports for the commit a424b6d to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18758      +/-   ##
==========================================
- Coverage   66.31%   66.15%   -0.16%     
==========================================
  Files        1620     1620              
  Lines       63080    63066      -14     
  Branches     6370     6370              
==========================================
- Hits        41832    41722     -110     
- Misses      19591    19687      +96     
  Partials     1657     1657              
Flag Coverage Δ
hive ?
mysql 81.45% <100.00%> (+0.02%) ⬆️
postgres 81.49% <100.00%> (+0.02%) ⬆️
presto ?
python 81.58% <100.00%> (-0.32%) ⬇️
sqlite 81.18% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx 50.73% <ø> (ø)
superset-frontend/src/views/App.tsx 0.00% <0.00%> (ø)
superset/config.py 91.92% <ø> (ø)
superset/initialization/__init__.py 90.65% <ø> (+0.45%) ⬆️
superset/views/database/views.py 91.89% <ø> (+3.43%) ⬆️
...board/components/nativeFilters/FilterBar/index.tsx 69.82% <50.00%> (ø)
...perset-frontend/src/views/components/MenuRight.tsx 80.48% <60.00%> (ø)
superset-frontend/src/views/components/Menu.tsx 55.26% <77.77%> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <100.00%> (ø)
...rset-frontend/src/components/DeleteModal/index.tsx 84.21% <100.00%> (ø)
... and 16 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 cf8b57e...a424b6d. Read the comment docs.

message = _(
"You cannot specify a namespace both in the name of the table: "
'"%(csv_table.table)s" and in the schema field: '
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the initial error. The csv_table.table and csv_table.schema variables are not inputs to the function.

@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch 7 times, most recently from ca21d93 to c1c0fb6 Compare February 17, 2022 01:52
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 17, 2022
@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch from c1c0fb6 to fef15bc Compare February 17, 2022 02:20
@@ -130,24 +130,29 @@ def form_get(self, form: CsvToDatabaseForm) -> None:

def form_post(self, form: CsvToDatabaseForm) -> Response:
database = form.con.data
csv_table = Table(table=form.name.data, schema=form.schema.data)

if "." in form.name.data:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a detail, but I think it is worth moving the format checking to a separate method / function, so that the name of the method says what you really want to check and hide the implementations here to improve readability. It currently says you want to check if it contains a period, but in fact you are checking to see if it contains a namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ad-m I've restructured the PR using an alternative formulation.

@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch 2 times, most recently from cb23f8c to 351500d Compare February 18, 2022 05:45
@pull-request-size pull-request-size bot added size/M and removed size/L labels Feb 18, 2022
@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch 4 times, most recently from 85da944 to 41264a5 Compare February 18, 2022 06:00
@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch from 41264a5 to 7f2df47 Compare February 18, 2022 06:01
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

Can we add a test to make sure this bit ends up being covered?

@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch from 7f2df47 to 7ad0d42 Compare February 24, 2022 00:50
@john-bodley
Copy link
Member Author

john-bodley commented Feb 24, 2022

@craig-rueda I initially (and wrongfully) thought the WTForms validation logic was all frontend and would be difficult to capture in a unit test but it's all server side and thus a backend unit test is viable. I've added the test as requested.

@john-bodley john-bodley force-pushed the john-bodley--fix-csv-upload-issue branch from 7ad0d42 to a424b6d Compare February 24, 2022 02:52
@john-bodley john-bodley merged commit 0994217 into apache:master Feb 25, 2022
@john-bodley john-bodley deleted the john-bodley--fix-csv-upload-issue branch February 25, 2022 18:12
villebro pushed a commit that referenced this pull request Apr 3, 2022
Co-authored-by: wiktor2200 <wiktor2200@users.noreply.github.com>
(cherry picked from commit 0994217)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants