From 467e0f49431bedcc959fd192c4be2fc89b8545e8 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Tue, 15 Sep 2026 12:32:07 +0100 Subject: [PATCH] Accept a node that cannot name its illuminator `tx_callsign` was the last required field of a node's configuration with no way to say "unknown", which left its producers inventing one: retina-gui substitutes the literal 'Tower' when the tower it selected has no callsign, and retina-simulation's tower resolver falls back to an empty string. Both are the objection 1.1.3 made of a substituted coordinate, that the server cannot later tell the placeholder apart from a name an owner gave it. So the field becomes nullable, on the coordinates' reasoning and by the same route: the validator, the column, and the published schema, which the contract is regenerated from in this commit. The empty string stays refused. Null is the one spelling of "unknown", which is what keeps a stored name distinguishable from its absence, and since the validator has always refused "" there is no legacy sentinel to grandfather the way (0, 0) had to be. A patch rather than a minor bump, on the test 1.1.3 applied to this same change on the coordinates: the document gains no field a client can read, and what a client generating from it sees is a widened type on a field it already had. Nothing in this repo reads the callsign, so no consumer gains null-handling. The producers need their own changes before either stops inventing a name, and those are out of scope here. Co-Authored-By: Claude Opus 5 --- backend/core/nodes.py | 4 +- .../versions/0009_nullable_tx_callsign.py | 37 ++++++++++++++++++ backend/routes/nodes.py | 11 +++++- backend/services/node_config.py | 39 +++++++++++++++---- backend/tests/test_node_config_endpoint.py | 13 +++++++ backend/tests/test_node_config_store.py | 15 +++++++ backend/tests/test_node_config_validation.py | 14 ++++++- backend/tests/test_node_openapi.py | 14 +++++-- contracts/nodes-v1.openapi.yaml | 14 +++++-- 9 files changed, 143 insertions(+), 18 deletions(-) create mode 100644 backend/migrations/versions/0009_nullable_tx_callsign.py diff --git a/backend/core/nodes.py b/backend/core/nodes.py index 9caf819d..7832df4f 100644 --- a/backend/core/nodes.py +++ b/backend/core/nodes.py @@ -100,7 +100,9 @@ class NodeConfig(Base): tx_lat: Mapped[float | None] = mapped_column(Float, nullable=True) tx_lon: Mapped[float | None] = mapped_column(Float, nullable=True) tx_alt_ft: Mapped[float | None] = mapped_column(Float, nullable=True) - tx_callsign: Mapped[str] = mapped_column(String(32)) + # Nullable since 1.2.2: an owner who cannot name the illuminator, the same + # case the coordinates above carry. + tx_callsign: Mapped[str | None] = mapped_column(String(32), nullable=True) fc_hz: Mapped[float] = mapped_column(Float) fs_hz: Mapped[float] = mapped_column(Float) # Both nullable, and neither null may be filled in. A null width means the diff --git a/backend/migrations/versions/0009_nullable_tx_callsign.py b/backend/migrations/versions/0009_nullable_tx_callsign.py new file mode 100644 index 00000000..9d122241 --- /dev/null +++ b/backend/migrations/versions/0009_nullable_tx_callsign.py @@ -0,0 +1,37 @@ +"""tx_callsign becomes nullable on node_configs. + +Revision ID: 0009 +Revises: 0008 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "0009" +down_revision = "0008" +branch_labels = None +depends_on = None + +# A downgrade cannot express a null, and code predating 1.2.2 has no +# null-handling for this column, so a rollback across this revision must be +# surfaced to a human rather than served as safe. The same grading, for the same +# reason, as 0005 on the coordinates. +rollback_safety = "destructive" + + +def upgrade() -> None: + # batch_alter_table because SQLite has no ALTER COLUMN: Alembic copies the + # table with the corrected definition and swaps it in. Existing rows keep the + # names they declared; the table is append-only, so this governs new versions + # only. + with op.batch_alter_table("node_configs") as batch: + batch.alter_column("tx_callsign", existing_type=sa.String(length=32), nullable=True) + + +def downgrade() -> None: + # Fails, loudly, once any node has registered without a callsign: the table + # copy hits the NOT NULL and leaves the database stamped at 0009. That is the + # honest outcome, since the alternative is inventing an illuminator name that + # nothing downstream could tell from one an owner gave. + with op.batch_alter_table("node_configs") as batch: + batch.alter_column("tx_callsign", existing_type=sa.String(length=32), nullable=False) diff --git a/backend/routes/nodes.py b/backend/routes/nodes.py index a585a2ea..4e571f14 100644 --- a/backend/routes/nodes.py +++ b/backend/routes/nodes.py @@ -98,7 +98,16 @@ # would be 1.3.0. Recorded so the block stays honest with itself, and because # nothing about the choice is load-bearing — the field is optional either way, # and no node behaves differently for the number in front of it. -NODE_API_VERSION = "1.2.1" +# +# 1.2.2 makes `tx_callsign` nullable, so a node whose owner cannot name the +# illuminator can register without one being invented for it. A patch on the +# test 1.1.3 applied to the same change on the coordinates: the document gains +# no field a client can read, and what a client generating from it sees is a +# type widening on a field it already had. +# +# The empty string stays refused. Null is the one way to say the illuminator is +# unnamed, which is what keeps a stored name distinguishable from its absence. +NODE_API_VERSION = "1.2.2" # No tag here: each sub-router carries the contract's own grouping, since those # are what a generated client is built around. diff --git a/backend/services/node_config.py b/backend/services/node_config.py index b0c2d2be..c01b5db0 100644 --- a/backend/services/node_config.py +++ b/backend/services/node_config.py @@ -80,6 +80,13 @@ def __init__(self, field: str, reason: str = "out of range") -> None: # antenna characterised, so null is what the whole fleet sends for both. _NULLABLE_BEAM = {"beam_width_deg", "beam_azimuth_deg"} +# Nullable since 1.2.2, on the coordinates' reasoning above: an owner who cannot +# name the illuminator has nothing to put here, and a substituted name is wrong +# data the server could not later tell apart from a real one. The empty string +# stays refused, so null is the single spelling of "unknown" and no stored row +# can hold the other. +_NULLABLE_CALLSIGN = {"tx_callsign"} + _SCHEMA_DESCRIPTION = """\ The receiver and illuminator geometry, the radio parameters and the association tolerances. Every field is required. @@ -89,6 +96,10 @@ def __init__(self, field: str, reason: str = "out of range") -> None: it places nothing on the map until a position arrives. A latitude and its longitude are given together or both null. +`tx_callsign` is nullable for the same reason, and the empty string is not: a +node that cannot name its illuminator sends null, which is the only way to say +so. + Necessary but not sufficient. A receiver and illuminator at the same point are refused, as is a value that is not a finite number, and neither is expressible here: both answer `400 invalid_config` naming the field.""" @@ -121,7 +132,7 @@ def config_json_schema() -> dict[str, Any]: """ properties = {field: _numeric_property(*bounds) for field, bounds in _NUMERIC_BOUNDS.items()} properties |= deepcopy(_UNTABLED_PROPERTIES) - nullable = _NULLABLE | _NULLABLE_BEAM + nullable = _NULLABLE | _NULLABLE_BEAM | _NULLABLE_CALLSIGN return { "type": "object", "title": "NodeConfig", @@ -139,6 +150,13 @@ def config_json_schema() -> dict[str, Any]: } +def _typed_branch(published: dict[str, Any], json_type: str) -> dict[str, Any]: + for alternative in published.get("anyOf", [published]): + if alternative.get("type") == json_type: + return alternative + return published + + def numeric_branch(published: dict[str, Any]) -> dict[str, Any]: """The number half of a published property, whether or not it is nullable. @@ -147,10 +165,12 @@ def numeric_branch(published: dict[str, Any]) -> dict[str, Any]: the type rather than on position in the `anyOf`, so reordering the branches cannot leave a caller reading the null one and finding no bounds at all. """ - for alternative in published.get("anyOf", [published]): - if alternative.get("type") == "number": - return alternative - return published + return _typed_branch(published, "number") + + +def string_branch(published: dict[str, Any]) -> dict[str, Any]: + """numeric_branch for the one field whose bounds are lengths, not magnitudes.""" + return _typed_branch(published, "string") def _as_finite_float(value: Any) -> tuple[float | None, str]: @@ -211,10 +231,15 @@ def validate_config(payload: dict[str, Any]) -> dict[str, Any]: raise ConfigInvalid(field) out[field] = value + # Null is the illuminator being unnamed, and "" is not a shorter way to say + # it: see _NULLABLE_CALLSIGN. callsign = payload["tx_callsign"] - if not isinstance(callsign, str) or not 1 <= len(callsign) <= 32: + if callsign is None: + out["tx_callsign"] = None + elif isinstance(callsign, str) and 1 <= len(callsign) <= 32: + out["tx_callsign"] = callsign + else: raise ConfigInvalid("tx_callsign") - out["tx_callsign"] = callsign # Required and nullable since 1.1.1: no node has its antenna characterised, # because retina-gui does not collect the geometry from owners, so null is what diff --git a/backend/tests/test_node_config_endpoint.py b/backend/tests/test_node_config_endpoint.py index 80c74c3d..c247c129 100644 --- a/backend/tests/test_node_config_endpoint.py +++ b/backend/tests/test_node_config_endpoint.py @@ -106,6 +106,19 @@ async def test_a_resend_with_both_antenna_fields_null_is_not_a_change(registered assert (rows[1].beam_width_deg, rows[1].beam_azimuth_deg) == (None, None) +async def test_a_null_callsign_is_accepted_and_stored(registered_node, node_client, node_session): + """An owner who cannot name the illuminator, which retina-gui otherwise fills in + with a placeholder the server could not tell apart from a real name.""" + token, node_id = registered_node + + response = node_client.put("/v1/nodes/config", headers=_auth(token), json=dict(CONFIG, tx_callsign=None)) + + assert response.status_code == 200 + assert response.json() == {"config_version": 2} + rows = await _versions(node_session, node_id) + assert rows[-1].tx_callsign is None + + async def test_a_changed_field_mints_the_next_version_and_supersedes_the_last( registered_node, node_client, node_session ): diff --git a/backend/tests/test_node_config_store.py b/backend/tests/test_node_config_store.py index b29f2479..28cce89e 100644 --- a/backend/tests/test_node_config_store.py +++ b/backend/tests/test_node_config_store.py @@ -269,3 +269,18 @@ async def test_a_null_position_round_trips(node_session): await node_session.refresh(row) assert row.rx_lat is None assert row.tx_lat == 34.90 + + +async def test_a_null_callsign_round_trips(node_session): + """The column carries what contract 1.2.2 accepts: a node whose owner cannot + name the illuminator. Minting and comparison need nothing of their own for it, + since both are already null-aware for the antenna fields above.""" + node_session.add(Node(node_id="test-null-sign", node_ref=mint_node_ref(), board_model="raspberrypi5-4gb")) + await node_session.flush() + + version = await upsert_config(node_session, "test-null-sign", _config(tx_callsign=None)) + named = await upsert_config(node_session, "test-null-sign", _config(tx_callsign="Wrotham")) + + assert (version, named) == (1, 2) + rows = await _rows(node_session, "test-null-sign") + assert [row.tx_callsign for row in rows] == [None, "Wrotham"] diff --git a/backend/tests/test_node_config_validation.py b/backend/tests/test_node_config_validation.py index 7a21a9d1..5a695684 100644 --- a/backend/tests/test_node_config_validation.py +++ b/backend/tests/test_node_config_validation.py @@ -8,6 +8,7 @@ config_json_schema, numeric_branch, position_status, + string_branch, validate_config, ) @@ -117,6 +118,8 @@ def test_zero_beam_azimuth_is_not_null(): ("delay_tolerance_us", -1), ("doppler_tolerance_hz", 0), ("doppler_tolerance_hz", -1), + # Still refused now that null is accepted: null is the one spelling of + # "unknown", and services/node_config.py says why. ("tx_callsign", ""), ("tx_callsign", "x" * 33), ], @@ -197,13 +200,20 @@ def test_a_non_numeric_type_is_rejected(value): assert excinfo.value.field == "rx_lat" -@pytest.mark.parametrize("value", [123, None, ["CRYSTAL_PALACE"]]) +@pytest.mark.parametrize("value", [123, ["CRYSTAL_PALACE"]]) def test_a_non_string_callsign_is_rejected(value): with pytest.raises(ConfigInvalid) as excinfo: validate_config(dict(VALID, tx_callsign=value)) assert excinfo.value.field == "tx_callsign" +def test_a_null_callsign_is_preserved_not_defaulted(): + """Nullable since contract 1.2.2, for the reason the coordinates are: an owner + who cannot name the illuminator says so, rather than a placeholder the server + could not later tell apart from a real name.""" + assert validate_config(dict(VALID, tx_callsign=None))["tx_callsign"] is None + + @pytest.mark.parametrize( "field,value", [ @@ -700,7 +710,7 @@ def test_every_published_bound_is_where_the_validator_refuses(field): def test_the_published_callsign_length_is_where_the_validator_refuses(): - schema = config_json_schema()["properties"]["tx_callsign"] + schema = string_branch(config_json_schema()["properties"]["tx_callsign"]) for length in (schema["minLength"], schema["maxLength"]): validate_config(dict(VALID, tx_callsign="x" * length)) diff --git a/backend/tests/test_node_openapi.py b/backend/tests/test_node_openapi.py index 6ceb49ea..9db6269c 100644 --- a/backend/tests/test_node_openapi.py +++ b/backend/tests/test_node_openapi.py @@ -18,7 +18,14 @@ from routes.nodes import NODE_API_SERVERS, NODE_API_VERSION from scripts.generate_openapi import CONTRACT_PATH, contract, render -from services.node_config import _NULLABLE, _NULLABLE_BEAM, _NUMERIC_BOUNDS, _REQUIRED, numeric_branch +from services.node_config import ( + _NULLABLE, + _NULLABLE_BEAM, + _NULLABLE_CALLSIGN, + _NUMERIC_BOUNDS, + _REQUIRED, + numeric_branch, +) FRAME = { "t": 1753900000.123, @@ -254,13 +261,14 @@ def test_every_bound_the_validator_enforces_reaches_the_schema(document): def test_the_nullable_fields_publish_a_null_branch(document): """The six coordinates, so a node whose owner cannot supply the geometry - still registers, and the two beam fields, which no node has characterised. + still registers, the two beam fields, which no node has characterised, and + the callsign, which an owner who cannot name the illuminator leaves null. A client generated from a document that omitted these cannot express the config the fleet actually sends.""" properties = _published_config(document)["properties"] nullable = {field for field, published in properties.items() if {"type": "null"} in published.get("anyOf", [])} - assert nullable == _NULLABLE | _NULLABLE_BEAM + assert nullable == _NULLABLE | _NULLABLE_BEAM | _NULLABLE_CALLSIGN def test_the_contact_operation_reaches_its_own_component(document): diff --git a/contracts/nodes-v1.openapi.yaml b/contracts/nodes-v1.openapi.yaml index 1556eebe..557d9f13 100644 --- a/contracts/nodes-v1.openapi.yaml +++ b/contracts/nodes-v1.openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: RETINA node ingest - version: 1.2.1 + version: 1.2.2 description: | The RETINA server's HTTP API. The paths under `/v1/nodes` are the RETINA node ingest contract, generated from the server and versioned as a unit; everything @@ -803,6 +803,10 @@ components: it places nothing on the map until a position arrives. A latitude and its longitude are given together or both null. + `tx_callsign` is nullable for the same reason, and the empty string is not: a + node that cannot name its illuminator sends null, which is the only way to say + so. + Necessary but not sufficient. A receiver and illuminator at the same point are refused, as is a value that is not a finite number, and neither is expressible here: both answer `400 invalid_config` naming the field. @@ -866,9 +870,11 @@ components: type: number exclusiveMinimum: 0.0 tx_callsign: - type: string - minLength: 1 - maxLength: 32 + anyOf: + - type: string + minLength: 1 + maxLength: 32 + - type: 'null' beam_width_deg: anyOf: - type: number