bito-code-review[bot] commented on code in PR #43523:
URL: https://github.com/apache/superset/pull/43523#discussion_r4067424855
##########
tests/integration_tests/charts/api_tests.py:
##########
@@ -1227,6 +1230,209 @@ def test_get_chart_no_data_access(self):
rv = self.client.get(uri)
assert rv.status_code == 404
+ @with_feature_flags(THUMBNAILS=True)
+ @with_config({"THUMBNAIL_UPDATED_CACHE_TTL": 300})
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ @patch("superset.charts.api.cache_chart_thumbnail")
+ @patch("superset.charts.api.ChartScreenshot.get_from_cache_key")
+ @patch("superset.charts.api.ChartScreenshot.supports_updated_staleness",
True)
+ def test_thumbnail_does_not_recompute_stale_updated(
+ self, mock_get_from_cache_key, mock_cache_task
+ ):
+ """The card-list thumbnail path must never opt into updated-staleness
+ recompute. A force-less request whose cached UPDATED entry is older
than
+ THUMBNAIL_UPDATED_CACHE_TTL -- but still valid and correctly scoped --
+ must be served straight from cache (200), not rescheduled. This guards
+ against accidentally propagating ``check_updated_staleness`` to this
+ high-traffic card path (the way the on-demand ``cache_screenshot``
+ endpoint opts in): adding it here would enqueue stale thumbnails and
+ return 202 while preserving no cached image, failing this test.
+
+ ``ChartScreenshot.supports_updated_staleness`` is forced True here so
the
+ test actually proves the kwarg is absent: were the card path to copy
+ ``check_updated_staleness=screenshot_obj.supports_updated_staleness``
the
+ flag would evaluate True and the stale entry would be rescheduled,
+ failing the assertions below. Because the card path calls
+ ``should_trigger_task()`` with no kwarg, they still hold."""
+ from datetime import datetime, timedelta
+
+ self.login(ADMIN_USERNAME)
+
+ chart = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ # A valid, correctly-scoped UPDATED entry, but 400s old against a 300s
TTL.
+ # Naive to match the cache's naive `datetime.now()` timestamps: a
tz-aware
+ # value here would either be read as future on a UTC-ahead host or
raise
+ # when subtracted from naive now().
+ stale_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake image data",
+ scope=f"chart:{chart.id}",
+ timestamp=stale_timestamp,
+ )
+
+ # Resolve the digest under the requesting user so the endpoint serves
the
+ # entry instead of redirecting to the canonical digest:
THUMBNAIL_EXECUTORS
+ # defaults to CURRENT_USER, so the digest is only resolvable with a
user in
+ # context.
+ with override_user(self.get_user(ADMIN_USERNAME)):
+ digest = chart.digest
+
+ rv = self.client.get(f"api/v1/chart/{chart.id}/thumbnail/{digest}/")
+
+ assert rv.status_code == 200
+ mock_cache_task.delay.assert_not_called()
+ assert rv.data == b"fake image data"
+
+ @with_feature_flags(THUMBNAILS=True)
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ @patch("superset.charts.api.ChartScreenshot.get_from_cache_key")
+ def test_screenshot_serves_retained_image_on_error(self,
mock_get_from_cache_key):
+ """A failed forced refresh leaves the on-demand screenshot entry in an
+ ERROR backoff while still carrying the retained last-good image. The
+ screenshot read path must serve those retained bytes (200) rather than
+ 404 for the length of the backoff -- gating serve on status == UPDATED
+ would reject the still-valid image for up to a day."""
+ self.login(ADMIN_USERNAME)
+
+ chart = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ # A valid, correctly-scoped image whose entry is in ERROR backoff.
+ payload = ScreenshotCachePayload(
+ b"fake image data",
+ scope=f"chart:{chart.id}",
+ )
+ payload.status = StatusValues.ERROR
+ mock_get_from_cache_key.return_value = payload
+
+ # Resolve the digest under the requesting user, mirroring the thumbnail
+ # test: THUMBNAIL_EXECUTORS defaults to CURRENT_USER.
+ with override_user(self.get_user(ADMIN_USERNAME)):
+ digest = chart.digest
+
+ rv = self.client.get(f"api/v1/chart/{chart.id}/screenshot/{digest}/")
+
+ assert rv.status_code == 200
+ assert rv.data == b"fake image data"
+
+ @with_feature_flags(THUMBNAILS=True)
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ @patch("superset.charts.api.cache_chart_thumbnail")
+ @patch("superset.charts.api.ChartScreenshot.cache")
+ @patch("superset.charts.api.ChartScreenshot.get_from_cache_key")
+ def test_cache_screenshot_forceless_leaves_worker_able_to_render(
+ self, mock_get_from_cache_key, mock_cache, mock_cache_task
+ ):
+ """Regression: a force-less ``cache_screenshot`` that triggers a render
+ must not leave the cache in a state that makes the worker skip. The
worker
+ (``cache_chart_thumbnail`` -> ``compute_and_cache``) re-reads the same
+ cache key and re-runs ``should_trigger_task(force=False,
+ check_updated_staleness=False)``. If the endpoint pre-wrote a fresh
+ COMPUTING entry, that re-check would see a non-stale COMPUTING entry,
skip
+ the render, and the screenshot would never be computed -- churning
+ COMPUTING every ``THUMBNAIL_COMPUTING_CACHE_TTL`` forever. ``force`` is
+ None when omitted (no schema default), so this is the ordinary
caller."""
+ self.login(ADMIN_USERNAME)
+
+ chart = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ # First render: cache miss -> a fresh PENDING payload triggers the
task.
+ mock_get_from_cache_key.return_value = None
+
+ rv = self.client.get(
+ f"api/v1/chart/{chart.id}/cache_screenshot/?q={rison.dumps({})}"
+ )
+
+ assert rv.status_code == 202
+ mock_cache_task.delay.assert_called_once()
+ # Force is falsy (omitted), so the worker relies on its own re-check.
+ assert mock_cache_task.delay.call_args.kwargs["force"] in (None, False)
+
+ # Whatever (if anything) the endpoint persisted, the worker will
re-read
+ # it; otherwise it re-reads the still-empty cache (a fresh PENDING).
Its
+ # own force-less gate MUST still fire, or the screenshot never renders.
+ if mock_cache.set.called:
+ worker_view = ScreenshotCachePayload.from_dict(
+ mock_cache.set.call_args[0][1]
+ )
+ else:
+ worker_view = ScreenshotCachePayload()
+ assert worker_view.should_trigger_task(
+ force=False,
+ expected_scope=f"chart:{chart.id}",
+ check_updated_staleness=False,
+ ), "endpoint left the cache in a state that makes the worker skip
render"
+
+ @with_feature_flags(THUMBNAILS=True)
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ @patch("superset.charts.api.cache_chart_thumbnail")
+ @patch("superset.charts.api.ChartScreenshot.cache")
+ @patch("superset.charts.api.ChartScreenshot.get_from_cache_key")
+ def test_cache_screenshot_retry_preserves_retained_image(
+ self, mock_get_from_cache_key, mock_cache, mock_cache_task
+ ):
+ """A force-less on-demand ``cache_screenshot`` that re-triggers a
render
+ after a prior failure (an ERROR entry past its TTL, still carrying the
+ last-good image) must (a) not wipe that image and (b) leave the entry
+ triggerable so the worker actually re-renders. The endpoint leaves the
+ entry untouched (mirroring the dashboard endpoint): the read path keeps
+ serving the retained image while the worker -- which itself flips the
+ entry to COMPUTING without discarding the image -- runs the retry."""
+ from datetime import datetime, timedelta
+
+ self.login(ADMIN_USERNAME)
+
+ chart = (
+ db.session.query(Slice)
+ .filter_by(slice_name="Girl Name Cloud")
+ .one_or_none()
+ )
+ # A retained, valid, correctly-scoped image whose entry is in ERROR
+ # backoff and 2 days old, past the 1-day THUMBNAIL_ERROR_CACHE_TTL, so
a
+ # force-less request re-triggers via the expired-ERROR branch.
+ stale_timestamp = (datetime.now() - timedelta(days=2)).isoformat()
+ payload = ScreenshotCachePayload(
+ b"fake image data",
+ scope=f"chart:{chart.id}",
+ timestamp=stale_timestamp,
+ )
+ payload.status = StatusValues.ERROR
+ mock_get_from_cache_key.return_value = payload
+
+ rv = self.client.get(
+ f"api/v1/chart/{chart.id}/cache_screenshot/?q={rison.dumps({})}"
+ )
+
+ # Trigger fires: the task is enqueued and the endpoint returns 202.
+ assert rv.status_code == 202
+ mock_cache_task.delay.assert_called_once()
+
+ # The endpoint must not overwrite the entry with an imageless payload.
+ # It leaves it untouched, so the worker re-reads the retained image and
+ # its force-less gate still fires (expired ERROR).
+ if mock_cache.set.called:
+ worker_view = ScreenshotCachePayload.from_dict(
+ mock_cache.set.call_args[0][1]
+ )
+ else:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Failing test assertion</b></div>
<div id="fix">
`assert worker_view.get_invalid_image_reason() is None` will fail: the
payload image is `b"fake image data"`, which `validate_screenshot_image`
(superset/utils/screenshots.py:92-102) rejects as `"undecodable"` because it
lacks PNG/JPEG magic bytes. Since `cache_screenshot` never calls `cache.set`,
the `else` branch keeps this payload, so the assertion deterministically fails.
Use real PNG/JPEG-prefixed bytes.
</div>
</div>
<small><i>Code Review Run #8124d5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
