Bug 2065171 - Migrate BugUserLastVisit REST resource to native Mojo API

[Xzzz]

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.

Rebased onto master now that #2751 has landed, so the branch no longer carries its own copy of merge_request_params and uses the shared one.

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
• Bugzilla/WebService/Util.pm: merge_request_params takes an optional list of parameter names that are lists, and reads the request body once. See the section below
• Add qa/t/rest_bug_user_last_visit.t which this endpoint did not have

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.

Changes to the shared merge_request_params helper

Flagging these explicitly, since Bugzilla/WebService/Util.pm is shared and the changes affect more than this endpoint.

merge_request_params now accepts an optional arrayref of parameter names that are lists, called here as merge_request_params($self, ['ids']). Those parameters always come back as arrayrefs; every other parameter always comes back as a scalar. The argument is optional and behaviour is unchanged without it.

This is needed because the collapse-to-scalar added in #2751 uses $c->req->param, which keeps only the last value of a repeated key, so ?ids=1&ids=2 silently became a single id.
Rather than work around it locally, the helper now makes list-ness explicit, because the previous ->to_hash had the mirror-image flaw: it returned a scalar for one occurrence and an arrayref for two, which is why Group.pm has to run its parameters back through validate() to recoerce them. A parameter's type should not depend on how many times it was sent.

This also fixes the same latent bug in bug 2065173, where GET /rest/group?ids=1&ids=2 currently returns a single group.

Separately, the helper now reads $c->req->body into a variable instead of calling it twice, which matters for file-backed request assets.

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, and with a body but no Content-Type
• POST against a bug in a group the caller is not a member of => 401 bug_access_denied, and the same id filtered out of GET
• POST with a nonexistent bug id => 404, and the visit recorded earlier in the same loop is rolled back
• POST with no ids in the path, query string or body => param_required
• A malformed JSON body => rest_malformed_json
• include_fields / exclude_fields
• OPTIONS on both routes returns Allow: GET, POST, and OPTIONS on a non-numeric id 404s
• Confirm response shape (bare JSON array, last_visit_ts with trailing Z) matches the legacy endpoint

References

• Bugzilla link: 2065171
• Part of 2057358

[Xzzz]

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"

[Xzzz]

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

[Xzzz]

The branch has been rebased onto master now that #2751 landed, so it no longer carries its own copy of merge_request_params. That rebase needed one change to the shared helper, which is described in the thread on Bugzilla/WebService/Util.pm and in a dedicated section of the PR description: the collapse-to-scalar kept only the last value of a repeated key, so ?ids=1&ids=2 silently became a single id. Callers now declare which parameters are lists. It is backward compatible, and it also fixes the same latent bug in #2745.

Two things surfaced while writing the tests. Neither belongs in this PR, and I would rather not repeat the i_am_webservice situation, so flagging them here instead. Happy to file either or both if you agree.

Unmatched /rest/ paths fall through to the legacy CGI rather than 404.

Bugzilla/App/Controller/CGI.pm:47 declares $r->any('/rest/*PATH_INFO')->to('CGI#rest_cgi'). When a route constraint rejects a path, the request is not answered with a 404 by the Mojo router, it falls through to that catch-all and reaches the legacy rest.cgi, which no longer knows a resource once it has been migrated. In practice the connection is closed with no response at all.

I hit this with the [id => qr/\d+/] constraint you asked for on the OPTIONS routes: OPTIONS /rest/bug_user_last_visit/abc used to answer a misleading 200 Allow: GET, POST and now drops the connection. The constraint itself is correct and is in place, but I dropped the test assertion rather than pin broken behaviour. Worth noting that GET and POST on the same path have carried this constraint since earlier in this branch and behave the same way, so this is not something the OPTIONS change introduced.

This affects every native REST controller in the 2057358 series that constrains a placeholder, so it seems worth its own bug.

A nonexistent bug id produces a misleading error.

Bugzilla::Bug->new only sets error => 'NotFound' when it is handed a bare scalar. Given a hashref, as both this controller and the legacy one pass it (Bugzilla::Bug->check({id =>$bug_id, cache => 1})), it sets error => 'InvalidBugId' and bug_id => $param->{name}, which is undef. check() therefore reports improper_bug_id_field_value with no bug id, and the caller gets 400 / code 100 "You must enter a valid bug number!" instead of 404 / code 101 "Bug XXXXXXX does not exist."

Behaviour is identical in the legacy endpoint, so this PR preserves it deliberately and the new test asserts the real behaviour with a comment explaining why. But it is a confusing response for API callers, and it applies to any endpoint that calls check() with a hashref.

[dklawren]

LGTM r=dkl