Bug 1573509 - Document integration best practices

[loganrosen]

Bug 1573509 tracks moving BMO integration guidance out of MozillaWiki and into the maintained API documentation. The remaining wiki material mixed useful operational guidance with obsolete BzAPI, authentication, and search details.

This adds a version-independent Integration Best Practices page grounded in current BMO behavior. It directs new integrations to native REST, documents the legacy /bzapi/ compatibility prefix, and covers bot accounts, API-key headers, incremental polling, response filtering, batching, pagination, BMO’s request-URL limit, and configuration-safe status and resolution searches. The page links to maintained REST documentation rather than the stale wiki pages.

https://bugzilla.mozilla.org/show_bug.cgi?id=1573509

[Copilot]

Pull request overview

Adds maintained, current guidance for BMO API integrations.

Changes:

• Documents supported APIs, authentication, polling, batching, and durable searches.
• Adds reusable REST documentation anchors.
• Updates BMO’s request-URL limit guidance.

Show a summary per file

File	Description
docs/en/rst/api/integration.rst	Adds integration best practices.
docs/en/rst/api/index.rst	Includes the new guide.
docs/en/rst/api/core/v1/general.rst	Adds anchors and URL-limit guidance.
docs/en/rst/api/core/v1/bugzilla.rst	Adds the REST time anchor.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Medium

[dklawren]

Nice addition — the guidance is the right shape. A few places where following the recipe literally would lose data for an integration, plus some smaller doc issues.

1. The polling loop can silently skip bugs: searches are capped at 10,000 results

Bug.search clamps limit to max_search_results (default 10000) — Bugzilla/WebService/Bug.pm:599-604. Passing limit=0 doesn't escape it: Bug.search never sets allow_unlimited, so Bugzilla::Search::_sql_limit re-applies the cap (Bugzilla/Search.pm:1308-1316). The response carries no truncation flag and no total count.

Against production, GET /rest/bug?last_change_time=&include_fields=id,last_change_time returns exactly 10000 bugs, ordered by bug_id ascending; the same query with offset=9990 shows there are more.

So an integration that has been down for a while — or that hits a mass change — follows the bullet "process every result before saving the new db_time", processes 10000 bugs, advances its watermark, and the modifications to the truncated tail are never returned by any later cycle. Permanent, silent loss.

Suggest the cycle explicitly page until a short page is returned, and sort by change time rather than relying on the default order:

• page through results with limit and offset until a page returns
  fewer than limit bugs — a search returns at most 10,000 bugs and gives
  no indication that results were truncated;

2. limit/offset paging needs an explicit order

Bugzilla::Search emits no ORDER BY at all when no order is given (Bugzilla/Search.pm:947-950). Without one, LIMIT/OFFSET page boundaries are not stable across requests, so bugs can be seen twice or not at all — which also makes the paging fix above unreliable on its own.

Line 71 should say to pass a stable order (e.g. order=bug_id, or order=changeddate%2Cbug_id for the polling case) whenever paging.

3. GET /rest/bug?id=123,456 silently drops IDs the caller can't see

The multi-ID form is dispatched to Bug.search, not to the per-bug route. Missing or access-restricted IDs are omitted from bugs with HTTP 200, no faults key, and no indication which IDs went missing — unlike GET /rest/bug/, which returns an explicit error. An integration that batches per this example will treat security-restricted bugs as deleted in its local mirror.

Worth a sentence: compare the returned IDs against the requested set and treat the difference as "not visible", not "gone".

4. De-duplicating on (bug id, last_change_time) drops same-second changes

The section correctly notes the one-second delta_ts precision as the reason for the overlap, but then prescribes a de-dup key that can't distinguish two changes landing in that same second. If a bug is modified twice within one clock second and the first change is processed in cycle N, the second is discarded by the de-dup rule in cycle N+1 and never processed.

De-dup should be scoped to the current overlap window (drop the key set at the start of each cycle) rather than kept as a permanent "already processed" set — or the doc should state that same-second re-changes require re-fetching the bug rather than trusting the timestamp.

5. Overlap window has no value, and none is derivable

Lines 44-46 say BMO doesn't guarantee a maximum replication lag and to ask the BMO team; lines 51-53 then require an overlap "that covers database replication lag". The only number in the paragraph is "one-second", so an implementer will size the overlap at ~1-2 seconds and silently skip every bug modified inside the real lag window — exactly the failure the section exists to prevent. Please give a concrete conservative default (e.g. "use at least a five-minute overlap unless you have confirmed a tighter bound with the BMO team").

