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