bito-code-review[bot] commented on code in PR #34525:
URL: https://github.com/apache/superset/pull/34525#discussion_r2771942217
##########
superset/tasks/cache.py:
##########
@@ -307,25 +229,32 @@ def cache_warmup(
logger.exception(message)
return message
- results: dict[str, list[str]] = {"scheduled": [], "errors": []}
- for task in strategy.get_tasks():
- username = task["username"]
- payload = json.dumps(task["payload"])
- if username:
+ results: dict[str, list[str]] = {"success": [], "errors": []}
+
+ user = security_manager.find_user(
+ username=current_app.config["SUPERSET_CACHE_WARMUP_USER"]
+ )
+ if not user:
+ message = (
+ f"Cache warmup user
'{current_app.config['SUPERSET_CACHE_WARMUP_USER']}' "
+ "not found. Please configure SUPERSET_CACHE_WARMUP_USER with a
valid username."
+ )
+ logger.error(message)
+ return message
+
+ wd = WebDriverSelenium(current_app.config["WEBDRIVER_TYPE"], user=user)
+
+ try:
+ for url in strategy.get_urls():
try:
- user = security_manager.get_user_by_username(username)
- cookies = MachineAuthProvider.get_auth_cookies(user)
- headers = {
- "Cookie": "session=%s" % cookies.get("session", ""),
- "Content-Type": "application/json",
- }
- logger.info("Scheduling %s", payload)
- fetch_url.delay(payload, headers)
- results["scheduled"].append(payload)
- except SchedulingError:
- logger.exception("Error scheduling fetch_url for payload: %s",
payload)
- results["errors"].append(payload)
- else:
- logger.warning("Executor not found for %s", payload)
+ logger.info("Fetching %s", url)
+ wd.get_screenshot(url, "grid-container")
+ results["success"].append(url)
+ except URLError:
+ logger.exception("Error warming up cache!")
+ results["errors"].append(url)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Error Handling Bug</b></div>
<div id="fix">
The except block catches URLError, but wd.get_screenshot() raises exceptions
like TimeoutException, WebDriverException, and StaleElementReferenceException.
This can cause unhandled exceptions, failing the cache warmup task. Consider
catching specific WebDriver exceptions for precision.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except Exception:
logger.exception("Error warming up cache!")
results["errors"].append(url)
````
</div>
</details>
</div>
<small><i>Code Review Run #b0b0be</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/utils/webdriver.py:
##########
@@ -399,6 +402,29 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
class WebDriverSelenium(WebDriverProxy):
+ def __init__(
+ self,
+ driver_type: str,
+ window: WindowSize | None = None,
+ user: User | None = None,
+ ):
+ super().__init__(driver_type, window)
+ self._user = user
+ self._driver: WebDriver | None = None
+
+ def __del__(self) -> None:
+ self._destroy()
+
+ @property
+ def driver(self) -> WebDriver:
+ if not self._driver:
+ self._driver = self._create()
+ assert self._driver # for mypy
+ self._driver.set_window_size(*self._window)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Use of assert statement detected</b></div>
<div id="fix">
Replace `assert` with explicit error handling or validation. Use `if not
self._driver: raise RuntimeError(...)` instead of `assert`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if not self._driver:
self._driver = self._create()
if not self._driver:
raise RuntimeError("Failed to create webdriver instance")
self._driver.set_window_size(*self._window)
````
</div>
</details>
</div>
<small><i>Code Review Run #b0b0be</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/tasks/cache.py:
##########
@@ -307,25 +229,32 @@ def cache_warmup(
logger.exception(message)
return message
- results: dict[str, list[str]] = {"scheduled": [], "errors": []}
- for task in strategy.get_tasks():
- username = task["username"]
- payload = json.dumps(task["payload"])
- if username:
+ results: dict[str, list[str]] = {"success": [], "errors": []}
+
+ user = security_manager.find_user(
+ username=current_app.config["SUPERSET_CACHE_WARMUP_USER"]
+ )
+ if not user:
+ message = (
+ f"Cache warmup user
'{current_app.config['SUPERSET_CACHE_WARMUP_USER']}' "
+ "not found. Please configure SUPERSET_CACHE_WARMUP_USER with a
valid username."
+ )
+ logger.error(message)
+ return message
+
+ wd = WebDriverSelenium(current_app.config["WEBDRIVER_TYPE"], user=user)
+
+ try:
+ for url in strategy.get_urls():
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Try-except block within loop</b></div>
<div id="fix">
Consider moving the try-except block outside the loop or restructuring to
avoid performance overhead from exception handling within the loop.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
try:
+ urls = strategy.get_urls()
+ except Exception as e:
+ logger.exception("Error getting URLs from strategy!")
+ return f"Error getting URLs: {str(e)}"
+
+ try:
for url in urls:
- try:
- logger.info("Fetching %s", url)
- wd.get_screenshot(url, "grid-container")
- results["success"].append(url)
- except URLError:
- logger.exception("Error warming up cache!")
- results["errors"].append(url)
+ logger.info("Fetching %s", url)
+ try:
+ wd.get_screenshot(url, "grid-container")
+ results["success"].append(url)
+ except URLError:
+ logger.exception("Error warming up cache!")
+ results["errors"].append(url)
finally:
```
</div>
</details>
</div>
<small><i>Code Review Run #b0b0be</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]
