From a16bd9600bc01a65157fceee1209e2c774544c8d Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Sun, 26 Jul 2026 16:46:50 +0530 Subject: [PATCH] fixes reviews from qodo free for open source --- semantica/provenance/manager.py | 29 ++++++++++++---------- tests/provenance/test_manager.py | 41 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/semantica/provenance/manager.py b/semantica/provenance/manager.py index 3ec7c6cb..c4f6434c 100644 --- a/semantica/provenance/manager.py +++ b/semantica/provenance/manager.py @@ -28,6 +28,7 @@ from typing import Optional, List, Dict, Any, Union from collections.abc import Mapping from datetime import datetime from contextlib import contextmanager +import threading from .schemas import ProvenanceEntry, SourceReference from .storage import ProvenanceStorage, InMemoryStorage, SQLiteStorage @@ -40,12 +41,8 @@ def default_storage_path(path: Optional[str]): Context manager for temporarily setting ProvenanceManager._default_storage_path. Guarantees restoration to the previous value on exit (safe for tests). """ - original_path = ProvenanceManager._default_storage_path - ProvenanceManager._default_storage_path = path - try: + with ProvenanceManager.default_storage_path(path): yield - finally: - ProvenanceManager._default_storage_path = original_path class ProvenanceManager: @@ -70,11 +67,14 @@ class ProvenanceManager: """ _default_storage_path: Optional[str] = None + _lock = threading.RLock() + _path_stack: List[Optional[str]] = [] @classmethod def set_default_storage_path(cls, path: Optional[str]) -> None: """Set a global default storage path for all new instances.""" - cls._default_storage_path = path + with cls._lock: + cls._default_storage_path = path @classmethod @contextmanager @@ -83,12 +83,16 @@ class ProvenanceManager: Context manager for temporarily setting the global default storage path. Guarantees restoration to the previous value on exit (safe for tests). """ - original_path = cls._default_storage_path - cls._default_storage_path = path + cls._lock.acquire() try: + cls._path_stack.append(cls._default_storage_path) + cls._default_storage_path = path yield finally: - cls._default_storage_path = original_path + cls._default_storage_path = ( + cls._path_stack.pop() if cls._path_stack else None + ) + cls._lock.release() def __init__( self, @@ -117,8 +121,9 @@ class ProvenanceManager: elif "storage_path" in config: storage_path = config.get("storage_path") - if not storage_path and self._default_storage_path: - storage_path = self._default_storage_path + if not storage_path: + with self._lock: + storage_path = self._default_storage_path if storage_path: self.storage = SQLiteStorage(storage_path) @@ -678,7 +683,7 @@ class ProvenanceManager: Dict containing entity_id, depth, count, lineage entries, and sources """ base_lineage = self.get_lineage(entity_id) - entries = base_lineage.get("lineage_chain") or base_lineage.get("entries", []) + entries = base_lineage.get("lineage_chain", []) if len(entries) > depth: entries = entries[:depth] sources = self.get_all_sources(entity_id) diff --git a/tests/provenance/test_manager.py b/tests/provenance/test_manager.py index d74521d3..157e8300 100644 --- a/tests/provenance/test_manager.py +++ b/tests/provenance/test_manager.py @@ -61,6 +61,47 @@ class TestProvenanceManager: pass assert ProvenanceManager._default_storage_path is None + # 6. Nested contexts should restore correctly from stack + outer_path = str(tmp_path / "outer.db") + inner_path = str(tmp_path / "inner.db") + with default_storage_path(outer_path): + assert ProvenanceManager._default_storage_path == outer_path + with default_storage_path(inner_path): + assert ProvenanceManager._default_storage_path == inner_path + assert ProvenanceManager._default_storage_path == outer_path + assert ProvenanceManager._default_storage_path is None + + def test_default_storage_path_concurrency_safety(self, tmp_path): + """Test that default_storage_path context manager is thread-safe under concurrent use.""" + import threading + import time + from semantica.provenance import default_storage_path, SQLiteStorage + + errors = [] + + def _worker(path, delay): + try: + with default_storage_path(path): + time.sleep(delay) + prov_mgr = ProvenanceManager() + assert isinstance(prov_mgr.storage, SQLiteStorage) + assert prov_mgr.storage.db_path == path + except Exception as e: + errors.append(e) + + path_a = str(tmp_path / "thread_a.db") + path_b = str(tmp_path / "thread_b.db") + + t1 = threading.Thread(target=_worker, args=(path_a, 0.05)) + t2 = threading.Thread(target=_worker, args=(path_b, 0.01)) + t1.start() + t2.start() + t1.join() + t2.join() + + assert not errors, f"Errors in concurrent context worker: {errors}" + assert ProvenanceManager._default_storage_path is None + def test_isolation_part_1_with_context_manager(self, tmp_path): """Part 1: Prove context manager sets default storage path for ProvenanceManager created inside context.""" from semantica.provenance import default_storage_path, SQLiteStorage