eschutho opened a new pull request, #42916: URL: https://github.com/apache/superset/pull/42916
### Summary Production logs show a recurring deprecation warning: `The 'md5' HASH_ALGORITHM is deprecated and retained only for backwards compatibility; prefer 'sha256' for namespace generation.` (`superset/key_value/utils.py:94`), firing on every call instead of only during a one-time migration. ### Root cause `get_shared_value()` looks up a value under the current hash-algorithm namespace and, on a miss, falls back to legacy algorithms (`HASH_ALGORITHM_FALLBACKS`, default `["md5"]`). On a fallback hit, it's designed to migrate the entry to the current algorithm's UUID via `KeyValueDAO.create_entry(...)` so future lookups skip the deprecated fallback entirely. `KeyValueDAO.create_entry()` only does `db.session.add(entry)` — it relies on the caller to commit. `get_shared_value()`, unlike its siblings `set_shared_value()` / `upsert_shared_value()` in the same file, was never wrapped in the `@transaction()` decorator that performs that commit. So the migration write was silently discarded on every call, and every subsequent lookup re-triggered the deprecated md5 fallback path forever instead of migrating once. ### Change Add `@transaction()` to `get_shared_value()`, matching the existing pattern already used by `set_shared_value()` and `upsert_shared_value()` in the same file. One line, additive only — no other logic changed. **No behavior change** for the read path or for callers; this only makes the already-intended migration write actually persist. **Disclosure note**: `get_shared_value()` is called standalone (outside any `@transaction()`) from the guest-token auth path (`SecurityManager.get_guest_user_from_request` → `_is_guest_token_revoked` → `get_current_guest_token_revocation_version`). After this fix, a fallback hit on that path now commits the *entire* current DB session, not just the migration write — the same "commits whatever's pending" exposure `set_shared_value()`/`upsert_shared_value()` have already had in this file for years, now extended to this function too. In practice this call happens early in request handling before other writes are queued, so no realistic unrelated-write risk, but flagging for reviewer awareness. ### Decisions made that were not in the instructions None. ### Test plan - New regression test `test_get_shared_value_commits_migration_to_current_algorithm` in `tests/unit_tests/key_value/test_shared_entries_migration.py`: mocks `superset.db.session.commit` and asserts it's called once when a fallback hit triggers the migration write. Verified it fails pre-fix (`AssertionError: Expected 'commit' to have been called once. Called 0 times.`) and passes post-fix. - Full `tests/unit_tests/key_value/` suite: 45/45 pass. - `ruff check` + `ruff format --check`: clean. - `mypy` on the changed file: zero errors attributable to it (pre-existing project-wide baseline noise only, unrelated). -- 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]
