Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Tags filter): Filter assets by tag ID #29412

Merged
merged 6 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions superset-frontend/src/components/ListView/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,10 @@ export enum FilterOperator {
DatasetIsCertified = 'dataset_is_certified',
DashboardHasCreatedBy = 'dashboard_has_created_by',
ChartHasCreatedBy = 'chart_has_created_by',
DashboardTags = 'dashboard_tags',
ChartTags = 'chart_tags',
SavedQueryTags = 'saved_query_tags',
DashboardTagByName = 'dashboard_tags',
DashboardTagById = 'dashboard_tag_id',
ChartTagByName = 'chart_tags',
ChartTagById = 'chart_tag_id',
SavedQueryTagByName = 'saved_query_tags',
SavedQueryTagById = 'saved_query_tag_id',
}
2 changes: 1 addition & 1 deletion superset-frontend/src/pages/ChartList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ function ChartList(props: ChartListProps) {
key: 'tags',
id: 'tags',
input: 'select',
operator: FilterOperator.ChartTags,
operator: FilterOperator.ChartTagById,
unfilteredLabel: t('All'),
fetchSelects: loadTags,
},
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/pages/DashboardList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ function DashboardList(props: DashboardListProps) {
key: 'tags',
id: 'tags',
input: 'select',
operator: FilterOperator.DashboardTags,
operator: FilterOperator.DashboardTagById,
unfilteredLabel: t('All'),
fetchSelects: loadTags,
},
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/pages/SavedQueryList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ function SavedQueryList({
id: 'tags',
key: 'tags',
input: 'select',
operator: FilterOperator.SavedQueryTags,
operator: FilterOperator.SavedQueryTagById,
fetchSelects: loadTags,
},
]
Expand Down
5 changes: 3 additions & 2 deletions superset/charts/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
ChartFilter,
ChartHasCreatedByFilter,
ChartOwnedCreatedFavoredByMeFilter,
ChartTagFilter,
ChartTagIdFilter,
ChartTagNameFilter,
)
from superset.charts.schemas import (
CHART_SCHEMAS,
Expand Down Expand Up @@ -238,7 +239,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
],
"slice_name": [ChartAllTextFilter],
"created_by": [ChartHasCreatedByFilter, ChartCreatedByMeFilter],
"tags": [ChartTagFilter],
"tags": [ChartTagNameFilter, ChartTagIdFilter],
}
# Will just affect _info endpoint
edit_columns = ["slice_name"]
Expand Down
17 changes: 15 additions & 2 deletions superset/charts/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,29 @@ class ChartFavoriteFilter(BaseFavoriteFilter): # pylint: disable=too-few-public
model = Slice


class ChartTagFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
class ChartTagNameFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all dashboards that a user has favored
Custom filter for the GET list that filters all charts associated with
a certain tag (by its name).
"""

arg_name = "chart_tags"
class_name = "slice"
model = Slice


class ChartTagIdFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all charts associated with
a certain tag (by its ID).
"""

arg_name = "chart_tag_id"
class_name = "slice"
id_based_filter = True
model = Slice


class ChartCertifiedFilter(BaseFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all certified charts
Expand Down
5 changes: 3 additions & 2 deletions superset/dashboards/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@
DashboardCreatedByMeFilter,
DashboardFavoriteFilter,
DashboardHasCreatedByFilter,
DashboardTagFilter,
DashboardTagIdFilter,
DashboardTagNameFilter,
DashboardTitleOrSlugFilter,
FilterRelatedRoles,
)
Expand Down Expand Up @@ -229,7 +230,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"dashboard_title": [DashboardTitleOrSlugFilter],
"id": [DashboardFavoriteFilter, DashboardCertifiedFilter],
"created_by": [DashboardCreatedByMeFilter, DashboardHasCreatedByFilter],
"tags": [DashboardTagFilter],
"tags": [DashboardTagIdFilter, DashboardTagNameFilter],
}

