fix: commit migration write in get_shared_value to stop recurring md5 deprecation warning - #42916
fix: commit migration write in get_shared_value to stop recurring md5 deprecation warning#42916eschutho wants to merge 1 commit into
Conversation
… deprecation warning get_shared_value() falls back to legacy hash algorithms and is designed to migrate matched entries to the current algorithm's UUID via KeyValueDAO.create_entry(), but the write was never committed: unlike its siblings set_shared_value()/upsert_shared_value(), it wasn't wrapped in the @transaction() decorator. The migration was silently discarded every call, so the deprecated md5 fallback path fired on every subsequent lookup instead of migrating once. Add @transaction() to get_shared_value(), matching the existing pattern used by its siblings in the same file. Fixes the recurring "The 'md5' HASH_ALGORITHM is deprecated..." warning seen in production logs (superset/key_value/utils.py:94).
Code Review Agent Run #5b5cb4Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42916 +/- ##
==========================================
- Coverage 66.42% 66.41% -0.01%
==========================================
Files 2857 2857
Lines 161293 161294 +1
Branches 37134 37134
==========================================
- Hits 107133 107131 -2
- Misses 52135 52137 +2
- Partials 2025 2026 +1
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| CODEC = JsonKeyValueCodec() | ||
|
|
||
|
|
||
| @transaction() |
There was a problem hiding this comment.
Suggestion: The transaction decorator makes this read helper commit every pending change in the current SQLAlchemy session whenever it is called outside an existing transaction. Read-only callers such as guest-token validation and retention-window resolution can therefore persist unrelated ORM mutations unexpectedly. Move the migration write into an explicitly scoped transaction or otherwise avoid committing unrelated session state from this read API. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Guest-token validation can commit unrelated session mutations.
- ⚠️ Retention-window reads can persist pending ORM changes.
- ❌ Partial request state may become durable unexpectedly.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/key_value/shared_entries.py
**Line:** 38:38
**Comment:**
*Api Mismatch: The transaction decorator makes this read helper commit every pending change in the current SQLAlchemy session whenever it is called outside an existing transaction. Read-only callers such as guest-token validation and retention-window resolution can therefore persist unrelated ORM mutations unexpectedly. Move the migration write into an explicitly scoped transaction or otherwise avoid committing unrelated session state from this read API.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| mock_dao.create_entry.assert_called_once() | ||
| mock_commit.assert_called_once() |
There was a problem hiding this comment.
Suggestion: The regression test only checks that create_entry and commit were called; it does not verify that the migration uses the current-algorithm UUID, the expected resource, value, and codec. An implementation that writes under the legacy UUID would still pass, so the test does not validate the migration behavior it claims to cover. Assert the complete create_entry call, including the expected current UUID. [possible bug]
Severity Level: Minor 🧹
- ⚠️ Migration regression coverage accepts an incorrect destination UUID.
- ⚠️ Future deprecated-algorithm lookups could continue indefinitely.
- ⚠️ Resource, value, or codec regressions remain undetected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/key_value/test_shared_entries_migration.py
**Line:** 179:180
**Comment:**
*Possible Bug: The regression test only checks that `create_entry` and `commit` were called; it does not verify that the migration uses the current-algorithm UUID, the expected resource, value, and codec. An implementation that writes under the legacy UUID would still pass, so the test does not validate the migration behavior it claims to cover. Assert the complete `create_entry` call, including the expected current UUID.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix1 participant
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 viaKeyValueDAO.create_entry(...)so future lookups skip the deprecated fallback entirely.KeyValueDAO.create_entry()only doesdb.session.add(entry)— it relies on the caller to commit.get_shared_value(), unlike its siblingsset_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()toget_shared_value(), matching the existing pattern already used byset_shared_value()andupsert_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" exposureset_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
test_get_shared_value_commits_migration_to_current_algorithmintests/unit_tests/key_value/test_shared_entries_migration.py: mockssuperset.db.session.commitand 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.tests/unit_tests/key_value/suite: 45/45 pass.ruff check+ruff format --check: clean.mypyon the changed file: zero errors attributable to it (pre-existing project-wide baseline noise only, unrelated).