bito-code-review[bot] commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4100511926
##########
superset/config.py:
##########
@@ -3545,6 +3639,28 @@ def _config_fingerprint(source: bytes | None) -> str:
logger.exception("Found but failed to import local superset_config")
raise
+if _canonical_history_retention_override:
+ VERSION_HISTORY_RETENTION_DAYS = _normalize_version_history_retention_days(
+ _canonical_history_retention_value, legacy=False
+ )
+if (
+ _legacy_history_retention_override
+ and "VERSION_HISTORY_RETENTION_DAYS" not in os.environ
+):
+ _legacy_history_retention_days: int =
_normalize_version_history_retention_days(
+ _legacy_history_retention_value, legacy=True
+ )
+ if not _canonical_history_retention_override:
+ VERSION_HISTORY_RETENTION_DAYS = _legacy_history_retention_days
+ elif VERSION_HISTORY_RETENTION_DAYS == _version_history_retention_seed:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Legacy deprecation warning skipped</b></div>
<div id="fix">
When a config file sets both keys and the canonical value differs from
`_version_history_retention_seed`, this `elif` skips normalization of
`_legacy_history_retention_value` entirely — so the deprecation warning for
`SUPERSET_VERSION_HISTORY_RETENTION_DAYS` (lines 1853-1858) never fires, unlike
the legacy-only path (3653-3654) which always warns. The legacy key is silently
ignored. Normalize/warn for the legacy key before the seed comparison.
</div>
</div>
<small><i>Code Review Run #4a899c</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/config.py:
##########
@@ -1794,31 +1826,59 @@ class ExportStorageConfig(TypedDict, total=False):
def _parse_version_history_retention_days() -> int:
"""Parse the retention window without making invalid input fatal."""
- value: str | None =
os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ value: str | None = os.environ.get("VERSION_HISTORY_RETENTION_DAYS")
+ legacy: bool = False
+ if value is None:
+ value = os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ legacy = value is not None
if value is None:
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ return _normalize_version_history_retention_days(value, legacy=legacy)
+
+
+def _normalize_version_history_retention_days(value: object, *, legacy: bool)
-> int:
+ """Normalize released legacy values without shortening retention on
upgrade."""
+ name: str = (
+ "SUPERSET_VERSION_HISTORY_RETENTION_DAYS"
+ if legacy
+ else "VERSION_HISTORY_RETENTION_DAYS"
+ )
try:
- retention_days = int(value)
+ if isinstance(value, bool) or not isinstance(value, (str, int)):
+ raise ValueError("Retention must be integer days")
+ retention_days: int = int(value)
except ValueError:
+ logger.warning("Invalid %s=%r; skipping pruning", name, value)
+ return 0
+ if legacy:
logger.warning(
- "Invalid SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
- value,
- _DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
+ "%s is deprecated; use VERSION_HISTORY_RETENTION_DAYS. "
+ "Legacy nonpositive values disable pruning.",
+ name,
)
- return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ if retention_days <= 0:
+ return 0
+ if retention_days < -1:
+ logger.warning("Invalid negative %s; skipping pruning", name)
+ return 0
if retention_days > _MAX_VERSION_HISTORY_RETENTION_DAYS:
logger.warning(
- "SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r exceeds the maximum "
- "of %d; using %d",
+ "%s=%r exceeds the maximum of %d; skipping pruning",
+ name,
value,
_MAX_VERSION_HISTORY_RETENTION_DAYS,
- _DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
)
- return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ return 0
+ if retention_days == -1:
+ logger.warning(
+ "VERSION_HISTORY_RETENTION_DAYS=-1 makes history eligible for "
+ "immediate pruning on the next scheduled run; use 0 to disable"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-706: Wrong var name in warning</b></div>
<div id="fix">
This warning hardcodes `VERSION_HISTORY_RETENTION_DAYS`, but
`_normalize_version_history_retention_days` also runs for the legacy
`SUPERSET_VERSION_HISTORY_RETENTION_DAYS` env var (lines 1832-1833) and for
legacy config overrides (line 3650), where `name` is the legacy key. A legacy
user setting -1 gets a warning naming a variable they never set. Use `name`
like the other warnings do.
([CWE-706](https://cwe.mitre.org/data/definitions/706.html))
</div>
</div>
<small><i>Code Review Run #4a899c</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]
