Copilot commented on code in PR #39390:
URL: https://github.com/apache/superset/pull/39390#discussion_r3190523186
##########
superset/utils/encrypt.py:
##########
@@ -99,14 +99,27 @@ def __init__(self, previous_secret_key: str) -> None:
def discover_encrypted_fields(self) -> dict[str, dict[str, EncryptedType]]:
"""
- Iterates over SqlAlchemy's metadata, looking for EncryptedType
- columns along the way. Builds up a dict of
+ Iterates over ORM-mapped tables, looking for EncryptedType columns
+ along the way. Builds up a dict of
table_name -> dict of col_name: enc type instance
+
+ Superset's ORM models inherit from Flask-AppBuilder's declarative base
+ (`flask_appbuilder.Model`), whose MetaData is distinct from
+ `db.metadata`. We combine both sources so encrypted columns are found
+ regardless of which base a model uses.
:return:
"""
+ from flask_appbuilder import ( # pylint:
disable=import-outside-toplevel
+ Model as FABModel,
+ )
+
meta_info: dict[str, Any] = {}
- for table_name, table in self._db.metadata.tables.items():
+ tables: dict[str, Any] = {}
+ tables.update(FABModel.metadata.tables)
+ tables.update(self._db.metadata.tables)
Review Comment:
The two `update()` calls don't truly *merge* sources: if both metadata
objects contain the same table key, the second `update()` silently overwrites
the first. To match the stated intent (“combine both sources”), consider either
(a) making the precedence explicit and documented (e.g., let FAB overwrite db,
or vice versa), or (b) merging without overwriting existing keys (e.g., only
add tables from the second source when absent) and/or logging when duplicates
are encountered. This avoids reintroducing the discovery bug if a table appears
in both metadata registries in the future.
##########
superset/utils/encrypt.py:
##########
@@ -99,14 +99,27 @@ def __init__(self, previous_secret_key: str) -> None:
def discover_encrypted_fields(self) -> dict[str, dict[str, EncryptedType]]:
"""
- Iterates over SqlAlchemy's metadata, looking for EncryptedType
- columns along the way. Builds up a dict of
+ Iterates over ORM-mapped tables, looking for EncryptedType columns
+ along the way. Builds up a dict of
table_name -> dict of col_name: enc type instance
+
+ Superset's ORM models inherit from Flask-AppBuilder's declarative base
+ (`flask_appbuilder.Model`), whose MetaData is distinct from
+ `db.metadata`. We combine both sources so encrypted columns are found
+ regardless of which base a model uses.
:return:
Review Comment:
The docstring ends with an empty `:return:` field. Either remove it or
specify what is returned (e.g., mapping of table name to encrypted column
name/type) to keep the docstring consistent and useful.
--
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]
