codeant-ai-for-open-source[bot] commented on code in PR #42916: URL: https://github.com/apache/superset/pull/42916#discussion_r3741055715
########## superset/key_value/shared_entries.py: ########## @@ -35,6 +35,7 @@ CODEC = JsonKeyValueCodec() +@transaction() Review 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] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Guest-token validation can commit unrelated session mutations. - ⚠️ Retention-window reads can persist pending ORM changes. - ❌ Partial request state may become durable unexpectedly. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2ceb6f84fc34d1eaf95be28d1df63f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2ceb6f84fc34d1eaf95be28d1df63f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42916&comment_hash=25696b0b617f069b26c79717143cfcaeab0df610a1efd313daf1c2961c0a72c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42916&comment_hash=25696b0b617f069b26c79717143cfcaeab0df610a1efd313daf1c2961c0a72c7&reaction=dislike'>👎</a> ########## tests/unit_tests/key_value/test_shared_entries_migration.py: ########## @@ -140,6 +140,46 @@ def test_upsert_shared_value_overwrites_existing_value() -> None: assert last_call.args[3] == expected_uuid +def test_get_shared_value_commits_migration_to_current_algorithm() -> None: + """A fallback hit migrates the entry to the current algorithm and commits it, + so future lookups no longer need the deprecated fallback algorithm.""" + from superset.key_value.shared_entries import get_shared_value + from superset.key_value.types import SharedKey + from superset.key_value.utils import get_uuid_namespace_with_algorithm + + key = SharedKey.DASHBOARD_PERMALINK_SALT + expected_value = "legacy_md5_salt" + + # Calculate what the MD5 UUID would be + namespace_md5 = get_uuid_namespace_with_algorithm("", "md5") + uuid_md5 = uuid3(namespace_md5, key) + + # Mock KeyValueDAO to simulate MD5 entry exists, SHA-256 doesn't + mock_dao = MagicMock() + + def mock_get_value(resource, uuid_key, codec): + if uuid_key == uuid_md5: + return expected_value + return None + + mock_dao.get_value.side_effect = mock_get_value + + mock_app = MagicMock() + mock_app.config = { + "HASH_ALGORITHM": "sha256", + "HASH_ALGORITHM_FALLBACKS": ["md5"], + } + + with patch("superset.key_value.shared_entries.KeyValueDAO", mock_dao): + with patch("superset.key_value.utils.current_app", mock_app): + with patch("superset.db.session.commit") as mock_commit: + result = get_shared_value(key) + + assert result == expected_value + mock_dao.create_entry.assert_called_once() + mock_commit.assert_called_once() Review 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] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Migration regression coverage accepts an incorrect destination UUID. - ⚠️ Future deprecated-algorithm lookups could continue indefinitely. - ⚠️ Resource, value, or codec regressions remain undetected. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b2a0ede04734456a41728c6c21e757e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5b2a0ede04734456a41728c6c21e757e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx 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 fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42916&comment_hash=01b49b93a1d9ce5f9dd98862818e6a38d3c1d24b835f79a6023eb867ced14a11&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42916&comment_hash=01b49b93a1d9ce5f9dd98862818e6a38d3c1d24b835f79a6023eb867ced14a11&reaction=dislike'>👎</a> -- 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]
