diff --git a/CHANGELOG.md b/CHANGELOG.md index 33d445e..be5c7ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,70 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **The statistics layer sent GLPI v1 field names to the v2 API, which + silently ignored them and returned unfiltered results.** v2 drops a + `filter=` conjunct whose field it does not recognise, honours the rest, + and answers 200/206 with no error — so the aggregations narrowed by date + and looked plausible while ignoring the user and entity selection + entirely. Measured against a live GLPI 11 instance, + `get_user_activity` reported `tickets_as_technician == tickets_as_recipient + == 963` — the window's *total* ticket count — for every user regardless of + who they were. Corrected: + - `entities_id==N` → `entity.id==N` (v2 types `entity` as an object). + - `users_id_lastupdater==N` → `user_editor.id==N`. + - `users_id_requester==N` (as `user_recipient_id`) → `user_recipient.id==N`, + which is what that parameter's *name* has always meant. Note the v2 + `user_recipient` field is `users_id_recipient` — who *recorded* the + ticket — not the requester link; the two are different people. + - `users_id_assign` / `users_id_requester` (as `user_id`) have **no v2 + equivalent at all**: the v2 `team` array cannot be joined by the RSQL + engine (the four contract-declared subfields answer HTTP 500 and every + other spelling is silently ignored — 19 spellings were tested). These + now resolve through the legacy v1 search engine, whose searchOption 5 + (`Technicien`) and 4 (`Demandeur`) map exactly onto the + `glpi_tickets_users` link types, and which fails **loudly** (HTTP 400) + on an unknown field instead of silently returning everything. +- **`rsql_any_filter` produced an unparenthesised OR group**, and RSQL binds + `;` (AND) tighter than `,` (OR). `get_ticket_statistics(entity_name=...)` + matching several entities emitted `date;e==1,e==2`, which the server reads + as `(date AND e==1) OR e==2` — the date window stopped applying to every + entity after the first. Measured live: 16,245 tickets returned where the + correct answer was 1,552. OR groups are now wrapped in parentheses. +- **v2 ticket searches counted soft-deleted tickets.** The v2 search includes + trashed tickets by default while v1 excludes them (59,690 live + 258 + trashed = 59,948), so every aggregation was inflated by the trash bin — for + one user 92% of matches were deleted tickets. All v2 ticket queries in the + statistics layer now pin `is_deleted==false`. +- Actor identifiers are validated before reaching the v1 search, which fails + *open* rather than rejecting bad input: `equals 0` matched 20,905 tickets + (a LEFT-JOIN-NULL "has no actor" match), an empty value matched the entire + baseline, and a non-numeric value returned the same arbitrary 3 rows + whatever the string. A non-positive or non-`int` id now raises + `GlpiValidationError`. +- **The per-ticket task fan-out is replaced by one bulk sweep.** The v2 API + publishes tasks only under `/Assistance/Ticket/{id}/Timeline/Task`, so + aggregating N tickets cost N requests. The v1 `TicketTask` *collection* + returns whole rows including `tickets_id`, paged 1000 at a time, so the + same aggregate now costs one page per 1000 tasks created since the window + opened. Measured live on a 120-ticket set: **120 requests / 11.7 s -> 2 + requests / 0.4 s**, with `ticket_count`, `task_count`, `total_duration`, + `duration_by_ticket` and `duration_by_user` all byte-identical between the + two paths. Below 25 tickets the per-ticket path is cheaper and is kept, so + clients without a v1 session are unaffected. + + Note v1 `search/TicketTask` is *not* usable for this: its searchOptions + expose the task id, content, category, date, privacy, technician, duration + and state, but no parent ticket id, so results cannot be attributed back + to a ticket. The plain collection endpoint is what carries `tickets_id`. + +- `get_user_activity` walks the date window **once** for all users instead of + twice per user. Combined with the corrected filters this took one user over + 90 days from **979 requests / 120 s to 9 requests / 5.1 s**, verified live. + + Actor-based statistics now require the legacy v1 session (`v1_base_url` + + `v1_user_token`) and raise `RuntimeError` naming the missing options when + it is absent, rather than returning a wrong number. + - `GLPITokenManager._refresh_access_token`'s retry decorator no longer retries a `GlpiServerError` from its fall-through to the nested `_acquire_token()` call. That nested call already carries its own diff --git a/glpi_python_client/clients/commons/_filters.py b/glpi_python_client/clients/commons/_filters.py index 1771d4e..12f9b49 100644 --- a/glpi_python_client/clients/commons/_filters.py +++ b/glpi_python_client/clients/commons/_filters.py @@ -43,12 +43,23 @@ def rsql_any_filter(*filters: str | None) -> str | None: """Join non-empty RSQL filter fragments with OR semantics. Empty fragments are ignored and an all-empty input returns ``None``. + + The joined result is wrapped in parentheses whenever it contains more + than one fragment. RSQL binds ``;`` (AND) tighter than ``,`` (OR), so + an unparenthesised group silently loses every preceding AND clause for + all but its first alternative: ``date;e==1,e==2`` parses as + ``(date AND e==1) OR e==2``, which matches every ``e==2`` ticket + regardless of the date window. Measured against a live GLPI 11 + instance, the unparenthesised form returned 16,245 tickets where the + parenthesised form correctly returned 1,552. """ parts = [fragment for fragment in filters if fragment] if not parts: return None - return ",".join(parts) + if len(parts) == 1: + return parts[0] + return "(" + ",".join(parts) + ")" def rsql_all_filter(*filters: str | None) -> str | None: diff --git a/glpi_python_client/clients/commons/tests/test_filters.py b/glpi_python_client/clients/commons/tests/test_filters.py index df1a897..fcef3b6 100644 --- a/glpi_python_client/clients/commons/tests/test_filters.py +++ b/glpi_python_client/clients/commons/tests/test_filters.py @@ -32,9 +32,33 @@ def test_rsql_all_filter_joins_with_semicolons() -> None: def test_rsql_any_filter_joins_with_commas() -> None: - """``rsql_any_filter`` joins non-empty parts with ``,``.""" + """``rsql_any_filter`` ORs parts and parenthesises the group.""" - assert rsql_any_filter("a==1", None, "b==2") == "a==1,b==2" + assert rsql_any_filter("a==1", None, "b==2") == "(a==1,b==2)" + + +def test_rsql_any_filter_single_part_is_not_wrapped() -> None: + """A lone fragment needs no group and is returned unchanged.""" + + assert rsql_any_filter(None, "a==1", "") == "a==1" + + +def test_rsql_any_filter_group_survives_an_and_join() -> None: + """The OR group keeps its AND clauses when nested in ``rsql_all_filter``. + + RSQL binds ``;`` tighter than ``,``. Without the parentheses this + composes to ``date;e==1,e==2``, which the server reads as + ``(date AND e==1) OR e==2`` -- so every ``e==2`` record matches + regardless of the date window. Measured against a live GLPI 11 + instance, that returned 16,245 tickets where the correct answer, + reproduced by the parenthesised form, was 1,552. + """ + + combined = rsql_all_filter( + "date_creation=ge=2026-01-01", + rsql_any_filter("entity.id==1", "entity.id==2"), + ) + assert combined == "date_creation=ge=2026-01-01;(entity.id==1,entity.id==2)" def test_escape_rsql_like_value_escapes_special_characters() -> None: diff --git a/glpi_python_client/clients/custom/_statistics.py b/glpi_python_client/clients/custom/_statistics.py index ccb2329..53e6179 100644 --- a/glpi_python_client/clients/custom/_statistics.py +++ b/glpi_python_client/clients/custom/_statistics.py @@ -34,6 +34,75 @@ GlpiTicketType, ) +#: The GLPI v2 ticket search includes soft-deleted ("trashed") tickets by +#: default, while the v1 search excludes them. Every aggregation here is +#: about live work, so the v2 queries pin the flag explicitly. Measured on +#: a live GLPI 11 instance: 59,690 live + 258 trashed = 59,948 unfiltered, +#: and for some users the trashed rows were the large majority of matches. +_LIVE_TICKETS = "is_deleted==false" + +#: v1 ``search/Ticket`` searchOption ids. The v2 API exposes no filterable +#: assignee at all -- its ``team`` array cannot be joined by the RSQL engine +#: (the contract-declared subfields answer HTTP 500 and every other spelling +#: is silently ignored) -- so actor-based selection has to go through v1. +_V1_SO_TICKET_ID = 2 +_V1_SO_REQUESTER = 4 # "Demandeur" -- glpi_tickets_users.users_id, type=1 +_V1_SO_ASSIGNEE = 5 # "Technicien" -- glpi_tickets_users.users_id, type=2 + +#: v1 rejects a ``range`` that starts past the end of the result set with +#: HTTP 400, so paging is bounded by ``totalcount`` rather than by probing. +_V1_SEARCH_PAGE_SIZE = 1000 + +#: Rows fetched per page from the v1 ``TicketTask`` collection. +_V1_TASK_PAGE_SIZE = 1000 + +#: Above this many tickets, one bulk v1 task sweep beats a per-ticket v2 +#: request each. The sweep costs one page per 1000 tasks created since the +#: window opened -- typically one or two -- while the per-ticket path costs +#: exactly ``len(ticket_ids)`` requests. Below the threshold the per-ticket +#: path is cheaper and needs no v1 session, so it stays the default. +_V1_TASK_BULK_THRESHOLD = 25 + + +def _validate_actor_id(value: int, parameter: str) -> int: + """Return ``value`` when it is usable as a GLPI user identifier. + + The v1 search engine fails *open* on a malformed actor value instead of + rejecting it, so a bad id yields a plausible-looking but meaningless + result set rather than an error. Measured on a live instance: + ``equals 0`` matched 20,905 tickets (a LEFT-JOIN-NULL "has no actor" + match), an empty value matched the entire 59,689-ticket baseline, and a + non-numeric value returned the same arbitrary 3 rows whatever the + string. Guarding at the boundary is what keeps this fix from + reintroducing the class of bug it exists to remove. + + Parameters + ---------- + value : int + Candidate GLPI user identifier. + parameter : str + Name of the public parameter, used in the error message. + + Returns + ------- + int + The validated identifier. + + Raises + ------ + GlpiValidationError + When ``value`` is not a positive integer. ``bool`` is rejected + explicitly because it is an ``int`` subclass in Python. + """ + + if isinstance(value, bool) or not isinstance(value, int) or value <= 0: + raise GlpiValidationError( + f"{parameter} must be a positive integer GLPI user id; got " + f"{value!r}. GLPI's v1 search silently returns unrelated rows " + "for 0, empty or non-numeric actor values instead of failing." + ) + return value + class TaskStatisticsResult(TypedDict): """Typed shape returned by :meth:`StatisticsMixin.get_task_statistics`.""" @@ -75,6 +144,177 @@ class UserActivityResult(TypedDict): class StatisticsMixin(TransportMixin): """Synchronous custom statistics built on the contract API mixins.""" + def _v1_ticket_ids_for_actor( + self, user_id: int, *, search_options: tuple[int, ...], parameter: str + ) -> set[int]: + """Return ids of tickets linking ``user_id`` under any given role. + + Actor selection cannot be expressed in the v2 API, so this reads the + v1 search engine, OR-ing one criterion per requested searchOption. + Unlike v2 -- which silently ignores a filter field it does not know + and answers with the complete unfiltered set -- v1 rejects an + unknown searchOption with HTTP 400, so a mistake here fails loudly. + + Parameters + ---------- + user_id : int + GLPI user identifier; validated by :func:`_validate_actor_id`. + search_options : tuple[int, ...] + v1 searchOption ids to OR together, e.g. + ``(_V1_SO_ASSIGNEE, _V1_SO_REQUESTER)``. + parameter : str + Public parameter name quoted in validation errors. + + Returns + ------- + set[int] + Ticket identifiers visible to the configured v1 session. + + Raises + ------ + GlpiValidationError + When ``user_id`` is not a positive integer. + RuntimeError + When the client has no v1 session configured. + """ + + uid = _validate_actor_id(user_id, parameter) + v1 = self._require_v1_session("actor-based ticket statistics") + + params: dict[str, object] = {"forcedisplay[0]": _V1_SO_TICKET_ID} + for index, option in enumerate(search_options): + if index: + params[f"criteria[{index}][link]"] = "OR" + params[f"criteria[{index}][field]"] = option + params[f"criteria[{index}][searchtype]"] = "equals" + params[f"criteria[{index}][value]"] = uid + + ids: set[int] = set() + start = 0 + while True: + page = dict(params) + page["range"] = f"{start}-{start + _V1_SEARCH_PAGE_SIZE - 1}" + payload = v1.request_json("GET", "search/Ticket", params=page) + if not isinstance(payload, dict): + break + rows = payload.get("data") + if isinstance(rows, list): + for row in rows: + if not isinstance(row, dict): + continue + raw = row.get(str(_V1_SO_TICKET_ID)) + if isinstance(raw, bool) or not isinstance(raw, (int, str)): + continue + try: + ids.add(int(raw)) + except (TypeError, ValueError): + continue + total = payload.get("totalcount") + # Bound by totalcount: asking for a range that starts past the + # end is an HTTP 400 on this API, not an empty page. + if not isinstance(total, int): + break + start += _V1_SEARCH_PAGE_SIZE + if start >= total: + break + return ids + + def _v1_task_statistics( + self, ticket_ids: list[int], *, since: date + ) -> TaskStatisticsResult: + """Aggregate tasks for ``ticket_ids`` with one bulk v1 sweep. + + Replaces the per-ticket fan-out for large ticket sets. The v2 API + publishes tasks only under ``/Assistance/Ticket/{id}/Timeline/Task``, + so aggregating N tickets costs N requests; the v1 ``TicketTask`` + collection returns whole rows -- including ``tickets_id`` -- and + pages 1000 at a time. + + Note that v1 ``search/TicketTask`` is *not* usable here: its + searchOptions expose the task's own id, content, category, date, + privacy, technician, duration and state, but no parent ticket id, + so results could not be attributed back to a ticket. + + Rows are swept newest-first and paging stops once a page predates + ``since``. A task cannot be created before the ticket it belongs to, + and every ticket under consideration was created on or after + ``since``, so no relevant task is missed. The upper end is + deliberately unbounded: a ticket created inside the window may still + gain tasks long afterwards. + + The returned aggregate is identical to :meth:`get_task_statistics` + for the same tickets -- v1 ``actiontime`` is v2 ``duration``, and v1 + ``users_id`` is the v2 task ``user`` (the author; the technician + lives in ``users_id_tech``, which v2 does not expose). Rows are + mapped into ``GetTicketTask`` and summarised by the same helper, so + the two paths cannot drift apart. + + Parameters + ---------- + ticket_ids : list[int] + Tickets whose tasks should be aggregated. + since : date + Lower bound on task creation; the start of the caller's window. + + Returns + ------- + TaskStatisticsResult + Same shape and keys as :meth:`get_task_statistics`. + + Raises + ------ + RuntimeError + When the client has no v1 session configured. + """ + + v1 = self._require_v1_session("bulk task statistics") + wanted = set(ticket_ids) + cutoff = since.isoformat() + tasks: list[GetTicketTask] = [] + start = 0 + while True: + payload = v1.request_json( + "GET", + "TicketTask", + params={ + "range": f"{start}-{start + _V1_TASK_PAGE_SIZE - 1}", + "sort": "date_creation", + "order": "DESC", + }, + ) + if not isinstance(payload, list) or not payload: + break + oldest_seen: str | None = None + for row in payload: + if not isinstance(row, dict): + continue + created = row.get("date_creation") + if isinstance(created, str) and created: + oldest_seen = created + ticket_id = row.get("tickets_id") + if not isinstance(ticket_id, int) or ticket_id not in wanted: + continue + author = row.get("users_id") + duration = row.get("actiontime") + tasks.append( + GetTicketTask( + id=row.get("id") if isinstance(row.get("id"), int) else None, + tickets_id=ticket_id, + duration=duration if isinstance(duration, int) else 0, + user=( + IdNameRef(id=author) + if isinstance(author, int) and author + else None + ), + ) + ) + if len(payload) < _V1_TASK_PAGE_SIZE: + break + if oldest_seen is not None and oldest_seen[:10] < cutoff: + break + start += _V1_TASK_PAGE_SIZE + return _summarize_tasks(ticket_ids, tasks) + def get_ticket_statistics( self, *, @@ -139,7 +379,7 @@ def get_ticket_statistics( entity_filter: str | None = None if entity_id is not None: - entity_filter = f"entities_id=={entity_id}" + entity_filter = f"entity.id=={entity_id}" elif entity_name is not None: name_filter = rsql_contains_filter("name", entity_name) or "" entities = self.search_entities( # type: ignore[attr-defined] @@ -149,13 +389,14 @@ def get_ticket_statistics( if not entities: return {"entities": {}} entity_filter = rsql_any_filter( - *(f"entities_id=={e.id}" for e in entities if e.id is not None) + *(f"entity.id=={e.id}" for e in entities if e.id is not None) ) date_filter = f"date_creation=ge={start.isoformat()};" date_filter += f"date_creation=le={end.isoformat()} 23:59:59" query = rsql_all_filter( date_filter, entity_filter, + _LIVE_TICKETS, extra_filter, ) tickets: list[GetTicket] = self.search_tickets( # type: ignore[attr-defined] @@ -282,7 +523,7 @@ def get_task_durations( entity_filter: str | None = None if entity_id is not None: - entity_filter = f"entities_id=={entity_id}" + entity_filter = f"entity.id=={entity_id}" elif entity_name is not None: name_filter = rsql_contains_filter("name", entity_name) or "" entities = self.search_entities( # type: ignore[attr-defined] @@ -300,31 +541,44 @@ def get_task_durations( tasks=None, ) entity_filter = rsql_any_filter( - *(f"entities_id=={e.id}" for e in entities if e.id is not None) + *(f"entity.id=={e.id}" for e in entities if e.id is not None) ) - user_filter: str | None = None + # ``user_id`` selects on the ticket's actors, which v2 cannot + # express; resolve the id set through v1 and intersect below. + actor_ticket_ids: set[int] | None = None if user_id is not None: - user_filter = rsql_any_filter( - f"users_id_assign=={user_id}", - f"users_id_requester=={user_id}", + actor_ticket_ids = self._v1_ticket_ids_for_actor( + user_id, + search_options=(_V1_SO_ASSIGNEE, _V1_SO_REQUESTER), + parameter="user_id", ) + if not actor_ticket_ids: + return TaskDurationsResult( + start_date=start.isoformat(), + end_date=end.isoformat(), + total_duration=0, + task_count=0, + duration_by_user={}, + duration_by_entity={}, + tasks=None, + ) editor_filter: str | None = None if user_editor_id is not None: - editor_filter = f"users_id_lastupdater=={user_editor_id}" + editor_filter = f"user_editor.id=={user_editor_id}" recipient_filter: str | None = None if user_recipient_id is not None: - recipient_filter = f"users_id_requester=={user_recipient_id}" + recipient_filter = f"user_recipient.id=={user_recipient_id}" rsql_filter = ( rsql_all_filter( date_filter, entity_filter, - user_filter, editor_filter, recipient_filter, + _LIVE_TICKETS, extra_filter, ) or "" @@ -337,11 +591,19 @@ def get_task_durations( batch_size=200, ): for ticket in batch: - if ticket.id is not None: - ticket_ids.append(ticket.id) - ticket_entity_map[ticket.id] = _entity_key(ticket.entity) + if ticket.id is None: + continue + if actor_ticket_ids is not None and ticket.id not in actor_ticket_ids: + continue + ticket_ids.append(ticket.id) + ticket_entity_map[ticket.id] = _entity_key(ticket.entity) - result = self.get_task_statistics(ticket_ids) + # One bulk v1 sweep replaces the per-ticket fan-out once the ticket + # set is big enough to pay for it; the aggregate is identical. + if self._v1 is not None and len(ticket_ids) >= _V1_TASK_BULK_THRESHOLD: + result = self._v1_task_statistics(ticket_ids, since=start) + else: + result = self.get_task_statistics(ticket_ids) duration_by_ticket = result["duration_by_ticket"] duration_by_entity: defaultdict[str, int] = defaultdict(int) @@ -472,21 +734,37 @@ def get_user_activity( date_range = f"date_creation=ge={start.isoformat()};" date_range += f"date_creation=le={end.isoformat()} 23:59:59" + # The date window is resolved once for every user rather than once + # per user per role. Previously each user drove two full pagings of + # the corpus, and because the actor clause was silently dropped by + # v2 both walks returned the same unfiltered window. + window_filter = rsql_all_filter(date_range, _LIVE_TICKETS) or "" + window_ids: set[int] = set() + for batch in self.iter_search_tickets( # type: ignore[attr-defined] + window_filter, + batch_size=200, + ): + for ticket in batch: + if ticket.id is not None: + window_ids.add(ticket.id) + users_output: dict[str, UserActivityEntry] = {} for uid in resolved_user_ids: display_key = user_display_map.get(uid, str(uid)) - tech_count = 0 - for batch in self.iter_search_tickets( # type: ignore[attr-defined] - f"users_id_assign=={uid};{date_range}", - batch_size=200, - ): - tech_count += len(batch) - recipient_count = 0 - for batch in self.iter_search_tickets( # type: ignore[attr-defined] - f"users_id_requester=={uid};{date_range}", - batch_size=200, - ): - recipient_count += len(batch) + # Assignee and requester are counted separately, so they are + # resolved as separate v1 id sets rather than one OR-ed query. + tech_count = len( + window_ids + & self._v1_ticket_ids_for_actor( + uid, search_options=(_V1_SO_ASSIGNEE,), parameter="user_id" + ) + ) + recipient_count = len( + window_ids + & self._v1_ticket_ids_for_actor( + uid, search_options=(_V1_SO_REQUESTER,), parameter="user_id" + ) + ) task_dur = self.get_task_durations( start_date=start_date, end_date=end_date, diff --git a/glpi_python_client/clients/custom/_statistics_async.py b/glpi_python_client/clients/custom/_statistics_async.py index fc4d00b..0cb4914 100644 --- a/glpi_python_client/clients/custom/_statistics_async.py +++ b/glpi_python_client/clients/custom/_statistics_async.py @@ -20,6 +20,10 @@ from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.custom._statistics import ( + _LIVE_TICKETS, + _V1_SO_ASSIGNEE, + _V1_SO_REQUESTER, + _V1_TASK_BULK_THRESHOLD, StatisticsMixin, TaskDurationsResult, TaskStatisticsResult, @@ -150,7 +154,7 @@ async def get_task_durations( # type: ignore[override] date_filter += f"date_creation=le={end.isoformat()} 23:59:59" entity_filter: str | None = None if entity_id is not None: - entity_filter = f"entities_id=={entity_id}" + entity_filter = f"entity.id=={entity_id}" elif entity_name is not None: name_filter = rsql_contains_filter("name", entity_name) or "" entities = await self.search_entities( # type: ignore[attr-defined] @@ -168,31 +172,47 @@ async def get_task_durations( # type: ignore[override] tasks=None, ) entity_filter = rsql_any_filter( - *(f"entities_id=={e.id}" for e in entities if e.id is not None) + *(f"entity.id=={e.id}" for e in entities if e.id is not None) ) - user_filter: str | None = None + # Mirrors the synchronous mixin: v2 cannot filter on ticket actors, + # so the id set comes from v1 and is intersected below. The v1 call + # is blocking, so it runs in a worker thread. + actor_ticket_ids: set[int] | None = None if user_id is not None: - user_filter = rsql_any_filter( - f"users_id_assign=={user_id}", - f"users_id_requester=={user_id}", + actor_ticket_ids = await asyncio.to_thread( + StatisticsMixin._v1_ticket_ids_for_actor, + self, + user_id, + search_options=(_V1_SO_ASSIGNEE, _V1_SO_REQUESTER), + parameter="user_id", ) + if not actor_ticket_ids: + return TaskDurationsResult( + start_date=start.isoformat(), + end_date=end.isoformat(), + total_duration=0, + task_count=0, + duration_by_user={}, + duration_by_entity={}, + tasks=None, + ) editor_filter: str | None = None if user_editor_id is not None: - editor_filter = f"users_id_lastupdater=={user_editor_id}" + editor_filter = f"user_editor.id=={user_editor_id}" recipient_filter: str | None = None if user_recipient_id is not None: - recipient_filter = f"users_id_requester=={user_recipient_id}" + recipient_filter = f"user_recipient.id=={user_recipient_id}" rsql_filter = ( rsql_all_filter( date_filter, entity_filter, - user_filter, editor_filter, recipient_filter, + _LIVE_TICKETS, extra_filter, ) or "" @@ -205,11 +225,25 @@ async def get_task_durations( # type: ignore[override] batch_size=200, ): for ticket in batch: - if ticket.id is not None: - ticket_ids.append(ticket.id) - ticket_entity_map[ticket.id] = _entity_key(ticket.entity) - - result = await self.get_task_statistics(ticket_ids) + if ticket.id is None: + continue + if actor_ticket_ids is not None and ticket.id not in actor_ticket_ids: + continue + ticket_ids.append(ticket.id) + ticket_entity_map[ticket.id] = _entity_key(ticket.entity) + + # Mirrors the synchronous mixin: one bulk v1 sweep replaces the + # per-ticket fan-out for large ticket sets. The v1 call is blocking, + # so it runs in a worker thread. + if self._v1 is not None and len(ticket_ids) >= _V1_TASK_BULK_THRESHOLD: + result = await asyncio.to_thread( + StatisticsMixin._v1_task_statistics, + self, + ticket_ids, + since=start, + ) + else: + result = await self.get_task_statistics(ticket_ids) duration_by_entity: defaultdict[str, int] = defaultdict(int) for tid, dur in result["duration_by_ticket"].items(): @@ -319,7 +353,7 @@ async def get_ticket_statistics( # type: ignore[override] entity_filter: str | None = None if entity_id is not None: - entity_filter = f"entities_id=={entity_id}" + entity_filter = f"entity.id=={entity_id}" elif entity_name is not None: name_filter = rsql_contains_filter("name", entity_name) or "" entities = await self.search_entities( # type: ignore[attr-defined] @@ -329,7 +363,7 @@ async def get_ticket_statistics( # type: ignore[override] if not entities: return {"entities": {}} entity_filter = rsql_any_filter( - *(f"entities_id=={e.id}" for e in entities if e.id is not None) + *(f"entity.id=={e.id}" for e in entities if e.id is not None) ) date_filter = f"date_creation=ge={start.isoformat()};" @@ -337,6 +371,7 @@ async def get_ticket_statistics( # type: ignore[override] query = rsql_all_filter( date_filter, entity_filter, + _LIVE_TICKETS, extra_filter, ) tickets = await self.search_tickets( # type: ignore[attr-defined] @@ -449,21 +484,39 @@ async def get_user_activity( # type: ignore[override] date_range = f"date_creation=ge={start.isoformat()};" date_range += f"date_creation=le={end.isoformat()} 23:59:59" + # Mirrors the synchronous mixin: the window is walked once for all + # users, and the per-role split comes from intersecting v1 id sets. + window_filter = rsql_all_filter(date_range, _LIVE_TICKETS) or "" + window_ids: set[int] = set() + async for batch in self.iter_search_tickets( # type: ignore[attr-defined] + window_filter, + batch_size=200, + ): + for ticket in batch: + if ticket.id is not None: + window_ids.add(ticket.id) + users_output: dict[str, UserActivityEntry] = {} for uid in resolved_user_ids: display_key = user_display_map.get(uid, str(uid)) - tech_count = 0 - async for batch in self.iter_search_tickets( # type: ignore[attr-defined] - f"users_id_assign=={uid};{date_range}", - batch_size=200, - ): - tech_count += len(batch) - recipient_count = 0 - async for batch in self.iter_search_tickets( # type: ignore[attr-defined] - f"users_id_requester=={uid};{date_range}", - batch_size=200, - ): - recipient_count += len(batch) + assigned_ids, requested_ids = await asyncio.gather( + asyncio.to_thread( + StatisticsMixin._v1_ticket_ids_for_actor, + self, + uid, + search_options=(_V1_SO_ASSIGNEE,), + parameter="user_id", + ), + asyncio.to_thread( + StatisticsMixin._v1_ticket_ids_for_actor, + self, + uid, + search_options=(_V1_SO_REQUESTER,), + parameter="user_id", + ), + ) + tech_count = len(window_ids & assigned_ids) + recipient_count = len(window_ids & requested_ids) task_dur = await self.get_task_durations( start_date=start_date, end_date=end_date, diff --git a/glpi_python_client/clients/custom/tests/test_statistics.py b/glpi_python_client/clients/custom/tests/test_statistics.py index 519b111..ebb187b 100644 --- a/glpi_python_client/clients/custom/tests/test_statistics.py +++ b/glpi_python_client/clients/custom/tests/test_statistics.py @@ -19,6 +19,10 @@ GlpiTicketType, GlpiValidationError, ) +from glpi_python_client.clients.custom._statistics import ( + _V1_SO_ASSIGNEE, + _V1_SO_REQUESTER, +) from glpi_python_client.models.api_schema._common import ( IdNameCompletenameRef, IdNameRef, @@ -30,6 +34,41 @@ from glpi_python_client.testing.utils import make_client +class _FakeV1Search: + """Stand-in v1 session answering ``search/Ticket`` actor lookups. + + Maps a v1 searchOption id to the ticket ids that option should match, + and mimics the wire shape the real endpoint returns: rows keyed by the + searchOption id as a *string*, plus a ``totalcount`` used to bound + paging (the live API answers HTTP 400 for a range past the end). + """ + + def __init__(self, by_option: dict[int, list[int]]) -> None: + self.by_option = by_option + self.calls: list[dict[str, Any]] = [] + + def request_json( + self, + method: str, + path: str, + *, + params: dict[str, object] | None = None, + json_body: dict[str, object] | None = None, + success_statuses: tuple[int, ...] = (200, 201, 204, 206), + failure_message: str | None = None, + ) -> object: + self.calls.append({"method": method, "path": path, "params": params}) + criteria = params or {} + matched: list[int] = [] + index = 0 + while f"criteria[{index}][field]" in criteria: + option = int(str(criteria[f"criteria[{index}][field]"])) + matched.extend(self.by_option.get(option, [])) + index += 1 + rows = [{"2": ticket_id} for ticket_id in sorted(set(matched))] + return {"totalcount": len(rows), "data": rows} + + @pytest.fixture def client() -> GlpiClient: """Return one in-memory test client.""" @@ -241,7 +280,12 @@ def fake_search( end_date="2026-01-31", entity_id=7, ) - assert "entities_id==7" in captured["filter"] + # v2 types Ticket.entity as object{id,name}; the bare `entities_id` is a + # v1 field name that v2 silently ignores, returning every ticket. + assert "entity.id==7" in captured["filter"] + assert "entities_id" not in captured["filter"] + # v2 counts soft-deleted tickets unless told otherwise. + assert "is_deleted==false" in captured["filter"] def test_get_ticket_statistics_entity_name_resolution(client: GlpiClient) -> None: @@ -269,8 +313,12 @@ def fake_search( end_date="2026-01-31", entity_name="Acme", ) - assert "entities_id==3" in captured_tickets["filter"] - assert "entities_id==4" in captured_tickets["filter"] + assert "entity.id==3" in captured_tickets["filter"] + assert "entity.id==4" in captured_tickets["filter"] + assert "entities_id" not in captured_tickets["filter"] + # The OR group must be parenthesised or the date window stops applying + # to every entity after the first. + assert "(entity.id==3,entity.id==4)" in captured_tickets["filter"] def test_get_ticket_statistics_entity_name_no_match(client: GlpiClient) -> None: @@ -471,16 +519,19 @@ def fake_search_users( ) -> list[GetUser]: return [GetUser(id=42, username="alice", firstname="Alice", realname="Smith")] - tech_calls: list[str] = [] - recip_calls: list[str] = [] + window_calls: list[str] = [] def fake_iter(rsql_filter: str = "", *, batch_size: int = 200): - if "users_id_assign" in rsql_filter: - tech_calls.append(rsql_filter) - yield [_make_ticket(1)] - else: - recip_calls.append(rsql_filter) - yield [] + # One date-window walk now serves every user and every role; the + # per-role split comes from the v1 id sets intersected below. + window_calls.append(rsql_filter) + yield [_make_ticket(1), _make_ticket(2)] + + # Ticket 1 is assigned to the user, ticket 2 is not; ticket 3 is + # assigned but falls outside the window, so it must not be counted. + client._v1 = _FakeV1Search( # type: ignore[assignment] + {_V1_SO_ASSIGNEE: [1, 3], _V1_SO_REQUESTER: []} + ) def fake_task_durations( *, @@ -511,9 +562,19 @@ def fake_task_durations( key = next(iter(users)) data = users[key] assert data["user_ids"] == [42] + # Ticket 1 is both in the window and assigned; ticket 3 is assigned but + # outside the window, so the intersection keeps exactly one. assert data["tickets_as_technician"] == 1 assert data["tickets_as_recipient"] == 0 assert "total_duration" in data["task_durations"] + # The window is walked once, not once per user per role, and it must + # exclude trashed tickets. + assert len(window_calls) == 1 + assert "is_deleted==false" in window_calls[0] + # A regression to the v1 field names would be invisible against the + # live server, so pin their absence here. + assert "users_id_assign" not in window_calls[0] + assert "users_id_requester" not in window_calls[0] def test_get_user_activity_raises_when_no_users_matched(client: GlpiClient) -> None: @@ -558,10 +619,12 @@ def fake_search_users( ] def fake_iter(rsql_filter: str = "", *, batch_size: int = 200): - if "users_id_assign" in rsql_filter: - yield [_make_ticket(1)] - else: - yield [] + yield [_make_ticket(1)] + + # Both merged users are assigned ticket 1, so each contributes 1. + client._v1 = _FakeV1Search( # type: ignore[assignment] + {_V1_SO_ASSIGNEE: [1], _V1_SO_REQUESTER: []} + ) def fake_task_durations( *, @@ -595,3 +658,121 @@ def fake_task_durations( assert sorted(data["user_ids"]) == [1, 2] assert data["tickets_as_technician"] == 2 # 1 per user assert data["task_durations"]["total_duration"] == 600 # 300 per user + + +class _FakeV1Tasks: + """v1 stand-in serving pages of raw ``TicketTask`` collection rows.""" + + def __init__(self, rows: list[dict[str, Any]], page_size: int = 1000) -> None: + self.rows = rows + self.page_size = page_size + self.calls: list[dict[str, Any]] = [] + + def request_json( + self, + method: str, + path: str, + *, + params: dict[str, object] | None = None, + json_body: dict[str, object] | None = None, + success_statuses: tuple[int, ...] = (200, 201, 204, 206), + failure_message: str | None = None, + ) -> object: + self.calls.append({"path": path, "params": params}) + rng = str((params or {}).get("range", "0-999")) + start = int(rng.split("-")[0]) + return self.rows[start : start + self.page_size] + + def close(self) -> None: + """No-op.""" + + +def _task_row( + task_id: int, ticket_id: int, duration: int, author: int +) -> dict[str, Any]: + return { + "id": task_id, + "tickets_id": ticket_id, + "actiontime": duration, + "users_id": author, + "users_id_tech": 999, + "date_creation": "2026-07-10 09:00:00", + } + + +def test_v1_task_statistics_matches_the_per_ticket_shape(client: GlpiClient) -> None: + """The bulk v1 sweep produces the same aggregate as the per-ticket path. + + v1 ``actiontime`` is v2 ``duration`` and v1 ``users_id`` is the v2 task + ``user``; both were confirmed equal against a live GLPI 11 instance, so + the optimisation must not change any returned value. + """ + + rows = [ + _task_row(1, 10, 600, 5), + _task_row(2, 10, 300, 5), + _task_row(3, 11, 900, 7), + # Belongs to a ticket outside the requested set and must be ignored. + _task_row(4, 99, 12345, 5), + ] + client._v1 = _FakeV1Tasks(rows) # type: ignore[assignment] + + result = client._v1_task_statistics([10, 11], since=date(2026, 7, 1)) + + assert result["ticket_count"] == 2 + assert result["task_count"] == 3 + assert result["total_duration"] == 1800 + assert result["duration_by_ticket"] == {10: 900, 11: 900} + assert result["duration_by_user"] == {"5": 900, "7": 900} + + +def test_v1_task_statistics_stops_paging_before_the_window( + client: GlpiClient, +) -> None: + """Paging stops once a page predates the window start. + + A task cannot exist before its ticket, so nothing relevant is skipped. + """ + + old = _task_row(5, 10, 60, 5) | {"date_creation": "2020-01-01 09:00:00"} + fake = _FakeV1Tasks([_task_row(1, 10, 600, 5), old], page_size=2) + client._v1 = fake # type: ignore[assignment] + + result = client._v1_task_statistics([10], since=date(2026, 7, 1)) + + # Both rows are in the first page, so they are both aggregated; the + # sweep then stops rather than requesting another page. + assert result["task_count"] == 2 + assert len(fake.calls) == 1 + assert fake.calls[0]["path"] == "TicketTask" + assert fake.calls[0]["params"]["sort"] == "date_creation" + assert fake.calls[0]["params"]["order"] == "DESC" + + +def test_get_task_durations_uses_per_ticket_path_below_threshold( + client: GlpiClient, +) -> None: + """A small ticket set keeps the v2 per-ticket path and needs no v1.""" + + def fake_iter(rsql_filter: str = "", *, batch_size: int = 200): + yield [_make_ticket(1)] + + calls: list[list[int]] = [] + + def fake_task_stats(ticket_ids: list[int]) -> dict[str, Any]: + calls.append(ticket_ids) + return { + "ticket_count": len(ticket_ids), + "task_count": 0, + "total_duration": 0, + "duration_by_user": {}, + "duration_by_ticket": {}, + } + + client.iter_search_tickets = fake_iter # type: ignore[method-assign] + client.get_task_statistics = fake_task_stats # type: ignore[method-assign] + # No v1 session configured at all: the small-set path must not need one. + result = client.get_task_durations(start_date="2026-07-01", end_date="2026-07-31") + + assert result["task_count"] == 0 + assert calls == [[1]] diff --git a/glpi_python_client/clients/custom/tests/test_statistics_async.py b/glpi_python_client/clients/custom/tests/test_statistics_async.py index 4a93bb8..778d519 100644 --- a/glpi_python_client/clients/custom/tests/test_statistics_async.py +++ b/glpi_python_client/clients/custom/tests/test_statistics_async.py @@ -210,9 +210,19 @@ async def fake_stats(ticket_ids: list[int]) -> dict[str, Any]: "duration_by_ticket": {1: 0}, } + class _FakeV1: + """v1 stand-in: ``user_id`` is resolved through the v1 search.""" + + def request_json(self, method: str, path: str, **kwargs: Any) -> object: + return {"totalcount": 1, "data": [{"2": 1}]} + + def close(self) -> None: + """No-op.""" + aclient.search_entities = fake_search_entities # type: ignore[method-assign] aclient.iter_search_tickets = fake_iter # type: ignore[method-assign] aclient.get_task_statistics = fake_stats # type: ignore[method-assign] + aclient._v1 = _FakeV1() # type: ignore[assignment] result = await aclient.get_task_durations( start_date="2026-01-01", @@ -224,9 +234,16 @@ async def fake_stats(ticket_ids: list[int]) -> dict[str, Any]: extra_filter="status==1", ) assert result["task_count"] == 0 - assert "entities_id==42" in captured["filter"] - assert "users_id_assign==7" in captured["filter"] - assert "users_id_lastupdater==8" in captured["filter"] - assert "users_id_requester==9" in captured["filter"] + assert "entity.id==42" in captured["filter"] + assert "user_editor.id==8" in captured["filter"] + assert "user_recipient.id==9" in captured["filter"] assert "status==1" in captured["filter"] + assert "is_deleted==false" in captured["filter"] + # ``user_id`` selects on actors, which v2 cannot express, so it is + # resolved through v1 and must not appear in the v2 filter at all. + assert "users_id_assign" not in captured["filter"] + assert "user_id" not in captured["filter"] + # None of the dropped v1 spellings may come back. + for dead in ("entities_id", "users_id_lastupdater", "users_id_requester"): + assert dead not in captured["filter"] await aclient.close() diff --git a/glpi_python_client/clients/tests/test_async_branches.py b/glpi_python_client/clients/tests/test_async_branches.py index 7a8decd..016059b 100644 --- a/glpi_python_client/clients/tests/test_async_branches.py +++ b/glpi_python_client/clients/tests/test_async_branches.py @@ -11,6 +11,32 @@ from glpi_python_client.testing.utils import FakeResponse, make_async_client +class _FakeV1Ids: + """Minimal v1 session returning a fixed ticket-id set for any actor query. + + ``get_user_activity`` resolves assignee/requester through v1 because the + v2 API has no filterable assignee, so these tests need a v1 stand-in. + """ + + def __init__(self, ticket_ids: list[int]) -> None: + self.ticket_ids = ticket_ids + + def request_json(self, method: str, path: str, **kwargs: Any) -> object: + rows = [{"2": ticket_id} for ticket_id in self.ticket_ids] + return {"totalcount": len(rows), "data": rows} + + def close(self) -> None: + """No-op; the real session is closed with the client.""" + + +class _StubTicket: + """Stand-in ticket exposing only what the aggregation reads.""" + + def __init__(self, ticket_id: int) -> None: + self.id = ticket_id + self.entity = None + + async def test_async_bridge_uses_provided_executor() -> None: """A custom executor is used to dispatch the wrapped sync call.""" @@ -552,6 +578,7 @@ async def fake_task_durations(**kwargs: Any) -> Any: client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] client.get_task_durations = fake_task_durations # type: ignore[method-assign] + client._v1 = _FakeV1Ids([]) # type: ignore[assignment] result = await client.get_user_activity(user_id=42) assert "users" in result @@ -608,6 +635,7 @@ async def fake_task_durations(**kwargs: Any) -> Any: client.search_users = fake_search_users # type: ignore[method-assign] client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] client.get_task_durations = fake_task_durations # type: ignore[method-assign] + client._v1 = _FakeV1Ids([]) # type: ignore[assignment] result = await client.get_user_activity(username="alice") assert "users" in result @@ -644,7 +672,7 @@ async def test_async_get_user_activity_counts_ticket_batches() -> None: client = make_async_client() async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - yield [{"id": 1}] + yield [_StubTicket(1)] async def fake_task_durations(**kwargs: Any) -> Any: return TaskDurationsResult( @@ -659,6 +687,8 @@ async def fake_task_durations(**kwargs: Any) -> Any: client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] client.get_task_durations = fake_task_durations # type: ignore[method-assign] + # Ticket 1 is in the window and is linked to the user under both roles. + client._v1 = _FakeV1Ids([1]) # type: ignore[assignment] result = await client.get_user_activity(user_id=99) entry = list(result["users"].values()) @@ -667,6 +697,56 @@ async def fake_task_durations(**kwargs: Any) -> Any: await client.close() +async def test_async_get_user_activity_counts_are_role_specific() -> None: + """Assignee and requester counts come from independent v1 id sets. + + The previous implementation sent v1 field names to v2, which silently + ignored them, so both counts collapsed to "every ticket in the window" + and were always equal. Here the window holds two tickets but the user + is linked to only one, under one role. + """ + + from glpi_python_client.clients.custom._statistics import TaskDurationsResult + + client = make_async_client() + + async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: + yield [_StubTicket(1), _StubTicket(2)] + + async def fake_task_durations(**kwargs: Any) -> Any: + return TaskDurationsResult( + start_date="2025-01-01", + end_date="2025-01-31", + total_duration=0, + task_count=0, + duration_by_user={}, + duration_by_entity={}, + tasks=None, + ) + + class _RoleAwareV1: + def request_json(self, method: str, path: str, **kwargs: Any) -> object: + params = kwargs.get("params") or {} + option = int(str(params.get("criteria[0][field]"))) + # searchOption 5 == assignee, 4 == requester. + ids = [1] if option == 5 else [] + rows = [{"2": ticket_id} for ticket_id in ids] + return {"totalcount": len(rows), "data": rows} + + def close(self) -> None: + """No-op; the real session is closed with the client.""" + + client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] + client.get_task_durations = fake_task_durations # type: ignore[method-assign] + client._v1 = _RoleAwareV1() # type: ignore[assignment] + + result = await client.get_user_activity(user_id=99) + entry = next(iter(result["users"].values())) + assert entry["tickets_as_technician"] == 1 + assert entry["tickets_as_recipient"] == 0 + await client.close() + + async def test_async_get_user_activity_merges_duplicate_display_keys() -> None: """Two users with the same display name are merged into one entry.""" @@ -702,6 +782,7 @@ async def fake_task_durations(**kwargs: Any) -> Any: client.search_users = fake_search_users # type: ignore[method-assign] client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] client.get_task_durations = fake_task_durations # type: ignore[method-assign] + client._v1 = _FakeV1Ids([]) # type: ignore[assignment] result = await client.get_user_activity(username="Smith") # Both users merge under "John Smith" key