Refactor import csv #4298

timifasubaa · 2018-01-26T09:46:26Z

This PR addresses issues #4285 , #4287 and #4289 . It also refactors the import csv code to make it more robust to errors

@john-bodley @mistercrunch @xrmx

xrmx · 2018-01-26T09:54:36Z

Would be nice to have tests to avoid regressions :)

xrmx · 2018-01-26T09:55:27Z

superset/views/core.py

-            flash(e, 'error')
-            return redirect('/tablemodelview/list/')
+            os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_filename))
+            flash(str(e), 'error')


Don't use str but six.text_type

xrmx · 2018-01-26T09:56:56Z

superset/views/core.py

-            .filter_by(sqlalchemy_uri=form.data.get('con'))
-            .one()
-        )
+        db_name = table.database.database_name
        message = _('CSV file "{0}" uploaded to table "{1}" in '


while you are at it could you please make it a unicode string with u?

timifasubaa · 2018-01-26T10:06:17Z

@xrmx I already added tests in the earlier PR. I now updated the tests to work with the new form field type.

timifasubaa · 2018-01-29T23:27:05Z

PING

john-bodley · 2018-01-30T23:43:05Z

superset/forms.py


 config = app.config


 class CsvToDatabaseForm(DynamicForm):
+    # pylint: disable=E0211
+    def all_db_items():
+        from superset.models import core as models


Why are these imports not global?

I wrongly assumed it would cause cyclical imports. I moved it to be global

john-bodley · 2018-01-30T23:43:23Z

superset/utils.py

+def upload_file(file):
+    """Takes in the name of the file to be uploaded and uploads that file"""
+
+    from superset import app


Why are these imports not global?

Because there will be cyclical imports if they are global.

Because it will cause cyclical import.

It would be cleaner to pass the upload_folder from the caller then

True. After removing the extra lines, I decided it was best to just call file.save directly as opposed to calling a function with just . one line.

john-bodley · 2018-01-30T23:46:46Z

superset/utils.py

+
+    from superset import app
+    config = app.config
+    if not file or not file.filename:


What happens when this Exception gets raised? Note in the previous version this check doesn't exist.

It gets caught in the try block in views/core.py and the error message gets shown at the top of the form

Can this happen in practice? You have one caller of this function so you can be pretty sure that a proper file is passed down.

True. I'll take it out.

john-bodley · 2018-01-30T23:47:41Z

superset/utils.py

+    if not file or not file.filename:
+        raise Exception
+    file.save(os.path.join(config['UPLOAD_FOLDER'], file.filename))
+    assert file.filename in os.listdir(config['UPLOAD_FOLDER'])


Similarly what happens if an AssertionError is raised?

I removed the assertion in favor of exceptions which are handled by surfacing the exception message at the top of the form.

xrmx · 2018-01-31T09:00:39Z

superset/views/core.py

-            os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_file.filename))
-            flash(e, 'error')
-            return redirect('/tablemodelview/list/')
+            if csv_filename in os.listdir(config['UPLOAD_FOLDER']):


try:
os.remove(os.path.join(config['UPLOAD_FOLDER'], csv_filename))
except OSError:
pass

xrmx · 2018-01-31T09:02:09Z

superset/views/core.py

+            table.database = form.data.get('con')
+            table.database_id = table.database.id
+            table.database.db_engine_spec.create_table_from_csv(form, table)
+        except IntegrityError:


You can probably unify these and the generic exception and just change the flash message if the exception is an instance of IntegrityError

Good idea! Done.

xrmx · 2018-01-31T09:03:07Z

tests/core_tests.py

@@ -804,15 +804,15 @@ def test_import_csv(self):
        test_file.write('paul,2\n')
        test_file.close()
        main_db_uri = db.session.query(
-            models.Database.sqlalchemy_uri)\
+            models.Database)\


If you move the closing parens on the line below you can avoid the backslash

xrmx · 2018-01-31T09:04:59Z

superset/utils.py

+    if not file or not file.filename:
+        raise Exception("No file chosen for upload")
+    file.save(os.path.join(config['UPLOAD_FOLDER'], file.filename))
+    if file.filename not in os.listdir(config['UPLOAD_FOLDER']):


Can this happen in practice? i assume file.save will return or raise something in case of error

True. Taken it out and left it to the exception that will be raised if something goes wrong.