base_order = ("changed_on", "desc")
Expand Down
17 changes: 15 additions & 2 deletions superset/dashboards/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,29 @@ class DashboardFavoriteFilter( # pylint: disable=too-few-public-methods
model = Dashboard


class DashboardTagFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
class DashboardTagNameFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all dashboards that a user has favored
Custom filter for the GET list that filters all dashboards associated with
a certain tag (by its name).
"""

arg_name = "dashboard_tags"
class_name = "Dashboard"
model = Dashboard


class DashboardTagIdFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all dashboards associated with
a certain tag (by its ID).
"""

arg_name = "dashboard_tag_id"
class_name = "Dashboard"
id_based_filter = True
model = Dashboard


class DashboardAccessFilter(BaseFilter): # pylint: disable=too-few-public-methods
"""
List dashboards with the following criteria:
Expand Down
15 changes: 7 additions & 8 deletions superset/queries/saved_queries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_babel import ngettext

from superset import is_feature_enabled
from superset.commands.importers.exceptions import (
IncorrectFormatError,
NoValidFilesFoundError,
Expand All @@ -46,7 +45,8 @@
SavedQueryAllTextFilter,
SavedQueryFavoriteFilter,
SavedQueryFilter,
SavedQueryTagFilter,
SavedQueryTagIdFilter,
SavedQueryTagNameFilter,
)
from superset.queries.saved_queries.schemas import (
get_delete_ids_schema,
Expand Down Expand Up @@ -124,9 +124,10 @@ class SavedQueryRestApi(BaseSupersetModelRestApi):
"schema",
"sql",
"sql_tables",
"tags.id",
"tags.name",
"tags.type",
]
if is_feature_enabled("TAGGING_SYSTEM"):
list_columns += ["tags.id", "tags.name", "tags.type"]
list_select_columns = list_columns + ["changed_by_fk", "changed_on"]
add_columns = [
"db_id",
Expand Down Expand Up @@ -161,15 +162,13 @@ class SavedQueryRestApi(BaseSupersetModelRestApi):
"schema",
"created_by",
"changed_by",
"tags",
]
if is_feature_enabled("TAGGING_SYSTEM"):
search_columns += ["tags"]
search_filters = {
"id": [SavedQueryFavoriteFilter],
"label": [SavedQueryAllTextFilter],
"tags": [SavedQueryTagNameFilter, SavedQueryTagIdFilter],
}
if is_feature_enabled("TAGGING_SYSTEM"):
search_filters["tags"] = [SavedQueryTagFilter]

apispec_parameter_schemas = {
"get_delete_ids_schema": get_delete_ids_schema,
Expand Down
17 changes: 15 additions & 2 deletions superset/queries/saved_queries/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,29 @@ class SavedQueryFavoriteFilter(BaseFavoriteFilter): # pylint: disable=too-few-p
model = SavedQuery


class SavedQueryTagFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
class SavedQueryTagNameFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all dashboards that a user has favored
Custom filter for the GET list that filters all saved queries associated with
a certain tag (by its name).
"""

arg_name = "saved_query_tags"
class_name = "query"
model = SavedQuery


class SavedQueryTagIdFilter(BaseTagFilter): # pylint: disable=too-few-public-methods
"""
Custom filter for the GET list that filters all saved queries associated with
a certain tag (by its ID).
"""

arg_name = "saved_query_tag_id"
class_name = "query"
id_based_filter = True
model = SavedQuery


class SavedQueryFilter(BaseFilter): # pylint: disable=too-few-public-methods
def apply(self, query: BaseQuery, value: Any) -> BaseQuery:
"""
Expand Down
11 changes: 11 additions & 0 deletions superset/views/base_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,19 @@ class BaseTagFilter(BaseFilter): # pylint: disable=too-few-public-methods
""" The Tag class_name to user """
model: type[Dashboard | Slice | SqllabQuery | SqlaTable] = Dashboard
""" The SQLAlchemy model """
id_based_filter = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cleaner to define a new BaseTagIdFilter, and rename this one to BaseTagNameFilter, to prevent the check in apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense! I just pushed a new commit with this change. I also ended up moving these two filters to superset.tag.filters as I believe it makes more sense -- let me know your thoughts 🙏


def apply(self, query: Query, value: Any) -> Query:
# ID based filter
if self.id_based_filter:
Copy link
Member

@hughhhh hughhhh Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write a small unit test to verify the query is properly writing the query we want?

I think something like this would be good for the base class

filtered_query = filter_.apply(query, None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hughhhh -- just added unit tests for the BaseTagFilter -- the ChartTagNameFilter, ChartTagIdFilter, DashboardTagNameFilter, DashboardTagIdFilter, SavedQueryTagNameFilter and SavedQueryTagIdFilter should have integration tests already 🙌

Let me know if you have any other concerns or feedback 🙏

tags_query = (
db.session.query(self.model.id)
.join(self.model.tags)
.filter(Tag.id == value)
)
return query.filter(self.model.id.in_(tags_query))

# Name based filter
ilike_value = f"%{value}%"
tags_query = (
db.session.query(self.model.id)
Expand Down
16 changes: 16 additions & 0 deletions tests/integration_tests/base_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from unittest.mock import Mock, patch, MagicMock

import pandas as pd
import prison
from flask import Response, g
from flask_appbuilder.security.sqla import models as ab_models
from flask_testing import TestCase
Expand All @@ -33,6 +34,7 @@
from sqlalchemy.sql import func
from sqlalchemy.dialects.mysql import dialect

from tests.integration_tests.constants import ADMIN_USERNAME
from tests.integration_tests.test_app import app, login
from superset.sql_parse import CtasMethod
from superset import db, security_manager
Expand Down Expand Up @@ -590,6 +592,20 @@ def insert_dashboard(
db.session.commit()
return dashboard

def get_list(
self,
asset_type: str,
filter: dict[str, Any] = {},
username: str = ADMIN_USERNAME,
) -> Response:
"""
Get list of assets, by default using admin account. Can be filtered.
"""
self.login(username)
uri = f"api/v1/{asset_type}/?q={prison.dumps(filter)}"
response = self.get_assert_metric(uri, "get_list")
return response


@contextmanager
def db_insert_temp_object(obj: DeclarativeMeta):
Expand Down
Loading
Loading