Amend ADR 8: Clarify CQS Terminology

custom-words.txt

@@ -12,6 +12,7 @@ clickjacking
 codebases
 CODEOWNERS
 CQRS
+CQS
 CTAP2
 deinitializer
 deinitializers

docs/architecture/adr/0008-server-CQRS-pattern.md

@@ -1,14 +1,27 @@
 ---
 adr: "0008"
-status: Accepted
+status: Superseded
 date: 2022-07-15
 tags: [server]
+superseded_by: "0028"
 ---
 
 # 0008 - Server: Adopt CQRS
 
 <AdrTable frontMatter={frontMatter}></AdrTable>
 
+:::note Superseded by ADR-0028
+
+This ADR originally established "CQRS" as the terminology for our pattern. However,
+[ADR-0028](./0028-server-adopt-cqs-terminology.md) clarifies that our implementation actually
+follows the simpler CQS (Command Query Separation) pattern rather than full CQRS. While the
+architectural decision to use commands and queries remains valid and in effect, the terminology has
+been updated to more accurately reflect our implementation.
+
+The guidance in this ADR is still relevant, but it should be read in conjunction with ADR-0028.
+
+:::
+
 ## Context and problem statement
 
 In Bitwarden Server, we currently use an `<<Entity>>Service` pattern to act on our entities. These

docs/architecture/adr/0028-server-adopt-cqs-terminology.md

@@ -0,0 +1,90 @@
+---
+adr: "0028"
+status: "Proposed"
+date: 2025-11-29
+tags: [server]
+---
+
+# 0028 - Server: Clarify CQS vs CQRS Terminology
+
+<AdrTable frontMatter={frontMatter}></AdrTable>
+
+## Context and problem statement
+
+In [ADR-0008](./0008-server-CQRS-pattern.md), we adopted what we called "CQRS" (Command Query
+Responsibility Segregation) for the server architecture. However, upon reflection and after several
+years of implementation, it has become clear that our actual implementation does not match the full
+scope and complexity of CQRS as defined in industry literature.
+
+**CQRS** is a comprehensive architectural pattern that separates the read and write models at the
+data storage level. A full CQRS implementation typically includes:
+
+- Separate data models for reads and writes
+- Event sourcing
+- Eventual consistency between read and write stores
+- Complex synchronization mechanisms
+- Often, separate databases or data stores for queries vs commands
+
+**CQS** (Command Query Separation) is a simpler, more focused principle that states:
+
+- Commands change state but don't return data (or only return operation results)
+- Queries return data but don't change state
+- Each operation should have a single responsibility
+
+Our implementation follows CQS principles: we break up large service classes into smaller command
+and query classes, but we do not maintain separate data models or storage layers. This is exactly
+what CQS aims to achieve - smaller, more focused classes that are easier to test and maintain.
+
+The terminology mismatch has caused confusion for developers joining the project, as they research
+CQRS and find it describes a much more complex architecture than what we've actually implemented.
+This creates an unnecessary learning curve and misaligned expectations.
+
+## Considered options
+
+- **Keep CQRS terminology** - Continue using CQRS terminology despite the mismatch. This is
+  difficult to justify.
+
+- **Adopt CQS terminology** - Update our documentation to use CQS terminology, which accurately
+  describes what we've implemented. This provides clarity for new developers and aligns our
+  documentation with our actual implementation.
+
+- **Implement full CQRS** - Actually implement the full CQRS pattern with separate read/write
+  models. This would be a large architectural change that has not been proposed and which is out of
+  scope in any case. This ADR is focused on aligning our documentation with our current practices.
+
+## Decision outcome
+
+Chosen option: **Adopt CQS terminology**.
+
+The terminology change better reflects our implementation and reduces confusion. It also
+acknowledges that we built what we needed: a pragmatic solution to break up large service classes
+without the additional complexity of full CQRS.
+
+### Positive consequences
+
+- **Clarity for new developers** - Developers can research CQS and find documentation that matches
+  our implementation
+- **Accurate documentation** - Our architecture documentation reflects what we actually built
+- **Reduced complexity** - We're not implying architectural complexity we haven't implemented
+- **Better expectations** - Team members understand the scope and scale of what we're maintaining
+
+### Negative consequences
+
+- **Historical confusion** - Older discussions, PRs, and code comments may still reference "CQRS"
+- **Name change overhead** - Some mental adjustment needed for developers familiar with the old
+  terminology
+
+### Migration plan
+
+1. Update this documentation to use CQS terminology, particularly the
+   [Server Architecture](../server/index.md) page.
+1. Update ADR-0008 status to "Superseded" with a reference to this ADR. Add a note referencing this
+   ADR.
+1. No code changes required - class names like `CreateCipherCommand` remain appropriate as "command"
+   and "query" are common to both patterns
+
+## Further reading
+
+- [Martin Fowler on CQS](https://martinfowler.com/bliki/CommandQuerySeparation.html) - a succinct
+  high level summary of our approach
+- [ADR-0008: Server CQRS Pattern](./0008-server-CQRS-pattern.md) (superseded by this ADR)

docs/architecture/adr/index.mdx

@@ -112,7 +112,7 @@ work.
 **Example scenarios:**
 
 - A technology choice has been agreed upon (e.g., "Use Jest for testing")
-- A pattern has been established as the standard approach (e.g., "Use CQRS for server architecture")
+- A pattern has been established as the standard approach (e.g., "Use CQS for server architecture")
 - A decision is complete and being followed across teams
 
 ### Rejected

docs/architecture/server/index.md

@@ -4,22 +4,33 @@ sidebar_position: 6
 
 # Server Architecture
 
-## CQRS ([ADR-0008](../adr/0008-server-CQRS-pattern.md))
+## Command Query Separation (CQS)
 
-Our server architecture uses the the Command and Query Responsibility Segregation (CQRS) pattern.
+Our server architecture uses the Command Query Separation (CQS) pattern.
 
 The main goal of this pattern is to break up large services focused on a single entity (e.g.
-`CipherService`) and move towards smaller, reusable classes based on actions or tasks (e.g.
-`CreateCipher`). In the future, this may enable other benefits such as enqueuing commands for
-execution, but for now the focus is on having smaller, reusable chunks of code.
+`CipherService`, which handles everything to do with a cipher) and move towards smaller classes
+based on discrete actions (e.g. `CreateCipherCommand`, which handles only creating a cipher). This
+results in smaller classes with fewer interdependencies that are easier to change and test.
+
+:::note
+
+Previously this pattern was referred to as Command Query Responsibility Segregation (CQRS). However,
+CQRS is a far more complex architecture with larger impacts than we wanted or needed. Our
+implementation in practice more closely resembled CQS, which has a more modest (but still effective)
+aim of simply breaking up large classes. This documentation has been rewritten to better reflect our
+practice and therefore refers only to CQS.
+
+:::
 
 ### Commands vs. queries
 