* move helpers to utils * make form use queryselector * refactor exception throwing and handling * update db_connection access point * nits (cherry picked from commit 6d37d97)

* move helpers to utils * make form use queryselector * refactor exception throwing and handling * update db_connection access point * nits

timifasubaa added 4 commits January 26, 2018 01:26

move helpers to utils

6d6533d

make form use queryselector

6da8563

refactor exception throwing and handling

40fa488

update db_connection access point

509b9a2

timifasubaa force-pushed the refactor_imoprt_csv branch from bb1615c to 4e2f47c Compare January 26, 2018 09:52

xrmx reviewed Jan 26, 2018

View reviewed changes

timifasubaa force-pushed the refactor_imoprt_csv branch from 4e2f47c to 13f9b94 Compare January 26, 2018 09:59

Merge branch 'master' into refactor_imoprt_csv

6c43b4c

timifasubaa force-pushed the refactor_imoprt_csv branch from 13f9b94 to 6c43b4c Compare January 26, 2018 10:04

timifasubaa force-pushed the refactor_imoprt_csv branch from c52ca34 to 78b94d5 Compare January 26, 2018 10:14

timifasubaa changed the title ~~Refactor imoprt csv~~ Refactor import csv Jan 26, 2018

timifasubaa force-pushed the refactor_imoprt_csv branch 2 times, most recently from 88013d8 to f7373f4 Compare January 26, 2018 17:38

john-bodley reviewed Jan 30, 2018

View reviewed changes

timifasubaa force-pushed the refactor_imoprt_csv branch from f7373f4 to 36f01a1 Compare January 31, 2018 07:49

xrmx reviewed Jan 31, 2018

View reviewed changes

timifasubaa force-pushed the refactor_imoprt_csv branch 7 times, most recently from c9b9c8c to 8e78608 Compare January 31, 2018 10:55

timifasubaa force-pushed the refactor_imoprt_csv branch from 8e78608 to dcaa5a9 Compare January 31, 2018 11:01

xrmx mentioned this pull request Jan 31, 2018

Upload a CSV, not working with MySQL #4287

Closed

3 tasks

nits

5f90554

timifasubaa force-pushed the refactor_imoprt_csv branch from dcaa5a9 to 5f90554 Compare February 2, 2018 19:41

mistercrunch merged commit 6d37d97 into apache:master Feb 4, 2018

graceguo-supercat mentioned this pull request Feb 5, 2018

Cherrypick a few fixes airbnb/superset-fork#7

Merged

timifasubaa mentioned this pull request Feb 5, 2018

refactor_csv hotfix #4345

Merged

This was referenced Mar 6, 2018

Import CSV File Into PosgreSQL Table. Error: password authentication failed #4285

Closed

[bug] Fix CSV upload feature for DB with password #4562

Merged

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018

Refactor import csv (apache#4298)

fedb4d7

* move helpers to utils * make form use queryselector * refactor exception throwing and handling * update db_connection access point * nits

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Refactor import csv (apache#4298)

43ab72f

* move helpers to utils * make form use queryselector * refactor exception throwing and handling * update db_connection access point * nits

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor import csv #4298

Refactor import csv #4298

timifasubaa commented Jan 26, 2018

xrmx commented Jan 26, 2018

xrmx Jan 26, 2018

timifasubaa Jan 27, 2018

xrmx Jan 26, 2018

timifasubaa Jan 27, 2018

timifasubaa commented Jan 26, 2018 •

edited

timifasubaa commented Jan 29, 2018

john-bodley Jan 30, 2018

timifasubaa Jan 31, 2018

john-bodley Jan 30, 2018

timifasubaa Jan 31, 2018

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018

timifasubaa Jan 31, 2018

john-bodley Jan 30, 2018

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018

timifasubaa Jan 31, 2018

john-bodley Jan 30, 2018

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018 •

edited

timifasubaa Jan 31, 2018

xrmx Jan 31, 2018 •

edited

timifasubaa Jan 31, 2018

Refactor import csv #4298

Refactor import csv #4298

Conversation

timifasubaa commented Jan 26, 2018

xrmx commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Jan 26, 2018 • edited

timifasubaa commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Jan 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timifasubaa commented Jan 26, 2018 •

edited

xrmx Jan 31, 2018 •

edited

xrmx Jan 31, 2018 •

edited