From 085cca9d8b0d5983abf23488a1ba01e0704327ba Mon Sep 17 00:00:00 2001 From: "man8-octoflow[bot]" <317008790+man8-octoflow[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:56:52 +0200 Subject: [PATCH 1/3] fix(models): drop range constraints from server-read join fields RoleEffort's six effort/time roll-ups and Time's spent/remain are supplied by the server, and AssignableEntity already states the rule for that class of field: a constraint on a server-supplied value fails the whole entity rather than the field, so one out-of-range record would abort a whole list() page with a ParseError. These two join models were asserting an invariant the API never promised. Live probing found the constraints dormant rather than firing, since the instance clamps every over-spend at zero, so this removes a latent failure rather than an active one. A consumer running against an instance or version that does not clamp the same way would hit it. Time's project description now states what a custom-activity entry holds. TP populates project from the custom activity's own project when there is no Assignable: over 445 live custom-activity entries, every one carried a non-null project, and on every one it equalled that activity's own. times.upsert keeps its write-side validation, which is where this library's own reasoning puts a range check, and its docstring now says why it is the only one. Co-Authored-By: Claude Opus 5 (1M context) --- src/targetprocess/_joins.py | 63 +++++++++++++++++++--------- src/targetprocess/resources/times.py | 8 ++-- tests/test_role_effort.py | 23 ++++++++-- tests/test_time.py | 26 +++++++++--- 4 files changed, 87 insertions(+), 33 deletions(-) diff --git a/src/targetprocess/_joins.py b/src/targetprocess/_joins.py index a7a8921..ab25e7d 100644 --- a/src/targetprocess/_joins.py +++ b/src/targetprocess/_joins.py @@ -73,33 +73,38 @@ class RoleEffort(Entity): Breaks an Assignable's effort down by Role. + Every numeric field here is a server-supplied roll-up, so none carries a + range constraint - the rule ``AssignableEntity`` states for the same class + of field: a constraint on a value TP computes fails the **whole entity** + rather than the field, so one out-of-range roll-up would abort a whole + ``list()`` page with a ``ParseError``. Write-side range checks belong where + the value originates - see ``times.upsert``. + Attributes: - initial_estimate: Initial effort estimate >= 0 - effort: Total effort >= 0 - effort_completed: Completed effort >= 0 - effort_todo: Remaining effort >= 0 - time_spent: Time spent >= 0 - time_remain: Time remaining >= 0 + initial_estimate: Initial effort estimate + effort: Total effort + effort_completed: Completed effort + effort_todo: Remaining effort + time_spent: Time spent + time_remain: Time remaining assignable: Assignable (work item) reference role: Role reference """ # Effort / time tracking initial_estimate: float | None = Field( - default=None, alias="InitialEstimate", ge=0, description="Initial estimate" + default=None, alias="InitialEstimate", description="Initial estimate" ) - effort: float | None = Field(default=None, alias="Effort", ge=0, description="Total effort") + effort: float | None = Field(default=None, alias="Effort", description="Total effort") effort_completed: float | None = Field( - default=None, alias="EffortCompleted", ge=0, description="Completed effort" + default=None, alias="EffortCompleted", description="Completed effort" ) effort_todo: float | None = Field( - default=None, alias="EffortToDo", ge=0, description="Remaining effort" - ) - time_spent: float | None = Field( - default=None, alias="TimeSpent", ge=0, description="Time spent" + default=None, alias="EffortToDo", description="Remaining effort" ) + time_spent: float | None = Field(default=None, alias="TimeSpent", description="Time spent") time_remain: float | None = Field( - default=None, alias="TimeRemain", ge=0, description="Time remaining" + default=None, alias="TimeRemain", description="Time remaining" ) # Relationships @@ -157,15 +162,29 @@ class Time(Entity): ``assignable``). They are declared so an entry's origin is readable without a second fetch. + ``project`` is populated whichever shape the entry takes, so a null + ``assignable`` does not imply a null project: TP takes it from the + Assignable for a work-item entry, and from the custom activity's own + project for a ``CustomActivity`` entry - every ``CustomActivity`` carries + one, being scoped to a project and a user. + + ``spent`` and ``remain`` carry no range constraint, for the reason + ``AssignableEntity`` gives: a constraint on a value the server supplies + fails the **whole entity** rather than the field, so one out-of-range + record would abort a whole ``list()`` page with a ``ParseError``. The + write path validates instead, where the value originates - see + ``times.upsert``. + Attributes: - spent: Hours spent >= 0 - remain: Hours remaining >= 0 + spent: Hours spent + remain: Hours remaining is_estimation: Entry records an estimate rather than actual time date: Date/time the work is logged against description: Free-text description of the work assignable: Assignable (work item) reference user: User the time is logged for - project: Project reference (TP derives it from the Assignable) + project: Project reference - from the Assignable, or from the + CustomActivity's own project on a custom-activity entry role: Role reference user_story: User story the entry was logged against, if any task: Task the entry was logged against, if any @@ -177,8 +196,8 @@ class Time(Entity): """ # Effort / time tracking - spent: float | None = Field(default=None, alias="Spent", ge=0, description="Hours spent") - remain: float | None = Field(default=None, alias="Remain", ge=0, description="Hours remaining") + spent: float | None = Field(default=None, alias="Spent", description="Hours spent") + remain: float | None = Field(default=None, alias="Remain", description="Hours remaining") is_estimation: bool | None = Field( default=None, alias="IsEstimation", description="Entry is an estimation" ) @@ -194,7 +213,11 @@ class Time(Entity): default=None, alias="Assignable", description="Assignable (work item)" ) user: UserRef | None = Field(default=None, alias="User", description="User") - project: EntityRef | None = Field(default=None, alias="Project", description="Project") + project: EntityRef | None = Field( + default=None, + alias="Project", + description="Project - from the Assignable, or the CustomActivity's own project", + ) role: EntityRef | None = Field(default=None, alias="Role", description="Role") # Narrow back-references: at most one is populated per entry. diff --git a/src/targetprocess/resources/times.py b/src/targetprocess/resources/times.py index 3507cf9..f5ca1d5 100644 --- a/src/targetprocess/resources/times.py +++ b/src/targetprocess/resources/times.py @@ -118,9 +118,11 @@ def _supplied_mutable_fields( ValueError: ``spent`` or ``remain`` is not a finite, non-boolean number ``>= 0``. Checked here, ahead of ``find_for_day`` and any create/update, so a bad value is rejected before a request is - sent - not written and then hit as a confusing ``ParseError`` on - the next read-back (``Time.spent``/``Time.remain`` carry - ``ge=0``). + sent. This is the only range check on a duration, and deliberately + so: the read models constrain nothing, because a constraint on a + server-supplied value fails the whole entity and would abort a + whole ``list()`` page with a ``ParseError``. The write path is + where the value originates, so it is where the check belongs. """ _validate_duration("spent", spent) if remain is not None: diff --git a/tests/test_role_effort.py b/tests/test_role_effort.py index 072360f..caa06ec 100644 --- a/tests/test_role_effort.py +++ b/tests/test_role_effort.py @@ -1,7 +1,6 @@ """Tests for RoleEffort model.""" import pytest -from pydantic import ValidationError from targetprocess.models import RoleEffort @@ -37,9 +36,25 @@ def test_role_effort_minimal_parsing() -> None: assert re.effort is None and re.role is None -def test_role_effort_effort_validation() -> None: - with pytest.raises(ValidationError): - RoleEffort.model_validate({"Id": 2, "ResourceType": "RoleEffort", "Effort": -1.0}) +@pytest.mark.parametrize( + ("alias", "attribute"), + [ + ("InitialEstimate", "initial_estimate"), + ("Effort", "effort"), + ("EffortCompleted", "effort_completed"), + ("EffortToDo", "effort_todo"), + ("TimeSpent", "time_spent"), + ("TimeRemain", "time_remain"), + ], +) +def test_role_effort_parses_a_negative_roll_up_the_server_sent(alias: str, attribute: str) -> None: + # Every one of these is a server-computed roll-up, so none constrains its + # range: a constraint on a server-supplied value fails the whole entity + # rather than the field, and one out-of-range roll-up would abort a whole + # list() page with a ParseError. Reporting what TP sent is worth more than + # asserting an invariant the API never promised. + effort = RoleEffort.model_validate({"Id": 2, "ResourceType": "RoleEffort", alias: -1.0}) + assert getattr(effort, attribute) == -1.0 def test_role_effort_has_no_name_field() -> None: diff --git a/tests/test_time.py b/tests/test_time.py index e2b55f4..da130e7 100644 --- a/tests/test_time.py +++ b/tests/test_time.py @@ -3,7 +3,6 @@ from datetime import datetime import pytest -from pydantic import ValidationError from targetprocess.models import Time @@ -71,10 +70,14 @@ def test_time_pascal_case_accessors() -> None: assert time.ResourceType == "Time" -@pytest.mark.parametrize("field", ["Spent", "Remain"]) -def test_time_rejects_negative_hours(field: str) -> None: - with pytest.raises(ValidationError): - Time.model_validate({"Id": 2, "ResourceType": "Time", field: -1.0}) +@pytest.mark.parametrize(("alias", "attribute"), [("Spent", "spent"), ("Remain", "remain")]) +def test_time_parses_a_negative_duration_the_server_sent(alias: str, attribute: str) -> None: + # Neither field constrains its range, because both are server-supplied and a + # constraint would fail the whole entity rather than the field - one + # out-of-range record would abort a whole list() page. The range check lives + # on the write path, in times.upsert, where the value originates. + time = Time.model_validate({"Id": 2, "ResourceType": "Time", alias: -1.0}) + assert getattr(time, attribute) == -1.0 def test_time_declares_the_narrow_back_references() -> None: @@ -96,12 +99,23 @@ def test_time_declares_the_narrow_back_references() -> None: def test_time_back_reference_covers_a_custom_activity_entry() -> None: # A CustomActivity entry has no Assignable at all - the back-reference is # the only record of what the time was logged against. + # + # Project is populated all the same, and this is the shape live TP sends: a + # null Assignable alongside a set Project, which TP takes from the custom + # activity's own project rather than from the (absent) Assignable. Asserted + # here so the docstring's claim has a test behind it. time = Time.model_validate( - {"Id": 4, "CustomActivity": {"Id": 47, "Name": "Admin"}, "Assignable": None} + { + "Id": 4, + "CustomActivity": {"Id": 47, "Name": "Admin"}, + "Assignable": None, + "Project": {"Id": 300, "Name": "Platform"}, + } ) assert time.assignable is None assert time.custom_activity is not None assert time.custom_activity.name == "Admin" + assert time.project is not None and time.project.id == 300 def test_time_tolerates_unmodelled_keys() -> None: From 253a686156706fef0ff780dd1ad2996065467f9d Mon Sep 17 00:00:00 2001 From: "man8-octoflow[bot]" <317008790+man8-octoflow[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:57:17 +0200 Subject: [PATCH 2/3] fix(entities): refuse the ignored Assignments filter on four more collections PortfolioEpics, TestPlanRuns, InboundAssignables and OutboundAssignables each expose assignable rows - every one of their /meta descriptions declares an Assignments collection - and each answers a where= on that collection with the unfiltered rows, exactly as the six typed assignable collections do. A caller narrowing one of them by assignee was receiving silently unfiltered results, which is the failure the refusal exists for. None of the four has a typed manager, so the generic entities map is both the only place the refusal can live and the only route a caller has to them. No new resource class is involved, and the set of refused paths is unchanged: this widens the collections covered, not the paths. Each was admitted against an unfiltered control on the same collection, as the declaration's own rule requires. The relation collections needed that comparison bounded to an Id range: they emit one row per relation, so an unfiltered page spends slots on duplicate ids and the page boundary falls elsewhere, which makes a page-to-page comparison misleading. Bounded, the distinct id sets are identical. The filter names a user Id no instance issues, so an honoured filter would return nothing, and each returned every row instead; where=(Id eq ) returns exactly one row on each, so where= demonstrably does reach the server there. Assignments.Count fails differently, and neither of the two ways expected: the query parser rejects it with HTTP 400, as it does Comments.Count, so it is not a silently ignored path at all. The refusal is kept rather than narrowed, because a ValueError naming the join-entity route serves a caller better than a 400 whose body names no field, and the reason text now states both shapes so it is true whichever path was written. Co-Authored-By: Claude Opus 5 (1M context) --- SPEC.md | 9 +- src/targetprocess/resources/base.py | 26 ++++-- src/targetprocess/resources/entities.py | 37 +++++++- .../test_ignored_filter_paths.py | 91 ++++++++++++++++++- 4 files changed, 142 insertions(+), 21 deletions(-) diff --git a/SPEC.md b/SPEC.md index 9123fbd..310ed1d 100644 --- a/SPEC.md +++ b/SPEC.md @@ -90,7 +90,8 @@ itself. indistinguishable from success. The confirmed case is a filter on the `Assignments` collection of an assignable, which returns the same rows as the unfiltered request, while the `Assignment` entity's own - `GeneralUser.Id` filter applies. + `GeneralUser.Id` filter applies. A collection `Count` is the loud exception: + `Assignments.Count` and `Comments.Count` are rejected with HTTP 400. - **Bulk endpoint**: writable collections expose `POST /api/v1/{collection}/bulk`, taking a JSON array of entity objects - an object carrying an `Id` updates that entity, an object without one @@ -688,7 +689,7 @@ at send time, and outside the set; `prettify` stays deliberately absent (see above). **Filter paths.** `BaseResource.ignored_filter_paths` maps the leading segment -of a `where=` path TP silently ignores on a collection to the reason and the +of a `where=` path TP will not filter on for a collection to the reason and the route to use instead. The known set is declared once, as `ASSIGNABLE_IGNORED_FILTER_PATHS` in `resources/base.py`, and referenced by the six assignable managers (`UserStory`, `Bug`, `Task`, `Feature`, `Epic`, @@ -696,7 +697,9 @@ six assignable managers (`UserStory`, `Bug`, `Task`, `Feature`, `Epic`, `order_by` or `include` - matching a path's leading segment in any casing outside quoted values, and refuses with a `ValueError` naming the collection and the route. `entities` mirrors the refusal for every spelling of those six -collections and for the polymorphic `Assignable` / `Assignables` collection. +collections and of each Assignable-derived collection with no typed manager, +named in `_UNTYPED_ASSIGNABLE_COLLECTIONS`: `Assignable`, `PortfolioEpic`, +`TestPlanRun`, `InboundAssignable` and `OutboundAssignable`. | Ignored path | Route instead | | --- | --- | diff --git a/src/targetprocess/resources/base.py b/src/targetprocess/resources/base.py index afcb259..8891a77 100644 --- a/src/targetprocess/resources/base.py +++ b/src/targetprocess/resources/base.py @@ -179,17 +179,25 @@ def _resolve_by_name[N: NamedEntity](candidates: Sequence[N], name: str, *, what return matches[0] -# The where= paths TargetProcess accepts and silently ignores on every collection -# whose entity type is an Assignable, keyed by the path's leading collection -# segment and mapped to the reason and the route to use instead. This is the one -# place the known set is declared: the assignable managers reference it and the -# generic entities path mirrors it. An entry is added only with live evidence - -# an unfiltered control returning the same rows. +# The where= paths TargetProcess will not filter on for a collection whose +# entity type is an Assignable, keyed by the path's leading collection segment +# and mapped to the reason and the route to use instead. This is the one place +# the known set is declared: the assignable managers reference it, and the +# generic entities path mirrors it for those managers' collections and for the +# Assignable-derived collections that have no typed manager. An entry is added +# only with live evidence - an unfiltered control returning the same rows. +# +# The refusal covers the whole Assignments prefix because no path under it is +# usable, though the two failure shapes differ: a filter on one of the +# collection's fields is accepted and ignored, while Assignments.Count is +# refused by the query parser. Both are stated, so the message is true +# whichever path a caller wrote. ASSIGNABLE_IGNORED_FILTER_PATHS: dict[str, str] = { "Assignments": ( - "TargetProcess accepts a filter on the Assignments collection and " - "ignores it, answering HTTP 200 with the unfiltered rows; query the " - "join entity instead - client.assignments.list(" + "TargetProcess does not filter on the Assignments collection: a filter " + "on one of its fields is accepted and ignored, answering HTTP 200 with " + "the unfiltered rows, and Assignments.Count is rejected outright with " + "HTTP 400; query the join entity instead - client.assignments.list(" 'where="GeneralUser.Id eq ", include=["Assignable"]) - and read ' "the work item off each assignment's assignable" ), diff --git a/src/targetprocess/resources/entities.py b/src/targetprocess/resources/entities.py index d9432fd..31c75e9 100644 --- a/src/targetprocess/resources/entities.py +++ b/src/targetprocess/resources/entities.py @@ -88,13 +88,38 @@ def _check_include(entity_type: str, include: list[str] | None) -> None: guarded.check_include(include) +# Assignable-derived collections with no typed manager, so reachable only +# through this accessor - which makes keying them here the only place the +# refusal can be applied. Each one's ``/meta`` declares an ``Assignments`` +# collection alongside the rest of the assignable surface (``AssignedUser``, +# ``Times``, ``RoleEfforts``, ``Impediments``, ``TeamStates``). Named in the +# singular; ``_spellings`` derives the plural TP addresses the collection by. +# +# ``PortfolioEpic``, ``TestPlanRun``, ``InboundAssignable`` and +# ``OutboundAssignable`` were each confirmed live, against an unfiltered control +# on the same collection. The polymorphic ``Assignable`` is carried on the +# narrower argument that it returns a superset of the same rows as the six typed +# collections, so it cannot behave differently from them. +# +# ``Inbound``/``OutboundAssignables`` are also read as ``include=`` collection +# properties of an item, which is unaffected: the refusal is on ``list()``'s +# ``where=`` only. +_UNTYPED_ASSIGNABLE_COLLECTIONS: tuple[str, ...] = ( + "Assignable", + "PortfolioEpic", + "TestPlanRun", + "InboundAssignable", + "OutboundAssignable", +) + + # The where= counterpart: the ignored-filter declaration of every assignable # collection (``BaseResource.ignored_filter_paths``), keyed by each spelling the # caller may use, so the generic path refuses the same filters before any # request rather than returning the unfiltered rows TP answers with. Keyed -# spelling -> reasons rather than spelling -> class, because the polymorphic -# ``Assignables`` collection has no typed manager and is the same server -# behaviour on a superset of the same rows. The walk test in +# spelling -> reasons rather than spelling -> class, because the collections in +# ``_UNTYPED_ASSIGNABLE_COLLECTIONS`` have no typed manager to consult and are +# the same server behaviour on the same rows. The walk test in # ``tests/test_resources/test_ignored_filter_paths.py`` covers this map. _IGNORED_FILTER_PATHS: dict[str, dict[str, str]] = { spelling: resource.ignored_filter_paths @@ -107,7 +132,11 @@ def _check_include(entity_type: str, include: list[str] | None) -> None: RequestsResource, ) for spelling in _spellings(resource.entity_type) -} | dict.fromkeys(_spellings("Assignable"), ASSIGNABLE_IGNORED_FILTER_PATHS) +} | { + spelling: ASSIGNABLE_IGNORED_FILTER_PATHS + for collection in _UNTYPED_ASSIGNABLE_COLLECTIONS + for spelling in _spellings(collection) +} def _check_where(entity_type: str, where: str | None) -> None: diff --git a/tests/test_resources/test_ignored_filter_paths.py b/tests/test_resources/test_ignored_filter_paths.py index f60e9eb..87c549f 100644 --- a/tests/test_resources/test_ignored_filter_paths.py +++ b/tests/test_resources/test_ignored_filter_paths.py @@ -37,6 +37,18 @@ EpicsResource: "Epics", RequestsResource: "Requests", } + +# The Assignable-derived collections with no typed manager, so reachable only +# through the generic accessor: the collection name TP addresses, mapped to the +# singular entity type it is declared as. The four beyond Assignable were each +# confirmed live against an unfiltered control on the same collection. +UNTYPED_ASSIGNABLE_COLLECTIONS: dict[str, str] = { + "Assignables": "Assignable", + "PortfolioEpics": "PortfolioEpic", + "TestPlanRuns": "TestPlanRun", + "InboundAssignables": "InboundAssignable", + "OutboundAssignables": "OutboundAssignable", +} IGNORED_WHERE = "Assignments.GeneralUser.Id eq 1" @@ -166,6 +178,21 @@ def test_the_known_set_is_declared_once_and_shared(): assert resource_cls.ignored_filter_paths is ASSIGNABLE_IGNORED_FILTER_PATHS, resource_cls +def test_the_reason_states_both_observed_failure_shapes(): + """One message covers both: a field filter is ignored, Assignments.Count is rejected. + + The prefix is refused whole because no path under it filters, but the two + shapes fail differently, and a caller who wrote either should read + something true. Pinned here so neither half can be dropped silently. + """ + from targetprocess.resources.base import ASSIGNABLE_IGNORED_FILTER_PATHS + + reason = ASSIGNABLE_IGNORED_FILTER_PATHS["Assignments"] + assert "ignored" in reason and "HTTP 200" in reason + assert "Assignments.Count" in reason and "HTTP 400" in reason + assert "client.assignments" in reason + + @pytest.mark.asyncio @pytest.mark.parametrize( "entity_type", @@ -193,6 +220,45 @@ async def test_entities_refuse_the_filter_for_every_assignable_spelling(entity_t handler.list.assert_not_called() +@pytest.mark.asyncio +@pytest.mark.parametrize( + "entity_type", + [ + spelling + for collection, singular in UNTYPED_ASSIGNABLE_COLLECTIONS.items() + for spelling in (collection, singular) + ], +) +async def test_untyped_assignable_collections_refuse_a_filter_on_assignments(entity_type: str): + """Each Assignable-derived collection without a typed manager refuses it too. + + These have no resource class to carry the declaration, so the generic + accessor's map is the only place the refusal can live - and the only route + a caller has to them. Both spellings TP accepts are covered. + """ + handler = AsyncMock(spec=RequestHandler) + entities = EntitiesResource(Mock(spec=TargetProcessClient), handler) + + with pytest.raises(ValueError, match="client.assignments") as excinfo: + async for _ in entities.list(entity_type, where=IGNORED_WHERE): + pass + assert f"is not supported on {entity_type}:" in str(excinfo.value) + handler.list.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("entity_type", sorted(UNTYPED_ASSIGNABLE_COLLECTIONS)) +async def test_untyped_assignable_collections_forward_a_valid_filter_unchanged(entity_type: str): + """Only the Assignments prefix is refused on them; their own fields pass through.""" + where = "(EntityState.IsFinal eq 'false') and (AssignedUser.Id eq 7)" + handler = AsyncMock(spec=RequestHandler) + handler.list, seen = scripted_list([]) + entities = EntitiesResource(Mock(spec=TargetProcessClient), handler) + + assert [item async for item in entities.list(entity_type, where=where)] == [] + assert (seen["entity_type"], seen["where"]) == (entity_type, where) + + @pytest.mark.asyncio @pytest.mark.parametrize("entity_type", ["Comment", "Project"]) async def test_entities_forward_the_filter_where_nothing_is_declared(entity_type: str): @@ -206,10 +272,18 @@ async def test_entities_forward_the_filter_where_nothing_is_declared(entity_type def test_every_manager_with_ignored_filter_paths_is_mirrored_on_the_generic_path(): - """The generic map reaches each declaring manager by every spelling, plus Assignables.""" + """The generic map reaches each declaring manager, and each untyped collection. + + Two dimensions, and they are independent: the *paths* refused (one, + ``Assignments``) and the *collections* the refusal is applied to. Widening + coverage to a collection adds spellings here and no path there. + """ from targetprocess.resources.assignables import AssignableResource from targetprocess.resources.base import ASSIGNABLE_IGNORED_FILTER_PATHS - from targetprocess.resources.entities import _IGNORED_FILTER_PATHS + from targetprocess.resources.entities import ( + _IGNORED_FILTER_PATHS, + _UNTYPED_ASSIGNABLE_COLLECTIONS, + ) # AssignableResource declares the set once for the six managers; it is their # base class rather than a manager, so it has no collection to mirror. @@ -227,11 +301,18 @@ def test_every_manager_with_ignored_filter_paths_is_mirrored_on_the_generic_path for spelling in {*_spellings(resource_cls.entity_type), collection, collection.upper()}: reached = _IGNORED_FILTER_PATHS.get(spelling.casefold()) assert reached is resource_cls.ignored_filter_paths, spelling - for spelling in ("assignable", "assignables"): - assert _IGNORED_FILTER_PATHS[spelling] is ASSIGNABLE_IGNORED_FILTER_PATHS + # The untyped collections this suite names are exactly the ones the module + # declares, so the two cannot drift apart unnoticed. + assert set(_UNTYPED_ASSIGNABLE_COLLECTIONS) == set(UNTYPED_ASSIGNABLE_COLLECTIONS.values()) + + for singular in UNTYPED_ASSIGNABLE_COLLECTIONS.values(): + for spelling in {*_spellings(singular), singular.upper()}: + reached = _IGNORED_FILTER_PATHS.get(spelling.casefold()) + assert reached is ASSIGNABLE_IGNORED_FILTER_PATHS, spelling declared_spellings = frozenset().union( - *(_spellings(cls.entity_type) for cls in ASSIGNABLE_COLLECTIONS), _spellings("Assignable") + *(_spellings(cls.entity_type) for cls in ASSIGNABLE_COLLECTIONS), + *(_spellings(singular) for singular in UNTYPED_ASSIGNABLE_COLLECTIONS.values()), ) assert set(_IGNORED_FILTER_PATHS) <= declared_spellings From 6f9ece23c07ca928ad3d016bafae653166145fd1 Mon Sep 17 00:00:00 2001 From: "man8-octoflow[bot]" <317008790+man8-octoflow[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:16:55 +0200 Subject: [PATCH 3/3] docs: align the range-constraint spec, changelog and coverage claims Follows the pre-push review of the two commits before it. SPEC.md's numeric-range section still carved RoleEffort and Time out of the no-bounds rule - the carve-out those commits remove - so the repository's normative document contradicted its own code. It now states the rule for the join models too, and names the attached-content byte-size fields as the remaining bounded ones. The coverage claim is narrowed to what the evidence closes. Four collections were checked because four were named for checking, not enumerated from the instance, so an untyped Assignable-derived collection absent from the map is unproven rather than cleared, and the TestPlan family is the obvious one left. SPEC.md and the map's own comment both say so now, because a reader of a published library's spec could otherwise take the guard for exhaustive. RoleEffort's docstring no longer calls every numeric field a server-supplied roll-up: the collection is writable per its own /meta, so the accurate statement is that each value arrives from the server on a read, whoever set it there. Nor does it imply a write-side check covers this model - times.upsert is a Time helper, and RoleEffort has no counterpart. Time's project paragraph drops a causal claim the data cannot support. No activity carries time on more than one project, so copying the activity's project and copying the project the entry was filed under are indistinguishable from that evidence. It also notes a narrowed read leaves the reference None. The refusal is now described as one TP will not filter on rather than one it silently ignores, in the Raises: blocks and docstrings a caller actually reads: Assignments.Count is rejected, not ignored. The reason-text test drops its word assertion and keeps the two status codes, which pin the same fact less brittly. CHANGELOG.md gains the entry README promises for every change, covering both directions - the new refusal breaks a caller who was receiving silently unfiltered rows, and the dropped bound removes a ValidationError a caller may have relied on. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 33 +++++++++++++++++ SPEC.md | 28 +++++++-------- src/targetprocess/_joins.py | 35 +++++++++---------- src/targetprocess/resources/base.py | 15 ++++---- src/targetprocess/resources/entities.py | 8 ++++- src/targetprocess/resources/times.py | 8 ++--- .../test_ignored_filter_paths.py | 4 +-- 7 files changed, 85 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 499ef9d..b77d78f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `list()` refuses a `where=` filter naming the `Assignments` collection on four + further collections - `PortfolioEpics`, `TestPlanRuns`, `InboundAssignables` + and `OutboundAssignables` - raising `ValueError` before any request is sent, + as it already did for the six typed assignable collections. TargetProcess + answers such a filter with HTTP 200 and the *unfiltered* rows, so code that + appeared to narrow one of these collections by assignee was in fact receiving + every row; it now raises rather than returning silently-wrong results. Each + collection was confirmed against an unfiltered control on the same collection + before being added. Only `list()`'s `where=` is affected - reading + `InboundAssignables` or `OutboundAssignables` through `include=` is unchanged. +- The refusal's message states both ways a path under `Assignments` fails: a + filter on one of the collection's fields is accepted and ignored (HTTP 200), + while `Assignments.Count` is rejected by the query parser (HTTP 400). + +### Fixed + +- The eight numeric fields on the `RoleEffort` and `Time` read models no longer + carry a `ge=0` bound - `InitialEstimate`, `Effort`, `EffortCompleted`, + `EffortToDo`, `TimeSpent` and `TimeRemain` on `RoleEffort`, and `Spent` and + `Remain` on `Time`. Each is supplied by the server, and a constraint on a + server-supplied value fails the *whole entity* rather than the field, so one + out-of-range record would have aborted an entire `list()` page with a + `ParseError` - the rule `AssignableEntity` already stated and these two models + broke. A consumer relying on `ValidationError` for a negative value on these + fields, including on assignment, no longer receives one; `times.upsert` keeps + its write-side check, where a range error is actionable. +- `Time`'s class docstring and its `project` field description say what a + custom-activity entry's `project` holds. Such an entry has no `Assignable`, + and the field is populated all the same, matching that activity's own project + on every entry observed. + ## [0.2.1] - 2026-09-18 ### Fixed diff --git a/SPEC.md b/SPEC.md index 310ed1d..37bcd06 100644 --- a/SPEC.md +++ b/SPEC.md @@ -85,13 +85,12 @@ itself. local `skip` counter, since that can silently skip or repeat records if TP ever returns a non-linear continuation URL. - **Unrecognised query input**: TP answers a query parameter it does not - recognise, and a `where=` naming a nested collection path it does not - filter on, with HTTP 200 and the unfiltered, unsorted rows - - indistinguishable from success. The confirmed case is a filter on the - `Assignments` collection of an assignable, which returns the same rows as - the unfiltered request, while the `Assignment` entity's own - `GeneralUser.Id` filter applies. A collection `Count` is the loud exception: - `Assignments.Count` and `Comments.Count` are rejected with HTTP 400. + recognise, and a `where=` naming a nested collection path it does not filter + on, with HTTP 200 and the unfiltered, unsorted rows - indistinguishable from + success. The confirmed case is a filter on the `Assignments` collection of an + assignable, which returns the same rows as the unfiltered request, while the + `Assignment` entity's own `GeneralUser.Id` filter applies. A collection + `Count` is the loud exception, rejected with HTTP 400 - `Comments.Count` too. - **Bulk endpoint**: writable collections expose `POST /api/v1/{collection}/bulk`, taking a JSON array of entity objects - an object carrying an `Id` updates that entity, an object without one @@ -507,10 +506,10 @@ entity back to TP. Fields TP computes and returns carry no `ge`/`le` bound. A constraint on a server-supplied value fails the **whole entity** rather than the field, so one out-of-range roll-up would abort an entire `list()` page with a `ParseError` - -and TP documents no bounds on effort, progress or flow metrics to assert. -`RoleEffort` and `Time` keep the bounds they were published with, and -`times.upsert` validates on the write side, where the value originates and a -range error is actionable. +and TP documents no bounds on effort, progress or flow metrics to assert. The +`RoleEffort` and `Time` join models carry none either; `times.upsert` validates +on the write side, where the value originates and a range error is actionable. +The attached-content byte-size fields still carry `ge=0`. `custom_fields` (`CustomFields`) sits on the `Entity` base, so custom-field values are reachable on every entity type: fetched with @@ -697,9 +696,10 @@ six assignable managers (`UserStory`, `Bug`, `Task`, `Feature`, `Epic`, `order_by` or `include` - matching a path's leading segment in any casing outside quoted values, and refuses with a `ValueError` naming the collection and the route. `entities` mirrors the refusal for every spelling of those six -collections and of each Assignable-derived collection with no typed manager, -named in `_UNTYPED_ASSIGNABLE_COLLECTIONS`: `Assignable`, `PortfolioEpic`, -`TestPlanRun`, `InboundAssignable` and `OutboundAssignable`. +collections and of the untyped Assignable-derived collections confirmed so far - +`Assignable`, `PortfolioEpic`, `TestPlanRun`, `InboundAssignable` and +`OutboundAssignable`, in `_UNTYPED_ASSIGNABLE_COLLECTIONS`. That is what has +been checked, not every candidate; the TestPlan family is unexamined. | Ignored path | Route instead | | --- | --- | diff --git a/src/targetprocess/_joins.py b/src/targetprocess/_joins.py index ab25e7d..c98d285 100644 --- a/src/targetprocess/_joins.py +++ b/src/targetprocess/_joins.py @@ -73,12 +73,13 @@ class RoleEffort(Entity): Breaks an Assignable's effort down by Role. - Every numeric field here is a server-supplied roll-up, so none carries a - range constraint - the rule ``AssignableEntity`` states for the same class - of field: a constraint on a value TP computes fails the **whole entity** - rather than the field, so one out-of-range roll-up would abort a whole - ``list()`` page with a ``ParseError``. Write-side range checks belong where - the value originates - see ``times.upsert``. + None of the numeric fields carries a range constraint. Each arrives from + the server on a read, whoever set it there, and ``AssignableEntity`` gives + the rule for that class of field: a constraint on a server-supplied value + fails the **whole entity** rather than the field, so one bad figure would + abort a whole ``list()`` page. Nothing validates one on the way in either - + the resource layer writes ``**fields`` and dumps no model - so unlike a + duration (see ``times.upsert``) this model has no write-side counterpart. Attributes: initial_estimate: Initial effort estimate @@ -162,18 +163,16 @@ class Time(Entity): ``assignable``). They are declared so an entry's origin is readable without a second fetch. - ``project`` is populated whichever shape the entry takes, so a null - ``assignable`` does not imply a null project: TP takes it from the - Assignable for a work-item entry, and from the custom activity's own - project for a ``CustomActivity`` entry - every ``CustomActivity`` carries - one, being scoped to a project and a user. + A null ``assignable`` does not imply a null ``project``: a + ``CustomActivity`` entry carries a project too, matching that activity's + own on every entry observed. Whether TP copies it from the activity or from + the project the entry was filed under cannot be told apart from that, since + an activity is itself scoped to one project. A narrowed read + (``result_include=``) leaves the reference ``None``, as it does any other. ``spent`` and ``remain`` carry no range constraint, for the reason - ``AssignableEntity`` gives: a constraint on a value the server supplies - fails the **whole entity** rather than the field, so one out-of-range - record would abort a whole ``list()`` page with a ``ParseError``. The - write path validates instead, where the value originates - see - ``times.upsert``. + ``AssignableEntity`` gives. ``times.upsert`` validates on the way in + instead, where the value originates. Attributes: spent: Hours spent @@ -183,8 +182,8 @@ class Time(Entity): description: Free-text description of the work assignable: Assignable (work item) reference user: User the time is logged for - project: Project reference - from the Assignable, or from the - CustomActivity's own project on a custom-activity entry + project: Project reference - the Assignable's, or on a custom-activity + entry the activity's own project role: Role reference user_story: User story the entry was logged against, if any task: Task the entry was logged against, if any diff --git a/src/targetprocess/resources/base.py b/src/targetprocess/resources/base.py index 8891a77..880c882 100644 --- a/src/targetprocess/resources/base.py +++ b/src/targetprocess/resources/base.py @@ -210,7 +210,7 @@ def _resolve_by_name[N: NamedEntity](candidates: Sequence[N], name: str, *, what def check_filter_paths(where: str | None, ignored: Mapping[str, str], *, resource: str) -> None: - """Refuse a ``where=`` filter naming a path TargetProcess is known to ignore. + """Refuse a ``where=`` filter naming a path TargetProcess will not filter on. A name in ``ignored`` matches only as the leading segment of a dotted path, in any casing: never preceded by a word character or a dot, and @@ -266,10 +266,11 @@ class BaseResource[T: Entity]: TP serves in a shape the model cannot hold, each mapped to the reason and the route to use instead; ``get`` and ``list`` refuse them with ``ValueError`` before any request is sent. - ignored_filter_paths: Leading ``where=`` path segments TP accepts - and silently ignores on this collection, each mapped to the - reason and the route to use instead; ``list`` refuses them with - ``ValueError`` before any request is sent. + ignored_filter_paths: Leading ``where=`` path segments TP will not + filter on for this collection - accepting and ignoring some, + rejecting others - each mapped to the reason and the route to use + instead; ``list`` refuses them with ``ValueError`` before any + request is sent. """ entity_type: str # Override in subclass @@ -369,7 +370,7 @@ def check_custom_field_names( @classmethod def check_where(cls, where: str | None) -> None: - """Refuse a ``where=`` naming a path this collection silently ignores. + """Refuse a ``where=`` naming a path this collection will not filter on. Applies :func:`check_filter_paths` with ``ignored_filter_paths``. A collection with nothing declared there accepts every filter, as @@ -538,7 +539,7 @@ async def list( ValueError: Both ``order_by`` and ``order_by_desc`` were passed, ``skip`` is negative, ``innertake`` is negative, ``include`` names a field this collection cannot hydrate, or ``where`` - names a path TP silently ignores on it (see + names a path TP will not filter on for it (see ``ignored_filter_paths``) (raised when iteration begins) AuthenticationError: Invalid credentials ForbiddenError: Insufficient permissions diff --git a/src/targetprocess/resources/entities.py b/src/targetprocess/resources/entities.py index 31c75e9..dd54971 100644 --- a/src/targetprocess/resources/entities.py +++ b/src/targetprocess/resources/entities.py @@ -101,6 +101,12 @@ def _check_include(entity_type: str, include: list[str] | None) -> None: # narrower argument that it returns a superset of the same rows as the six typed # collections, so it cannot behave differently from them. # +# This is what has been checked, not every candidate: the collections were named +# for checking rather than enumerated from the instance, so an untyped +# Assignable-derived collection missing here is unproven, not cleared. The +# TestPlan family is the obvious one left - ``TestPlanRun`` is covered and +# ``TestPlan`` is not. Add one the same way, with its own unfiltered control. +# # ``Inbound``/``OutboundAssignables`` are also read as ``include=`` collection # properties of an item, which is unaffected: the refusal is on ``list()``'s # ``where=`` only. @@ -319,7 +325,7 @@ async def list( ``skip`` is negative, ``innertake`` is negative, ``include`` names a field the typed resource for this collection refuses to hydrate, or ``where`` names a path TP - silently ignores on an assignable collection (raised when + will not filter on for an assignable collection (raised when iteration begins) AuthenticationError: Invalid credentials ForbiddenError: Insufficient permissions diff --git a/src/targetprocess/resources/times.py b/src/targetprocess/resources/times.py index f5ca1d5..03674bf 100644 --- a/src/targetprocess/resources/times.py +++ b/src/targetprocess/resources/times.py @@ -119,10 +119,10 @@ def _supplied_mutable_fields( number ``>= 0``. Checked here, ahead of ``find_for_day`` and any create/update, so a bad value is rejected before a request is sent. This is the only range check on a duration, and deliberately - so: the read models constrain nothing, because a constraint on a - server-supplied value fails the whole entity and would abort a - whole ``list()`` page with a ``ParseError``. The write path is - where the value originates, so it is where the check belongs. + so: ``Time.spent`` and ``Time.remain`` constrain nothing, because a + constraint on a server-supplied value fails the whole entity and + would abort a whole ``list()`` page with a ``ParseError``. The write + path is where the value originates, so it is where the check belongs. """ _validate_duration("spent", spent) if remain is not None: diff --git a/tests/test_resources/test_ignored_filter_paths.py b/tests/test_resources/test_ignored_filter_paths.py index 87c549f..37473c1 100644 --- a/tests/test_resources/test_ignored_filter_paths.py +++ b/tests/test_resources/test_ignored_filter_paths.py @@ -1,4 +1,4 @@ -"""Tests for the refusal of where= paths TargetProcess accepts and silently ignores. +"""Tests for the refusal of where= paths TargetProcess will not filter on. A filter on the Assignments collection of an assignable answers HTTP 200 with the unfiltered rows, so the assignable managers and the generic entities path refuse @@ -188,7 +188,7 @@ def test_the_reason_states_both_observed_failure_shapes(): from targetprocess.resources.base import ASSIGNABLE_IGNORED_FILTER_PATHS reason = ASSIGNABLE_IGNORED_FILTER_PATHS["Assignments"] - assert "ignored" in reason and "HTTP 200" in reason + assert "HTTP 200" in reason assert "Assignments.Count" in reason and "HTTP 400" in reason assert "client.assignments" in reason