-**Commands** are write operations, e.g. `RotateOrganizationApiKeyCommand`. They should never read
-from the database.
+**Commands** are write operations, e.g. `RotateOrganizationApiKeyCommand`. They change the state of
+the system. They may return data about the operation result (e.g. the updated object or an error
+message), but otherwise should not be used to return data to its caller.
 
-**Queries** are read operations, e.g. `GetOrganizationApiKeyQuery`. They should never write to the
-database.
+**Queries** are read operations, e.g. `GetOrganizationApiKeyQuery`. They should only return a value
+and should never change the state of the system.
 
 The database is the most common data source we deal with, but others are possible. For example, a
 query could also get data from a remote server.
@@ -28,18 +39,17 @@ Each query or command should have a single responsibility. For example: delete a
 file, rotate an API key. They are designed around verbs or actions (e.g.
 `RotateOrganizationApiKeyCommand`), not domains or entities (e.g. `ApiKeyService`).
 
-### Writing commands or queries
+Which you use will often follow the HTTP verb: a POST operation will generally call a command,
+whereas a GET operation will generally call a query.
 
-A simple query may just be a repository call to fetch data from the database. (We already use
-repositories, and this is not what we're concerned about here.) However, more complex queries can
-require additional logic around the repository call, which will require their own class. Commands
-always need their own class.
+Teams have wide discretion in how they structure their commands and queries.
 
-The class, interface and public method should be named after the action. For example:
+### Structure of a command
 
-```csharp
-namespace Bit.Core.OrganizationFeatures.OrganizationApiKeys;
+A command is just a class. The class, interface and public method should be named after the action.
+For example:
 
+```csharp
 public class RotateOrganizationApiKeyCommand : IRotateOrganizationApiKeyCommand
 {
   public async Task<OrganizationApiKey> RotateApiKeyAsync(OrganizationApiKey organizationApiKey)
@@ -49,51 +59,43 @@ public class RotateOrganizationApiKeyCommand : IRotateOrganizationApiKeyCommand
 }
 ```
 
-The query/command should only expose public methods that run the complete action. It should not have
+The command should only expose public methods that run the complete action. It should not have
 public helper methods.
 
-The directory structure and namespaces should be organized by feature. Interfaces should be stored
-in a separate sub-folder. For example:
-
-```text
-  Core/
-    └── OrganizationFeatures/
-        └── OrganizationApiKeys/
-            ├── Interfaces/
-            │   └── IRotateOrganizationApiKeyCommand.cs
-            └── RotateOrganizationApiKeyCommand.cs
-```
+A command will usually follow these steps:
 
-### Maintaining the command/query distinction
+1. Fetch additional data required to process the request (if required)
+1. Validate the request
+1. Perform the action (state change)
+1. Perform any side effects (e.g. sending emails or push notifications)
+1. Return information about the outcome to the user (e.g. an error message or the successfully
+   created or updated object)
 
-By separating read and write operations, CQRS encourages us to maintain loose coupling between
-classes. There are two golden rules to follow when using CQRS in our codebase:
+If you have complex validation logic, it can be useful to move it to a separate validator class.
+This makes the validator and the command easier to understand, test and maintain.
 
-- **Commands should never read and queries should never write**
-- **Commands and queries should never call each other**
+Some teams have defined their own request and result objects to pass data to and from commands and
+validators. This is optional but can be useful to avoid primitive obsession and have strongly typed
+interfaces.
 
-Both of these lead to tight coupling between classes, reduce opportunities for code re-use, and
-conflate the command/query distinction.
+### Structure of a query
 
-You can generally avoid these problems by:
+A simple query may not require its own class if it is appropriately encapsulated by a single
+database call. In that case, the "query" is just a repository method.
 
-- writing your commands so that they receive all the data they need in their arguments, rather than
-  fetching the data themselves
-- calling queries and commands sequentially (one after the other), passing the results along the
-  call chain
+However, more complex queries can require additional logic in addition to the repository call. In
+this case, it is appropriate to define a separate query class.
 
-For example, if we need to update an API key for an organization, it might be tempting to have an
-`UpdateApiKeyCommand` which fetches the current API key and then updates it. However, we can break
-this down into two separate queries/commands, which are called separately:
+A query is just a class. The class, interface and public method should be named after the data being
+queried. For example:
 
 ```csharp
-var currentApiKey = await _getOrganizationApiKeyQuery.GetOrganizationApiKeyAsync(orgId);
-await _rotateOrganizationApiKeyCommand.RotateApiKeyAsync(currentApiKey);
+public interface IGetOrganizationApiKeyQuery
+{
+    Task<OrganizationApiKey> GetOrganizationApiKeyAsync(Guid organizationId, OrganizationApiKeyType organizationApiKeyType);
+}
 ```
 
-This has unit testing benefits as well - instead of having lengthy "arrange" phases where you mock
-query results, you can simply supply different argument values using the `Autodata` attribute.
-
 ### Avoid [primitive obsession](https://refactoring.guru/smells/primitive-obsession)
 
 Where practical, your commands and queries should take and return whole objects (e.g. `User`) rather
@@ -103,3 +105,10 @@ than individual properties (e.g. `userId`).
 
 Lots of optional parameters can quickly become difficult to work with. Instead, consider using
 method overloading to provide different entry points into your command or query.
+
+## Further reading
+
+- [ADR-0028: Server CQS Terminology](../adr/0028-server-adopt-cqs-terminology.md) - Clarifies why we
+  use CQS instead of CQRS terminology
+- [ADR-0008: Server CQRS Pattern](../adr/0008-server-CQRS-pattern.md) - Original architectural
+  decision (superseded by ADR-0028)