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