Repository navigation
Establish the principles of a service-oriented architecture - #862
Conversation
Codifies the ten decisions that define what a service is and how it behaves under a service-oriented architecture: where boundaries come from, who owns a resource's data store, how a boundary is crossed, when a local copy is permitted, and what every service publishes. Recorded as Proposed so the decisions can be discussed and refined before ratification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a single new file, No new findings this pass. The internal cross-references hold at head — rule 10's delete sub-bullet names rule 11, and the Plan's outbox item names rules 10 and 11, both of which exist. No code, dependency, or configuration files are in the diff, and Aikido, Build, Lint, and Cloudflare Pages are all green on Code Review Details
|
Deploying contributing-docs with
|
| Latest commit: |
3387674
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f1bd3776.contributing-docs.pages.dev |
| Branch Preview URL: | https://docs-adr-0035-service-orient.contributing-docs.pages.dev |
Clarified the breaking changes policy and API versioning process for services.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
theMickster
left a comment
theMickster
left a comment
There was a problem hiding this comment.
I think we have a good start and are 80% of the way there to a great ADR.
I have noted some specific concerns with textbook AI writing fingerprints that should be addressed; but let's be sure to re-read the whole document to pull out anything else.
We have a lot of the [X] rather than [Y] writing style that we should review and consider if each one is required. I realize that these statements are important parts of an ADR because we are comparing decision options, but there's like 10+ uses of the writing style and they cannot all be required for the ADR to stand strong.
I also think we can cut many of the overly verbose and run-on statements throughout the ADR without loosing technical merit.
Co-authored-by: Mick Letofsky <mletofsky@bitwarden.com>
…r to service-oriented-architecture
…rchitecture' into docs/adr-0035-service-oriented-architecture
mike-gorman-bitwarden
changed the title
| - Any cache used `MUST` be owned and invalidated by the owning service. | ||
| - Serving results from cache `MUST NOT` bypass authorization the owning service would otherwise | ||
| enforce. | ||
| 8. Services that need to read, write, or validate data owned by another service `SHOULD` do so via |
There was a problem hiding this comment.
issue: This feels like a must to me. I don't want to get in a world where we have the option to directly hit APIs to bypass guards built into a client or scatter implementation details to far-flung API call sites.
There was a problem hiding this comment.
Mike and I actually talked about this one and softened the language so as to not rule this out.
There was a problem hiding this comment.
Is there something you foresee that being beneficial for? I think it's likely to produce unspoken contracts and action at a distance effects
| 10. Services `MUST` publish events for every state change using the "transactional outbox" pattern, | ||
| regardless of whether there are any known consumers. |
There was a problem hiding this comment.
Is there value in limiting this scope initially? I may be missing things, but my read of this architecture is that only deletes are needed to propagate cascades.
Additionally, this rule binds us to broad-scoped state change statements, rather than tightly typed and defined messages. The latter requires knowledge of consumer needs, but the former creates significant challenges in potentially needing to combine data streams.
There was a problem hiding this comment.
The idea here is that if every service broadcasts every state change "as a matter of course", when there is a need to build this integration or that, the team that needs to do so has everything they need. There is never a situation where, in order to make something work, team A has to talk to team B to get them to start doing something. It is a pattern I found to be a very effective way of working.
Having said that, I think that we actually do have a consumer for all of these events - and that is our audit log (a.k.a. Events Service) which, of course, wants a permanent record of everything that's happened.
In addition to that, are the consumers you mentioned - those that want to make sure they delete related data when parent resources are deleted.
So the requirement is for those three reasons:
- To facilitate the audit log.
- To facilitate cascade deletes.
- To facilitate the next thing we haven't thought of so that, when it does come up, it doesn't require the "upstream" team to make changes to their code.
There was a problem hiding this comment.
Event logs are very far from an audit trail of everything that happens in the Bitwarden database. What's more is there's a layer of authorization that needs to occur on them and it's not obvious to me where that belongs.
I also want to point out that
To facilitate the next thing we haven't thought of so that, when it does come up, it doesn't require the "upstream" team to make changes to their code.
and
but the exact details of what is included in the event is up to the service to decide
are statements in conflict. You're trying to preempt what events will be needed without knowledge of what that may be.
My push back here is that this rule will result in generic, all-encompassing change events that are difficult to consume, annoying to maintain, and mostly ignored.
There was a problem hiding this comment.
Probably best discussed in person but I am curious to know what makes you think they would be difficult to consume? Presumably, all events share some common top-level "shape" like category, occurredAt, description, and severity which would go a long way towards making them pretty straight-forward to consume.
As for maintenance, I would expect that most transactions go through some common framework that, in addition to persisting the changes, publishes the corresponding event in which case these events all originate from one place, which shouldn't be too hard to maintain.
And if every change is something that we need to record for auditing, then, none of these events are ignored.
As for authorization, I agree that's something that needs to be worked out.
|
Howdy @quexten & @MGibson1 👋🏼 To clarify, I am not suggesting rushing towards an Approved state of the ADR. I understand it's a meaty one. Thank you both! |
- Rule 10 now requires a "resource deleted" event for a resource other services may depend on, so rule 11's MUST on the consumer is satisfiable. The rest of rule 10 stays SHOULD. - Rules 4 and 5 gate on a service-to-service API rather than an undefined "internal service". Both "internal" and "private" already name the client-facing API, in the server repo and in public-api.md respectively, and the property belongs to the API surface rather than to the service. - The versioning consequence no longer claims rule 5 guarantees a new version never breaks, which it stopped doing when it became SHOULD NOT. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rules 4 and 5 and the Plan entry referred to the same forthcoming page by two different names. It is now the Service-to-Service API Standards everywhere, matching rule 4's gate and avoiding "internal", which already names the client-facing API in the server repo and in public-api.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The qualifier "a resource other services may depend on" required the owner to judge whether a dependent exists, which the Positive consequences bullet says owners do not need to know. A dependent added later would find no event to subscribe to and be in breach of rule 11 again. Rule 10 already publishes regardless of known consumers, so the sub-bullet now follows suit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addressed, or pending acceptance.
6 participants
Establishes the principles services must adhere to in a service-oriented architecture.