Establish standards for internal APIs - #863
mike-gorman-bitwarden wants to merge 8 commits into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed at Structural checks are clean: no The three findings below are gaps rather than contradictions, and none blocks the merge. The Code Review Details
|
Deploying contributing-docs with
|
| Latest commit: |
580c68d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c9b47bf0.contributing-docs.pages.dev |
| Branch Preview URL: | https://docs-adr-0035-internal-api-s.contributing-docs.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ```http | ||
| Deprecation: @1688169599 | ||
| Sunset: Sun, 30 Jun 2024 23:59:59 UTC |
There was a problem hiding this comment.
Sunset example ends in UTC, but an HTTP-date must end in GMT.
Details and fix
RFC 8594 defines Sunset as an HTTP-date per RFC 7231 §7.1.1.1. All three accepted forms (IMF-fixdate, RFC 850, and asctime-with-zone) terminate in the literal token GMT; UTC is not valid in any of them. Strict parsers reject it — Go's http.ParseTime, .NET's "r" round-trip format, and Python's email.utils.parsedate_to_datetime all fail on UTC.
The table immediately below this example already describes the value correctly as "an HTTP date", so the example is the only thing out of step — and it is the part that gets copied.
| Sunset: Sun, 30 Jun 2024 23:59:59 UTC | |
| Sunset: Sun, 30 Jun 2024 23:59:59 GMT |
Reference: RFC 8594 §3
There was a problem hiding this comment.
Fixed in 4d434aa — changed to GMT.
Worth recording how this got in: the example was copied verbatim from RFC 9745 §5, which itself shows Sunset: Sun, 30 Jun 2024 23:59:59 UTC. But Sunset is defined by RFC 8594, which specifies Sunset = HTTP-date and uses GMT in both of its own examples. So RFC 9745's example contradicts the RFC it references, and this finding is correct against the defining spec.
| { | ||
| "filter": { | ||
| "and": [ | ||
| { "==": [{ "var": "status" }, "invited"] }, |
There was a problem hiding this comment.
🎨 SUGGESTED: This example uses lowercase "invited" where the standard's own rule calls for all caps.
Details and fix
The JSON section states that constrained values "should enumerate the valid values in all caps (e.g. RED, GREEN, BLUE) both in the OpenAPI spec and in example JSON". The sibling examples follow it — Advanced queries uses "ACTIVE" / "INVITED" and Bulk updates uses "PENDING" / "DISABLED". Only this one deviates.
Runtime behaviour is unaffected (the same section requires case-insensitive validation), but this is example JSON in a document whose stated audience includes AI agents, so it is the form most likely to be copied into a real enum definition.
| { "==": [{ "var": "status" }, "invited"] }, | |
| { "==": [{ "var": "status" }, "INVITED"] }, |
There was a problem hiding this comment.
Fixed in 4d434aa — changed to "INVITED".
Also checked the rest of the page for the same slip; this was the only one.
|
All four findings from the review are addressed in 4d434aa:
The last two had no inline thread, so noting them here. |
| A request that uses any other operator, or references a field that does not exist, `MUST` be | ||
| rejected with `400`. |
There was a problem hiding this comment.
MUST return 400, but the page's own "400 vs. 422" rule sends this case to 422.
Details and fix
Two normative statements conflict:
- 400 vs. 422 (line 205): "Return
400for requests that are malformed and cannot even be parsed (e.g. invalid JSON)." Line 206: "Return422for invalid requests." - Request validation (line 868) lists "Field names specified by
sortorfieldsparameters are valid field names" as part of exhaustive validation — which routes to422.
A JSON Logic body using an unsupported operator, or naming a field that does not exist, parses fine as JSON. It is a semantic validation failure, and an unknown field name here is the same class of error as an unknown name in sort or fields, which the page already assigns to 422. Two services reading this will implement different status codes for the same request, which is the divergence the standard exists to prevent.
| A request that uses any other operator, or references a field that does not exist, `MUST` be | |
| rejected with `400`. | |
| A request that uses any other operator, or references a field that does not exist, `MUST` be | |
| rejected with `422`. |
If 400 is deliberate here — on the grounds that the filter expression is the request rather than a field within it — then it belongs in the "400 vs. 422" section as a named exception, the way the payload-reference case is called out in 400 vs 404.
There was a problem hiding this comment.
Addressed in 43cc6df, and the underlying inconsistency you surfaced was resolved rather than patched.
- Advanced queries now rejects an unsupported operator or unknown field with
422. - The
400 vs 404section is now404 vs. 422— a payload reference that does not resolve is422, not400. - The Errors example moved to
"status": "422", since it illustrates exactly that case. - The four disambiguation subsections were reordered into numeric sequence after the rename.
So payload-content errors are now uniformly 422 across the page, and 400 is reserved for requests that cannot be parsed.
| Errors `MUST` be returned as an array of error objects within a top-level `errors` field where each | ||
| error populates the following fields: |
There was a problem hiding this comment.
🎨 SUGGESTED: "each error populates the following fields" reads as all nine being required, which the example below contradicts and the source members make impossible.
Details and fix
Two things follow from the literal reading:
source.header,source.parameter, andsource.pointerare alternatives — a single error is attributable to a header, or a query parameter, or a body member. They cannot all be populated.- The example on lines 898-912 omits
id, yetidis the one field the page later depends on: the500guidance below says "Theidfield is how a caller and an operator correlate a report with the logged detail." Ifidis genuinely required, the canonical example is the thing engineers copy and it is missing it.
Suggest splitting the table into the members every error carries (code, detail, id, status, title) and the source members, of which at most one applies — and adding id to the example. This is the most-copied section of any API standard, and the page names AI agents as part of its audience, so a literal reading is likely to be implemented literally.
There was a problem hiding this comment.
Addressed in 7a3c077 — softened the lead-in so it no longer reads as all nine fields being required, and called out that at most one source member applies:
Each error populates the following fields, except that at most one
sourcemember applies to any one error.
Also added id to the example, since the 500 guidance below it depends on id for correlation and the example was the one place it was missing.
| --- | ||
| sidebar_position: 0 | ||
| --- | ||
|
|
||
| # Server Architecture |
There was a problem hiding this comment.
🎨 SUGGESTED: The published /architecture/server/* URLs can be preserved without adding a Docusaurus plugin — Cloudflare Pages reads a _redirects file.
Details and fix
The PR description notes the 404s as a known consequence, with "no redirect plugin is configured in this repo" as the reason. That is true of @docusaurus/plugin-client-redirects, but this site deploys to Cloudflare Pages, which honours a _redirects file at the root of the build output. Docusaurus copies everything in static/ to that root, so a new static/_redirects containing one line needs no dependency and no config change:
/architecture/server/* /architecture/service-oriented-architecture/:splat 301
That covers the section index, command-query-separation, and model-separation-of-concerns in one rule. Worth it because /architecture/server/command-query-separation is the kind of URL that gets pasted into PR reviews and onboarding notes outside this repo, where the in-repo link updates do not reach.
Reference: Cloudflare Pages redirects
There was a problem hiding this comment.
Good catch — done in 7a3c077. Added static/_redirects with the single splat rule you suggested.
Verified the mechanism before adopting it: Cloudflare's docs confirm _redirects belongs in the static asset directory (static/ for Docusaurus), that :splat and 301 are supported, and that the 2,000-rule cap is not a concern here. The PR description has been left to be updated, since "no redirect plugin is configured" is no longer the operative reason.
mike-gorman-bitwarden
changed the title
| The only exceptions are [advanced queries](#advanced-queries), [bulk updates](#bulk-updates), and | ||
| [bulk deletes](#bulk-deletes), which use `POST` because the request carries a body. |
There was a problem hiding this comment.
🎨 SUGGESTED: The "only exceptions" list omits actions, which also MUST use POST without creating a resource.
Details and fix
The table above declares that POST MUST mean "Create a resource", and this sentence claims to enumerate every exception to it. Acting upon resources is a fourth: it states "HTTP verb MUST be POST", and POST /api/v1/users/{id}/actions/send-email creates nothing. It also carries a body, so the stated rationale applies to it too.
As written, two MUSTs conflict for the one shape most likely to send a reader to this section — the non-CRUD operation the General section introduces as the "5th type of API".
| The only exceptions are [advanced queries](#advanced-queries), [bulk updates](#bulk-updates), and | |
| [bulk deletes](#bulk-deletes), which use `POST` because the request carries a body. | |
| The only exceptions are [actions](#acting-upon-resources), [advanced queries](#advanced-queries), | |
| [bulk updates](#bulk-updates), and [bulk deletes](#bulk-deletes), which use `POST` for operations | |
| that are not creates. |
There was a problem hiding this comment.
Fixed in 7567420 — actions added to the list, and the reason reworded from "the request carries a body" to "without creating a resource", which is the property all four actually share.
| - Because our APIs are largely based on JSON:API, services `MUST` accept `application/vnd.api+json` | ||
| if provided by the client, even though the API itself only advertises `application/json`. |
There was a problem hiding this comment.
🎨 SUGGESTED: "accept ... if provided by the client" does not say whether it means Content-Type or Accept, and the two readings produce different status codes.
Details and fix
This bullet sits between two that each govern one header: the one above governs Accept and mandates 406, the one below governs Content-Type and mandates 415. This one says only "if provided by the client", so both readings are available:
- Read as
Content-Type: a client sendingAccept: application/vnd.api+json— the default for an off-the-shelf JSON:API client — falls under the406rule, because the service only returnsapplication/json. - Read as
Accept: a client sendingContent-Type: application/vnd.api+jsonfalls under the415rule.
Either way one direction is rejected by a MUST, and two teams implementing from this page can land on different codes for the same request. ADR-0035 writes it as "accepts and returns the application/vnd.api+json media type" in its JSON:API-strictly con, which reads as though both directions were intended.
| - Because our APIs are largely based on JSON:API, services `MUST` accept `application/vnd.api+json` | |
| if provided by the client, even though the API itself only advertises `application/json`. | |
| - Because our APIs are largely based on JSON:API, services `MUST` accept `application/vnd.api+json` | |
| in either the `Content-Type` or the `Accept` header, even though the API itself only advertises | |
| `application/json`. An `Accept` of `application/vnd.api+json` `MUST NOT` be rejected with `406`. |
There was a problem hiding this comment.
Fixed in 7567420 — the rule now names both directions explicitly: services MUST accept a request whose Content-Type is application/vnd.api+json, and MUST honor an Accept of application/vnd.api+json.
| APIs that support specifying multiple values for a query parameter `MUST` do so via a | ||
| comma-delimited list. This allows APIs that started with single-value query parameters to evolve to | ||
| supporting multiple-values without changing the external-facing contract. When multiple values are | ||
| provided, "or" query semantics `MUST` be applied. |
There was a problem hiding this comment.
🎨 SUGGESTED: Comma is the multi-value delimiter but no escape is defined, so a value containing a comma cannot be filtered on.
Details and fix
filter[name]=Acme, Inc. parses as two values, Acme and Inc., and the standard gives no way to say otherwise. The same gap applies to a literal * under Filtering for an API that supports wildcards. Person names, organization names, and address fields routinely contain commas, so this is reachable on the first real filterable string attribute.
It also cuts against the rationale in this paragraph. Evolving a single-value parameter to multi-value does change the external-facing contract for exactly those callers: a request that previously matched the one literal value Acme, Inc. afterwards matches two.
Two ways to close it, either of which is a sentence:
- Define an escape for a literal delimiter, and say it applies to
*as well. - State that exact matches on values containing the delimiter go through advanced queries, where the value is a JSON string and needs no delimiter at all.
Option 2 costs nothing to specify and the mechanism already exists on the page — but it is worth saying out loud, since a reader hitting this will otherwise invent their own encoding.
There was a problem hiding this comment.
Not taking this one. Defining an escape syntax for commas in filter values is more specification than the case warrants — and callers who need a value containing a comma have advanced queries, where the expression is JSON and the delimiter problem does not arise.
| To keep from having to _also_ revert all of the consumers, APIs `MUST` ignore unrecognized fields, | ||
| parameters, and headers. |
There was a problem hiding this comment.
filter[...] parameter falls under this MUST ignore rule, so a filter silently widens the result set instead of erroring.
Details and fix
The same semantic error gets opposite treatment depending on which mechanism the caller used:
- Advanced queries (line 750): a JSON Logic expression that "references a field that does not exist"
MUSTbe rejected with422. - This rule (line 147): an unrecognized query parameter
MUSTbe ignored — andfilter[{name}]is a query parameter, withnameonlySHOULD-ing to match a resource attribute (Filtering, line 546).
Ignoring an unrecognized filter is not a no-op the way ignoring an unrecognized field is. GET /api/v1/users?filter[organizatonId]=X (typo, or a filter the service has not shipped yet) returns every user rather than an error, and the caller cannot tell the difference between "no filter was applied" and "everything matched". A read-many result that is broader than the caller asked for is the shape that leads to acting on records that were never meant to be selected.
| To keep from having to _also_ revert all of the consumers, APIs `MUST` ignore unrecognized fields, | |
| parameters, and headers. | |
| To keep from having to _also_ revert all of the consumers, APIs `MUST` ignore unrecognized fields, | |
| parameters, and headers. The one exception is an unrecognized `filter[{name}]` parameter, which | |
| `MUST` be rejected with `422`: ignoring a filter silently widens the result set rather than | |
| narrowing it. |
The trade-off is worth naming: rejecting cuts against this section's rollback rationale, since a client passing a filter a rolled-back service no longer knows would now fail. If that is the preferred outcome, the alternative is equally short — say explicitly that filters are best-effort and that callers MUST NOT rely on an unrecognized filter having been applied. Either way it wants to be stated, because as written the two sections disagree and implementers will split.
There was a problem hiding this comment.
Agreed and taken. The asymmetry you identified is the decisive part: ignoring an unrecognized field means the service does less than the caller asked, which is fail-safe; ignoring an unrecognized filter means it returns more, which is not.
Added to the Unrecognized fields section:
The one exception is an unrecognized
filter[{name}]parameter, whichMUSTbe rejected with422. Ignoring an unrecognized field means doing less than the caller asked; ignoring an unrecognized filter means returning more.
On the trade-off you named — yes, this does cut against the rollback rationale, and a client passing a filter a rolled-back service no longer knows will now fail rather than degrade. We judged the failure mode of the alternative worse: a rolled-back service silently returning every record is a worse outcome than a caller getting a 422. It also makes the treatment consistent with Advanced queries, where an unknown field name is already a 422.
Commit to follow shortly.
There was a problem hiding this comment.
Landed in 580c68d, and generalized past the original finding. Rather than carving out filter alone, the exception now covers every parameter that shapes the result:
The exception is a parameter that shapes the result. Ignoring an unrecognized field means doing less than the caller asked; ignoring one of these means returning something other than what was asked for. APIs
MUSTreject the request with422when given:
- a
filter[{name}],sort, orfieldsparameter naming a field they do not support, or- paging parameters when the API does not support paging, or for a paging style it does not implement.
sort and fields have the same fail-open shape you described for filter — an ignored fields returns every field, an ignored sort returns arbitrary order — and mismatched paging parameters were a third case in the same family that the standard had not addressed at all.
| - Fields whose value is set by the server `MUST` be marked as `readOnly`. If a request includes a | ||
| read-only field, the API `MUST` ignore it. |
There was a problem hiding this comment.
🎨 SUGGESTED: This MUST makes version readOnly, which Optimistic concurrency explicitly forbids.
Details and fix
version is set by the server — "Upon successfully updating the resource, the API MUST update the version identifier" (line 471) — so this rule requires it to be marked readOnly, and the second sentence then requires the API to ignore it on the way in. Line 466 requires the opposite: "a version field marked required and not readOnly".
The consequence is not just editorial. The Request validation section anticipates the SDK stripping non-conforming input before it reaches developer code; a framework applying this rule generically would strip version from requests, and the 409 on line 470 would then never fire. A lost update that silently succeeds is the failure mode optimistic concurrency exists to prevent.
version is the only field with this property — createdAt, updatedAt, createdBy, and updatedBy are all server-set and genuinely read-only — so a named carve-out closes it:
| - Fields whose value is set by the server `MUST` be marked as `readOnly`. If a request includes a | |
| read-only field, the API `MUST` ignore it. | |
| - Fields whose value is set by the server `MUST` be marked as `readOnly`, the one exception being | |
| `version` — see [Optimistic concurrency](#optimistic-concurrency). If a request includes a | |
| read-only field, the API `MUST` ignore it. |
There was a problem hiding this comment.
Not taking this one — it is a deliberate decision rather than an oversight.
The contradiction is real and was raised in review before this PR was opened. The chosen resolution was to keep the general rule simple and handle the exception where version is actually discussed, which is why Optimistic concurrency reads "marked required and not readOnly". Specific rules superseding general ones is a normal reading convention, and the alternative means carving a named exception into a general rule for the single field it affects.
The framework argument was also on the table at the time — a generator or SDK applying the readOnly rule generically could strip version and silently disable the 409. That is a real risk, and the answer is that the implementation follows the specific rule, not that the general rule grows a clause.
| - a `filter[{name}]`, `sort`, or `fields` parameter naming a field they do not support, or | ||
| - paging parameters when the API does not support paging, or for a paging style it does not | ||
| implement. |
There was a problem hiding this comment.
permanent=true is the one fail-open parameter this list misses — an ignored hard-delete request silently leaves the data recoverable.
Details and fix
The new exception covers filter, sort, fields, and paging. It does not cover the two parameters in Soft deletes, and permanent fails in the same direction the exception exists to prevent — worse, in fact, because the divergence is in the effect rather than the result set.
Soft deletes is MAY/SHOULD guidance, so a service that soft-deletes but never implemented hard deletes treats permanent as unrecognized. Then:
- Caller sends
DELETE /api/v1/users/123?permanent=true. - Line 147 applies — unrecognized parameter,
MUSTignore. - Service soft-deletes and returns
204, per line 494. - The caller has an explicit success for an erasure that did not happen, and no way to tell.
For a delete requested as permanent — the shape a retention or subject-erasure request takes — "returning something other than what was asked for" understates it: the record is still there. includeDeleted=true has the same gap but fails the safe way (fewer records than asked), so it is worth naming in the same bullet rather than a separate rule.
| - a `filter[{name}]`, `sort`, or `fields` parameter naming a field they do not support, or | |
| - paging parameters when the API does not support paging, or for a paging style it does not | |
| implement. | |
| - a `filter[{name}]`, `sort`, or `fields` parameter naming a field they do not support, or | |
| - paging parameters when the API does not support paging, or for a paging style it does not | |
| implement, or | |
| - a `permanent` or `includeDeleted` parameter when the API does not implement it. Silently | |
| soft-deleting a resource the caller asked to delete permanently reports an erasure that did not | |
| happen. |
| APIs `SHOULD` support paging and those that do `MUST` support either **offset/limit** style paging | ||
| or **cursor** style paging. |
There was a problem hiding this comment.
🎨 SUGGESTED: page[size] has no upper bound, so page[size]=1000000 is a conforming request the service must honor.
Details and fix
Neither paging style documents a maximum or a default for page[size], and the page opens with "Where our standards are silent, JSON:API standards are assumed" — JSON:API is silent here too. So a conforming service has no basis to refuse an arbitrarily large page, and a caller has no basis to expect one to be refused.
Two things follow that the standard otherwise prevents:
- Services diverge. One clamps to 200, one returns the lot, one
422s. A caller that works against the first two breaks against the third. - A single unauthenticated-at-the-edge-but-service-to-service call can pull an entire table into memory, on the read-many endpoint that every resource has.
The second bullet matters more now that line 152 makes mismatched paging parameters a 422 — the page has taken a position on paging parameters it cannot honor, but not on one it can honor and should not.
One sentence in this section closes it, e.g.:
APIs that support paging
MUSTdocument a maximumpage[size]and a default applied when the caller omits it. A request exceeding the maximumMUSTbe clamped to it rather than rejected, and the applied sizeMUSTbe reported inmeta.pageSize.
Clamping rather than rejecting keeps it consistent with meta.pageSize already being described as "the current page size" rather than the requested one.
| @@ -0,0 +1 @@ | |||
| /architecture/server/* /architecture/service-oriented-architecture/:splat 301 | |||
There was a problem hiding this comment.
🎨 SUGGESTED: The splat does not match the section index itself, so /architecture/server still 404s.
Details and fix
/architecture/server/* requires the literal prefix /architecture/server/ — trailing slash included — before the splat matches. A request for /architecture/server is one character short of that prefix and falls through to a 404, because the asset that used to answer it (docs/architecture/server/index.md) is deleted in this PR.
That bare path is the published URL, not a variant: docusaurus.config.js sets no trailingSlash, so Docusaurus emits /architecture/server in links and the sitemap. It is also the URL for the section landing page — the one most likely to have been bookmarked or linked from outside this repo, which is the case the redirect was added for.
The child pages are fine; /architecture/server/command-query-separation matches. Only the index is affected, and one more line covers it:
| /architecture/server/* /architecture/service-oriented-architecture/:splat 301 | |
| /architecture/server /architecture/service-oriented-architecture/ 301 | |
| /architecture/server/* /architecture/service-oriented-architecture/:splat 301 |
More specific rule first, since Cloudflare applies the first match.
Reference: Cloudflare Pages redirects
|
Superseded by #865, which carries the same ADR and standard as ADR-0036, without the directory rename. |
1 participant
Adds ADR 0035, recording the decision to base internal service-to-service APIs on JSON:API,
adopted selectively, with formally versioned APIs — and publishes the standard itself under
Architecture › Service-Oriented Architecture.
The ADR and the standard land together, so the ADR ships as
Acceptedand the standard is adoptedon merge — the same pattern PR #842 used for ADR-0034 and the documentation standard.
The standard covers the full request/response contract: API paths and versioning, breaking changes
and deprecation, JSON and naming conventions, standard responses, the CRUD operations, filtering,
sorting, paging, sparse fieldsets, advanced queries, bulk operations, optimistic concurrency,
request validation, and errors. It closes with a FAQ anticipating the questions we expect.
This also renames
docs/architecture/server/todocs/architecture/service-oriented-architecture/and relabels the section from "Server Architecture". Command Query Separation and Model separation
of concerns move with it, and the inbound link in ADR-0008 is updated.
Two areas are deliberately deferred and marked as such in the standard: a standard for
authentication, authorization, and request context, and a standard for jobs (the resource a
202 Acceptedreturns). Both are listed in the ADR's Plan.The previously published URLs under
/architecture/server/are preserved. All in-repo referenceswere updated, and
static/_redirectscarries a single rule that301s the old paths to the newsection — Cloudflare Pages reads that file from the build root, so this needs no plugin or config
change.