Copilot commented on code in PR #36792:
URL: https://github.com/apache/superset/pull/36792#discussion_r2662323293
##########
superset/commands/database/uploaders/base.py:
##########
@@ -159,6 +159,13 @@ def run(self) -> None:
if not self._model:
return
+ # Treat empty or frontend-sent "undefined" schema as no schema
+ if not self._schema or self._schema == "undefined":
+ logger.warning(
+ "CSV UPLOAD: schema was empty or undefined, using database
default"
+ )
+ self._schema = None
Review Comment:
This new behavior (handling empty or "undefined" schema values) lacks test
coverage. The existing tests in
tests/integration_tests/databases/commands/upload_test.py cover various upload
scenarios, but none explicitly test the case where schema is "undefined" or an
empty string. Consider adding a test case to verify that uploads succeed when
schema is "undefined" or empty.
##########
superset/commands/database/uploaders/base.py:
##########
@@ -159,6 +159,13 @@ def run(self) -> None:
if not self._model:
return
+ # Treat empty or frontend-sent "undefined" schema as no schema
+ if not self._schema or self._schema == "undefined":
+ logger.warning(
+ "CSV UPLOAD: schema was empty or undefined, using database
default"
+ )
+ self._schema = None
Review Comment:
The schema normalization happens after validation (line 158), but the
validate() method at line 196 checks schema_allows_file_upload with the
original "undefined" value. This could cause the validation to fail before the
schema is normalized to None. Consider moving this normalization logic before
the validate() call, or into the __init__ method.
##########
superset/commands/database/uploaders/base.py:
##########
@@ -159,6 +159,13 @@ def run(self) -> None:
if not self._model:
return
+ # Treat empty or frontend-sent "undefined" schema as no schema
+ if not self._schema or self._schema == "undefined":
+ logger.warning(
+ "CSV UPLOAD: schema was empty or undefined, using database
default"
+ )
+ self._schema = None
Review Comment:
The codebase already has a utility function parse_js_uri_path_item in
superset/utils/core.py that handles JavaScript "undefined" and "null" strings
with the eval_undefined parameter. Consider using this existing utility instead
of implementing custom logic. For example: self._schema =
parse_js_uri_path_item(self._schema, eval_undefined=True). This would be more
consistent with how other parts of the codebase handle JavaScript-sent
undefined values (see superset/databases/api.py line 1008).
##########
superset/commands/database/uploaders/base.py:
##########
@@ -159,6 +159,13 @@ def run(self) -> None:
if not self._model:
return
+ # Treat empty or frontend-sent "undefined" schema as no schema
+ if not self._schema or self._schema == "undefined":
+ logger.warning(
+ "CSV UPLOAD: schema was empty or undefined, using database
default"
Review Comment:
The log message refers specifically to "CSV UPLOAD" but this code path
handles Excel and Columnar file uploads as well. Consider using a more generic
message like "File upload: schema was empty or undefined, using database
default" to accurately reflect all supported file types.
```suggestion
"File upload: schema was empty or undefined, using database
default"
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
