diff --git a/SPEC.md b/SPEC.md index 9eb460177..bafd33540 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1700,12 +1700,28 @@ under-counted once already: do NOT show it: all three now decode a nested person's id as the `FlexibleInt64` it is, so `recordings.summarize()` gives `7` in every port. -The merge-safe composites refuse such a body before writing, so on the write -path it is the refusing direction — no id is invented and no partial update is -sent. On a plain read it is a string where the type says number. Both are -decoder coverage, field by field against the reference, rather than normalizer -reach, and are tracked in -[PR #913](https://github.com/basecamp/basecamp-sdk/pull/913). +The gap shows on a merge-safe write as well as on a plain read. Both are decoder +coverage, field by field against the reference, rather than normalizer reach, +and they are closed to different extents: + +- **The write path is closed, with one residual in TypeScript.** The + merge-safe composites' id-list guard reads a person id itself, by the same + `ParseInt` scan the normalizer uses, at exactly the three fields it writes + back — `assignees`, `completion_subscribers` and a schedule entry's + `participants` ([PR #913](https://github.com/basecamp/basecamp-sdk/pull/913)). + It also answers the two shapes no walk reaches: an absent `id` is `0`, and a + `null` element is the zero person, as in the reference. An explicit + `"id": null` still fails the read. Python and Ruby match the reference on + every measured row. TypeScript does not on the JavaScript number boundary, + in both directions: an id past 2^53 is REFUSED where the reference reads it, + rather than rounded into a different person's id; and a JSON number spelled + `1024.0` or `1e3` is ACCEPTED where the reference refuses it, because + `JSON.parse` has made it an integer before any code can look. +- **The plain read is still open.** A generated service read in Ruby, Python + and TypeScript still returns an untagged person's string id as the string, + where the reference's decoder gives the number. (Ruby's generated + `Types::Person` does read the id flexibly; the service hashes do not.) + Closing it needs a decoder at those reads, not a wider walk. **The oracle, not the documentation.** Every port that reasoned from `ParseInt`'s docs rather than probing it got something wrong. The corpus and diff --git a/python/src/basecamp/_person_id.py b/python/src/basecamp/_person_id.py index 8040f41c7..b204829ae 100644 --- a/python/src/basecamp/_person_id.py +++ b/python/src/basecamp/_person_id.py @@ -2,12 +2,13 @@ BC3 serializes person ids as JSON strings in some responses and as JSON numbers in others, and it spells the system actors -- ``LocalPerson``, whose id is -``"basecamp"`` or ``"campfire"`` -- in the same string field. Two places in this -SDK have to turn that string into a number: the pre-decode normalizer that every -response body walks (``generated/services/_base.py`` and its async twin) and the -flexible id reader in the recording-summary decode -(``services/_campfire_index.py``). The rule lives HERE so those two cannot drift -apart; they had two copies of it, and the copies disagreed. +``"basecamp"`` or ``"campfire"`` -- in the same string field. Three places in +this SDK have to turn that string into a number: the pre-decode normalizer that +every response body walks (``generated/services/_base.py`` and its async twin), +the flexible id reader in the recording-summary decode +(``services/_campfire_index.py``), and the merge-safe composites' id-list guard +(``services/_merge_safe.py``). The rule lives HERE so they cannot drift apart; +the first two had two copies of it, and the copies disagreed. So does the WALK that finds those objects (:func:`normalize_person_ids`), for the same reason: the sync and async base services had a copy each, and which people a @@ -137,10 +138,11 @@ def coerce_person_id(obj: MutableMapping[str, Any]) -> None: * a range error leaves the string UNTOUCHED (``:63``) so the reader refuses it. The readers that do refuse it are the ones with a Go struct behind them: ``services/_campfire_index._decoded_flexible_int64`` raises - "overflows int64", and ``services/_merge_safe`` refuses a non-int id - outright. Coercing it here instead -- which ``int()`` was doing -- hands - back a Python arbitrary-precision int for a value that does not fit the - wire's ``int64``, and every reader downstream then believes it. + "overflows int64", and ``services/_merge_safe`` refuses a RANGE + refusal as not a person id. Coercing it here instead -- which ``int()`` + was doing -- hands back a Python arbitrary-precision int for a value that + does not fit the wire's ``int64``, and every reader downstream then + believes it. An object whose ``id`` is already a number, or absent, is left alone -- which is also what makes this IDEMPOTENT, and :func:`normalize_person_ids` depends @@ -192,10 +194,11 @@ def normalize_person_ids(obj: Any, *, embedded_people: bool = False) -> None: That missing decoder is also why this pass does NOT close every string person id off the wire, and does not try to -- see "WHERE PASS 2 RUNS" below. Off Go's two surfaces a string id in ``creator``, ``assignees`` or a schedule's - ``participants`` stays a string, which ``services/_merge_safe``'s - ``writable_id_list`` refuses: ``schedules.edit_entry`` refuses a body the - reference accepts. That is decoder coverage rather than normalizer reach, and - it is tracked in PR #913. + ``participants`` stays a string. That is decoder coverage rather than + normalizer reach, and it is closed at the reader rather than here: + ``services/_merge_safe``'s ``writable_id_list`` reads a string person id + through :func:`parse_int64` itself (PR #913), where it used to require an + ``int`` and made ``schedules.edit_entry`` refuse a body the reference accepts. ONE walk here where Go runs two, and the two are equivalent because :func:`coerce_person_id` is idempotent and both passes apply it unchanged. @@ -224,12 +227,14 @@ def normalize_person_ids(obj: Any, *, embedded_people: bool = False) -> None: for a body the reference refuses outright. Accepting direction, identity field, which is the class this work exists to remove. - The cost of narrowing is real and is stated rather than hidden: the - ``schedules.edit_entry`` write path described above refuses string - ``participants`` ids again, because schedules is not one of Go's two - surfaces and Python has no decoder standing in for ``FlexibleInt64`` - there. That is decoder coverage, not normalizer reach, and PR #913 - (card 42) owns it. + The cost of narrowing is real and is stated rather than hidden: schedules is + not one of Go's two surfaces and Python has no decoder standing in for + ``FlexibleInt64`` there, so string ``participants`` ids are no longer + converted on the way in. That is decoder coverage, not normalizer reach. + The ``schedules.edit_entry`` WRITE path is closed at the reader instead -- + ``services/_merge_safe`` reads the id by :func:`parse_int64` (PR #913) -- + while a plain generated read still hands the string back (SPEC.md section 10, + "Person Ids Off the Wire"). """ if isinstance(obj, list): for item in obj: diff --git a/python/src/basecamp/services/_merge_safe.py b/python/src/basecamp/services/_merge_safe.py index 44e598a94..f6a0182ae 100644 --- a/python/src/basecamp/services/_merge_safe.py +++ b/python/src/basecamp/services/_merge_safe.py @@ -35,9 +35,15 @@ from typing import Any +from basecamp._person_id import Refusal, parse_int64 from basecamp._security import truncate as _truncate from basecamp.errors import ApiError +# The wire's person id is an `int64`. Named here rather than imported, because +# `basecamp._person_id` keeps its own copies private to its scan. +_INT64_MIN = -(2**63) +_INT64_MAX = 2**63 - 1 + _RESEND_HINT = ( "The merge-safe update/edit resend this field verbatim, so a coerced or empty value " "would overwrite the current one. Use {escape} to write the record deliberately." @@ -192,6 +198,60 @@ def writable_boolean(body: dict[str, Any], key: str, *, record: str, escape: str return required_writable_boolean(body, key, record=record, escape=escape) +def _person_id_from_wire(value: object) -> int | None: + """A PERSON's id as the reference reads one, or ``None`` when it could not. + + A person id is the one field in the generated model the reference decodes + *flexibly* (``types.FlexibleInt64``, reached through ``Person.Id``), so it + does not follow the rule the other id fields do: + + * an ``int`` in int64 range is itself — and one OUTSIDE that range is a + decode failure there, so it is a refusal here; + * ``bool`` is not an id, even though it is an ``int`` in Python; + * a STRING is what ``ParseInt`` makes of it, sign included; + * a string that refuses on SYNTAX is ``0`` and *not an error*, because that + is the sentinel BC3 serves for system-generated entities (``"basecamp"``, + ``"campfire"``) and the reference reads it as the system actor; + * a string that refuses on RANGE fails the read; + * anything else — a float, a list, a mapping, ``None`` — is a decode + failure. + + The grammar is :func:`basecamp._person_id.parse_int64`, the same scan the + pre-decode normalizer walks with, so the walk and this reader cannot drift: + a person the walk reached and a person it did not must land on the same id, + and until this called the shared scan they did not. + + ``None`` is a refusal and an ABSENT key is not, which is the one place the + two part company: ``encoding/json`` calls the flexible decoder's own + ``UnmarshalJSON`` for a JSON null, its number path leaves the buffer empty, + and ``ParseInt("")`` fails — so ``{"id": null}`` fails the read while a + missing ``"id"`` is the zero value with no error at all. The caller keeps + them apart; an absent key never reaches this function. + + The message belongs to the field rather than to this reader, so a refusal is + reported as ``None`` and the caller raises. + """ + if isinstance(value, bool): + return None + if isinstance(value, int): + # A direct range check, not `parse_int64(str(value))`. Routing it + # through the scan would avoid naming the bounds twice, but `str` of an + # `int` past 4300 digits raises a raw ValueError under Python's + # default int-string limit — and, where a host has lifted that limit, + # is quadratic. Python's arbitrary-precision `int` is the reason a check + # is needed at all: the reference's decoder refuses a JSON number past + # int64, and without this it would sail through into the PUT. + return value if _INT64_MIN <= value <= _INT64_MAX else None + if isinstance(value, str): + parsed = parse_int64(value) + if parsed is Refusal.SYNTAX: + return 0 + if parsed is Refusal.RANGE: + return None + return parsed + return None + + def writable_id_list(body: dict[str, Any], key: str, *, record: str, escape: str) -> list[int]: """Read a list of person records and project it to their integer IDs. @@ -199,13 +259,38 @@ def writable_id_list(body: dict[str, Any], key: str, *, record: str, escape: str comprehension it replaces (``[p["id"] for p in body.get(key) or []]``) has three ways to go wrong on malformed data: a non-list iterates as something else (a string yields characters, a dict yields its keys), a non-mapping - element raises ``TypeError``, and a non-integer ``id`` rides through + element raises ``TypeError``, and a wrong-typed ``id`` rides through verbatim into the full-replace PUT — the same corruption as a wrong-typed string, one level down. - ``bool`` is excluded explicitly: it subclasses ``int`` in Python, so - ``isinstance(True, int)`` is true and ``True`` would otherwise pass as a - person ID. + What it must NOT do is refuse an id the reference accepts, which is the + other half of the same defect and the one this guard had (#913). The + reference reads each element through the generated ``Person``, whose ``Id`` + is the flexible decoder, and ``fieldsFromTodo`` appends what that produced + with no filter of any kind — 0 included. So a string id, an absent id and a + null ELEMENT are all values the reference writes back, and refusing them + fails a call the reference completes. + + It must not lean on the pre-decode normalizer having reached the person + first. Which people that walk finds is the walk's rule — the reference's own + positional pass covers ``creator`` and ``participants`` and nothing else — + and a person it did not find arrives here exactly as BC3 sent it. How often + that is a string the fixtures cannot say: none of the person objects in + ``spec/fixtures`` carries a string id. What they do show is that the marker + the ``personable_type`` pass keys on is often missing — 3 of 7 ``assignees`` + people omit it. + + Per-element shapes and where each comes from: + + * a ``None`` element is ``0``. A JSON ``null`` in the array decodes to the + reference's zero ``Person``, whose ``Id`` is 0 and is appended like any + other. + * a non-mapping element is a malformed response. The reference cannot + unmarshal a number, string or boolean into a ``Person`` and fails the + whole decode. + * an ABSENT ``"id"`` is ``0`` (the zero value, no error), while a ``None`` + one is malformed — see :func:`_person_id_from_wire` for why those two + differ here and nowhere else. """ value = body.get(key) if value is None: @@ -217,20 +302,21 @@ def writable_id_list(body: dict[str, Any], key: str, *, record: str, escape: str ) ids: list[int] = [] for index, element in enumerate(value): + if element is None: + ids.append(0) + continue if not isinstance(element, dict): raise malformed( f"{record} field {key!r}[{index}] is not an object: {describe(element)}", _RESEND_HINT.format(escape=escape), ) - person_id = element.get("id") + if "id" not in element: + ids.append(0) + continue + person_id = _person_id_from_wire(element["id"]) if person_id is None: raise malformed( - f"{record} field {key!r}[{index}] has no 'id'", - _RESEND_HINT.format(escape=escape), - ) - if isinstance(person_id, bool) or not isinstance(person_id, int): - raise malformed( - f"{record} field {key!r}[{index}].id is not an integer: {describe(person_id)}", + f"{record} field {key!r}[{index}].id is not a person id: {describe(element['id'])}", _RESEND_HINT.format(escape=escape), ) ids.append(person_id) diff --git a/python/tests/services/test_schedules.py b/python/tests/services/test_schedules.py index 755f82b4b..5a0254a15 100644 --- a/python/tests/services/test_schedules.py +++ b/python/tests/services/test_schedules.py @@ -307,31 +307,26 @@ def test_untouched_carve_outs_stay_off_the_wire(self): assert "notify" not in body @respx.mock - def test_a_participant_with_a_string_id_is_refused_where_the_reference_accepts_it(self): - # A KNOWN DIVERGENCE, PINNED rather than left to be discovered. BC3 - # writes a person id as a string on some paths, and an embedded - # participant frequently omits `personable_type`. The reference reads it - # through its decoder -- generated `ScheduleEntry.Participants` is - # `[]Person`, and `Person.Id` is `types.FlexibleInt64` -- and completes - # the update. This refuses it. + def test_a_participant_with_a_string_id_is_read_as_the_reference_reads_it(self): + # The divergence this test used to pin, flipped. BC3 writes a person id as + # a string on some paths, and an embedded participant frequently omits + # `personable_type`. The reference reads it through its decoder -- + # generated `ScheduleEntry.Participants` is `[]Person`, and `Person.Id` is + # `types.FlexibleInt64` -- and completes the update. # - # The positional normalizer briefly covered this by running on every - # response body. That over-reached: the same `creator` / `participants` - # keys hold plain-`int64` people on `UpcomingScheduleEntry`, which the - # reference refuses and the normalizer turned into the system actor. It - # now runs only on Go's two surfaces (gauges, notifications), and - # schedules is not one of them. - # - # The refusal is the SAFE direction: no id is invented, and no PUT is - # sent, so a refused read never becomes a partial participant list. - # Closing it properly is decoder coverage, field by field against the - # reference, and PR #913 (card 42) owns it. This is the test to flip. + # The positional normalizer does not reach schedules: covering it there by + # running on every response over-reached, because the same `creator` / + # `participants` keys hold plain-`int64` people on `UpcomingScheduleEntry`, + # which the reference refuses. So the string arrives at the merge-safe + # guard untouched, and the guard reads it by the reference's grammar -- + # decoder coverage at exactly this field, not normalizer reach at every + # field so named. `"+007"` is person 7 to `ParseInt`. _, put_route = _routes(_entry(participants=[{"id": "1049715915", "name": "Ann"}, {"id": "+007", "name": "Bo"}])) - with pytest.raises(ApiError), _sync_schedules().edit_entry(entry_id=5001) as e: + with _sync_schedules().edit_entry(entry_id=5001) as e: e.participant_ids = [*e.participant_ids, 1049715914] - assert not put_route.called + assert _put_body(put_route)["participant_ids"] == [1049715915, 7, 1049715914] @respx.mock def test_assigning_the_read_backs_own_value_still_sends_it(self): @@ -632,18 +627,18 @@ def test_edit_refuses_a_missing_all_day_before_writing(self): [ pytest.param("nobody", id="string"), pytest.param([42], id="non-object-element"), - pytest.param([{"name": "no id"}], id="missing-id"), pytest.param([{"id": True}], id="bool-id"), + pytest.param([{"id": None}], id="null-id"), + pytest.param([{"id": 2**63}], id="id-beyond-int64"), # A plain numeric string is NOT here any more, and was wrong to be: - # the normalizer converts it before this guard sees it, and the - # reference accepts it too (Go's generated `Person.Id` is - # `types.FlexibleInt64`). See - # `TestSyncEdit.test_a_participant_with_a_string_id_and_no_personable_type_still_seeds`. + # the reference accepts it (Go's generated `Person.Id` is + # `types.FlexibleInt64`). The normalizer does not reach schedule + # participants, so the guard reads the string itself -- see + # `test_edit_reads_the_participant_ids_the_reference_reads`. # - # A string past int64 still belongs here. The normalizer leaves it - # exactly as it arrived -- that is the RANGE outcome, and the whole - # point of leaving it is that the next reader refuses rather than - # inventing an id -- so this guard is the reader that refuses. + # A string past int64 still belongs here: that is the RANGE outcome + # of the same scan, and the reference's decoder fails the read on + # it rather than inventing an id. pytest.param([{"id": "9223372036854775808"}], id="range-string-id"), ], ) @@ -656,6 +651,38 @@ def test_edit_refuses_malformed_participants_before_writing(self, participants): assert not put_route.called assert respx.calls.call_count == 1 + # The other half of the same guard: a person id the reference ACCEPTS must + # not be refused here. A participant's id is read through the reference's + # flexible decoder, so a bare string id is the number it spells, a + # non-numeric sentinel is the system actor 0, and an absent id is the zero + # value — measured through the reference's own EditEntry composite. + @respx.mock + @pytest.mark.parametrize( + ("participants", "expected"), + [ + pytest.param([{"id": "1049715914"}], [1049715914], id="string-id"), + pytest.param([{"id": "+5"}], [5], id="plus-signed-string-id"), + pytest.param([{"id": "basecamp"}], [0], id="sentinel-id"), + pytest.param([{"name": "no id"}], [0], id="missing-id"), + pytest.param([None], [0], id="null-element"), + ], + ) + def test_edit_reads_the_participant_ids_the_reference_reads(self, participants, expected): + _, put_route = _routes(_entry(participants=participants)) + + with _sync_schedules().edit_entry(entry_id=5001) as e: + # Writing back what the guard read is the ADDRESS, not a no-op: the + # carve-out reaches the wire only when the caller assigns it. It is + # the reference's ``f.SetParticipantIDs(f.ParticipantIDs())``, which + # is how the oracle measured the projection at this site. Held in a + # local so the read is asserted on its own. + read_back = e.participant_ids + assert read_back == expected + e.participant_ids = read_back + + assert put_route.called + assert json.loads(put_route.calls[-1].request.content)["participant_ids"] == expected + @respx.mock @pytest.mark.parametrize("value", [42, {"href": "x"}, True]) def test_edit_refuses_a_non_string_join_url_before_writing(self, value): diff --git a/python/tests/services/test_todos.py b/python/tests/services/test_todos.py index 6b79fefd8..9a52913d0 100644 --- a/python/tests/services/test_todos.py +++ b/python/tests/services/test_todos.py @@ -441,6 +441,10 @@ async def test_async_retries_on_503_then_succeeds(self): _ID_LISTS = ["assignees", "completion_subscribers"] +# The response key each id list is read from, and the request key it is written +# back to. +_ID_LIST_BODY_KEYS = {"assignees": "assignee_ids", "completion_subscribers": "completion_subscriber_ids"} + def _malformed_routes(todo: dict): get_route = respx.get(f"{BASE}/todos/42").mock(return_value=httpx.Response(200, json=todo)) @@ -540,13 +544,18 @@ def test_update_refuses_a_non_object_element_before_writing(self, field): assert not put_route.called @respx.mock - @pytest.mark.parametrize("bad_id", ["100", 10.5, None, True, [], {}]) + @pytest.mark.parametrize("bad_id", [10.5, 10.0, None, True, [], {}, 2**63, -(2**63) - 1]) @pytest.mark.parametrize("field", _ID_LISTS) - def test_update_refuses_a_non_integer_person_id_before_writing(self, field, bad_id): - # The ID lists are resent in full, so a string, float, bool or null id - # would be written as the complete assignee set. `True` matters - # specifically: bool subclasses int in Python, so a naive isinstance - # check passes it. + def test_update_refuses_a_non_person_id_before_writing(self, field, bad_id): + # The ID lists are resent in full, so a float, bool, null or + # out-of-int64 id would be written as the complete assignee set. `True` + # matters specifically: bool subclasses int in Python, so a naive + # isinstance check passes it. `10.0` matters for the same reason one + # level over: it is integral, and the reference still refuses it, because + # `json.Number("10.0").Int64()` is a syntax error there. + # + # A STRING is not in this list and must not be: see + # test_update_reads_a_string_person_id_the_way_the_reference_does. get_route, put_route = _malformed_routes(_todo(**{field: [{"id": bad_id, "name": "Jane"}]})) with pytest.raises(ApiError) as excinfo: @@ -556,6 +565,81 @@ def test_update_refuses_a_non_integer_person_id_before_writing(self, field, bad_ assert get_route.called assert not put_route.called + # Measured through the reference's own todos Update composite: a Person's id + # is the one field the generated model decodes flexibly (types.FlexibleInt64 + # via Person.Id), and `fieldsFromTodo` appends whatever that produced with no + # filter at all. These ids reach this guard exactly as BC3 sent them whenever + # the pre-decode normalizer did not find the person first — and which people + # it finds is the walk's rule, not this guard's: 3 of 7 `assignees` people + # in spec/fixtures omit the `personable_type` marker it keys on. + @respx.mock + @pytest.mark.parametrize( + ("raw_id", "expected"), + [ + ("1049715914", 1049715914), # the live case: a bare numeric string + ("007", 7), # ParseInt takes leading zeros at base 10 + ("-5", -5), + ("+5", 5), # ParseInt takes a leading plus; a `^-?\\d+$` regex does not + ("9223372036854775807", 2**63 - 1), + ("-9223372036854775808", -(2**63)), + ("basecamp", 0), # the LocalPerson sentinel, read as the system actor + ("", 0), + (" 12", 0), # Go trims nothing; int(" 12") would say 12 + ("0x10", 0), # no base detection + ("12.0", 0), + ("1_0", 0), # PEP 515 underscores are not Go digits + ("\uff17", 0), # fullwidth seven: Go tests the ASCII byte range + ("18446744073709551615x", 0), # syntax refusal wins, uint64 max intact + ], + ) + @pytest.mark.parametrize("field", _ID_LISTS) + def test_update_reads_a_string_person_id_the_way_the_reference_does(self, field, raw_id, expected): + get_route, put_route = _malformed_routes(_todo(**{field: [{"id": raw_id, "name": "Jane"}]})) + + _sync_todos().update(todo_id=42, content="New title") + + assert get_route.called + assert _put_body(put_route)[_ID_LIST_BODY_KEYS[field]] == [expected] + + @respx.mock + @pytest.mark.parametrize("raw_id", ["9223372036854775808", "18446744073709551616x"]) + @pytest.mark.parametrize("field", _ID_LISTS) + def test_update_refuses_a_string_person_id_that_overflows_int64(self, field, raw_id): + # The RANGE refusal and the SYNTAX refusal go opposite ways, and which + # one fires depends on where the disqualifying byte sits. + # "18446744073709551615x" is the system actor (0, above); + # "18446744073709551616x" is one digit larger, overflows inside the scan + # before the 'x' is ever reached, and fails the read. + get_route, put_route = _malformed_routes(_todo(**{field: [{"id": raw_id, "name": "Jane"}]})) + + with pytest.raises(ApiError) as excinfo: + _sync_todos().update(todo_id=42, content="New title") + + assert f"Todo field {field!r}[0].id is not a person id" in str(excinfo.value) + assert get_route.called + assert not put_route.called + + @respx.mock + @pytest.mark.parametrize( + ("people", "expected"), + [ + ([{"name": "Jane"}], [0]), # id absent is the zero value, not an error + ([None], [0]), # a null element decodes to the reference's zero Person + ([{"id": 7}, {"id": "basecamp"}], [7, 0]), # one real person, one system actor + ], + ) + @pytest.mark.parametrize("field", _ID_LISTS) + def test_update_writes_back_the_person_shapes_the_reference_writes_back(self, field, people, expected): + # An ABSENT id is 0 with no error while a NULL id fails the read: the + # reference's flexible decoder is only called for a value that is there, + # and its number path turns a JSON null into ParseInt(""), which fails. + get_route, put_route = _malformed_routes(_todo(**{field: people})) + + _sync_todos().update(todo_id=42, content="New title") + + assert get_route.called + assert _put_body(put_route)[_ID_LIST_BODY_KEYS[field]] == expected + @respx.mock @pytest.mark.parametrize("raw", [b"[]", b'"todo"', b"42", b"null", b"true"]) def test_update_refuses_a_non_object_response_before_writing(self, raw): diff --git a/ruby/lib/basecamp/http.rb b/ruby/lib/basecamp/http.rb index 44eada6e3..5a4bc1aa5 100644 --- a/ruby/lib/basecamp/http.rb +++ b/ruby/lib/basecamp/http.rb @@ -28,10 +28,11 @@ class Http # unconverted: 62 of the 74 measured rows of GoPersonIds::CORPUS differed # from the reference for a bare {"creator":{"id":…}}, and the twelve that # agreed did so only because "leave the string" is also what doing nothing - # looks like. The visible cost was on a WRITE: {Services::MergeSafe} requires - # an Integer id, so a merge-safe update of a schedule entry whose + # looks like. The visible cost was on a WRITE: {Services::MergeSafe} then + # required an Integer id, so a merge-safe update of a schedule entry whose # participants came back with string ids raised where the reference reads - # the number and proceeds. + # the number and proceeds. (It now reads a person id itself, through + # {Ids.person_from_wire}; this pass is still what the reference does here.) # # Whichever way a person is found, the id rule is the same one # ({coerce_person_id}, the reference's shared coercePersonID): @@ -123,12 +124,13 @@ def self.embedded_people_url?(url) # # Ruby has no decoder on the generated path — +get+ returns a raw Hash — so # that second route does not exist here, and an "assignees" id that arrives - # as a string stays one. That is a real gap, measured on a write - # (MergeSafe refuses it where the reference completes the update), and it is + # as a string stays one. That gap was measured on a write, and it is # deliberately NOT closed by widening this list: the faithful site is the # reader, field by field against the reference, because some person ids in # the model really are plain int64 there (TemplateLibraryConfirmationPerson) - # and a blanket sweep would break them. Tracked as its own unit of work. + # and a blanket sweep would break them. It is closed at that reader: + # {Services::MergeSafe.writable_id_list} reads a string person id through + # {Ids.person_from_wire}. def self.coerce_embedded_person_ids(obj) creator = obj["creator"] coerce_person_id(creator) if creator.is_a?(Hash) diff --git a/ruby/lib/basecamp/services/merge_safe.rb b/ruby/lib/basecamp/services/merge_safe.rb index 36ae56c5a..e1ef5d456 100644 --- a/ruby/lib/basecamp/services/merge_safe.rb +++ b/ruby/lib/basecamp/services/merge_safe.rb @@ -202,12 +202,25 @@ def writable_boolean(body, key, record:, escape:) # replaces ((body[key] || []).map { |p| p["id"] }) has three ways # to go wrong on malformed data: a non-Array has no +map+ (or, for a Hash, # maps over its pairs), a non-Hash element raises TypeError on +[]+, and a - # non-Integer +id+ rides through verbatim into the full-replace PUT — the + # wrong-typed +id+ rides through verbatim into the full-replace PUT — the # same corruption as a wrong-typed string, one level down. # - # +true+/+false+ are refused explicitly: they are not Integers in Ruby, so - # +is_a?(Integer)+ already rejects them, but the message names them as ids - # rather than as an unexplained type error. + # What it must NOT do is refuse an id the reference ACCEPTS, which is the + # other half of the same defect and the one this guard had (#913). A + # person's id is the single field in the generated model the reference + # decodes flexibly, and +fieldsFromTodo+ appends what that produced with + # no filter of any kind — 0 included. + # + # It must not lean on {Basecamp::Http}'s pre-decode normalizer having + # reached the person first. Which people that walk finds is the walk's + # rule — the reference's positional pass covers +creator+ and + # +participants+ and nothing else — and a person it did not find arrives + # here exactly as BC3 sent it. How often that is a String the fixtures + # cannot say (none of the person objects in +spec/fixtures+ carries one); + # what they show is that the marker the +personable_type+ pass keys on is + # often missing, on 3 of 7 +assignees+ people. The rule itself lives in + # {Basecamp::Ids.person_from_wire}, where the read path already reads one, + # so the two cannot drift. def writable_id_list(body, key, record:, escape:) value = body[key] return [] if value.nil? @@ -225,7 +238,23 @@ def writable_id_list(body, key, record:, escape:) end # Validates one element of an id-list field and returns its id. + # + # Three element shapes are accepted that a strict reading would refuse, + # each because the reference accepts it: + # + # * a +nil+ element is 0 — a JSON null in the array decodes to the + # reference's zero Person, whose id is 0 and is appended like any other; + # * an element with NO "id" is 0, the zero value, with no error; + # * an element whose "id" is a String is whatever ParseInt makes of it, + # and a String that is not a number at all is the system actor 0. + # + # An explicit "id" => nil is NOT the same as an absent one and + # still fails the read, which is why the absent case is tested with + # +key?+ rather than by asking for the value: see + # {Basecamp::Ids.person_from_wire} for the decoder line that splits them. def person_id(element, index, key, record:, escape:) + return 0 if element.nil? + unless element.is_a?(Hash) raise malformed( "#{record} field #{key.inspect}[#{index}] is not an object: #{describe(element)}", @@ -233,17 +262,12 @@ def person_id(element, index, key, record:, escape:) ) end - id = element["id"] - if id.nil? - raise malformed( - "#{record} field #{key.inspect}[#{index}] has no \"id\"", - format(RESEND_HINT, escape: escape) - ) - end + return 0 unless element.key?("id") - unless id.is_a?(Integer) + id = Ids.person_from_wire(element["id"]) + if id.nil? raise malformed( - "#{record} field #{key.inspect}[#{index}].id is not an integer: #{describe(id)}", + "#{record} field #{key.inspect}[#{index}].id is not a person id: #{describe(element["id"])}", format(RESEND_HINT, escape: escape) ) end diff --git a/ruby/test/basecamp/services/schedules_service_test.rb b/ruby/test/basecamp/services/schedules_service_test.rb index 96043ce40..5c10eb6b3 100644 --- a/ruby/test/basecamp/services/schedules_service_test.rb +++ b/ruby/test/basecamp/services/schedules_service_test.rb @@ -371,18 +371,14 @@ def test_update_entry_addresses_participant_ids # Person.Id is types.FlexibleInt64 -- so its merge-safe read sees the number # and completes the update. # - # THIS SDK REFUSES IT, AND THAT IS A KNOWN DIVERGENCE, PINNED rather than left - # to be discovered. Ruby has no decoder on the generated path. The positional - # normalizer briefly covered this by running on every response, and that - # over-reached: the same "creator" / "participants" keys hold plain-int64 - # people on UpcomingScheduleEntry, which the reference refuses and the - # normalizer turned into the system actor. So it was narrowed to the two - # surfaces Go normalizes (gauges, notifications), and schedules is not one. - # - # The refusal is the SAFE direction -- no id is invented and no partial - # participant list is written, because the guard runs before the PUT. Closing - # it properly is decoder coverage, field by field against the reference, and - # PR #913 (card 42) owns it. These are the tests to flip when it lands. + # Ruby has no decoder on the generated path, and the positional normalizer does + # not reach schedules: covering it by running on every response over-reached, + # because the same "creator" / "participants" keys hold plain-int64 people on + # UpcomingScheduleEntry, which the reference refuses. So the string arrives at + # MergeSafe untouched, and MergeSafe reads it through Ids.person_from_wire -- + # decoder coverage at exactly this field, not normalizer reach at every field + # so named. These tests pinned the refusal while that was a known divergence; + # they are flipped to what the reference does. def entry_with_untagged_string_participants full_entry( "participants" => [ @@ -392,32 +388,44 @@ def entry_with_untagged_string_participants ) end - def test_update_entry_refuses_string_participant_ids_the_reference_accepts + def test_update_entry_completes_with_string_participant_ids_as_the_reference_does captured = stub_entry_get_and_put(entry: entry_with_untagged_string_participants) - error = assert_raises(Basecamp::ApiError) do - @account.schedules.update_entry(entry_id: 789, summary: "Team Meeting & Kickoff") + @account.schedules.update_entry(entry_id: 789, summary: "Team Meeting & Kickoff") + + assert_equal "Team Meeting & Kickoff", captured[:bodies].first["summary"] + # Read, not written: participant_ids is a carve-out that reaches the wire + # only when addressed, as it does in the reference. + assert_not captured[:bodies].first.key?("participant_ids") + end + + def test_edit_entry_reads_string_participant_ids_as_the_reference_does + captured = stub_entry_get_and_put(entry: entry_with_untagged_string_participants) + + @account.schedules.edit_entry(entry_id: 789) do |fields| + read_back = fields.participant_ids + assert_equal [ 1049715914, 1049715915 ], read_back + fields.participant_ids = read_back end - assert_match(/"participants"\[0\]\.id is not an integer/, error.message) - # No PUT: a refused read never becomes a partial update. - assert_empty captured[:bodies] + assert_equal [ 1049715914, 1049715915 ], captured[:bodies].first["participant_ids"] end - def test_edit_entry_refuses_a_sentinel_participant_rather_than_inventing_the_system_actor - # The reference reads "basecamp" here as person 0, through FlexibleInt64. - # This SDK does not normalize schedules, so it refuses -- and it must not - # get to 0 by some other route, which would be the accepting-direction - # mistake the narrowing exists to remove. + def test_edit_entry_reads_a_sentinel_participant_as_the_system_actor_the_reference_names + # The reference reads "basecamp" here as person 0, through FlexibleInt64, and + # appends it. That is its DECODER's rule at this field, which is the route + # MergeSafe takes now -- not the normalizer's, whose over-reach was minting 0 + # on UpcomingScheduleEntry, a plain-int64 person the reference refuses. + # MergeSafe never reads that schema. Confirmed with the operator on card 42: + # follow the reference. entry = full_entry("participants" => [ { "id" => "basecamp", "name" => "Basecamp" } ]) captured = stub_entry_get_and_put(entry: entry) - assert_raises(Basecamp::ApiError) do - @account.schedules.edit_entry(entry_id: 789) do |fields| - fields.participant_ids = fields.participant_ids - end + @account.schedules.edit_entry(entry_id: 789) do |fields| + fields.participant_ids = fields.participant_ids end - assert_empty captured[:bodies] + + assert_equal [ 0 ], captured[:bodies].first["participant_ids"] end # notify is a directive, not state: it has nothing in the read-back to seed @@ -799,11 +807,12 @@ def test_update_entry_refuses_a_malformed_join_url [ [ "a non-array", "nope", %(Schedule entry field "participants" is not an array) ], [ "a non-object element", [ 42 ], %(Schedule entry field "participants"[0] is not an object) ], - [ "an element with no id", [ { "name" => "Victor" } ], %(Schedule entry field "participants"[0] has no "id") ], - [ "an out of range id", [ { "id" => "18446744073709551616" } ], - %(Schedule entry field "participants"[0].id is not an integer) ], - [ "a float id", [ { "id" => 12.5 } ], %(Schedule entry field "participants"[0].id is not an integer) ], - [ "a boolean id", [ { "id" => true } ], %(Schedule entry field "participants"[0].id is not an integer) ] + [ "a boolean id", [ { "id" => true } ], %(Schedule entry field "participants"[0].id is not a person id) ], + [ "a null id", [ { "id" => nil } ], %(Schedule entry field "participants"[0].id is not a person id) ], + [ "a float id", [ { "id" => 12.5 } ], %(Schedule entry field "participants"[0].id is not a person id) ], + [ "an id beyond int64", [ { "id" => 2**63 } ], %(Schedule entry field "participants"[0].id is not a person id) ], + [ "an out of range string id", [ { "id" => "18446744073709551616" } ], + %(Schedule entry field "participants"[0].id is not a person id) ] ].each do |label, participants, message| define_method("test_update_entry_refuses_#{label.tr(" ", "_")}_in_participants") do captured = stub_entry_get_and_put(entry: full_entry("participants" => participants)) @@ -818,6 +827,37 @@ def test_update_entry_refuses_a_malformed_join_url end end + # The other half of the same guard: a participant id the reference ACCEPTS + # must not be refused here. A person's id is the one field the generated model + # decodes flexibly, so a bare string id is the number it spells, a non-numeric + # sentinel is the system actor 0, an absent id is the zero value and a null + # ELEMENT is the zero Person. Measured through the reference's own EditEntry + # composite; an explicit "id" => nil is NOT absent and still fails the + # read, which is the row above. + [ [ "a bare string id", [ { "id" => "1049715914" } ], [ 1049715914 ] ], + [ "a plus-signed string id", [ { "id" => "+5" } ], [ 5 ] ], + [ "a sentinel id", [ { "id" => "basecamp" } ], [ 0 ] ], + [ "an element with no id", [ { "name" => "Victor" } ], [ 0 ] ], + [ "a null element", [ nil ], [ 0 ] ], + [ "one person and one system actor", [ { "id" => 7 }, { "id" => "basecamp" } ], [ 7, 0 ] ] + ].each do |label, participants, expected| + # The seed only reaches the wire on an explicit address, so the block + # re-addresses it with exactly what the guard read — the reference's + # `f.SetParticipantIDs(f.ParticipantIDs())`, which is how the oracle + # measured the projection at this site. + define_method("test_edit_entry_reads_#{label.tr(" ", "_")}_the_way_the_reference_does") do + captured = stub_entry_get_and_put(entry: full_entry("participants" => participants)) + + @account.schedules.edit_entry(entry_id: 789) do |e| + read_back = e.participant_ids + assert_equal expected, read_back + e.participant_ids = read_back + end + + assert_equal expected, captured[:bodies].first["participant_ids"] + end + end + # One level up from the field guards: a successful GET can return a scalar, an # Array or nil, and body["summary"] would raise a raw TypeError on an # Integer or Array — or return a silent nil substring match on a String — diff --git a/ruby/test/basecamp/services/todos_service_test.rb b/ruby/test/basecamp/services/todos_service_test.rb index ed4c2afa3..13fbed045 100644 --- a/ruby/test/basecamp/services/todos_service_test.rb +++ b/ruby/test/basecamp/services/todos_service_test.rb @@ -340,6 +340,11 @@ def test_description_attachment_dimensions_decode WRITABLE_STRINGS = %w[content description due_on starts_on].freeze ID_LIST_FIELDS = %w[assignees completion_subscribers].freeze + # The response key each id list is read from, and the request key it is + # written back to. + ID_LIST_BODY_KEYS = { "assignees" => "assignee_ids", + "completion_subscribers" => "completion_subscriber_ids" }.freeze + WRITABLE_STRINGS.each do |field| MALFORMED_VALUES.each do |malformed| define_method("test_update_refuses_a_malformed_#{field}_#{malformed.inspect}") do @@ -422,10 +427,12 @@ def test_description_attachment_dimensions_decode assert_not_requested :put, "#{BASE_URL}/12345/todos/456" end - # The id lists are resent in full, so a string, float or null id would be - # written as the complete assignee set. - [ "100", 10.5, nil, true, [], {} ].each do |bad_id| - define_method("test_update_refuses_a_non_integer_#{field}_id_#{bad_id.inspect}") do + # The id lists are resent in full, so a float, boolean, null or + # out-of-int64 id would be written as the complete assignee set. A STRING is + # not in this list and must not be: the reference decodes a person id + # flexibly, so it reads one — see the acceptance cases below. + [ 10.5, 10.0, nil, true, [], {}, 2**63, -(2**63) - 1, "9223372036854775808" ].each do |bad_id| + define_method("test_update_refuses_a_non_person_#{field}_id_#{bad_id.inspect}") do stub_todo_get_and_put(todo: full_todo(field => [ { "id" => bad_id, "name" => "Jane" } ])) error = assert_raises(Basecamp::ApiError) do @@ -436,6 +443,51 @@ def test_description_attachment_dimensions_decode assert_not_requested :put, "#{BASE_URL}/12345/todos/456" end end + + # The other half of the same guard. Measured through the reference's own + # todos Update composite: Person#id is the one field the generated model + # decodes flexibly, and fieldsFromTodo appends what that produced with no + # filter at all. These ids reach the guard exactly as BC3 sent them whenever + # the pre-decode normalizer did not find the person first, and which people + # it finds is the walk's rule, not the guard's: 3 of 7 `assignees` people in + # spec/fixtures omit the `personable_type` marker it keys on. + { "1049715914" => 1049715914, # the live case: a bare numeric string + "007" => 7, # ParseInt takes leading zeros at base 10 + "-5" => -5, + "+5" => 5, # ...and a leading plus, which /\A-?\d+\z/ does not + "9223372036854775807" => (2**63) - 1, + "-9223372036854775808" => -(2**63), + "basecamp" => 0, # the LocalPerson sentinel, read as the system actor + "" => 0, + " 12" => 0, # Go trims nothing; Integer(" 12") would say 12 + "0x10" => 0, # ...and would say 16 here + "010" => 10, # ...and 8 here: not a refusal, a different PERSON + "12.0" => 0, + "1_0" => 0, + "\uFF17" => 0 }.each do |raw, expected| # fullwidth seven + define_method("test_update_reads_the_#{field}_id_#{raw.inspect}_as_the_reference_does") do + captured = stub_todo_get_and_put(todo: full_todo(field => [ { "id" => raw, "name" => "Jane" } ])) + + @account.todos.update(todo_id: 456, content: "New title") + + assert_equal [ expected ], captured[:body][ID_LIST_BODY_KEYS.fetch(field)] + end + end + + # An ABSENT id is the zero value with no error, while a NULL one fails the + # read: the reference's flexible decoder is only called for a value that is + # there, and its number path turns a JSON null into ParseInt(""), which + # fails. A null ELEMENT decodes to the zero Person, id and all. + { "an element with no id" => [ { "name" => "Jane" } ], + "a null element" => [ nil ] }.each do |label, people| + define_method("test_update_writes_back_#{label.tr(" ", "_")}_in_#{field}_as_zero") do + captured = stub_todo_get_and_put(todo: full_todo(field => people)) + + @account.todos.update(todo_id: 456, content: "New title") + + assert_equal [ 0 ], captured[:body][ID_LIST_BODY_KEYS.fetch(field)] + end + end end # One level up from the field guards: a successful GET can return a scalar, diff --git a/typescript/src/person-id.ts b/typescript/src/person-id.ts index 5ec087d95..c3a609033 100644 --- a/typescript/src/person-id.ts +++ b/typescript/src/person-id.ts @@ -1,13 +1,15 @@ /** * Go's `strconv.ParseInt(s, 10, 64)` over a person id, written out. * - * Two places in this SDK turn a person id that arrived as a JSON STRING into a - * number, and they answer to the same Go line: the pre-decode normalizer in + * Three places in this SDK turn a person id that arrived as a JSON STRING into + * a number, and they answer to the same Go line: the pre-decode normalizer in * `services/base.ts` stands where `coercePersonID` stands * (`go/pkg/basecamp/normalize.go:45`), and the read-side comparison in - * `services/mentions.ts` stands where `FlexibleInt64` stands - * (`go/pkg/types/flexible_int64.go:34`). Both call {@link scanPersonId}, so the - * rule cannot drift into two rules that disagree about the same string. + * `services/mentions.ts` and the merge-safe id-list guard in + * `services/merge-safe.ts` stand where `FlexibleInt64` stands + * (`go/pkg/types/flexible_int64.go:34`). All of them call + * {@link scanPersonId}, so the rule cannot drift into rules that disagree about + * the same string. * * Do not reach for `Number()`, `parseInt()`, or a `^-?\d+$` test in their place. * Each is wrong in a direction that matters and the three are wrong differently: diff --git a/typescript/src/services/base.ts b/typescript/src/services/base.ts index 3c8829e60..7a029f540 100644 --- a/typescript/src/services/base.ts +++ b/typescript/src/services/base.ts @@ -239,8 +239,9 @@ function normalizePersonIds(obj: unknown): void { * The gap that widening was covering is real and is NOT closed here: `assignees`, * `subscribers` and `completion_subscribers` carry string ids that this SDK now * leaves as strings, where Go's decoder converts them. That is decoder coverage - * rather than normalizer reach, it needs a per-field audit against the - * reference, and it belongs to PR #913. + * rather than normalizer reach. On the WRITE path it is closed at the reader — + * `writableIdList` in `merge-safe.ts` reads the id by the same scan (PR #913). + * On a plain generated read it is still open: see SPEC.md section 10, "Person Ids Off the Wire". */ function normalizeEmbeddedPersonIds(obj: unknown): void { if (!obj || typeof obj !== "object") return; diff --git a/typescript/src/services/merge-safe.ts b/typescript/src/services/merge-safe.ts index ccc93b2af..148d6f404 100644 --- a/typescript/src/services/merge-safe.ts +++ b/typescript/src/services/merge-safe.ts @@ -31,6 +31,7 @@ * intended end state for all of them. */ import { Errors, truncateErrorMessage, type BasecampError } from "../errors.js"; +import { personIdNumber, scanPersonId } from "../person-id.js"; const resendHint = (escape: string): string => "The merge-safe update/edit resend this field verbatim, so a coerced or empty value " + @@ -225,14 +226,22 @@ export function writableBoolean( * The analogue of {@link writableString} for the ID-list fields. The `.map()` * it replaces (`(body[key] ?? []).map((p) => p.id)`) has three ways to go wrong * on malformed data: a non-array has no `.map` (a raw `TypeError`), a - * non-object element yields `undefined`, and a non-integer `id` rides through + * non-object element yields `undefined`, and a wrong-typed `id` rides through * verbatim into the full-replace PUT — the same corruption as a wrong-typed * string, one level down. * - * `Number.isInteger` is the test rather than `typeof === "number"`: `1.5` and - * `NaN` are numbers and neither is a person ID. Booleans fail it outright, - * which is what we want — JavaScript would happily coerce `true` to `1` - * downstream. + * What it must NOT do is refuse an id the reference accepts. A person id is the + * one field the reference decodes flexibly (`Person.Id` is + * `types.FlexibleInt64`), and its projection appends what that produced with no + * filter, `0` included. So a STRING id is read by the shared `ParseInt` scan, + * an absent id is `0` and a `null` element is `0`; the per-branch comments + * below give the reasons. + * + * Two residuals are JavaScript's, not choices this could make differently: a + * number past 2^53 is refused (`Number.isSafeInteger`) rather than written as a + * rounded, different person's id, where the reference reads it; and a JSON + * number spelled `1024.0` or `1e3` is accepted, where the reference refuses it, + * because `JSON.parse` has already made it an integer. */ export function writableIdList( body: Record, @@ -248,25 +257,66 @@ export function writableIdList( ); } return value.map((element: unknown, index: number) => { - if (typeof element !== "object" || element === null || Array.isArray(element)) { + // A null element is 0, not a malformed one. The reference decodes a JSON + // null in this array into its zero `Person`, whose `Id` is 0, and + // `fieldsFromTodo` appends that like any other id — measured through the + // reference's own Update composite. + if (element === null) return 0; + if (typeof element !== "object" || Array.isArray(element)) { throw malformedResponse( `${opts.record} field "${key}"[${index}] is not an object: ${describeValue(element)}`, resendHint(opts.escape) ); } - const id = (element as Record)["id"]; - if (id === undefined || id === null) { - throw malformedResponse( - `${opts.record} field "${key}"[${index}] has no "id"`, - resendHint(opts.escape) - ); - } - if (typeof id !== "number" || !Number.isInteger(id)) { + const record = element as Record; + // An ABSENT id is the zero value with no error, while an explicit null + // FAILS the read. The reference's flexible decoder is only called for a + // value that is there; for a JSON null it IS called, its number path leaves + // the buffer empty, and `ParseInt("")` fails. Two different answers, so the + // two cases cannot be tested together with `id == null`. + if (!("id" in record)) return 0; + const id = record["id"]; + const refuse = (): never => { throw malformedResponse( - `${opts.record} field "${key}"[${index}].id is not an integer: ${describeValue(id)}`, + `${opts.record} field "${key}"[${index}].id is not a person id: ${describeValue(id)}`, resendHint(opts.escape) ); + }; + // A NUMBER id. `Number.isInteger` is the test rather than `typeof`: `1.5` + // and `NaN` are numbers and neither is a person id, and a boolean fails it + // outright, which is what we want — JavaScript would coerce `true` to 1 + // downstream. `Number.isSafeInteger` is the second half and is the same + // judgment `personIdNumber` makes for a string: past 2^53 `JSON.parse` has + // already rounded, so the only honest answers are "unreadable" or a wrong + // id, and the reference refuses this row anyway once it is past int64. + if (typeof id === "number") return Number.isSafeInteger(id) ? id : refuse(); + // A STRING id. BC3 serializes person ids as strings in some responses, and + // this reader must not lean on the pre-decode normalizer having converted + // one: which keys that walk covers is the walk's rule, and it is being held + // to the reference's own positional surfaces. The reference has no gap + // here either way, because it covers this site with a DECODER — `Person.Id` + // is `types.FlexibleInt64`, and `fieldsFromTodo` appends what that produced + // without filtering. Same grammar as the walk, so the two cannot disagree + // about a person depending on which of them saw it first. + if (typeof id === "string") { + const scan = scanPersonId(id); + // A SYNTAX refusal is the system actor, not an error: that is what the + // reference reads `"basecamp"` as, and the RANGE refusal beside it fails + // the read. Which one a string earns is decided inside the scan, so they + // cannot be told apart after the fact by looking at the string. + if (scan.kind === "syntax") return 0; + if (scan.kind === "range") return refuse(); + // Outside ±(2^53 - 1) the number is already rounded, so refusing the ID + // is the local resolution `personIdNumber` documents — a residual + // divergence from a reference that answers in `int64`, recorded rather + // than papered over with a wrong id. + return personIdNumber(scan.value) ?? refuse(); } - return id; + // Everything else is a decode failure at the reference, an explicit + // `"id": null` included: the flexible decoder IS called for a JSON null, + // its number path leaves the buffer empty, and `ParseInt("")` fails. An + // absent key never reaches here, and is 0 — the two cannot be tested + // together with `id == null`. + return refuse(); }); } diff --git a/typescript/tests/services/merge-safe.test.ts b/typescript/tests/services/merge-safe.test.ts new file mode 100644 index 000000000..94aeffd79 --- /dev/null +++ b/typescript/tests/services/merge-safe.test.ts @@ -0,0 +1,88 @@ +/** + * The id-list guard on its own, with no normalizer walk in front of it. + * + * The composite tests in todos.test.ts and schedules.test.ts go through the + * transport, so what they prove depends on which keys the pre-decode walk + * covers: a walk that converted a string id first would let them pass whether + * or not the guard reads one. That happened once, while the walk still named + * `assignees`. These call `writableIdList` directly, so the guard is pinned on + * its own whatever the walk does. + * + * Every expectation is the reference's, measured through its own todos Update + * and schedules EditEntry composites (`Person.Id` is `types.FlexibleInt64`, and + * `fieldsFromTodo` appends whatever that produced with no filter) — with two + * exceptions, both JavaScript's number boundary, marked where they appear. An + * id past 2^53 the reference reads as a number is refused here, because it has + * already been rounded and the alternative is writing a different person's id. + * And a JSON number spelled `1024.0` or `1e3`, which the reference refuses, is + * accepted: `JSON.parse` hands the guard `1024` and `1000`, so there is nothing + * left to tell apart. That second row is pinned below too, so a change in + * either direction is a decision rather than an accident. + */ +import { describe, it, expect } from "vitest"; +import { writableIdList } from "../../src/services/merge-safe.js"; +import { BasecampError } from "../../src/errors.js"; + +const opts = { record: "Todo", escape: "replace()" }; + +const read = (people: unknown) => writableIdList({ assignees: people }, "assignees", opts); + +describe("writableIdList", () => { + it.each([ + ["a number", 1049715914, 1049715914], + ["a bare numeric string", "1049715914", 1049715914], + ["leading zeros", "007", 7], + ["a minus sign", "-5", -5], + ["a plus sign", "+5", 5], // which /^-?\d+$/ rejects and ParseInt accepts + ["the LocalPerson sentinel", "basecamp", 0], + ["an empty string", "", 0], + ["a leading space", " 12", 0], // Go trims nothing; Number(" 12") says 12 + ["a hex literal", "0x10", 0], + ["an exponent", "1e3", 0], // Number("1e3") says 1000 + ["a decimal point", "12.0", 0], + ["a numeric separator", "1_0", 0], + ["a fullwidth digit", "7", 0], + ["junk after uint64 max", "18446744073709551615x", 0], // syntax wins the scan + ["the largest safe integer", "9007199254740991", 9007199254740991], + ])("reads %s the way the reference does", (_label, id, expected) => { + expect(read([{ id, name: "Jane" }])).toEqual([expected]); + }); + + it("reads an absent id as the zero value, and a null element as the zero Person", () => { + expect(read([{ name: "Jane" }, null, { id: 7 }])).toEqual([0, 0, 7]); + }); + + it.each([ + // RANGE: the reference fails the read. "…616x" overflows inside the scan + // before the 'x' is reached, one digit past the "…615x" sentinel above. + ["a string overflowing int64", "9223372036854775808"], + ["overflow before the junk", "18446744073709551616x"], + // Past 2^53 the value is representable to the reference but not here, so + // the id is refused rather than rounded into a different person's. + ["a string past 2^53", "9007199254740993"], + ["a number past 2^53", 9007199254740994], + // An explicit null reaches the flexible decoder and fails; only ABSENT is 0. + ["a null id", null], + ["a boolean id", true], + ["a fractional id", 10.5], + ["NaN", Number.NaN], + ["an object id", {}], + ["an array id", [1]], + ])("refuses %s", (_label, id) => { + expect(() => read([{ id, name: "Jane" }])).toThrow(BasecampError); + expect(() => read([{ id, name: "Jane" }])).toThrow(/"assignees"\[0\]\.id is not a person id/); + }); + + it("accepts an integral-float or exponent JSON number, which the reference refuses", () => { + // A RESIDUAL DIVERGENCE, pinned so it is not mistaken for correctness: Go's + // decoder refuses `1024.0` and `1e3` as person ids. `JSON.parse` has already + // turned both into integers, so they are indistinguishable here. + const parsed = JSON.parse('{"assignees":[{"id":1024.0},{"id":1e3}]}') as Record; + expect(writableIdList(parsed, "assignees", opts)).toEqual([1024, 1000]); + }); + + it("refuses a non-object element and a non-array field", () => { + expect(() => read([5])).toThrow(/"assignees"\[0\] is not an object/); + expect(() => read({ id: 7 })).toThrow(/"assignees" is not an array/); + }); +}); diff --git a/typescript/tests/services/schedules.test.ts b/typescript/tests/services/schedules.test.ts index f43aab615..c4be0e41d 100644 --- a/typescript/tests/services/schedules.test.ts +++ b/typescript/tests/services/schedules.test.ts @@ -1182,16 +1182,58 @@ describe("SchedulesService", () => { expectResponseError(error, /Schedule entry field "highlighted" is not a boolean/, requests); }); - it("editEntry refuses a malformed participants list before writing", async () => { + it("editEntry refuses a null participant id before writing", async () => { const requests: string[] = []; - serve(sampleEntry(4101, { participants: [{ name: "No ID" }] }), requests); + serve(sampleEntry(4101, { participants: [{ id: null, name: "No ID" }] }), requests); const error = await rejection( service.editEntry(4101, (e) => { e.summary = "New summary"; }), ); - expectResponseError(error, /Schedule entry field "participants"\[0\] has no "id"/, requests); + expectResponseError(error, /Schedule entry field "participants"\[0\]\.id is not a person id/, requests); + }); + + // The other half of the same guard, and the reason the null above is NOT + // tested together with an absent id: the reference answers the two + // differently. A missing `id` never reaches its flexible decoder and is the + // zero value; a JSON null DOES reach it and fails the read. A null ELEMENT + // decodes to the zero `Person`, id and all. Both measured through the + // reference's own EditEntry composite. + it.each([ + ["an element with no id", [{ name: "No ID" }]], + ["a null element", [null]], + ])("editEntry reads %s as the system actor 0", async (_label, participants) => { + const requests: string[] = []; + let putBody: Record = {}; + server.use( + http.get(`${BASE_URL}/schedule_entries/4101`, () => { + requests.push("GET"); + return HttpResponse.json(sampleEntry(4101, { participants })); + }), + http.put(`${BASE_URL}/schedule_entries/4101`, async ({ request }) => { + requests.push("PUT"); + putBody = (await request.json()) as Record; + return HttpResponse.json(sampleEntry(4101)); + }), + ); + + await service.editEntry(4101, (e) => { + // Writing back what the guard read is the ADDRESS, not a no-op: the + // carve-out reaches the wire only when the setter fires, and + // `editEntry` tracks that through a Proxy rather than by diffing a + // snapshot, precisely so that assigning the read-back value still + // counts as asking for a write. It is the reference's + // `f.SetParticipantIDs(f.ParticipantIDs())`, which is how the oracle + // measured the projection at this site. Held in a local so the read is + // asserted on its own, and so the assignment is not a self-assignment. + const readBack = e.participantIds; + expect(readBack).toEqual([0]); + e.participantIds = readBack; + }); + + expect(requests).toEqual(["GET", "PUT"]); + expect(putBody.participant_ids).toEqual([0]); }); // One level up from the field guards: a successful GET can return a diff --git a/typescript/tests/services/todos.test.ts b/typescript/tests/services/todos.test.ts index b339b4984..1f2a31b4b 100644 --- a/typescript/tests/services/todos.test.ts +++ b/typescript/tests/services/todos.test.ts @@ -555,6 +555,12 @@ describe("TodosService", () => { const writableStrings = ["content", "description", "due_on", "starts_on"] as const; const idLists = ["assignees", "completion_subscribers"] as const; + // The response key each id list is read from, and the request key it is + // written back to. + const idListBodyKeys: Record = { + assignees: "assignee_ids", + completion_subscribers: "completion_subscriber_ids", + }; // Serve a GET carrying `body` and a PUT that records that it happened. // `body` is typed as MSW's own response-body type rather than `unknown`: @@ -681,25 +687,18 @@ describe("TodosService", () => { // The ID lists are resent in full, so a float, boolean or null id would // be written as the complete assignee set. // - // A STRING id is on this list and its presence is a KNOWN DIVERGENCE, - // pinned here rather than left to be discovered. BC3 serializes person - // ids as strings on some payloads, and `generated.Person.Id` is a - // `types.FlexibleInt64`, so the reference reads `"007"` as person 7 and - // completes the update; this refuses it. - // - // The normalizer briefly converted it, by walking `assignees` on every - // response. That over-reached — the same key name carries plain-`int64` - // people on other schemas, which the reference refuses and this turned - // into the system actor — so the walk was narrowed to the two keys and - // the two services Go actually normalizes. Closing this properly is - // decoder coverage, field by field against the reference, not normalizer - // reach: PR #913 (card 42) owns it. + // A STRING id is not on this list. BC3 serializes person ids as strings + // on some payloads, and `generated.Person.Id` is a `types.FlexibleInt64`, + // so the reference reads `"007"` as person 7 and completes the update. + // The normalizer does not reach `assignees` — that key carries a plain + // `int64` person on another schema, so walking it by name over-reached — + // and the guard reads the id itself instead, which is decoder coverage at + // exactly this field rather than normalizer reach at every field so named. it.each([ ["float", 10.5], ["NaN", Number.NaN], ["null", null], ["boolean", true], - ["string", "007"], ])(`update refuses a %s ${field} id before writing`, async (_label, badId) => { const requests: string[] = []; serve(fullTodo(42, { [field]: [{ id: badId, name: "Jane" }] }), requests); @@ -708,28 +707,121 @@ describe("TodosService", () => { expectResponseError(error, new RegExp(`Todo field "${field}"\\[0\\]`), requests); }); - it(`refuses a string ${field} id that the reference accepts, and PR #913 owns`, async () => { - // THE DIVERGENCE, STATED AS A TEST so it cannot be mistaken for - // correctness. Go reads `"007"` as person 7 through `FlexibleInt64` and - // completes this update. This SDK has no decoder, the positional - // normalizer no longer reaches `assignees` (it never should have: that - // key carries `MyAssignmentAssignee`, a plain `int64`, on another - // schema), and so the merge-safe guard refuses the response. - // - // The refusal is at least honest -- it does not invent an id or write a - // partial assignee set -- and it is the pre-existing gap card 42 was - // filed for. When PR #913 lands the decoder coverage, this test is the - // one to flip. + it(`completes an update whose ${field} id arrived as a string, as the reference does`, async () => { + // The divergence this test used to pin, flipped. Go reads `"007"` as + // person 7 through `FlexibleInt64` and completes this update. The + // positional normalizer does not reach `assignees` (that key carries + // `MyAssignmentAssignee`, a plain `int64`, on another schema), so the + // string arrives at the merge-safe guard untouched — and the guard now + // reads it by the same scan the normalizer uses, rather than refusing. const requests: string[] = []; - serve(fullTodo(42, { [field]: [{ id: "007", name: "Jane" }] }), requests); + let putBody: Record = {}; + server.use( + http.get(`${BASE_URL}/todos/42`, () => { + requests.push("GET"); + return HttpResponse.json(fullTodo(42, { [field]: [{ id: "007", name: "Jane" }] }) as JsonBodyType); + }), + http.put(`${BASE_URL}/todos/42`, async ({ request }) => { + requests.push("PUT"); + putBody = (await request.json()) as Record; + return HttpResponse.json(fullTodo(42) as JsonBodyType); + }) + ); - const error = await rejection(client.todos.update(42, { content: "New title" })); - expectResponseError(error, new RegExp(`Todo field "${field}"\\[0\\]`), requests); + await client.todos.update(42, { content: "New title" }); - // No PUT happened: the guard runs before the write, so a refused read - // never turns into a partial update. - expect(requests).toEqual(["GET"]); + expect(requests).toEqual(["GET", "PUT"]); + expect(putBody[idListBodyKeys[field]]).toEqual([7]); }); + + // A string id read the way the reference reads one, through the whole + // composite. The normalizer does not walk `assignees`, so these ids reach + // the guard as strings and the guard is what reads them; merge-safe.test.ts + // pins the guard on its own as well, independent of what any walk covers. + // Measured through the reference's own Update composite. + it.each([ + ["a bare numeric string", "1049715914", 1049715914], + ["leading zeros", "007", 7], + ["a minus sign", "-5", -5], + ["a plus sign", "+5", 5], // which /^-?\d+$/ rejects and ParseInt accepts + ["the LocalPerson sentinel", "basecamp", 0], + ["an empty string", "", 0], + ["a leading space", " 12", 0], // Go trims nothing; Number(" 12") says 12 + ["a hex literal", "0x10", 0], + ["a decimal point", "12.0", 0], + ["a numeric separator", "1_0", 0], + ["a fullwidth digit", "\uFF17", 0], + ["junk after uint64 max", "18446744073709551615x", 0], // syntax wins the scan + ])( + `update reads %s ${field} id the way the reference does`, + async (_label, rawId, expected) => { + let putBody: Record = {}; + server.use( + http.get(`${BASE_URL}/todos/42`, () => + HttpResponse.json(fullTodo(42, { [field]: [{ id: rawId, name: "Jane" }] }) as JsonBodyType) + ), + http.put(`${BASE_URL}/todos/42`, async ({ request }) => { + putBody = (await request.json()) as Record; + return HttpResponse.json(fullTodo(42) as JsonBodyType); + }) + ); + + await client.todos.update(42, { content: "New title" }); + + expect(putBody[idListBodyKeys[field]]).toEqual([expected]); + } + ); + + // The RANGE refusal, which goes the other way from the SYNTAX one above + // and is decided one digit earlier: "…615x" is the system actor, "…616x" + // overflows inside the scan before the 'x' is reached and fails the read. + it.each([["9223372036854775808"], ["18446744073709551616x"]])( + `update refuses the ${field} id %s, whose digits overflow int64`, + async (rawId) => { + const requests: string[] = []; + serve(fullTodo(42, { [field]: [{ id: rawId, name: "Jane" }] }), requests); + + const error = await rejection(client.todos.update(42, { content: "New title" })); + expectResponseError( + error, + new RegExp(`Todo field "${field}"\\[0\\]\\.id is not a person id`), + requests + ); + } + ); + + // Two shapes the reference WRITES BACK rather than refusing, measured + // through its own Update composite: an element with no `id` is the zero + // value of the flexible decoder, and a null ELEMENT decodes to the zero + // `Person`. An explicit `"id": null` is neither — it reaches the decoder, + // whose number path fails on an empty buffer — which is why it stays in + // the refusal table above. + it.each([ + ["an element with no id", [{ name: "Jane" }]], + ["a null element", [null]], + ])( + `update writes back %s in ${field} as the system actor 0`, + async (_label, people) => { + const requests: string[] = []; + let putBody: Record = {}; + server.use( + http.get(`${BASE_URL}/todos/42`, () => { + requests.push("GET"); + return HttpResponse.json(fullTodo(42, { [field]: people }) as JsonBodyType); + }), + http.put(`${BASE_URL}/todos/42`, async ({ request }) => { + requests.push("PUT"); + putBody = (await request.json()) as Record; + return HttpResponse.json(fullTodo(42) as JsonBodyType); + }) + ); + + await client.todos.update(42, { content: "New title" }); + + expect(requests).toEqual(["GET", "PUT"]); + expect(putBody[idListBodyKeys[field]]).toEqual([0]); + } + ); } // One level up from the field guards: a successful GET can return a