[GitHub] [incubator-superset] sloypog commented on a change in pull request #8556: [WIP] [SIP] Proposal for Improvement on CSV Upload

[GitBox <gi...@apache.org>]

sloypog commented on a change in pull request #8556: [WIP] [SIP] Proposal for Improvement on CSV Upload
URL: https://github.com/apache/incubator-superset/pull/8556#discussion_r348083602
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -401,39 +410,50 @@ def _allowed_file(filename: str) -> bool:
                 extension is not None and extension[1:] in config["ALLOWED_EXTENSIONS"]
             )
 
-        filename = secure_filename(form.csv_file.data.filename)
+        filename = secure_filename(csv_filename)
         if not _allowed_file(filename):
             raise Exception("Invalid file type selected")
         csv_to_df_kwargs = {
             "filepath_or_buffer": filename,
-            "sep": form.sep.data,
-            "header": form.header.data if form.header.data else 0,
-            "index_col": form.index_col.data,
-            "mangle_dupe_cols": form.mangle_dupe_cols.data,
-            "skipinitialspace": form.skipinitialspace.data,
-            "skiprows": form.skiprows.data,
-            "nrows": form.nrows.data,
-            "skip_blank_lines": form.skip_blank_lines.data,
-            "parse_dates": form.parse_dates.data,
-            "infer_datetime_format": form.infer_datetime_format.data,
+            "sep": form_data["delimiter"],
+            # frontend already does int-check, check again in case of tampering
+            "header": 0 if not form_data["headerRow"] else int(form_data["headerRow"]),
+            "index_col": None
+            if not form_data["indexColumn"]
 
 Review comment:
   Hello, thanks for the feedback.
   We can see where you are coming from and tested this out.
   In the current implementation we always send all the form fields to the backend. If they are empty, we send an empty field which is converted to an empty string.
   Therefore the logic 'form_data.get("indexColumn") or None' does not work as we will never fall into the 'or' case.
   Furthermore we would need additional logic for some arguments since we have to cast them back to their intended values.
   Due to these reasons we went with the current implementation.
   Your recommendation regarding using 'form_data["indexColumn"] or None' is more readable and cleaner than our previous implementation.
   We used this wherever possible but sadly were not able to use it for all values (those where we need int(value) or None).
   For those we used the 'None if not' construct. 
   If you have further feedback or happen to know a workaround to the problem mentioned we would love to hear from you again.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org