Skip to content

memory: LadybugMemoryStore.add() drops superseded_by, so an imported retraction resurrects as current #110

Description

@Shashankss1205

What happens

LadybugMemoryStore.add() writes superseded_at but never creates the SUPERSEDED_BY edge, and superseded_by is reconstructed from that edge. So a claim that arrives already superseded lands with superseded_at set and superseded_by NULL — and Claim.is_current is superseded_by is None.

A retracted claim comes back as current. Two of the three backends agree; this one does not.

import tempfile
from grapharc.memory import LadybugMemoryStore, MemoryStore, SQLiteMemoryStore
from grapharc.memory.store import Claim

src = SQLiteMemoryStore(tempfile.mktemp(suffix=".sqlite"))
src.add(Claim(id="a", subject="svc", predicate="owner", object="alice", source="t"))
src.supersede("a", Claim(id="b", subject="svc", predicate="owner", object="bob", source="t"))
exported = [src.get("a"), src.get("b")]      # [('a', superseded_by='b'), ('b', None)]

for label, make in [
    ("MemoryStore", lambda: MemoryStore()),
    ("SQLite     ", lambda: SQLiteMemoryStore(tempfile.mktemp(suffix=".sqlite"))),
    ("Ladybug    ", lambda: LadybugMemoryStore(tempfile.mkdtemp() + "/db")),
]:
    dst = make()
    for c in exported:
        dst.add(c)
    got = dst.get("a")
    print(f"{label}  superseded_by={str(got.superseded_by):5}  is_current={got.is_current!s:5}  "
          f"current(svc)={[c.id for c in dst.current('svc')]}")
MemoryStore  superseded_by=b      is_current=False  current(svc)=['b']
SQLite       superseded_by=b      is_current=False  current(svc)=['b']
Ladybug      superseded_by=None   is_current=True   current(svc)=['a', 'b']

current("svc") is supposed to answer "what is true now". On this backend it answers alice and bob — the retraction is gone and the store contradicts itself, because superseded_at on that same row says it was retracted.

supersede() is not affected: it creates the edge itself. Only claims that arrive already-superseded through add() are — which is every import, replay, backup restore, or copy between backends.

Where in the code

  • grapharc/memory/ladybug_store.py_write rebuilds ABOUT/MENTIONS edges and never touches SUPERSEDED_BY
  • same file — _UPSERT does set superseded_at, which is what makes the resulting row self-contradictory rather than merely incomplete
  • same file — _OPTIONAL_SUPERSEDER / _PROJECTION, where superseded_by is read off the edge
  • same file — current(), whose filter is WITH c, n WHERE n IS NULL, i.e. the edge
  • grapharc/memory/store.pyClaim.is_current is superseded_by is None; MemoryStore.current filters on it

Why this is not a one-line fix

_write cannot simply create the edge, because the target claim may not exist yet — and in the most natural import order it does not. all_claims() returns oldest-first, so a replay presents a (superseded by b) before b.

Three options, each with a real cost. I have the repro set up and am happy to implement whichever you prefer.

1. Fail closed. add() raises when claim.superseded_by names a claim the store does not have. No schema change, and it turns silent corruption into a loud, actionable error. But it breaks the natural replay order outright: replaying all_claims() in its own returned order raises on the first claim.

2. Create a stub target node (MERGE (n:Claim {id: ...})) and let the later add fill it in via ON MATCH SET. Order-independent and it fits the existing upsert semantics — but the stub leaks into reads, verified:

get('ghost')   -> ValidationError: 6 validation errors for Claim (subject: Input should be a valid string, got None)
all_claims()   -> ValidationError: (same)
current('svc') -> []            # safe, it filters on subject_norm

So it needs a guard such as WHERE c.subject IS NOT NULL on _query and get. And a stub has no seq, while ON MATCH SET deliberately does not set one — ORDER BY c.seq then orders on NULL, which test_add_is_an_upsert_that_keeps_insertion_order is exactly about.

3. Persist superseded_by as a node property and derive the edge from it. Order-independent, durable, and the edge stays walkable for the Cypher path this backend exists for. The cost is a schema change — CREATE NODE TABLE IF NOT EXISTS will not add a column to a database that already exists, so it needs a migration — and it revises a stated design decision, since the module says in as many words:

superseded_by is not a column — it is reconstructed from the edge, so every read pairs its MATCH with this OPTIONAL MATCH and this projection.

My preference is 3: it is the only one that is both order-independent and durable, and the docstring's claim can be re-stated honestly (the edge remains what you walk in Cypher; the property is what survives an out-of-order import). But it is a persistence-format decision, so it should be yours rather than mine.

Worth noting for whichever option wins

tests/test_ladybug_store.py::test_it_returns_exactly_what_the_sqlite_backend_returns is the natural place for the regression test, and the fact that it passes today is the interesting part: the two backends are compared, but only along paths that go through supersede(). The equivalence that broke is the one nothing asked for — add() of a claim that is already superseded.

Acceptance criteria

  • Importing a superseded claim into a fresh store leaves it superseded, on all three backends, in whatever order the claims arrive (or fails loudly, if option 1 is chosen)
  • current() never returns a claim whose superseded_at is set
  • The three backends give the same answer for the import case, asserted by a test that compares them
  • uv run pytest green, uv run ruff check . clean

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions