Fix/v2 filter correctness - #17
Merged
Merged
Conversation
The v2 API silently ignores a `filter=` conjunct whose field it does not
recognise: it drops that conjunct, honours the rest, and returns 200/206
with no error. The statistics layer was built on v1 field names, so its
user and entity selection never applied while the date clause still did --
the results narrowed, looked plausible, and were wrong.
Measured live before this change, get_user_activity reported
tickets_as_technician == tickets_as_recipient == 963 (the window's total
ticket count) for every user, whoever they were.
entities_id==N -> entity.id==N
users_id_lastupdater -> user_editor.id==N
user_recipient_id -> user_recipient.id==N (matches the parameter name;
v2 user_recipient is who RECORDED the ticket, which
is not the requester -- do not conflate them)
user_id (actors) -> v1 search/Ticket searchOption 5 / 4
There is no v2 assignee filter. Its `team` array cannot be joined by the
RSQL engine: the four contract-declared subfields answer HTTP 500 and all
19 other spellings tested are silently ignored, returning the full corpus
byte-identically to a `nosuchfield==N` control. v1 searchOptions 5
("Technicien") and 4 ("Demandeur") map exactly onto the glpi_tickets_users
link types and, unlike v2, fail loudly with HTTP 400 on an unknown field.
Also fixed, both found while verifying the above:
* rsql_any_filter emitted an unparenthesised OR group. RSQL binds `;`
tighter than `,`, so `date;e==1,e==2` parses as `(date AND e==1) OR e==2`
and the date window stopped applying to every entity after the first.
Live: 16,245 returned where the correct answer was 1,552.
* v2 counts soft-deleted tickets by default, v1 does not (59,690 + 258 =
59,948). Every aggregation was inflated by the trash bin; for one user
92% of matches were deleted. All v2 ticket queries now pin
is_deleted==false.
Actor ids are validated before reaching v1, which fails open rather than
rejecting bad input: `equals 0` matches 20,905 tickets, an empty value
matches the entire baseline, and a non-numeric value returns the same
arbitrary 3 rows whatever the string.
get_user_activity now walks the date window once for all users instead of
twice per user. One user over 90 days: 979 requests / 120s -> 9 / 5.1s.
Actor statistics now require the v1 session and raise RuntimeError naming
the missing options, rather than returning a wrong number.
Verified live: user 70 has 927 assigned tickets all-time and 0 created in
the last 90 days; the fixed code returns 0 where v1 independently agrees,
and the previous code returned 963.
The unit tests that asserted the old filter strings are updated -- they
passed *because* of the bug, asserting what the client built rather than
what the server honoured, which is why this survived.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
==========================================
- Coverage 97.39% 96.69% -0.71%
==========================================
Files 79 79
Lines 2536 2634 +98
==========================================
+ Hits 2470 2547 +77
- Misses 66 87 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
get_task_durations aggregated tasks by issuing one v2 request per ticket,
because v2 publishes tasks only under /Assistance/Ticket/{id}/Timeline/Task.
That fan-out was ~97% of the method's cost.
The v1 TicketTask *collection* endpoint returns whole rows -- crucially
including tickets_id -- and pages 1000 at a time, sorted. Sweeping it
newest-first and stopping once a page predates the window start replaces
the fan-out entirely.
v1 search/TicketTask is NOT usable here and this is worth recording: its
complete searchOption set is the task's own id, content, category, date,
privacy, technician, duration and state -- there is no parent ticket id, so
rows cannot be attributed back to a ticket. Only the collection endpoint
carries tickets_id. (Its searchText filter is also a LIKE, not an exact
match: searchText[tickets_id]=32 returns 2409 rows, and a plain tickets_id
param is silently ignored -- neither is usable for exact selection.)
Correctness is preserved by construction: v1 rows are mapped into
GetTicketTask and handed to the same _summarize_tasks helper the v2 path
uses, so the two cannot drift apart. The field mapping was verified against
live data rather than assumed -- v1 actiontime == v2 duration, and v1
users_id == the v2 task `user` (the author). Note v2 does not expose
users_id_tech, so `user` is the author, not the technician; the bulk path
deliberately reproduces that rather than silently changing what
duration_by_user means.
Paging stops below the window start, which is safe because a task cannot be
created before the ticket it belongs to. The upper end is deliberately
unbounded: a ticket created inside the window may gain tasks long after it.
Below 25 tickets the per-ticket path is cheaper and is kept, so v2-only
clients keep working without a v1 session.
Verified live on a 120-ticket set:
BULK (v1) : 2 requests 0.4s
PER-TICKET (v2) : 120 requests 11.7s
ticket_count / task_count / total_duration / duration_by_ticket /
duration_by_user -> all IDENTICAL
Async path verified separately: 3 requests, 1.3s.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.