codeant-ai-for-open-source[bot] commented on code in PR #41076: URL: https://github.com/apache/superset/pull/41076#discussion_r3560230550
########## superset/versioning/activity/kinds.py: ########## @@ -0,0 +1,210 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Kind translation tables, shared types, and the shadow-model loader. + +The activity-view module operates in two "kind" namespaces — the +table-stored value (``"chart"`` / ``"dashboard"`` / ``"dataset"``) that +``version_changes.entity_kind`` carries, and the Python class-name +form (``"Slice"`` / ``"Dashboard"`` / ``"SqlaTable"``) used internally +for dispatch and returned in the DTO's ``entity_kind`` field. The four +mappings here translate between them. Adjacent kind-keyed dicts live +here too: the per-kind human-readable label, the user-facing +lowercase form, and the 404 exception class. + +The :func:`load_live_model` helper exists in the same module +because each lookup is keyed on the same set of class names — keeping +it adjacent to the mappings makes the kind-translation surface +discoverable at a glance. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Iterator +from dataclasses import dataclass + +from flask_appbuilder import Model + +from superset.commands.chart.exceptions import ChartNotFoundError +from superset.commands.dashboard.exceptions import DashboardNotFoundError +from superset.commands.dataset.exceptions import DatasetNotFoundError +from superset.versioning.changes import ENTITY_KIND_BY_CLASS_NAME + +# Max entity ids per ``IN (...)`` clause across the activity-view queries. +# Bounds the bind-parameter count under SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` +# floor (default 999 in many builds); Postgres/MySQL accept the full list but +# the chunk is dialect-agnostic. 500 leaves headroom for the other bound +# params on each statement. +ENTITY_ID_CHUNK_SIZE = 500 + + +def chunked_ids( + ids: Iterable[int], size: int = ENTITY_ID_CHUNK_SIZE +) -> Iterator[list[int]]: + """Yield *ids* in fixed-size lists (final chunk may be smaller). Shared by + the activity-view query helpers so every ``IN (...)`` stays under the + SQLite bind-variable floor.""" + items = list(ids) + for i in range(0, len(items), size): + yield items[i : i + size] + + +# ---- Kind translation ----------------------------------------------------- + +# ``version_changes.entity_kind`` stores the friendly downstream-tooling +# value (``"chart"``, ``"dashboard"``, ``"dataset"``) per the +# ``ENTITY_KIND_BY_CLASS_NAME``. The activity-view DTO returns the +# Python class name instead (``"Slice"``, ``"Dashboard"``, +# ``"SqlaTable"``) so the contract aligns with ``__class__.__name__`` +# (the ``ActivityRecord`` DTO). Translate at the boundary. +TABLE_KIND_TO_API: dict[str, str] = { + table_kind: class_name + for class_name, table_kind in ENTITY_KIND_BY_CLASS_NAME.items() +} +API_KIND_TO_TABLE: dict[str, str] = dict(ENTITY_KIND_BY_CLASS_NAME) + +# Human-readable label for summary headlines +# ("Dataset updated: Sales Transactions"). Keyed by the internal API kind +# (Python class name; matches ``model_cls.__name__``). +API_KIND_LABEL: dict[str, str] = { + "Dashboard": "Dashboard", + "Slice": "Chart", + "SqlaTable": "Dataset", +} + +# User-facing lowercase rendering of the kind. This is what appears in +# the JSON response's ``entity_kind`` field and the +# ``ActivityRecordSchema.entity_kind`` enum. Internal code keeps the +# Python class-name form because it matches ``model_cls.__name__`` and is +# convenient for dispatch — translation happens at serialization time +# only, in :func:`render.apply_record_decoration`. +USER_FACING_KIND: dict[str, str] = { + "Dashboard": "dashboard", + "Slice": "chart", + "SqlaTable": "dataset", +} + +# 404 exception class per API kind. Each accepts a string positional arg +# (the path-entity UUID) that gets formatted into the exception message. +NOT_FOUND_EXC: dict[str, type[Exception]] = { + "Dashboard": DashboardNotFoundError, + "Slice": ChartNotFoundError, + "SqlaTable": DatasetNotFoundError, +} + +# Per-API-kind (model class name, display column) used by +# ``_resolve_names_for_kind`` to read the user-facing entity name from +# the shadow table valid at a given transaction. +NAME_COLUMN: dict[str, tuple[str, str]] = { + "Dashboard": ("Dashboard", "dashboard_title"), + "Slice": ("Slice", "slice_name"), + "SqlaTable": ("SqlaTable", "table_name"), +} + + +# ---- Types ---------------------------------------------------------------- + + +@dataclass(frozen=True) +class Window: + """A validity window in Continuum transaction-id space, half-open as + ``[start_tx, end_tx)``. + + Immutable and equal-by-attributes — two windows with the same + ``start_tx`` / ``end_tx`` are interchangeable. Constructor rejects + ``end_tx <= start_tx``. ``end_tx = None`` means "open ended + (current)" and acts as positive infinity throughout the helpers. + + Helper methods (``contains`` / ``intersect`` / ``merges_with``) + live on the type so callers don't re-implement the half-open + predicate inline. Previously a ``tuple[int, int | None]`` alias; + promoted to a dataclass so a function accepting a ``Window`` can't + silently accept any other 2-tuple and so the constructor enforces + the half-open invariant. + """ + + start_tx: int + end_tx: int | None + + def __post_init__(self) -> None: + if self.end_tx is not None and self.end_tx <= self.start_tx: + raise ValueError( + f"Window end_tx must be > start_tx; " + f"got [{self.start_tx}, {self.end_tx})" + ) + + def contains(self, tx_id: int) -> bool: + """``True`` iff *tx_id* falls inside this half-open interval.""" + return self.start_tx <= tx_id and (self.end_tx is None or tx_id < self.end_tx) + + def intersect(self, other: Window) -> Window | None: + """Return the clipped overlap of this window with *other*, or + ``None`` when they are disjoint. ``end_tx = None`` acts as + positive infinity on either side.""" + start = max(self.start_tx, other.start_tx) + end: int | None + if self.end_tx is None: + end = other.end_tx + elif other.end_tx is None: + end = self.end_tx + else: + end = min(self.end_tx, other.end_tx) + if end is not None and end <= start: + return None + return Window(start, end) + + def merges_with(self, other: Window) -> bool: + """``True`` iff *self* and *other* overlap or touch (so their + union is one contiguous window). Assumes the caller has placed + them in start-ascending order.""" + if self.end_tx is None: + # self extends to +∞; everything past it merges in. + return True + return other.start_tx <= self.end_tx + + +#: A related-entity scope row: ``(api_kind, entity_id, [windows])``. +#: ``api_kind`` is the DTO-facing kind (``"Slice"``, etc.), not the +#: table-stored kind. Modelled as a tuple alias — the +#: ``(api_kind, entity_id)`` pair is logically a key with the window +#: list as its value, so a dict shape may fit better than a flat +#: dataclass. +EntityWindows = tuple[str, int, list[Window]] + + +def load_live_model(model_name: str) -> type[Model]: + """Inline-import a live model class by name. Deferred until call + time because the versioning package is initialised before all model + mappers are configured (same idiom used throughout + :mod:`superset.versioning.changes`). + + Returns the LIVE model class (``Dashboard`` / ``Slice`` / + ``SqlaTable``), not a Continuum shadow class — callers derive the + shadow table separately via ``version_class(...)`` where needed.""" + # pylint: disable=import-outside-toplevel + if model_name == "Dashboard": + from superset.models.dashboard import Dashboard + + return Dashboard + if model_name == "Slice": + from superset.models.slice import Slice + + return Slice + if model_name == "SqlaTable": + from superset.connectors.sqla.models import SqlaTable + + return SqlaTable + raise LookupError(f"No shadow class registered for {model_name!r}") Review Comment: **Suggestion:** The exception message says a “shadow class” is missing, but this helper loads live model classes, not shadow versions. This creates misleading runtime diagnostics when an unsupported kind is passed. Update the message to reference the live model registry so operators can accurately troubleshoot failures. [comment mismatch] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx ⚠️ Misleading error when activity model lookup fails. ⚠️ Operators may debug shadow registry instead of live. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a Python shell in the Superset environment with this PR code deployed. 2. Import the helper from `superset/versioning/activity/kinds.py` (defined at lines 188-197): `from superset.versioning.activity.kinds import load_live_model`. 3. Call `load_live_model("FooModel")`, where `"FooModel"` is not one of `"Dashboard"`, `"Slice"`, or `"SqlaTable"` (the supported live model names wired in the `if` branches at lines 199-208). 4. Observe that `load_live_model` raises `LookupError` at `superset/versioning/activity/kinds.py:210` with message `No shadow class registered for 'FooModel'`, even though the function’s docstring (lines 188-197) states it returns live model classes and callers derive shadow tables separately via `version_class(...)`; this mismatch makes runtime diagnostics misleading about which registry is at fault. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=575177774e0b48b38051220cc77bb8bc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=575177774e0b48b38051220cc77bb8bc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/versioning/activity/kinds.py **Line:** 210:210 **Comment:** *Comment Mismatch: The exception message says a “shadow class” is missing, but this helper loads live model classes, not shadow versions. This creates misleading runtime diagnostics when an unsupported kind is passed. Update the message to reference the live model registry so operators can accurately troubleshoot failures. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=bb47ad8365c36b4997a9f0d3ef3048a74f58868b136e12c108a65a89fbea5e97&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=bb47ad8365c36b4997a9f0d3ef3048a74f58868b136e12c108a65a89fbea5e97&reaction=dislike'>👎</a> -- 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]
