dpgaspar commented on code in PR #35018:
URL: https://github.com/apache/superset/pull/35018#discussion_r2352431921
##########
superset/daos/base.py:
##########
@@ -251,3 +457,231 @@ def filter_by(cls, **filter_by: Any) -> list[T]:
cls.id_column_name, data_model
).apply(query, None)
return query.filter_by(**filter_by).all()
+
+ @classmethod
+ def apply_column_operators(
+ cls, query: Any, column_operators: Optional[List[ColumnOperator]] =
None
+ ) -> Any:
+ """
+ Apply column operators (list of ColumnOperator) to the query using
+ ColumnOperatorEnum logic. Raises ValueError if a filter references a
+ non-existent column.
+ """
+ if not column_operators:
+ return query
+ for c in column_operators:
+ if not isinstance(c, ColumnOperator):
+ continue
+ col = c.col
+ opr = c.opr
+ value = c.value
Review Comment:
nit(optional): col, opr, value = c.col, c.opr, c.value
##########
superset/daos/base.py:
##########
@@ -83,52 +197,145 @@ def find_by_id_or_uuid(
return None
@classmethod
- def find_by_id(
- cls,
- model_id: str | int,
- skip_base_filter: bool = False,
- ) -> T | None:
+ def _apply_base_filter(
+ cls, query: Any, skip_base_filter: bool = False, data_model: Any = None
+ ) -> Any:
"""
- Find a model by id, if defined applies `base_filter`
+ Apply the base_filter to the query if it exists and skip_base_filter
is False.
"""
- query = db.session.query(cls.model_cls)
if cls.base_filter and not skip_base_filter:
- data_model = SQLAInterface(cls.model_cls, db.session)
+ if data_model is None:
+ data_model = SQLAInterface(cls.model_cls, db.session)
query = cls.base_filter( # pylint: disable=not-callable
cls.id_column_name, data_model
).apply(query, None)
- id_column = getattr(cls.model_cls, cls.id_column_name)
+ return query
+
+ @classmethod
+ def _convert_value_for_column(cls, column: Any, value: Any) -> Any:
+ """
+ Convert a value to the appropriate type for a given SQLAlchemy column.
+
+ Args:
+ column: SQLAlchemy column object
+ value: Value to convert
+
+ Returns:
+ Converted value or None if conversion fails
+ """
+ if (
+ hasattr(column.type, "python_type")
+ and column.type.python_type == uuid_lib.UUID
+ ):
+ if isinstance(value, str):
+ try:
+ return uuid_lib.UUID(value)
+ except (ValueError, AttributeError):
+ return None
+ return value
+
+ @classmethod
+ def _find_by_column(
+ cls,
+ column_name: str,
+ value: str | int,
+ skip_base_filter: bool = False,
+ ) -> T | None:
+ """
+ Private method to find a model by any column value.
+
+ Args:
+ column_name: Name of the column to search by
+ value: Value to search for
+ skip_base_filter: Whether to skip base filtering
+
+ Returns:
+ Model instance or None if not found
+ """
+ query = db.session.query(cls.model_cls)
+ query = cls._apply_base_filter(query, skip_base_filter)
+
+ if not hasattr(cls.model_cls, column_name):
+ return None
+
+ column = getattr(cls.model_cls, column_name)
+ converted_value = cls._convert_value_for_column(column, value)
+ if converted_value is None:
+ return None
+
try:
- return query.filter(id_column == model_id).one_or_none()
+ return query.filter(column == converted_value).one_or_none()
except StatementError:
# can happen if int is passed instead of a string or similar
return None
+ @classmethod
+ def find_by_id(
+ cls,
+ model_id: str | int,
+ skip_base_filter: bool = False,
+ id_column: str | None = None,
+ ) -> T | None:
+ """
+ Find a model by ID using specified or default ID column.
+
+ Args:
+ model_id: ID value to search for
+ skip_base_filter: Whether to skip base filtering
+ id_column: Column name to use (defaults to cls.id_column_name)
+
+ Returns:
+ Model instance or None if not found
+ """
+ column = id_column or cls.id_column_name
+ return cls._find_by_column(column, model_id, skip_base_filter)
+
@classmethod
def find_by_ids(
cls,
- model_ids: list[str] | list[int],
+ model_ids: Sequence[str | int],
skip_base_filter: bool = False,
+ id_column: str | None = None,
) -> list[T]:
"""
Find a List of models by a list of ids, if defined applies
`base_filter`
+
+ :param model_ids: List of IDs to find
+ :param skip_base_filter: If true, skip applying the base filter
+ :param id_column: Optional column name to use for ID lookup
+ (defaults to id_column_name)
"""
- id_col = getattr(cls.model_cls, cls.id_column_name, None)
+ column = id_column or cls.id_column_name
+ id_col = getattr(cls.model_cls, column, None)
if id_col is None or not model_ids:
return []
- query = db.session.query(cls.model_cls).filter(id_col.in_(model_ids))
- if cls.base_filter and not skip_base_filter:
- data_model = SQLAInterface(cls.model_cls, db.session)
- query = cls.base_filter( # pylint: disable=not-callable
- cls.id_column_name, data_model
- ).apply(query, None)
+
+ # Convert IDs to appropriate types based on column type
+ converted_ids: list[str | int | uuid_lib.UUID] = []
+ for id_val in model_ids:
+ converted_value = cls._convert_value_for_column(id_col, id_val)
+ if converted_value is not None:
+ converted_ids.append(converted_value)
+
+ # Validate type consistency for better error handling
+ if len(converted_ids) > 1:
+ types_found = set(map(type, converted_ids))
+ if len(types_found) > 1:
+ logger.warning(
+ "Mixed ID types detected for %s: %s",
+ cls.model_cls.__name__ if cls.model_cls else "Unknown",
+ [t.__name__ for t in types_found],
+ )
Review Comment:
Looks good, but should it fail with ValueError?
##########
superset/daos/base.py:
##########
@@ -251,3 +455,205 @@ def filter_by(cls, **filter_by: Any) -> list[T]:
cls.id_column_name, data_model
).apply(query, None)
return query.filter_by(**filter_by).all()
+
+ @classmethod
+ def apply_column_operators(
+ cls, query: Any, column_operators: Optional[List[ColumnOperator]] =
None
+ ) -> Any:
+ """
+ Apply column operators (list of ColumnOperator) to the query using
+ ColumnOperatorEnum logic. Raises ValueError if a filter references a
+ non-existent column.
+ """
+ if not column_operators:
+ return query
+ for c in column_operators:
+ if not isinstance(c, ColumnOperator):
+ continue
+ col = c.col
+ opr = c.opr
+ value = c.value
+ if not col or not hasattr(cls.model_cls, col):
+ model_name = cls.model_cls.__name__ if cls.model_cls else
"Unknown"
+ logging.error(
+ f"Invalid filter: column '{col}' does not exist on
{model_name}"
+ )
+ raise ValueError(
+ f"Invalid filter: column '{col}' does not exist on
{model_name}"
+ )
+ column = getattr(cls.model_cls, col)
+ try:
+ # Always use ColumnOperatorEnum's apply method
+ operator_enum = ColumnOperatorEnum(opr)
+ query = query.filter(operator_enum.apply(column, value))
+ except Exception as e:
+ logging.error(f"Error applying filter on column '{col}': {e}")
+ raise
+ return query
+
+ @classmethod
+ def get_filterable_columns_and_operators(cls) -> Dict[str, List[str]]:
+ """
+ Returns a dict mapping filterable columns (including hybrid/computed
fields if
+ present) to their supported operators. Used by MCP tools to
dynamically expose
+ filter options. Custom fields supported by the DAO but not present on
the model
+ should be documented here.
+ """
+
+ mapper = inspect(cls.model_cls)
+ columns = {c.key: c for c in mapper.columns}
+ # Add hybrid properties
+ hybrids = {
+ name: attr
+ for name, attr in vars(cls.model_cls).items()
+ if isinstance(attr, hybrid_property)
+ }
+ # You may add custom fields here, e.g.:
+ # custom_fields = {"tags": ["eq", "in_", "like"], ...}
+ custom_fields: Dict[str, List[str]] = {}
+
+ filterable = {}
+ for name, col in columns.items():
+ if isinstance(col.type, (sa.String, sa.Text)):
+ filterable[name] = TYPE_OPERATOR_MAP["string"]
+ elif isinstance(col.type, (sa.Boolean,)):
+ filterable[name] = TYPE_OPERATOR_MAP["boolean"]
+ elif isinstance(col.type, (sa.Integer, sa.Float, sa.Numeric)):
+ filterable[name] = TYPE_OPERATOR_MAP["number"]
+ elif isinstance(col.type, (sa.DateTime, sa.Date, sa.Time)):
+ filterable[name] = TYPE_OPERATOR_MAP["datetime"]
+ else:
+ # Fallback to eq/ne/null
+ filterable[name] = ["eq", "ne", "is_null", "is_not_null"]
+ # Add hybrid properties as string fields by default
+ for name in hybrids:
+ filterable[name] = TYPE_OPERATOR_MAP["string"]
+ # Add custom fields
+ filterable.update(custom_fields)
+ return filterable
+
+ @classmethod
+ def _build_query(
+ cls,
+ column_operators: Optional[List[ColumnOperator]] = None,
+ search: Optional[str] = None,
+ search_columns: Optional[List[str]] = None,
+ custom_filters: Optional[Dict[str, BaseFilter]] = None,
+ skip_base_filter: bool = False,
+ data_model: Optional[SQLAInterface] = None,
+ ) -> Any:
+ """
+ Build a SQLAlchemy query with base filter, column operators, search,
and
+ custom filters.
+ """
+ if data_model is None:
+ data_model = SQLAInterface(cls.model_cls, db.session)
+ query = data_model.session.query(cls.model_cls)
+ query = cls._apply_base_filter(
+ query, skip_base_filter=skip_base_filter, data_model=data_model
+ )
+ if search and search_columns:
+ search_filters = []
+ for column_name in search_columns:
+ if hasattr(cls.model_cls, column_name):
+ column = getattr(cls.model_cls, column_name)
+ search_filters.append(cast(column,
Text).ilike(f"%{search}%"))
Review Comment:
This is safe because we are using SQLAlchemy ORM
SQLAlchemy doesn't directly concatenate this into the SQL string. Instead,
it:
Creates a parameterized query with placeholders
Passes the value '%' + search + '%' as a parameter separately from the SQL
structure
The actual SQL sent to the database looks something like:
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
