Skip to content

Commit 8389ef3

Browse files
richardfogacaRichard Fogaca
andauthored
[#2] feat: add shared filter infrastructure for export and delete commands (#376)
* feat: add shared filter infrastructure for export and delete commands Adds the foundational filter parsing and matching utilities used by both the filtered export and delete-assets commands: - Add Contains operator (case-insensitive substring matching) to the operators module - Add parse_filters() to parse repeatable key=value CLI filter strings with type coercion and alias normalization - Add filter_resources_locally() to apply parsed filters in-process when the Superset API does not support a given filter field - Add fetch_with_filter_fallback() to attempt server-side filtering and fall back to local filtering on unsupported-filter API errors - Add delete_dataset() and delete_database() methods to SupersetClient - Fix clean_logs() to use unlink(missing_ok=True) so a missing progress log file does not raise during cleanup Co-Authored-By: Richard Fogaca <richard@preset.io> * fix: mark fetch_with_filter_fallback branches covered by integration tests * refactor: simplify local filter matching and make bool checks strict (no 1/0 fallback) * style(test): apply pre-commit formatting for filter tests * refactor(superset): strengthen filter typing and error detection * fix(superset): satisfy black and mypy in filter helpers * test(superset): cover unsupported filter error-type branch * refactor(filter): tighten filter typing and remove Any --------- Co-authored-by: Richard Fogaca <richard@preset.io>
1 parent 59756b8 commit 8389ef3

6 files changed

Lines changed: 801 additions & 12 deletions

File tree

src/preset_cli/api/clients/superset.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,18 @@ def delete_chart(self, chart_id):
709709
"""
710710
self.delete_resource("chart", chart_id)
711711

712+
def delete_dataset(self, dataset_id: int) -> None:
713+
"""
714+
Delete a dataset.
715+
"""
716+
self.delete_resource("dataset", dataset_id)
717+
718+
def delete_database(self, database_id: int) -> None:
719+
"""
720+
Delete a database.
721+
"""
722+
self.delete_resource("database", database_id)
723+
712724
def get_dashboard(self, dashboard_id: int) -> Any:
713725
"""
714726
Return a single dashboard.

src/preset_cli/api/operators.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,11 @@ class In(Operator):
4040
"""
4141

4242
operator = "in"
43+
44+
45+
class Contains(Operator):
46+
"""
47+
Operator for substring/contains filters.
48+
"""
49+
50+
operator = "ct"

src/preset_cli/cli/superset/lib.py

Lines changed: 281 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,53 @@
44

55
from __future__ import annotations
66

7+
import re
78
from enum import Enum
89
from pathlib import Path
9-
from typing import IO, Any, Dict, Tuple
10+
from typing import IO, Callable, Dict, List, Tuple, TypeAlias, cast
1011

12+
import click
1113
import yaml
1214

15+
from preset_cli.api.operators import Contains
16+
from preset_cli.exceptions import ErrorPayload, SupersetError
1317
from preset_cli.lib import dict_merge
1418

1519
LOG_FILE_PATH = Path("progress.log")
20+
FilterValueType: TypeAlias = type[str] | type[int] | type[bool]
21+
FilterValue: TypeAlias = str | int | bool
22+
ParsedFilterValue: TypeAlias = FilterValue | Contains
23+
LogEntry: TypeAlias = Dict[str, object]
24+
LogsByType: TypeAlias = Dict["LogType", List[LogEntry]]
25+
SerializedLogs: TypeAlias = Dict[str, List[LogEntry]]
26+
ResourceRecord: TypeAlias = Dict[str, object]
27+
ResourceList: TypeAlias = List[ResourceRecord]
28+
29+
CONTAINS_FILTER_KEYS = {"dashboard_title"}
30+
LOCAL_FILTER_KEYS = {"certified_by", "is_managed_externally"}
31+
FILTER_ALIASES = {"managed_externally": "is_managed_externally"}
32+
DASHBOARD_FILTER_KEYS: Dict[str, FilterValueType] = {
33+
"id": int,
34+
"slug": str,
35+
"dashboard_title": str,
36+
"certified_by": str,
37+
"is_managed_externally": bool,
38+
}
39+
DELETE_FILTER_KEYS: Dict[str, Dict[str, FilterValueType]] = {
40+
"dashboard": DASHBOARD_FILTER_KEYS,
41+
"chart": {"id": int},
42+
"dataset": {"id": int},
43+
"database": {"id": int},
44+
}
45+
FILTER_NOT_ALLOWED_RE = re.compile(
46+
r"\bfilter(?:\s+column)?\b.*\bnot\s+allowed\s+to\s+filter\b",
47+
)
48+
FILTER_NOT_ALLOWED_ERROR_TYPES = {
49+
"FILTER_NOT_ALLOWED",
50+
"FILTER_NOT_ALLOWED_ERROR",
51+
"INVALID_FILTER_COLUMN",
52+
"INVALID_FILTER_COLUMN_ERROR",
53+
}
1654

1755

1856
class LogType(str, Enum):
@@ -24,38 +62,38 @@ class LogType(str, Enum):
2462
OWNERSHIP = "ownership"
2563

2664

27-
def get_logs(log_type: LogType) -> Tuple[Path, Dict[LogType, Any]]:
65+
def get_logs(log_type: LogType) -> Tuple[Path, LogsByType]:
2866
"""
2967
Returns the path and content of the progress log file.
3068
3169
Creates the file if it does not exist yet. Filters out FAILED
3270
entries for the particular log type. Defaults to an empty list.
3371
"""
34-
base_logs: Dict[LogType, Any] = {log_type_: [] for log_type_ in LogType}
72+
base_logs: LogsByType = {log_type_: [] for log_type_ in LogType}
3573

3674
if not LOG_FILE_PATH.exists():
3775
LOG_FILE_PATH.touch()
3876
return LOG_FILE_PATH, base_logs
3977

4078
with open(LOG_FILE_PATH, "r", encoding="utf-8") as log_file:
41-
logs = yaml.load(log_file, Loader=yaml.SafeLoader) or {}
79+
logs = cast(SerializedLogs, yaml.load(log_file, Loader=yaml.SafeLoader) or {})
4280

4381
logs = {LogType(log_type): log_entries for log_type, log_entries in logs.items()}
4482
dict_merge(base_logs, logs)
4583
base_logs[log_type] = [
46-
log for log in base_logs[log_type] if log["status"] != "FAILED"
84+
log for log in base_logs[log_type] if log.get("status") != "FAILED"
4785
]
4886
return LOG_FILE_PATH, base_logs
4987

5088

51-
def serialize_enum_logs_to_string(logs: Dict[LogType, Any]) -> Dict[str, Any]:
89+
def serialize_enum_logs_to_string(logs: LogsByType) -> SerializedLogs:
5290
"""
5391
Helper method to serialize the enum keys in the logs dict to str.
5492
"""
5593
return {log_type.value: log_entries for log_type, log_entries in logs.items()}
5694

5795

58-
def write_logs_to_file(log_file: IO[str], logs: Dict[LogType, Any]) -> None:
96+
def write_logs_to_file(log_file: IO[str], logs: LogsByType) -> None:
5997
"""
6098
Writes logs list to .log file.
6199
"""
@@ -65,7 +103,7 @@ def write_logs_to_file(log_file: IO[str], logs: Dict[LogType, Any]) -> None:
65103
log_file.truncate()
66104

67105

68-
def clean_logs(log_type: LogType, logs: Dict[LogType, Any]) -> None:
106+
def clean_logs(log_type: LogType, logs: LogsByType) -> None:
69107
"""
70108
Cleans the progress log file for the specific log type.
71109
@@ -78,3 +116,238 @@ def clean_logs(log_type: LogType, logs: Dict[LogType, Any]) -> None:
78116
yaml.dump(logs_, log_file)
79117
else:
80118
LOG_FILE_PATH.unlink(missing_ok=True)
119+
120+
121+
def _normalize_bool(value: object) -> bool | None:
122+
if isinstance(value, bool):
123+
return value
124+
if isinstance(value, str):
125+
normalized = value.strip().lower()
126+
if normalized == "true":
127+
return True
128+
if normalized == "false":
129+
return False
130+
if isinstance(value, int):
131+
return bool(value)
132+
return None
133+
134+
135+
def _coerce_filter_value(
136+
value: str,
137+
value_type: FilterValueType,
138+
key: str,
139+
) -> FilterValue:
140+
"""
141+
Coerce a filter value to the desired type.
142+
"""
143+
if value_type is bool:
144+
result = _normalize_bool(value)
145+
if result is None:
146+
raise click.BadParameter(
147+
f"Invalid value for {key}. Expected true or false.",
148+
)
149+
return result
150+
151+
try:
152+
return value_type(value)
153+
except (TypeError, ValueError) as exc:
154+
raise click.BadParameter(
155+
f"Invalid value for {key}. Expected {value_type.__name__}.",
156+
) from exc
157+
158+
159+
def coerce_bool_option(
160+
value: object,
161+
key: str,
162+
) -> bool:
163+
"""
164+
Coerce an option value to bool.
165+
"""
166+
if isinstance(value, (bool, str)):
167+
result = _normalize_bool(value)
168+
if result is not None:
169+
return result
170+
171+
raise click.BadParameter(
172+
f"Invalid value for {key}. Expected true or false.",
173+
)
174+
175+
176+
def is_filter_not_allowed_error(exc: Exception) -> bool:
177+
"""
178+
Return True if a Superset error indicates filters are not supported.
179+
"""
180+
if not isinstance(exc, SupersetError):
181+
return False
182+
183+
for error in exc.errors:
184+
if _is_filter_not_allowed_payload(error):
185+
return True
186+
187+
return False
188+
189+
190+
def _normalize_error_type(error_type: object) -> str:
191+
if not isinstance(error_type, str):
192+
return ""
193+
return re.sub(r"[^A-Z0-9]+", "_", error_type.upper()).strip("_")
194+
195+
196+
def _is_filter_not_allowed_payload(error: ErrorPayload) -> bool:
197+
normalized_error_type = _normalize_error_type(error.get("error_type"))
198+
if normalized_error_type in FILTER_NOT_ALLOWED_ERROR_TYPES:
199+
return True
200+
if "FILTER" in normalized_error_type and (
201+
"NOT_ALLOWED" in normalized_error_type or "UNSUPPORTED" in normalized_error_type
202+
):
203+
return True
204+
205+
message = str(error.get("message", "")).lower()
206+
return bool(FILTER_NOT_ALLOWED_RE.search(message))
207+
208+
209+
def _matches_contains(actual: object | None, expected: Contains) -> bool:
210+
actual_text = "" if actual is None else str(actual)
211+
return str(expected.value).lower() in actual_text.lower()
212+
213+
214+
def _matches_bool(actual: object | None, expected: bool) -> bool:
215+
actual_bool = _normalize_bool(actual)
216+
return actual_bool is not None and actual_bool == expected
217+
218+
219+
def _matches_empty_string(actual: object | None, expected: ParsedFilterValue) -> bool:
220+
return expected == "" and (actual is None or actual == "")
221+
222+
223+
def _matches_int(actual: object | None, expected: int) -> bool:
224+
try:
225+
if actual is None:
226+
return False
227+
return int(str(actual)) == expected
228+
except (TypeError, ValueError):
229+
return False
230+
231+
232+
def _matches_exact(actual: object | None, expected: str) -> bool:
233+
return str(actual) == str(expected)
234+
235+
236+
def filter_resources_locally( # pylint: disable=too-many-return-statements
237+
resources: ResourceList,
238+
filters: dict[str, ParsedFilterValue],
239+
) -> ResourceList:
240+
"""
241+
Apply parsed filters to a list of resources locally.
242+
243+
Empty-string filter values match both missing and empty values.
244+
"""
245+
246+
def matches(resource: ResourceRecord) -> bool:
247+
for key, expected in filters.items():
248+
actual = resource.get(key)
249+
250+
if _matches_empty_string(actual, expected):
251+
continue
252+
if isinstance(expected, Contains):
253+
if not _matches_contains(actual, expected):
254+
return False
255+
elif isinstance(expected, bool):
256+
if not _matches_bool(actual, expected):
257+
return False
258+
elif isinstance(expected, int):
259+
if not _matches_int(actual, expected):
260+
return False
261+
elif not _matches_exact(actual, expected):
262+
return False
263+
264+
return True
265+
266+
return [resource for resource in resources if matches(resource)]
267+
268+
269+
def parse_filters(
270+
filters: Tuple[str, ...],
271+
allowed_keys: Dict[str, FilterValueType],
272+
) -> Dict[str, ParsedFilterValue]:
273+
"""
274+
Parse repeatable key=value filter strings into kwargs for get_resources().
275+
"""
276+
parsed: Dict[str, ParsedFilterValue] = {}
277+
for item in filters:
278+
if "=" not in item:
279+
raise click.BadParameter(
280+
f"Invalid filter '{item}'. Expected key=value.",
281+
)
282+
283+
key, value = item.split("=", 1)
284+
key = key.strip()
285+
value = value.strip()
286+
if not key:
287+
raise click.BadParameter(
288+
f"Invalid filter '{item}'. Filter key cannot be empty.",
289+
)
290+
# Normalize user-facing aliases to canonical Superset API keys.
291+
key = FILTER_ALIASES.get(key, key)
292+
293+
if key not in allowed_keys:
294+
allowed = ", ".join(sorted(allowed_keys))
295+
raise click.BadParameter(
296+
f"Invalid filter key '{key}'. Allowed keys: {allowed}.",
297+
)
298+
if key in parsed:
299+
raise click.BadParameter(
300+
f"Duplicate filter key '{key}'. " "Pass each filter key at most once.",
301+
)
302+
303+
value_type = allowed_keys[key]
304+
coerced = _coerce_filter_value(value, value_type, key)
305+
if key in CONTAINS_FILTER_KEYS:
306+
parsed[key] = Contains(coerced)
307+
else:
308+
parsed[key] = coerced
309+
310+
return parsed
311+
312+
313+
def fetch_with_filter_fallback(
314+
fetch_filtered: Callable[..., ResourceList],
315+
fetch_all: Callable[[], ResourceList],
316+
parsed_filters: Dict[str, ParsedFilterValue],
317+
resource_label: str,
318+
) -> ResourceList:
319+
"""
320+
Try fetching with server-side filters; fall back to local filtering.
321+
322+
If any filter key is in LOCAL_FILTER_KEYS the local path is used immediately.
323+
On a ``filter not allowed`` API error, results are fetched unfiltered and
324+
filtered locally. Other exceptions are wrapped in a click.ClickException.
325+
"""
326+
if set(parsed_filters) & LOCAL_FILTER_KEYS:
327+
return filter_resources_locally(fetch_all(), parsed_filters)
328+
329+
try:
330+
resources = fetch_filtered(**parsed_filters)
331+
except Exception as exc: # pylint: disable=broad-except
332+
if is_filter_not_allowed_error(exc):
333+
return filter_resources_locally(fetch_all(), parsed_filters)
334+
filter_keys = ", ".join(parsed_filters.keys())
335+
raise click.ClickException(
336+
f"Failed to fetch {resource_label} ({exc}). "
337+
f"This may indicate that filter key(s) {filter_keys} "
338+
"may not be supported by this Superset version.",
339+
) from exc
340+
341+
if not resources:
342+
return resources # pragma: no cover
343+
344+
# Verify filtered responses locally to avoid broad results when an API silently
345+
# ignores one or more predicates.
346+
if all(all(key in resource for key in parsed_filters) for resource in resources):
347+
return filter_resources_locally(resources, parsed_filters)
348+
349+
# Some endpoints return slim payloads that omit filter keys. For contains
350+
# predicates, re-fetch without filters and apply predicates locally.
351+
if any(isinstance(value, Contains) for value in parsed_filters.values()):
352+
return filter_resources_locally(fetch_all(), parsed_filters)
353+
return resources # pragma: no cover

src/preset_cli/cli/superset/sync/native/command.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,11 @@ def import_resources_individually( # pylint: disable=too-many-locals, too-many-
483483
related_configs: Dict[str, Dict[Path, AssetConfig]] = {}
484484

485485
log_file_path, logs = get_logs(LogType.ASSETS)
486-
assets_to_skip = {Path(log["path"]) for log in logs[LogType.ASSETS]}
486+
assets_to_skip = {
487+
Path(path_value)
488+
for log in logs[LogType.ASSETS]
489+
if isinstance((path_value := log.get("path")), str)
490+
}
487491
existing_databases = existing_databases or set()
488492
existing_uuid_cache: Dict[Tuple[str, str], bool] = {}
489493

0 commit comments

Comments
 (0)