refactor(graphql): give each cell an id and a summary object

api/src/damnit_api/graphql/directives.py

@@ -19,8 +19,8 @@ def lightweight(field: DirectiveValue[DamnitRun | Cell]):
     fields = field if isinstance(field, list) else [field]
 
     for cell in get_cells(fields):
-        if cell is not None and cell.dtype in HEAVY_DATA:
-            cell.value = None
+        if cell is not None and cell.summary.dtype in HEAVY_DATA:
+            cell.summary.value = None
 
     # Return original field
     return field

api/src/damnit_api/runs/types.py

@@ -86,11 +86,22 @@ def from_attrs(cls, attributes):
 
 
 @strawberry.type
-class Cell:
-    name: str
+class CellSummary:
     value: Any | None
     dtype: DamnitType
-    error: CellError | None = None
+
+
+@strawberry.type
+class Cell:
+    # `id` is the cell's global identity, "{database}:{proposal}:{run}:{name}".
+    # `database` is in it because the same (proposal, run, name) can be served
+    # through two databases (a guest proposal also opened directly), so it is
+    # scoped the same way the run's own key is. `error` describes the whole cell:
+    # a failed variable still carries a summary, with a null value.
+    id: strawberry.ID
+    name: str
+    error: CellError | None
+    summary: CellSummary
 
 
 def _unwrap(entry):
@@ -126,7 +137,8 @@ def cells(self, names: list[str] | None = None) -> list[Cell]:
         return [c for c in self._cells if c.name in requested]
 
     @classmethod
-    def _iter_cells(cls, record):
+    def _iter_cells(cls, record, *, database, proposal, run):
+        prefix = f"{database}:{proposal}:{run}:"
         for name, entry in record.items():
             if entry is None:
                 continue
@@ -135,22 +147,47 @@ def _iter_cells(cls, record):
             dtype = cls.get_dtype(name, entry)
             value, dtype = serialize(entry["value"], dtype=dtype)
             error = CellError.from_attrs(entry.get("attributes"))
-            yield Cell(name=name, value=Any(value), dtype=dtype, error=error)
+            if error is not None:
+                # A failed cell has no value to render, so its summary type is
+                # not worth keeping. The client merges a cell's summary without
+                # being able to see the error alongside it, so a heavy dtype
+                # here would look like a value @lightweight held back and pin
+                # whatever the cell held before it failed.
+                dtype = DamnitType.STRING
+            yield Cell(
+                id=strawberry.ID(prefix + name),
+                name=name,
+                error=error,
+                summary=CellSummary(value=Any(value), dtype=dtype),
+            )
 
     @classmethod
     def from_db(cls, record, *, database):
+        database = str(database)
+        # Cell ids join their parts with ":", so a part carrying one of its own
+        # would let two different cells share an id and collide in the client's
+        # cache. Only `database` can: it is the handle the client sent, and a
+        # path handle is coming.
+        if ":" in database:
+            msg = f"Database handle may not contain ':': {database!r}"
+            raise ValueError(msg)
+
         # Both callers key their rows on (proposal, run), so a record without
         # them is a bug upstream. Fail here rather than mint a `"None"`
         # proposal that quietly becomes a cache key on the client.
         proposal = _unwrap(record["proposal"])
         if proposal is None:
             msg = "Run record has no proposal."
             raise ValueError(msg)
+        proposal = str(proposal)
+        run = int(_unwrap(record["run"]))
         return cls(
-            database=str(database),
-            proposal=str(proposal),
-            run=int(_unwrap(record["run"])),
-            _cells=list(cls._iter_cells(record)),
+            database=database,
+            proposal=proposal,
+            run=run,
+            _cells=list(
+                cls._iter_cells(record, database=database, proposal=proposal, run=run)
+            ),
         )
 
     @staticmethod

api/tests/graphql/test_models.py

@@ -180,14 +180,57 @@ def test_from_db_includes_error_for_failed_variable():
 
     by_name = {v.name: v for v in run.cells()}
     assert by_name["run"].error is None
-    assert by_name["broken"].value is None
+    assert by_name["broken"].summary.value is None
     assert by_name["broken"].error == CellError(message=ERROR_ATTRS["error"], cls="Foo")
 
 
+def test_from_db_drops_a_heavy_summary_type_when_the_variable_failed():
+    # The client merges a cell's summary without being able to see the error
+    # beside it, so a heavy dtype on a null value reads as one @lightweight held
+    # back and pins whatever the cell held before it failed.
+    record = {
+        "proposal": {"value": 900485},
+        "run": {"value": 1},
+        "broken": {
+            "value": None,
+            "summary_type": "trendline",
+            "attributes": json.dumps(ERROR_ATTRS),
+        },
+    }
+    run = DamnitRun.from_db(record, database="900485")
+
+    assert run.cells(names=["broken"])[0].summary.dtype == DamnitType.STRING
+
+
 def test_from_db_populates_identity_trio():
     record = {"proposal": {"value": 900485}, "run": {"value": 348}}
     run = DamnitRun.from_db(record, database="900485")
 
     assert run.database == "900485"
     assert run.proposal == "900485"
     assert run.run == 348
