Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API by Xzzz · Pull Request #2743 · mozilla/bmo

Xzzz · 2026-09-14T14:25:06Z

Summary

Ports Bugzilla::WebService::BugUserLastVisit's get/update methods into a native Bugzilla::API::V1::BugUserLastVisit Mojo controller, mirroring the pattern already used for Classification/Component/Teams/Reminders/Configuration/Bugzilla (system info).

This is a child bug of 2057358, see there for details.

Changes

Add Bugzilla/API/V1/BugUserLastVisit.pm: GET/POST /rest/bug_user_last_visit and /rest/bug_user_last_visit/<id> (login required), same JSON response shape as the legacy endpoints
Delete Bugzilla/WebService/BugUserLastVisit.pm and Bugzilla/WebService/Server/REST/Resources/BugUserLastVisit.pm
Remove the BugUserLastVisit entry from WS_DISPATCH in Bugzilla/WebService/Constants.pm and drop the corresponding use line in Bugzilla/WebService/Server/REST.pm

Breaking change: removing the WS_DISPATCH entry also removes BugUserLastVisit.get/update from JSON-RPC and XML-RPC, not just the legacy REST dispatcher, since all three share that table. Native Mojo routes only serve REST. This matches the same tradeoff already made in the Classification and Bugzilla (system-info) migrations earlier in this series.

Test plan

GET /rest/bug_user_last_visit (anonymous => login_required, authenticated => list of last-visited bugs)
GET /rest/bug_user_last_visit/<id>
GET /rest/bug_user_last_visit?ids=<id>&ids=<id>
POST /rest/bug_user_last_visit/<id>
POST /rest/bug_user_last_visit with {"ids":[...]} body
OPTIONS on both routes returns Allow: GET, POST
Confirm response shape (bare JSON array, last_visit_ts with trailing Z) matches the legacy endpoint

References

Bugzilla link: 2065171
Part of 2057358

…_fields

…er body

Xzzz · 2026-09-14T20:08:21Z

Reading API doc, I realized that docs/en/rst/api/core/v1/general.rst and the legacy REST dispatcher (Bugzilla::WebService::Server::REST::_retrieve_json_params) both establish that query-string params override the body for non-GET requests.
My _request_params fix does the opposite: it merges {%$params,%$body_params}, so JSON body wins on a key collision.

=> Fixed in "Bug 2065171 - Fix ids/include_fields precedence: query string wins over body"

`_request_params->{ids} // []` made a missing ids param filter to nothing instead of returning every visited bug, since an empty arrayref is truthy. Legacy left $ids undef when the param is absent, skipping filter entirely. Return undef in that case matches legacy behavior. An empty array still filters to nothing.

_request_params duplicated the query-string/JSON-body merge logic. Now call a single shared Bugzilla::WebService::Util::merge_request_params helper, so it's a one-place change to drop later if query-string-on-POST support is ever removed.

Xzzz · 2026-09-17T12:56:53Z

Pushed a follow-up commit: _request_params now calls a shared Bugzilla::WebService::Util::merge_request_params helper instead of doing the query-string/JSON-body merge inline. Extracted a single reusable helper rather than keeping two copies.
The include_fields/exclude_fields split stays local here since it's specific to this resource.
=> No behavior change

_ids_from_request short-circuited to the path id whenever present, never consulting the merged query-string/body params. Legacy's _retrieve_json_params merges non-GET request-body/query params in *after* the path-derived params, so those win for POST. For GET, the path id still wins (legacy's override step only ran for non-GET requests), so that precedence is unchanged. Also switch from $self->param('id') (a truthiness check that also falls back to a same-named query param) to $self->stash('id') (defined check, route-placeholder only). This fixes two more bugs: - /bug_user_last_visit/0 was falling through to the no-ids branch since "0" is falsy - a stray ?id=5 query parameter (distinct from ids) was being treated as if it were a path id

Covers: anonymous access requiring login, OPTIONS, POST via path id, POST via a JSON ids body, POST with a JSON body posted with no Content-Type header, GET via path id vs query-string ids precedence, GET via query-string ids, and GET with no ids returning every visited bug

dklawren · 2026-09-18T20:30:14Z

+  if (length $c->req->body) {
+    my $body_params;
+    try { $body_params = decode_json($c->req->body); }
+    catch { $body_params = undef; };
+    $params = {%$body_params, %$params} if ref $body_params eq 'HASH';
+  }


a malformed JSON body is silently discarded here, which can produce a wrong result instead of an error

legacy _retrieve_json_params threw json_rpc_invalid_params on a decode failure. now the catch sets undef and the request continues.