6. general.rst: the 414 doesn't follow the JSON error contract, and the sizing figure was dropped

The new paragraph sits four lines under the Errors section that documents {"error": true, "message": ..., "code": ...}, but the 414 is produced by the Varnish front end: content-type: text/plain, body Error: URI Too Long. A client that follows the surrounding section and calls response.json() on the error body throws a decode exception instead of handling a recoverable "split the request" condition. Worth one sentence saying this response comes from the front end and is not JSON.

Separately, the rewrite removes "roughly 1,000 bug IDs in the id parameter", which is the only actionable sizing hint — and integration.rst:71-73 now links here specifically for batch sizing. Please keep the ID-count figure.

7. f custom-search advice can clobber an existing chart

Bug.search computes the next chart index from the count of f params, not the maximum: my $last_field_id = @field_ids ? max @field_ids + 1 : 1; (Bugzilla/WebService/Bug.pm:621-622) parses as max(@field_ids + 1), i.e. count + 1. So a caller using non-contiguous charts — f1/o1/v1 plus f3/o3/v3 — gets $last_field_id = 3, and the last_change_time filter overwrites f3/o3/v3. Since this page recommends combining f1/o1/v1 with last_change_time, it should tell readers to number charts contiguously from 1.

Smaller items

• Duplication: include_fields and batching are covered in "Poll responsibly" (57-60) and again in "Minimize requests and responses" (65-70), twenty lines apart. Fold one into the other or cross-reference.
• Webhooks: line 37 sends readers to Matrix "to discuss webhooks", but BMO already ships the Webhooks extension with a user-ped API (docs/en/rst/extensions/Webhooks/api/v1/webhooks.rst), already in this toctree. Linking it lets integrators self-serve.
• Overlap with integrating/apis.rst: that page (_api-list) also answers "which API should I use" and describes CSV/XML exports favourably, while this page says not to rely on them. Neither page links to the other, so they will drift. A cross-link (or moving the "which interface" advice to one page) would help.

[loganrosen]

@dklawren Thanks for the detailed review. I pushed updates addressing all of the requested changes:

• The polling recipe now uses immutable bug_id ordering for safe offset pagination, pages with an explicit limit of at most 10,000 until a short page, de-duplicates only within one cycle, and recommends at least a five-minute overlap unless BMO confirms another bound.
• Batched lookups now treat omitted IDs as not visible rather than deleted; paged searches require stable ordering; and custom-search fields must use contiguous chart numbers.
• The REST overview now documents the front-end plain-text 414 response and the approximate 1,000-ID URL sizing guidance.
• The guide now links Webhooks and the general API overview, and consolidates field-selection and batching guidance.

[Copilot]

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Medium

[dklawren]

Minor issues to address before merge:

• Silent limit cap breaks the stated termination test. Bug.pm:599-604 silently clamps limit to max_search_results. Doc says "page … until a page contains fewer than limit bugs" and separately advises ≤10,000 — but a reader who passes limit=20000 gets 10,000 back, reads 10000 < 20000, and stops early with silent data loss. Say the cap is silent and that the page-size used in the comparison must be the server-effective limit. Same paragraph should note limit=0 removes limit/offset entirely (Bug.pm:605-608) and is not a paging strategy.

• id=123,456 truncation. Same cap applies: >10,000 IDs in one call silently truncates. The "compare returned IDs with requested set" advice would then misreport truncated bugs as "not visible". Worth one clause.

• /rest/bug/ behaves differently. Bug.get throws on missing/invisible bugs unless permissive=1 (Bug.pm:468-483). Doc only describes the search form; a sentence contrasting the two avoids the common surprise.

• BMO-specific text in an upstream-shared file. docs/en/rst/api/core/v1/general.rst is core API doc; "BMO's front end … around 8 KB" makes it fork-specific and will conflict on upstream sync. Suggest keeping general.rst generic (server-configuration-dependent, may return 414) and putting the 8 KB number on the new BMO page, with the anchor staying where it is.

• No rate-limit / backoff guidance. BMO enforces per-IP rate limits on anonymous reads (Bug.pm:333,454,1335). A "Poll responsibly" section should say: authenticate to avoid anonymous limits, honor 429, back off on 5xx.

• Unsourced numbers. "no more than once every five minutes" and the five-minute overlap are policy, not code-derived. Cite the source (bug 1573509 / BMO team decision) so they don't drift silently. The two "confirm with the BMO team" hedges also read as unresolved — consider resolving one before landing.