+
+
+def test_from_db_scopes_cell_ids_by_database():
+    # The same (proposal, run, name) served through two databases (a guest
+    # proposal also opened directly) must key distinct normalized cells, so the
+    # second database cannot overwrite the first's cached value.
+    record = {
+        "proposal": {"value": 900485},
+        "run": {"value": 348},
+        "n_trains": {"value": 3641},
+    }
+    guest = DamnitRun.from_db(record, database="900405")
+    direct = DamnitRun.from_db(record, database="900485")
+
+    assert guest.cells(names=["n_trains"])[0].id == "900405:900485:348:n_trains"
+    assert direct.cells(names=["n_trains"])[0].id == "900485:900485:348:n_trains"
+
+
+def test_from_db_rejects_a_database_handle_carrying_a_colon():
+    # Cell ids join their parts with ":", so a handle carrying one of its own
+    # would let two different cells share an id and collide in the client cache.
+    record = {"proposal": {"value": 900485}, "run": {"value": 348}}
+
+    with pytest.raises(ValueError, match="may not contain"):
+        DamnitRun.from_db(record, database="900405:900485")

api/tests/graphql/test_queries.py

@@ -51,7 +51,7 @@ async def test_runs_query(graphql_schema, mocked_fetch_cells, mocked_fetch_info)
           runs(database: {{proposal: "{PROPOSAL}"}}, per_page: $per_page) {{
             cells {{
               name
-              value
+              summary {{ value }}
             }}
           }}
         }}
@@ -72,6 +72,44 @@ async def test_runs_query(graphql_schema, mocked_fetch_cells, mocked_fetch_info)
     assert mocked_fetch_info.called
 
 
+@pytest.mark.asyncio
+async def test_lightweight_directive_blanks_heavy_values(
+    graphql_schema, mocker, mocked_fetch_info
+):
+    # A heavy cell (array) next to a scalar, so the directive has both to sort.
+    record = {
+        "proposal": {"value": PROPOSAL},
+        "run": {"value": 348},
+        "n_trains": {"value": 3641, "summary_type": None},
+        "spectrum": {"value": [1.0, 2.0, 3.0], "summary_type": "trendline"},
+    }
+    mocker.patch(
+        "damnit_api.graphql.queries.fetch_cells",
+        return_value=[record],
+    )
+
+    query = f"""
+        query {{
+          runs(database: {{proposal: "{PROPOSAL}"}}, per_page: 2) @lightweight {{
+            cells {{
+              name
+              summary {{ value dtype }}
+            }}
+          }}
+        }}
+    """
+    result = await graphql_schema.execute(query)
+
+    assert result.errors is None
+
+    cells = {c["name"]: c["summary"] for c in result.data["runs"][0]["cells"]}
+    # The heavy value is held back, but its dtype still describes the cell.
+    assert cells["spectrum"]["value"] is None
+    assert cells["spectrum"]["dtype"] == "array"
+    # A scalar is left untouched.
+    assert cells["n_trains"]["value"] == 3641
+
+
 @pytest.mark.asyncio
 async def test_runs_query_returns_identity_trio(
     graphql_schema, mocked_fetch_cells, mocked_fetch_info
@@ -245,7 +283,7 @@ async def test_runs_query_partial_name_match(graphql_schema, real_damnit_db):
           runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{
             cells(names: ["alpha", "run"]) {{
               name
-              value
+              summary {{ value }}
             }}
           }}
         }}
@@ -254,7 +292,7 @@ async def test_runs_query_partial_name_match(graphql_schema, real_damnit_db):
 
     assert result.errors is None
     runs = result.data["runs"]
-    by_run = [{v["name"]: v["value"] for v in r["cells"]} for r in runs]
+    by_run = [{v["name"]: v["summary"]["value"] for v in r["cells"]} for r in runs]
     assert by_run == [
         {"alpha": "a1", "run": 1},
         {"alpha": "a2", "run": 2},
@@ -356,7 +394,7 @@ async def test_runs_query_includes_guests_active_block_first(
           runs(database: {{proposal: "{proposal}"}}, per_page: 10) {{
             proposal
             run
-            cells(names: ["alpha"]) {{ name value }}
+            cells(names: ["alpha"]) {{ name summary {{ value }} }}
           }}
         }}
     """
@@ -372,7 +410,10 @@ async def test_runs_query_includes_guests_active_block_first(
         ("888888", 1),
     ]
 
-    alpha = [{v["name"]: v["value"] for v in r["cells"]}.get("alpha") for r in runs]
+    alpha = [
+        {v["name"]: v["summary"]["value"] for v in r["cells"]}.get("alpha")
+        for r in runs
+    ]
     # The colliding run 1 keeps each proposal's own latest value.
     assert alpha == ["a1", "a2", "guest_a1"]
 

api/tests/graphql/test_subscriptions.py

@@ -35,7 +35,7 @@
           database
           proposal
           run
-          cells { name value dtype }
+          cells { name summary { value dtype } }
         }
         metadata {
           runs { proposal run }
@@ -150,7 +150,11 @@ async def test_run_updates(
             "run": DatabaseVariable(value=NEW_RUN, damnit_dtype=DamnitType.NUMBER),
         }
         got = {
-            v["name"]: {"value": v["value"], "dtype": v["dtype"]} for v in run["cells"]
+            v["name"]: {
+                "value": v["summary"]["value"],
+                "dtype": v["summary"]["dtype"],
+            }
+            for v in run["cells"]
         }
         assert got == {
             name: {"value": var.damnit_value, "dtype": var.damnit_dtype.value}