concrete case: a client sends POST /rest/bug_user_last_visit/123 with an intended body of {"ids":[456]}, but the body is truncated. decode fails, $body_params is undef, and the path id wins, so the endpoint returns 200 and records a visit against bug 123 instead of 456. no error reaches the caller.

the bare POST /rest/bug_user_last_visit variant degrades into a confusing param_required for the same reason.

suggestion: when the body is non-empty and the content type is JSON but decoding fails, raise rest_malformed_json (already used in Bugzilla/API/V1/Reminders.pm:69). keep ignoring genuinely non-JSON bodies so urlencoded and multipart form posts still work.

worth also calling this file out in the PR description, since it is shared and the change affects more than this endpoint

dklawren · 2026-09-18T20:30:14Z

+my $bug_id_1 = create_bug('bug_user_last_visit test bug 1');
+my $bug_id_2 = create_bug('bug_user_last_visit test bug 2');
+
+### Section 1: Anonymous access requires login


the negative cases that would actually prove behavioral parity are missing

a group-restricted bug the user cannot see - this is the security relevant one, and the parity claim in the PR description rests on it. qa/config already has restricted-group fixtures used by other qa/t/rest_*.t files

anonymous POST (only anonymous GET is covered)

a nonexistent bug id, including that the transaction is rolled back mid-loop

POST /rest/bug_user_last_visit with no ids anywhere, expecting param_required

include_fields / exclude_fields, which _request_params explicitly handles

the first one plus the malformed-body case on Util.pm are the same underlying gap: failures that quietly turn into plausible-looking successes

dklawren · 2026-09-18T20:30:14Z

+    @last_visits = grep { $id_set{$_->bug_id} } @last_visits;
+  }
+
+  my $params = $self->_request_params;


the request body gets read four times per POST on a hot path

_request_params runs twice per request, once inside _ids_from_request (line 47 in get, line 84 in update) and again here, and merge_request_params itself calls $c->req->body twice (Bugzilla/WebService/Util.pm:316,318). for a file-backed request asset each body call re-slurps from disk, so one POST slurps 4x and JSON-decodes 2x.

extensions/BugModal/web/bug_modal.js:1699 fires a GET plus a POST on every bug page view, so this runs constantly.

suggestion: compute my $params = $self->_request_params once in get and update and pass it into _ids_from_request, and hoist my $body = $c->req->body into a variable in merge_request_params

dklawren · 2026-09-18T20:30:14Z

+  foreach my $path ('/', '/:id') {
+    $routes->options($path)->to('V1::BugUserLastVisit#options');
+  }


the OPTIONS routes use a bare /:id while GET and POST on lines 23 and 25 constrain [id => qr/\d+/]

so OPTIONS /rest/bug_user_last_visit/abc answers 200 Allow: GET, POST, advertising methods that 404 on that path

suggestion: apply the same [id => qr/\d+/] constraint in the OPTIONS loop

Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API

1bbf944

dklawren requested changes Sep 14, 2026

View reviewed changes

Xzzz added 4 commits September 14, 2026 19:49

Bug 2065171 - Merge query-string and JSON body params for ids/include…

dc33218

…_fields

Bug 2065171 - Reject non-array ids with invalid_params

f2621a2

Bug 2065171 - Use resolved bug id, restrict :id route to digits

aeaff1f

Bug 2065171 - Fix ids/include_fields precedence: query string wins ov…

89aabbf

…er body

dklawren requested changes Sep 14, 2026

View reviewed changes

Comment thread Bugzilla/API/V1/BugUserLastVisit.pm Outdated

dklawren requested changes Sep 16, 2026

View reviewed changes

Comment thread Bugzilla/API/V1/BugUserLastVisit.pm Outdated

Comment thread Bugzilla/API/V1/BugUserLastVisit.pm

Xzzz mentioned this pull request Sep 17, 2026

Bug 2065173 - Migrate Group REST resource to native Mojo API #2745

Open

Xzzz added 2 commits September 17, 2026 16:02

Xzzz requested a review from dklawren September 17, 2026 16:03

dklawren requested changes Sep 18, 2026

View reviewed changes

Navigation Menu

Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API - #2743

Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API#2743
Xzzz wants to merge 9 commits into
mozilla:masterfrom
Xzzz:bug-2065171

Xzzz commented Sep 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xzzz commented Sep 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xzzz commented Sep 17, 2026

Uh oh!

dklawren Sep 18, 2026

Uh oh!

dklawren Sep 18, 2026

Uh oh!

dklawren Sep 18, 2026

Uh oh!

dklawren Sep 18, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Xzzz commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

References

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xzzz commented Sep 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xzzz commented Sep 17, 2026

Uh oh!

dklawren Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

dklawren Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

dklawren Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

dklawren Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xzzz commented Sep 14, 2026 •

edited

Loading