Repository navigation
fix: downgrade md5 namespace-fallback deprecation log to info - #43884
Conversation
_uuid_namespace_from_md5 fires on every legacy key-value/permalink lookup that falls back to the deprecated md5 HASH_ALGORITHM. For the permalink path this repeats indefinitely until each legacy entry is migrated; apache#42916 already fixed the get_shared_value path to migrate on first hit. Either way, expected fallback behavior doesn't warrant WARNING severity. Mirrors the same downgrade already applied to query_object.py's deprecated-field logging (apache#43520). Test pins the exact log level so a regression back to warning is caught.
Code Review Agent Run #b1aaceActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@rebenitez1802 assigning you as reviewer on this one — you approved the closely-related #42916 (the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43884 +/- ##
===========================================
+ Coverage 66.42% 78.65% +12.23%
===========================================
Files 2857 2871 +14
Lines 161293 165784 +4491
Branches 37134 37976 +842
===========================================
+ Hits 107133 130392 +23259
+ Misses 52135 32893 -19242
- Partials 2025 2499 +474
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve: correct, low-risk severity downgrade with a properly tightened test — no blocking issues. Verified against the code: both call sites behave as described (permalink create never migrates the legacy entry so it re-logs indefinitely; get_shared_value persists a migrated sha256 entry so it logs at most once), the ["md5"] fallback default is confirmed, and nothing in-repo keys off this message at WARNING level.
A few optional, non-blocking notes:
-
The log fires on the md5 probe, not on an actual legacy hit.
_uuid_namespace_from_md5(superset/key_value/utils.py:94) logs whenever the md5 namespace is computed, which in the permalink path happens on the unconditional fallback probe (superset/commands/dashboard/permalink/create.py:87-91) before any lookup. So on a stock install every first-time permalink creation emits this even when no md5 data exists, and the message namesHASH_ALGORITHM(which issha256) while the real trigger isHASH_ALGORITHM_FALLBACKS. Pre-existing and arguably out of scope, but since these exact lines are being edited it'd be cheap to fire only on an actual fallback hit and/or reword to name the fallback list. -
INFO vs DEBUG is a judgment call. The "expected, indefinitely repeating" rationale is close to DEBUG; many deployments run the root logger at INFO, so INFO still adds steady non-actionable volume. INFO is defensible and matches the
#42927precedent — just flagging the alternative. -
Precedent citation:
#43520isn't merged tomasteryet (superset/common/query_object.pystill useslogger.warningthere); the landed WARNING→INFO analogue with the same rationale is#42927(query_object.py:295-305). Worth citing that instead so the precedent is verifiable. -
Test: the combined
levelno == logging.INFO and "deprecated" in …predicate is airtight against a regression back towarning. Optional: add a negative guard asserting no WARNING-level record for the message to pin intent explicitly.
2 participants
Decisions made that were not in the instructions
None.
What
Datadog prod logs show recurring warning-level noise:
The 'md5' HASH_ALGORITHM is deprecated and retained only for backwards compatibility; prefer 'sha256' for namespace generation.from_uuid_namespace_from_md5(superset/key_value/utils.py).This fires via
get_uuid_namespace_with_algorithm(seed, "md5")from two fallback-lookup call sites wheneverHASH_ALGORITHM_FALLBACKS(default["md5"]) resolves a legacy pre-SHA-256 entry:superset/commands/dashboard/permalink/create.py:CreateDashboardPermalinkCommand.run-- a fallback hit does not migrate the legacy entry, so every future access of that permalink re-hits the md5 fallback and re-logs. Still indefinite.superset/key_value/shared_entries.py:get_shared_value-- as of fix: commit migration write in get_shared_value to stop recurring md5 deprecation warning #42916 (merged 2026-08-12), a fallback hit now persists a migrated current-algorithm entry, so this path logs at most once per legacy entry, not indefinitely.Either way, this is expected fallback behavior, not a one-off actionable event -- warning-level logging is the wrong severity for the repeat case.
Change
Downgrade the single
logger.warningcall in_uuid_namespace_from_md5tologger.info. No other behavior change -- the md5 hashing/namespace-generation logic itself is untouched. Mirrors the identical downgrade already applied toQueryObject's deprecated-field warnings in #43520 for the same "expected, indefinite repeat" reasoning.Test plan
tests/unit_tests/key_value/utils_test.py(renamedtest_uuid_namespace_from_md5_warns->test_uuid_namespace_from_md5_logs_deprecation_at_info) to assertrecord.levelno == logging.INFOexplicitly, not just message content. Verified it fails if reverted tologger.warningand passes on this fix.uvx ruff@0.9.7 checkandformat --checkclean on both changed files.pytest tests/unit_tests/key_value/utils_test.py-- 24 passed.