Herestothegamers commented on code in PR #42603:
URL: https://github.com/apache/superset/pull/42603#discussion_r3725193074
##########
superset/views/database/validators.py:
##########
@@ -52,5 +52,10 @@ def schema_allows_file_upload(database: Database, schema:
Optional[str]) -> bool
if not database.allow_file_upload:
return False
if schemas := database.get_schema_access_for_file_upload():
- return schema in schemas
+ # Databases may report schema names in uppercase while the allow-list
+ # is inputted manually, so compare case-insensitively — mirroring the
+ # ``upload_allowed`` filtering of the database schemas endpoint.
+ return schema is not None and schema.lower() in {
+ allowed.lower() for allowed in schemas
+ }
Review Comment:
Addressed in 4d09913. User-supplied schemas are back to exact allow-list
matching, so an allow-list entry `public` can no longer authorize the quoted
case-variant `"PUBLIC"` on engines with case-sensitive identifiers.
Case-insensitive matching now applies only to the engine-resolved default
schema (`engine_resolved=True`), where the check is safe by construction: the
write uses the engine-reported name itself, so the case-fold cannot steer the
upload to a different physical schema. For context, the `upload_allowed`
filtering in `superset/databases/api.py` already compares the allow-list
case-insensitively when presenting schemas in the upload UI — this PR now
mirrors that only for the default-schema path while keeping authorization exact
for explicit input.
##########
superset/commands/database/uploaders/base.py:
##########
@@ -261,10 +261,28 @@ def validate_file_size(cls, file: Any) -> None:
if size is not None and size > max_file_size:
raise DatabaseUploadFileTooLarge()
+ @staticmethod
+ def _resolve_default_schema(database: Database) -> Optional[str]:
+ """Resolve the database's default schema so uploaded datasets carry an
+ explicit schema instead of NULL, which would otherwise duplicate an
+ existing dataset over the same table (see #36305)."""
+ try:
+ return database.get_default_schema(database.get_default_catalog())
+ except Exception: # pylint: disable=broad-except
+ # Resolution opens an inspector connection; a failure here must
+ # degrade to the no-schema behavior rather than fail the upload.
+ logger.warning(
+ "Unable to resolve default schema for upload; proceeding
without one",
+ exc_info=True,
+ )
+ return None
+
def validate(self) -> None:
self._model = DatabaseDAO.find_by_id(self._model_id)
if not self._model:
raise DatabaseNotFoundError()
+ if not self._schema:
+ self._schema = self._resolve_default_schema(self._model)
if not schema_allows_file_upload(self._model, self._schema):
raise DatabaseSchemaUploadNotAllowed()
Review Comment:
Addressed in 4d09913: the engine-resolved default schema is now compared
case-insensitively against the allow-list (`engine_resolved=True` in
`schema_allows_file_upload`), matching the `upload_allowed` filtering of the
schemas endpoint, so a database reporting `PUBLIC` with an allow-list entry
`public` is accepted. Explicit user-supplied schemas keep exact matching — see
the discussion on the later validators.py thread for why the fold is restricted
to the resolved-default path.
--
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]
