bito-code-review[bot] commented on code in PR #44144:
URL: https://github.com/apache/superset/pull/44144#discussion_r4064648825
##########
superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts:
##########
@@ -74,8 +104,12 @@ export const useDownloadScreenshot = (
const downloadScreenshot = useCallback(
(format: DownloadScreenshotFormat) => {
let retries = 0;
+ let maxRetries = Math.ceil(
+ (DEFAULT_SCREENSHOT_TASK_TIMEOUT_SECONDS * 1000) / RETRY_INTERVAL,
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated retry math</b></div>
<div id="fix">
The `Math.ceil((timeoutSeconds * 1000) / RETRY_INTERVAL)` computation is
duplicated at lines 107-109 (default) and 231-233 (backend
`task_timeout_seconds`). If the formula changes (e.g. adding a buffer or
rounding policy), the two can diverge. Extract a small helper
`computeMaxRetries(timeoutSeconds)` and call it from both sites.
</div>
</div>
<small><i>Code Review Run #83ad27</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
##########
tests/unit_tests/utils/screenshot_test.py:
##########
@@ -77,6 +107,193 @@ def test_get_screenshot(mocker: MockerFixture,
screenshot_obj):
assert screenshot_data == fake_bytes
+def test_complete_dashboard_capture_uses_internal_driver_policy() -> None:
+ screenshot = DashboardScreenshot(
+ "http://example.com";,
+ "digest",
+ require_complete_capture=True,
+ )
+
+ driver = screenshot.driver()
+
+ assert isinstance(driver, WebDriverPlaywright)
+ assert driver._require_complete_capture is True # pylint:
disable=protected-access
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Private attribute coupling</b></div>
<div id="fix">
Reaching into `driver._require_complete_capture` couples the test to a
private attribute of `WebDriverPlaywright` (webdriver.py:198). A rename or
refactor of the private field breaks this test even when behavior is unchanged;
asserting the public contract (e.g. via patched constructor kwargs) is more
robust.
</div>
</div>
<small><i>Code Review Run #83ad27</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
##########
superset/extensions/metastore_cache.py:
##########
@@ -111,6 +111,81 @@ def add(self, key: str, value: Any, timeout: Optional[int]
= None) -> bool:
db.session.rollback() # pylint: disable=consider-using-transaction
return False
+ def compare_and_set(
+ self,
+ key: str,
+ value: Any,
+ expected: Any | None,
+ timeout: Optional[int] = None,
+ ) -> bool:
+ """Atomically replace ``expected`` at ``key`` with ``value``.
+
+ ``None`` represents an absent or expired entry. Existing values are
+ replaced with one conditional SQL update, including on SQLite where
+ ``SELECT FOR UPDATE`` is unavailable. Concurrent attempts to create an
+ absent entry are serialized by the key's unique constraint.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.key_value import KeyValueDAO
+ from superset.key_value.models import KeyValueEntry
+ from superset.utils.core import get_user_id
+
+ try:
+ cache_key = self.get_key(key)
+ now = datetime.now()
+ expires_on = self._get_expiry(timeout)
+ updates = {
+ KeyValueEntry.value: self.codec.encode(value),
+ KeyValueEntry.expires_on: expires_on,
+ KeyValueEntry.changed_on: now,
+ KeyValueEntry.changed_by_fk: get_user_id(),
+ }
+ query = db.session.query(KeyValueEntry).filter_by(
+ resource=RESOURCE.value,
+ uuid=cache_key,
+ )
+
+ if expected is not None:
+ updated = query.filter(
+ KeyValueEntry.value == self.codec.encode(expected),
+ (
+ KeyValueEntry.expires_on.is_(None)
+ | (KeyValueEntry.expires_on > now)
+ ),
+ ).update(updates, synchronize_session=False)
+ if updated != 1:
+ db.session.rollback() # pylint:
disable=consider-using-transaction
+ return False
+ else:
+ # Reuse an expired row when present; otherwise the unique key
+ # makes the insert below an atomic create-if-absent operation.
+ updated = query.filter(
+ KeyValueEntry.expires_on.is_not(None),
+ KeyValueEntry.expires_on <= now,
+ ).update(updates, synchronize_session=False)
+ if updated != 1:
+ KeyValueDAO.create_entry(
+ resource=RESOURCE,
+ key=cache_key,
+ value=value,
+ codec=self.codec,
+ expires_on=expires_on,
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CAS create skips audit fields</b></div>
<div id="fix">
`KeyValueDAO.create_entry` sets only `created_on`/`created_by_fk` — never
`changed_on`/`changed_by_fk` — so rows created by the `expected is None` branch
get NULL audit fields, while the update branches (lines 137-142) populate both.
Assign `entry.changed_on = now` and `entry.changed_by_fk = get_user_id()` on
the returned entry before `db.session.commit()`.
</div>
</div>
<small><i>Code Review Run #83ad27</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]
