codeant-ai-for-open-source[bot] commented on code in PR #41076: URL: https://github.com/apache/superset/pull/41076#discussion_r3560236305
########## tests/integration_tests/versioning/activity_view_tests.py: ########## @@ -0,0 +1,1250 @@ +# 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. +"""Integration tests for the cross-entity activity-view API (sc-107283). + +US1 — dashboard activity stream: ``GET /api/v1/dashboard/<uuid>/activity/``. +Tests for US2 (chart activity) and US3 (dataset activity) come in later +phases. + +Per spec T053 / sc-103156 T062, every test that mutates a fixture entity +wraps the test body in ``try``/``finally`` with +``metadata_db.session.rollback()`` in the ``finally``. The rationale is +documented in the spec — Continuum captures dirty mappers during +autoflush, so leaving an instrumented attribute dirty pollutes +downstream tests via the shadow tables. +""" + +from __future__ import annotations + +from typing import Any +from uuid import uuid4 + +import pytest + +from superset.connectors.sqla.models import SqlaTable +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice +from superset.utils import json as _json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) +from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401 + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) + + +def _get_birth_names_dataset() -> SqlaTable: + return ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + + +def _persist_fixture_state() -> None: + """Force the fixture's pending INSERTs to commit so subsequent edits + produce *new* version rows instead of being batched into the + creation transaction. Mirrors the same helper in + ``tests/integration_tests/dashboards/version_history_tests.py``. + """ + db.session.commit() + + +def _get_birth_names_dashboard() -> Dashboard: + return ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + + +class TestDashboardActivityView(SupersetTestCase): + """T017–T026 — ``GET /api/v1/dashboard/<uuid>/activity/`` (US1).""" + + @pytest.fixture(autouse=True) + def _load_data(self, load_birth_names_dashboard_with_slices): # noqa: PT004, F811 + pass + + def _activity(self, dashboard_uuid: str, **query: Any) -> Any: + return self.client.get( + f"/api/v1/dashboard/{dashboard_uuid}/activity/", + query_string=query, + ) + + # ---- 4xx boundary cases ---- + + def test_activity_returns_404_for_unknown_uuid(self) -> None: + """AV-009: unknown path entity → 404.""" + self.login(ADMIN_USERNAME) + rv = self._activity("00000000-0000-0000-0000-000000000000") + assert rv.status_code == 404 + + def test_activity_returns_400_for_invalid_uuid(self) -> None: + """A malformed UUID is rejected by the endpoint, not by Werkzeug.""" + self.login(ADMIN_USERNAME) + rv = self._activity("not-a-uuid") + assert rv.status_code == 400 + + def test_activity_returns_400_for_invalid_include(self) -> None: + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + self.login(ADMIN_USERNAME) + rv = self._activity(str(dashboard.uuid), include="sibling") + assert rv.status_code == 400 + + def test_activity_returns_400_for_invalid_since(self) -> None: + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + self.login(ADMIN_USERNAME) + rv = self._activity(str(dashboard.uuid), since="yesterday") + assert rv.status_code == 400 + + def test_activity_allows_read_non_owner(self) -> None: + """Activity is a read endpoint: a non-owner with read access (Alpha, + which carries broad read + datasource access) can read a dashboard's + activity stream — ``raise_for_access(dashboard=)`` does not reject — + so the endpoint returns 200. Visibility of *related* rows is filtered + separately, inside the activity layer.""" + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + + self.login(ALPHA_USERNAME) + rv = self._activity(dashboard_uuid) + assert rv.status_code == 200 + + def test_visibility_filter_silently_drops_inaccessible_related(self) -> None: + """AV-008 security control: a related record whose entity the caller + cannot read is *silently* dropped — absent from the result, no + placeholder, no contribution to count. Exercises the real + enforcement point (``filter_records_by_visibility`` → + ``DashboardAccessFilter``) with a restricted Gamma principal. + + Setup uses two dashboards (no datasource needed): one owned by + Gamma (readable), and an admin-owned one Gamma may not read. + """ + from superset import security_manager + from superset.subjects.utils import get_user_subject + from superset.versioning.activity.visibility import ( + filter_records_by_visibility, + ) + + admin = security_manager.find_user(ADMIN_USERNAME) + gamma = security_manager.find_user(GAMMA_USERNAME) + # Access control is subject-based (``editors``) since the Subject + # work replaced ``owners``; grant read via each user's Subject. + visible = Dashboard( + dashboard_title=f"vis-probe-owned {uuid4().hex[:8]}", + slug=f"vis-owned-{uuid4().hex[:8]}", + published=False, + editors=[get_user_subject(gamma.id)], + ) + hidden = Dashboard( + dashboard_title=f"vis-probe-hidden {uuid4().hex[:8]}", + slug=f"vis-hidden-{uuid4().hex[:8]}", + published=False, + editors=[get_user_subject(admin.id)], + ) + db.session.add_all([visible, hidden]) + db.session.commit() + visible_id, hidden_id = visible.id, hidden.id + try: + records = [ + {"entity_kind": "dashboard", "entity_id": visible_id}, + {"entity_kind": "dashboard", "entity_id": hidden_id}, + ] + self.login(GAMMA_USERNAME) + filtered = filter_records_by_visibility(records) + kept_ids = {r["entity_id"] for r in filtered} + + assert visible_id in kept_ids, ( + "gamma-owned dashboard must stay visible to Gamma" + ) + assert hidden_id not in kept_ids, ( + "unpublished admin-owned dashboard must be dropped for Gamma" + ) + # Silent: the dropped record leaves nothing behind (no placeholder). + assert len(filtered) == 1 + finally: + db.session.rollback() + for did in (visible_id, hidden_id): + obj = db.session.query(Dashboard).filter(Dashboard.id == did).first() + if obj is not None: + db.session.delete(obj) + db.session.commit() + + # ---- 200 happy paths ---- + + def test_activity_returns_200_with_envelope_shape(self) -> None: + """Smoke test: the endpoint returns the documented envelope shape + (``result`` list + ``count`` integer) even when the dashboard has + no activity yet.""" + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + + self.login(ADMIN_USERNAME) + rv = self._activity(dashboard_uuid) + assert rv.status_code == 200 + body = _json.loads(rv.data.decode("utf-8")) + assert "result" in body + assert "count" in body + assert isinstance(body["result"], list) + assert isinstance(body["count"], int) + + def test_activity_includes_chart_edit_as_related(self) -> None: + """T018 / AS-1 of US1: editing a chart on the dashboard surfaces + the chart-edit record with ``entity_kind=Slice`` and + ``source=related``.""" + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + chart_on_dashboard = next(iter(dashboard.slices), None) + assert chart_on_dashboard is not None + chart_id = chart_on_dashboard.id + original_name = chart_on_dashboard.slice_name + + try: + chart_on_dashboard.slice_name = f"{original_name} (edited)" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._activity(dashboard_uuid) + assert rv.status_code == 200 + body = _json.loads(rv.data.decode("utf-8")) + related = [ + r + for r in body["result"] + if r["entity_kind"] == "chart" and r["source"] == "related" + ] + assert related, ( + "Expected at least one Slice/related record from the chart " + "edit; got: " + f"{[(r['entity_kind'], r['source']) for r in body['result']]}" + ) + # Spot-check the carry-through of denormalized fields + sample = related[0] + assert sample["entity_uuid"] is not None + assert "transaction_id" in sample + assert "issued_at" in sample + finally: + db.session.rollback() + chart = db.session.query(Slice).filter(Slice.id == chart_id).one() + chart.slice_name = original_name + db.session.commit() + + def test_activity_include_self_excludes_related(self) -> None: + """T023 / AV-016: ``?include=self`` filters out related records.""" + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + chart_on_dashboard = next(iter(dashboard.slices), None) + assert chart_on_dashboard is not None + chart_id = chart_on_dashboard.id + original_name = chart_on_dashboard.slice_name + + try: + chart_on_dashboard.slice_name = f"{original_name} (edited self)" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._activity(dashboard_uuid, include="self") + assert rv.status_code == 200 + body = _json.loads(rv.data.decode("utf-8")) + for record in body["result"]: + assert record["source"] == "self", ( + f"include=self leaked a non-self record: {record}" + ) + assert record["entity_kind"] == "dashboard" + finally: + db.session.rollback() + chart = db.session.query(Slice).filter(Slice.id == chart_id).one() + chart.slice_name = original_name + db.session.commit() + + def test_activity_include_related_excludes_self(self) -> None: + """T024 / AV-016: ``?include=related`` returns only related records.""" + _persist_fixture_state() + dashboard = _get_birth_names_dashboard() + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + original_title = dashboard.dashboard_title + dashboard_id = dashboard.id + + try: + # Edit the dashboard's own field so we have a self record to + # filter out, and edit a chart on it so we have a related + # record to keep. + dashboard.dashboard_title = f"{original_title} (edited dash)" + db.session.commit() + chart_on_dashboard = next(iter(dashboard.slices), None) + assert chart_on_dashboard is not None + chart_id = chart_on_dashboard.id + chart_original_name = chart_on_dashboard.slice_name + chart_on_dashboard.slice_name = f"{chart_original_name} (edited chart)" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._activity(dashboard_uuid, include="related") + assert rv.status_code == 200 + body = _json.loads(rv.data.decode("utf-8")) + for record in body["result"]: + assert record["source"] == "related", ( + f"include=related leaked a self record: {record}" + ) + assert record["entity_kind"] != "dashboard" + finally: + db.session.rollback() + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + dashboard.dashboard_title = original_title + chart = db.session.query(Slice).filter(Slice.id == chart_id).one() + chart.slice_name = chart_original_name + db.session.commit() Review Comment: **Suggestion:** `chart_id` and `chart_original_name` are created inside the `try` block but used unconditionally in `finally`; if an earlier assertion/exception fires before assignment, cleanup raises `UnboundLocalError` and masks the real failure. Initialize these variables before `try` and guard the chart-restore cleanup when they are unavailable. [incorrect variable usage] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Activity-view dashboard test failures masked by cleanup UnboundLocalError. - ⚠️ Harder to diagnose fixture or data regressions in tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the dashboard activity tests, e.g. `pytest tests/integration_tests/versioning/activity_view_tests.py::TestDashboardActivityView::test_activity_include_related_excludes_self`, which exercises the block starting at line 290 in `tests/integration_tests/versioning/activity_view_tests.py`. 2. Inside `test_activity_include_related_excludes_self`, note that `chart_id` and `chart_original_name` are first assigned inside the `try:` block after the assertion at lines 305–307: - line 305: `chart_on_dashboard = next(iter(dashboard.slices), None)` - line 306: `assert chart_on_dashboard is not None` - line 307: `chart_id = chart_on_dashboard.id` - line 308: `chart_original_name = chart_on_dashboard.slice_name` 3. If the fixture or database state is ever such that `dashboard.slices` is empty (for example, the birth_names dashboard is created without slices or prior tests/fixture changesfail to attach any charts), the assertion at line 306 fails before `chart_id` and `chart_original_name` are assigned. 4. When that assertion (or any earlier exception in the `try:` block before line 307) fires, Python immediately enters the `finally:` block at lines 321–329, which unconditionally executes: - line 327: `chart = db.session.query(Slice).filter(Slice.id == chart_id).one()` - line 328: `chart.slice_name = chart_original_name` Because `chart_id` and/or `chart_original_name` were never bound, this raises `UnboundLocalError` in the cleanup, masking the original failure that actually caused the test to abort. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5110c51fecc9482cbf11b28589ac3691&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=5110c51fecc9482cbf11b28589ac3691&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:** tests/integration_tests/versioning/activity_view_tests.py **Line:** 321:329 **Comment:** *Incorrect Variable Usage: `chart_id` and `chart_original_name` are created inside the `try` block but used unconditionally in `finally`; if an earlier assertion/exception fires before assignment, cleanup raises `UnboundLocalError` and masks the real failure. Initialize these variables before `try` and guard the chart-restore cleanup when they are unavailable. 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=f2cc2cf06cca0430b8bd6ae4c5b49f884ac639a0cf789daacbabfcd02c0060ac&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=f2cc2cf06cca0430b8bd6ae4c5b49f884ac639a0cf789daacbabfcd02c0060ac&reaction=dislike'>👎</a> ########## tests/integration_tests/versioning/perf_validation_tests.py: ########## @@ -0,0 +1,416 @@ +# 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. +"""T044 — Performance validation for entity version history. + +Skipped by default. Run on demand: + + SUPERSET_PERF_VALIDATION=1 pytest \ + tests/integration_tests/versioning/perf_validation_tests.py -v -s + +Measures the three success criteria defined in the spec: + + * SC-002: version list endpoint responds in under 1 second + * SC-004: save path p95 overhead under 50 ms with Continuum tracking + on vs. off (FR-014) + +(SC-003, the restore-endpoint benchmark, lands with the restore endpoint +in a later PR.) + +The test prints a summary table suitable for pasting into the PR +description. It also asserts each target so regressions fail loudly +when the harness is re-run. +""" + +from __future__ import annotations + +import os +import statistics +import time +from typing import Any + +import pytest +import sqlalchemy as sa +from sqlalchemy_continuum import version_class, versioning_manager + +from superset.extensions import db +from superset.models.slice import Slice +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401 + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) + +SKIP_REASON = "Performance validation is manual. Set SUPERSET_PERF_VALIDATION=1 to run." + +# Thresholds from spec.md §Success Criteria. +LIST_ENDPOINT_MAX_MS = 1000 # SC-002 +SAVE_OVERHEAD_P95_MAX_MS = 50 # SC-004 + +# Activity-view thresholds (sc-107283 §Success Criteria). +ACTIVITY_ENDPOINT_P95_MAX_MS = 1500 # SC-AV-001 + + +def _save_chart_once(chart: Slice, suffix: str) -> None: + """One ORM-level save path, mimicking what ChartDAO.update does.""" + chart.slice_name = f"{chart.slice_name[:64]}_{suffix}" + db.session.commit() + + +def _timings_ms(seconds: list[float]) -> dict[str, float]: + ms = sorted(s * 1000.0 for s in seconds) + return { + "p50": statistics.median(ms), + "p95": ms[int(len(ms) * 0.95) - 1] if len(ms) >= 20 else max(ms), + "max": max(ms), + "n": len(ms), + } + + [email protected]( + not os.environ.get("SUPERSET_PERF_VALIDATION"), + reason=SKIP_REASON, +) +class PerfValidationTests(SupersetTestCase): + """Runs only when SUPERSET_PERF_VALIDATION=1 is set.""" + + @pytest.fixture(autouse=True) + def _load_data(self, load_birth_names_dashboard_with_slices: Any) -> None: # noqa: F811, PT004 + pass + + def _seed_chart_with_n_versions(self, n: int) -> Slice: + """Save a chart N times to produce N version rows.""" + chart = db.session.query(Slice).first() + assert chart is not None, "birth_names fixture should provide charts" + + for i in range(n): + _save_chart_once(chart, f"v{i}") + db.session.commit() + return chart + + def test_sc002_list_endpoint_under_1s(self) -> None: + """SC-002: list endpoint responds in under 1 second.""" + self.login(ADMIN_USERNAME) + + # Generate enough versions to exercise the retention-capped state. + chart = self._seed_chart_with_n_versions(24) + chart_uuid = str(chart.uuid) + url = f"/api/v1/chart/{chart_uuid}/versions/" + + # Warm up the endpoint once (JIT caching, mapper configuration, etc.) + self.client.get(url) + + timings: list[float] = [] + for _ in range(10): + t0 = time.perf_counter() + response = self.client.get(url) + timings.append(time.perf_counter() - t0) + assert response.status_code == 200 + + stats = _timings_ms(timings) + print( + f"\n[SC-002] GET /versions/ (24 versions) " + f"p50={stats['p50']:.1f}ms p95={stats['p95']:.1f}ms " + f"max={stats['max']:.1f}ms n={stats['n']}" + ) + assert stats["p95"] < LIST_ENDPOINT_MAX_MS, ( + f"SC-002 failed: list endpoint p95 {stats['p95']:.1f}ms " + f">= {LIST_ENDPOINT_MAX_MS}ms" + ) + + # SC-003 (restore endpoint under 3s) intentionally omitted here: the + # version-restore route (POST /<uuid>/versions/<version_uuid>/restore) + # ships in a later PR, so there is nothing to time at this point in the + # stack. Re-add this benchmark alongside the restore endpoint. + + def test_sc004_save_overhead_under_50ms(self) -> None: + """SC-004: save path p95 overhead under 50ms (FR-014). + + Toggling Continuum on and off mid-process corrupts its internal + ``units_of_work`` state and is not a reliable measurement. Instead + this test directly measures the wall-clock time spent inside the + four session-level listeners Continuum attaches to + ``sa.orm.session.Session`` — ``before_flush``, ``after_flush``, + ``after_commit``, ``after_rollback`` — plus Superset's own + baseline / snapshot / retention-prune listeners (attached to + ``db.session``). The cumulative listener time per save is the + marginal overhead version capture adds over a save with + versioning removed entirely, because without these listeners + the ORM would not execute any of that code. + + The approach: + 1. Wrap each known listener with a timing proxy that adds its + wall-clock time to a per-save accumulator. + 2. Save the same chart N times, recording each save's + accumulator value. + 3. Compute p50 / p95 of the per-save overhead. + + This matches the measurement intent of SC-004 (how much does + versioning cost per save) without the fragility of toggling + Continuum mid-test. + """ + self.login(ADMIN_USERNAME) + + chart = db.session.query(Slice).first() + assert chart is not None + + # Per-save accumulator incremented by the wrapped listeners. + acc = [0.0] + + def wrap_listener(original: Any) -> Any: + def wrapper(*args: Any, **kwargs: Any) -> Any: + t0 = time.perf_counter() + try: + return original(*args, **kwargs) + finally: + acc[0] += time.perf_counter() - t0 + + wrapper.__wrapped__ = original # type: ignore[attr-defined] + return wrapper + + # Instrument Continuum's four session listeners by detaching the + # bound method, wrapping, and re-attaching under a single-use + # listener handle we can cleanly remove on teardown. + session_target = sa.orm.session.Session + attached: list[tuple[str, Any]] = [] + for event_name, listener in list(versioning_manager.session_listeners.items()): + sa.event.remove(session_target, event_name, listener) + wrapped = wrap_listener(listener) + sa.event.listen(session_target, event_name, wrapped) + attached.append((event_name, wrapped)) Review Comment: **Suggestion:** The overhead meter wraps only `versioning_manager.session_listeners` on `sa.orm.session.Session`, but the Superset versioning listeners being benchmarked are registered on `db.session`. This undercounts save-path overhead and can produce false passes for SC-004/AV-SC-003. Wrap and time the `db.session` listeners as well (or switch to a measurement point that includes both listener groups) so the benchmark reflects real save overhead. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Perf harness underestimates save-path overhead from Superset listeners. - ⚠️ Performance regressions may slip past SC-004/AV-SC-003 checks. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the perf harness by setting `SUPERSET_PERF_VALIDATION=1` and run `pytest tests/integration_tests/versioning/perf_validation_tests.py::PerfValidationTests::test_sc004_save_overhead_under_50ms` (and similarly `::test_av_sc003_save_path_p95_unaffected_by_activity_view`), which execute the listener-wrapping code at lines 185–195 and 376–382 in `tests/integration_tests/versioning/perf_validation_tests.py`. 2. In `test_sc004_save_overhead_under_50ms`, observe the instrumentation block at lines 188–194: - line 188: `session_target = sa.orm.session.Session` - line 190: loop over `versioning_manager.session_listeners.items()` - lines 191–193: remove and re-attach each listener on `session_target` wrapped with `wrap_listener`, accumulating per-save overhead in `acc[0]`. This only wraps the Continuum listeners stored in `versioning_manager.session_listeners` on the `Session` class. 3. Separately, inspect `superset/versioning/changes/listener.py` at lines 428–542, where Superset’s own versioning listeners are registered directly on `db.session` (a scoped session), not on the `Session` class: - line 428: `def compute_change_records(session: Session, ...)` - line 521+: `event.listen(db.session, "before_flush", compute_change_records)` - line 522+: `event.listen(db.session, "after_flush", flush_change_records)` - line 523+: `event.listen(db.session, "after_commit", reset_processed_after_commit)` - line 524+: `event.listen(db.session, "after_rollback", reset_action_kind_after_rollback)` These functions run on every save and contribute to versioning overhead but are not part of `versioning_manager.session_listeners`. 4. When the perf tests run, both the Continuum listeners (wrapped and timed) and the Superset `db.session` listeners (unwrapped) execute on each commit. However, the accumulator `acc[0]` in `test_sc004_save_overhead_under_50ms` and `test_av_sc003_save_path_p95_unaffected_by_activity_view` only includes time spent in the wrapped `versioning_manager.session_listeners` attached to `sa.orm.session.Session`. Time spent in Superset’s `db.session` listeners (compute_change_records, flush_change_records, etc.) is completely omitted, so the reported p50/p95 “overhead” stats can be significantly lower than the true save-path overhead of the full versioning stack. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=984817fb15114bb3b789b3baa179f5ed&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=984817fb15114bb3b789b3baa179f5ed&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:** tests/integration_tests/versioning/perf_validation_tests.py **Line:** 188:194 **Comment:** *Logic Error: The overhead meter wraps only `versioning_manager.session_listeners` on `sa.orm.session.Session`, but the Superset versioning listeners being benchmarked are registered on `db.session`. This undercounts save-path overhead and can produce false passes for SC-004/AV-SC-003. Wrap and time the `db.session` listeners as well (or switch to a measurement point that includes both listener groups) so the benchmark reflects real save overhead. 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=ef0f646ea86c021ac275919180693ab982a7d59728d6f86e89dc3eca0a241355&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=ef0f646ea86c021ac275919180693ab982a7d59728d6f86e89dc3eca0a241355&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]
