Re: [I] Prevent the reuse of detached session objects in database commit contexts (tooling-trusted-releases)

["asf-tooling (via GitHub)" <gi...@apache.org>]

asf-tooling commented on issue #299:
URL: https://github.com/apache/tooling-trusted-releases/issues/299#issuecomment-4403370378

   <!-- gofannon-issue-triage-bot v2 -->
   
   **Automated triage** — analyzed at `main@837830e8`
   
   **Type:** `refactor`  •  **Classification:** `actionable`  •  **Confidence:** `medium`
   **Application domain(s):** `data_storage`, `infrastructure`
   
   ### Summary
   The issue requests preventing reuse of detached SQLAlchemy session objects when passed into database commit contexts. This is a recurring bug pattern (referenced as cause of #297) where model objects fetched in one session are used in another session's commit context, causing DetachedInstanceError or silent data corruption. The fix should be at the type level, making it harder to accidentally pass stale/detached objects across session boundaries.
   
   ### Where this lives in the code today
   
   #### `atr/db/__init__.py` — `ensure_session` (lines 833-839)
   _needs modification_
   This is the primary entry point where a caller-provided session (potentially containing detached objects from another context) gets reused. The pattern of passing `caller_data: db.Session | None` allows detached objects to be accessed in wrong contexts.
   
   ```python
   @contextlib.asynccontextmanager
   async def ensure_session(caller_data: db.Session | None = None) -> AsyncGenerator[Session]:
       if caller_data is not None:
           yield caller_data
       else:
           async with session() as data:
               yield data
   ```
   
   #### `atr/db/__init__.py` — `session` (lines 857-861)
   _currently does this_
   Creates a new session context. Objects fetched within this context become detached once it exits, but can still be passed to other functions that operate on different sessions.
   
   ```python
   @contextlib.asynccontextmanager
   async def session(log_queries: bool = False) -> AsyncGenerator[Session]:
       async with sessionmaker()() as data:
           yield data
   ```
   
   #### `atr/db/interaction.py` — `all_releases` (lines 88-111)
   _currently does this_
   Example of the problematic pattern: objects fetched in one session scope are modified outside it (release.project = project), and the `project` parameter itself could be a detached object from another session.
   
   ```python
   async def all_releases(project: sql.Project) -> list[sql.Release]:
       """Get all releases for the project, sorted by version."""
       query = sqlmodel.select(sql.Release).where(sql.Release.project_key == project.key)
   
       results = []
       async with db.session() as data:
           for result in (await data.execute(query)).all():
               release = result[0]
               results.append(release)
   
       for release in results:
           release.project = project
   
       try:
           # This rejects any non PEP 440 versions
           results.sort(key=lambda r: version.Version(r.version), reverse=True)
       except Exception as e:
   ```
   
   #### `atr/db/interaction.py` — `releases_by_phase` (lines 330-350)
   _currently does this_
   Another instance of the pattern where a detached `project` object is assigned to newly-fetched releases after the session closes, which can cause issues if those releases are later used in a commit context.
   
   ```python
   async def releases_by_phase(project: sql.Project, phase: sql.ReleasePhase) -> list[sql.Release]:
       """Get the releases for the project by phase."""
   
       query = (
           sqlmodel.select(sql.Release)
           .where(
               sql.Release.project_key == project.key,
               sql.Release.phase == phase,
           )
           .order_by(sql.validate_instrumented_attribute(sql.Release.created).desc())
       )
   
       results = []
       async with db.session() as data:
           for result in (await data.execute(query)).all():
               release = result[0]
               results.append(release)
   
       for release in results:
           # Don't need to eager load and lose it when the session closes
           release.project = project
       return results
   ```
   
   #### `atr/storage/__init__.py` — `Write` (lines 217-303)
   _needs modification_
   The Write class holds a db.Session reference and passes it to WriteAs* classes. If model objects from a different session are added/merged in this session's commit context, DetachedInstanceError occurs.
   
   ```python
   class Write:
       # Read and Write have authenticator methods which return access outcomes
       # TODO: Still need to send some runtime credentials guarantee to the WriteAs* classes
       def __init__(self, authorisation: principal.Authorisation, data: db.Session):
           self.__authorisation: Final[principal.Authorisation] = authorisation
           self.__data: Final[db.Session] = data
   ```
   
   #### `atr/storage/__init__.py` — `WriteAsGeneralPublic` (lines 119-133)
   _needs modification_
   Each writer receives the raw db.Session. Writers can inadvertently add detached objects from outside this session to the commit context.
   
   ```python
   class WriteAsGeneralPublic(WriteAs):
       def __init__(self, write: Write, data: db.Session):
           self.announce = writers.announce.GeneralPublic(write, self, data)
           self.cache = writers.cache.GeneralPublic(write, self, data)
           self.checks = writers.checks.GeneralPublic(write, self, data)
           self.keys = writers.keys.GeneralPublic(write, self, data)
           self.mail = writers.mail.GeneralPublic(write, self, data)
           self.policy = writers.policy.GeneralPublic(write, self, data)
           self.project = writers.project.GeneralPublic(write, self, data)
           self.release = writers.release.GeneralPublic(write, self, data)
           self.revision = writers.revision.GeneralPublic(write, self, data)
           self.sbom = writers.sbom.GeneralPublic(write, self, data)
           self.ssh = writers.ssh.GeneralPublic(write, self, data)
           self.tokens = writers.tokens.GeneralPublic(write, self, data)
           self.vote = writers.vote.GeneralPublic(write, self, data)
   ```
   
   #### `atr/db/__init__.py` — `Session` (lines 146-160)
   _needs modification_
   The Session class is where type-level guards could be added. For example, overriding `add()` and `merge()` to check object session identity, or introducing a branded/tagged session type that prevents mixing objects from different sessions.
   
   ```python
   class Session(sqlalchemy.ext.asyncio.AsyncSession):
       def __init__(self, *args: Any, **kwargs: Any) -> None:
           explicit_value_passed_by_sessionmaker = kwargs.pop("log_queries", None)
           super().__init__(*args, **kwargs)
   
           self.log_queries: bool = global_log_query
           if explicit_value_passed_by_sessionmaker is not None:
               self.log_queries = explicit_value_passed_by_sessionmaker
   ```
   
   ### Where new code would go
   - `atr/db/__init__.py` — after symbol Session
     Add a type-safe wrapper or runtime guard in the Session class that detects when a model object from a different session is being added/merged/committed.
   
   ### Proposed approach
   The approach should be two-fold: (1) Add runtime detection in `Session.add()` and `Session.merge()` to raise a clear error when a detached object from another session is being used, and (2) Introduce type-level distinction so that functions receiving model objects mark them as potentially-detached (e.g., a `Detached[T]` wrapper or protocol) making it a type error to pass them directly to session operations without explicit re-attachment.
   
   For the immediate fix, overriding `Session.add()` to check `sqlalchemy.inspect(instance).detached` would catch the most common case at runtime with a clear error message. For the type-level approach, a `NewType` or generic wrapper could distinguish between session-bound and detached model instances, though this requires more extensive refactoring of function signatures throughout `atr/db/interaction.py` and the storage writers.
   
   ### Suggested patches
   
   #### `atr/db/__init__.py`
   Add runtime guards to Session.add() and Session.merge() that detect detached objects and raise a clear error, preventing the class of bugs described in #297.
   
   ````diff
   --- a/atr/db/__init__.py
   +++ b/atr/db/__init__.py
   @@ -146,6 +146,38 @@ class Session(sqlalchemy.ext.asyncio.AsyncSession):
        def __init__(self, *args: Any, **kwargs: Any) -> None:
            explicit_value_passed_by_sessionmaker = kwargs.pop("log_queries", None)
            super().__init__(*args, **kwargs)
    
            self.log_queries: bool = global_log_query
            if explicit_value_passed_by_sessionmaker is not None:
                self.log_queries = explicit_value_passed_by_sessionmaker
    
   +    def _check_not_detached(self, instance: Any, operation: str) -> None:
   +        """Raise if instance is a detached SQLModel object from another session.
   +
   +        This prevents the reuse of objects fetched in a different session
   +        context, which causes DetachedInstanceError on commit or silent
   +        data corruption.
   +        """
   +        if not isinstance(instance, sqlmodel.SQLModel):
   +            return
   +        try:
   +            state = sqlalchemy.inspect(instance)
   +        except sqlalchemy.exc.NoInspectionAvailable:
   +            return
   +        if state.detached:
   +            raise RuntimeError(
   +                f"Cannot {operation} a detached instance of {type(instance).__name__} "
   +                f"(identity={state.identity}). The object was likely loaded in a "
   +                f"different session. Use session.merge() with an explicit refresh, "
   +                f"or re-query the object in this session."
   +            )
   +
   +    def add(self, instance: Any, _warn: bool = True) -> None:  # type: ignore[override]
   +        self._check_not_detached(instance, "add")
   +        super().add(instance, _warn=_warn)
   +
   +    def add_all(self, instances: Any) -> None:  # type: ignore[override]
   +        for instance in instances:
   +            self._check_not_detached(instance, "add_all")
   +        super().add_all(instances)
   +
        # TODO: Need to type all of these arguments correctly
    
        async def begin_immediate(self) -> None:
   ````
   
   ### Open questions
   - Issue #297 is referenced but not available — understanding the exact detached-object scenario that triggered it would help refine the solution.
   - Should `Session.merge()` also be guarded, or is merge the intended escape hatch for re-attaching detached objects?
   - A full type-level solution (e.g., `Detached[T]` wrapper type) would require significant refactoring of `atr/db/interaction.py` functions that return model objects outside session scope — is that level of investment desired?
   - Some functions in interaction.py deliberately assign detached objects (e.g., `release.project = project` in `releases_by_phase`). These would need to be refactored to either pass the session through or use only scalar attributes from detached objects.
   - Should the guard also check for objects that are `transient` (never persisted) vs `detached` (were persisted in another session)?
   
   ### Files examined
   - `atr/db/__init__.py`
   - `atr/storage/__init__.py`
   - `atr/storage/outcome.py`
   - `atr/db/interaction.py`
   - `atr/models/sql.py`
   - `atr/storage/readers/__init__.py`
   - `atr/storage/writers/__init__.py`
   - `atr/principal.py`
   
   ---
   *Draft from a triage agent. A human reviewer should validate before merging any change. The agent did not run tests or verify diffs apply.*


-- 
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: dev-unsubscribe@tooling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org