Define service-to-service API standards - #868
mike-gorman-bitwarden wants to merge 4 commits into
Conversation
Deploying contributing-docs with
|
| Latest commit: |
ecdbc9a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4ebd3cc2.contributing-docs.pages.dev |
| Branch Preview URL: | https://docs-api-standards.contributing-docs.pages.dev |
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
|
||
| # Service-to-service API standards | ||
|
|
||
| {/* cspell:ignore reate reates eletes elete pdate pdates fieldsets */} |
There was a problem hiding this comment.
⛏️ Same comment from earlier.
audreyality
left a comment
audreyality
left a comment
There was a problem hiding this comment.
📝 I took a look at the broad content in this review, without exploring the REST semantics.
| > **Why versioning in the path?** It is visible in logs, traces, routing rules and curl commands; it | ||
| > needs no content negotiation to read; and it lets two versions coexist behind one host. Header and | ||
| > media-type versioning are both defensible but harder to operate. |
There was a problem hiding this comment.
🎨 Not specific to this standard: we should also log API version independent of the URL so that we can query it without having to parse the URL.
| **Scope.** Service-to-service APIs, whose callers are other Bitwarden services. Bitwarden's existing | ||
| public API is out of scope and is not changing, as is the internet-reachable surface our own clients | ||
| call — the part the server repository refers to as "internal". |
There was a problem hiding this comment.
⛏️ We should also exclude APIs whose express intent is to implement a standard from this scope. These will likely be related to infrastructure, such as OpenTelemetry or health checks.
| return - for both the happy path and the not-so-happy path. Developers `SHOULD` strive to think in | ||
| terms of resources and be on guard against **API proliferation** that can result from over-tailoring |
There was a problem hiding this comment.
❓ What does it mean to "think in terms of resources"? This is one of those thorny questions that REST kind of hand-waves, and so many folks have their own hand-wavy version of what constitutes a resource.
Especially difficult to define are situations where resources have integrity requirements with an impedance mismatch between REST and their persistent representation.
Think of the difference between a JSON-LD link and a database foreign key. JSON-LD links are RESTful and much more expressive as a result. So if you think of references in terms of hypertext, you get a very different set of constraints than someone who thinks of them as database entities.
|
|
||
| ### Operation identifiers | ||
|
|
||
| Every operation `MUST` carry an explicit, stable `operationId`. "v1" APIs `SHOULD NOT` include the |
There was a problem hiding this comment.
🤔 Is the operation identifier the same as a version identifier? If not, what makes each identifier distinct? Is the operation identifier composed of path information?
❓ I think there's a term mismatch here?
| Every operation `MUST` carry an explicit, stable `operationId`. "v1" APIs `SHOULD NOT` include the | |
| Every operation `MUST` carry an explicit, stable `operationId`. "v1" APIs `SHOULD` include the |
If I'm making a new interface, then it should include the version. If it shouldn't, that's an exception that requires justification with the architecture group.
| /api/v1/users/123/addresses | ||
| ``` | ||
|
|
||
| Paths `SHOULD` be "hackable". If `GET /api/v1/users/123/addresses/456` returns the details about |
There was a problem hiding this comment.
⛏️ Minor language tweak
| Paths `SHOULD` be "hackable". If `GET /api/v1/users/123/addresses/456` returns the details about | |
| Paths `SHOULD` be traversable. If `GET /api/v1/users/123/addresses/456` returns the details about |
🤔 We need to take care in these standards that they cannot be used to enumerate sensitive data structures. Take, for example, a construction as follows:
GET /api/v1/vault/123/ciphers/112GET /api/v1/vault/123/ciphers/235GET /api/v1/vault/123/ciphers/5813
I should not get permissions failures when I examine these ciphers without access to vault/123, because that would let me know that 112 is a valid cipher for the vault, but 113 is not. Instead, the entire ciphers sub-resource should not exist (404). While this property is useful, it must be designed with care.
🎨 This is a situation where it makes sense to explore the cross-cutting concern. The pieces are explained, but disconnected, and so the complexity of URL design comes from inference. Since, for many of our APIs, zero-knowledge is a governing non-functional requirement, it's important that this complexity is directly described by the standard.
| Otherwise, if changes need to be made that _would_ be breaking changes, a new version of the API | ||
| `MUST` be created and the old one [deprecated](#deprecation). |
There was a problem hiding this comment.
⛏️ This buries the lede after a large number of exceptions qualifying SHOULD NOT
| > **Adding a value to a constrained field deserves a second look.** It is additive, so it is not a | ||
| > breaking change by the definition above, and a caller that treats the field as an open string is | ||
| > unaffected. But a generated client that deserializes the field into a closed enumeration will fail | ||
| > on a value it has never seen — and it will fail at the client, on a change that looked safe from | ||
| > the service. Consider whether the callers of that field are tolerant of unknown values before | ||
| > adding one. |
There was a problem hiding this comment.
💭 The client interfaces could be designed to allow for this circumstance, and a mechanism to do this baked-in. In particular, the client library should update with the new value 3 releases before the service can emit it. (This number is derived from our self-host compatibility policy. It could be shorter since this is an internal API.)
And this could be augmented with Roslyn lints prohibiting the new value on the server and warning about the new value in the client in the iterim.
| - Field values that are constrained to a fixed set of values `SHOULD` enumerate the valid values in | ||
| all caps (e.g. `RED`, `GREEN`, `BLUE`) both in the OpenAPI spec and in example JSON to help | ||
| distinguish these fields from free-text string fields. At runtime, however, APIs `MUST` ignore | ||
| case when validating these values. |
There was a problem hiding this comment.
⛏️ Unless we also restrict symbols to ASCII, this isn't workable in practice. Unicode has all kinds of interesting casing rules.
🤔 I wonder if it would be better to use a structure to identify these fields instead of transforming their text value. Discriminated unions are a powerful way to ensure data shapes are constrained, and allowing any enum to become a discriminated union has proven very useful to me in API design.
// `kind` signals the constraint
{ "someFixedValue": { "kind": "Value" } }
// this works especially well when the new value has new required fields
{ "someFixedValue": { "kind": "NewValue", "required": "😎" } }| - Fields holding a date or date/time `SHOULD` end in `At` (e.g. `createdAt`, `expiresAt`). | ||
| - Boolean fields `MUST NOT` be prefixed with `is` (e.g. `active`, not `isActive`). | ||
| - Fields whose value is the identifier of another resource `SHOULD NOT` be suffixed with `Id`. A | ||
| string-valued `assignedTo` is self-evidently the identifier of the user it is assigned to; | ||
| `assignedToId` adds nothing. This standard does not apply to fields that reference external | ||
| identifiers. |
There was a problem hiding this comment.
🤔 These conditions seem either arbitrary or contradictory. At is allowed, even though an ISO-formatted date is more self-evident than an identifier.
| - Because our APIs are largely based on JSON:API, services `MUST` accept a request whose | ||
| `Content-Type` is `application/vnd.api+json`, and `MUST` honor an `Accept` of | ||
| `application/vnd.api+json`, even though the API itself only advertises `application/json`. |
There was a problem hiding this comment.
❓ Why don't we advertise application/vnd.api+json if we accept it?
3 participants
📔 Objective
To define API standards for internal APIs (not exposed to the internet).
Note to the reviewer
When faced with a 1,400-line standard, it is easy to feel overwhelmed, or that it imposes too much of a burden on the developer. I would agree if compliance to the standard required writing all of the code yourself. But it is our intention to equip you with a framework and prescriptive programming model to give you as much compliance as we can "for free" with the guarantee that no request ever reaches your code that does not conform to the declared request model. Compliant error handling should be just throwing an exception and, for everything else, classes and helper methods to make it easy.
So, as you're reviewing the standard, try not to think about what code will be needed to conform but whether the standard is a good standard? Does it make sense? Is it necessary? Is it overly restrictive? Should it be
MUSTrather thanSHOULD, orSHOULDrather thanMUST? Or some completely different standard? Our commitment to you is a forthcoming framework and programming model that will make it easy to write compliant APIs.