Amend ADR 8: Clarify CQS Terminology (#719)

Author: eliykat

custom-words.txt

@@ -13,6 +13,7 @@ codebases
 CODEOWNERS
 COSE
 CQRS
+CQS
 CTAP2
 deinitializer
 deinitializers

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

@@ -5,10 +5,19 @@ date: 2022-07-15
 tags: [server]
 ---
 
-# 0008 - Server: Adopt CQRS
+# 0008 - Server: Adopt CQS
 
 <AdrTable frontMatter={frontMatter}></AdrTable>
 
+:::note Terminology clarification
+
+This ADR originally used "CQRS" (Command Query Responsibility Segregation) terminology. However, our
+implementation more accurately reflects CQS (Command Query Separation), which is a simpler pattern
+focused on breaking up large service classes rather than separating read/write data models. The
+content has been updated to use the more accurate CQS terminology.
+
+:::
+
 ## Context and problem statement
 
 In Bitwarden Server, we currently use an `<<Entity>>Service` pattern to act on our entities. These
@@ -30,27 +39,26 @@ method parameters, which goes against our typical DI pattern.
 - **`<<Entity>>Services`** -- Discussed above
 - **Queries and Commands** -- Fundamentally our problem is that the `<<Entity>>Service` name
   encapsulates absolutely anything you can do with that entity and excludes any code reuse across
-  different entities. The CQRS pattern creates classes based on the action being taken on the
-  entity. This naturally limits the classes scope and allows for reuse should two entities need to
-  implement the same command behavior.
-  https://docs.microsoft.com/en-us/azure/architecture/patterns/cqrs
+  different entities. The CQS pattern creates classes based on the action being taken on the entity.
+  This naturally limits the classes scope and allows for reuse should two entities need to implement
+  the same command behavior.
 - **Small Feature-based services** -- This design would break `<<Entity>>Service` into
   `<<Feature>>Service`, but ultimately runs into the same problems. As a feature grows, this service
   would become bloated and tightly coupled to other services.
 
 ## Decision outcome
 
-Chosen option: **Queries and Commands**
+Chosen option: **Queries and Commands (CQS pattern)**
 
-Commands seem all-around the better decision to the incumbent. We gain code reuse and limit class
-scope. In addition, we have an iterative path to a full-blown CQRS pipeline, with queued work.
+Commands seem all-around the better decision to the incumbent, primarily because it limits class
+scope.
 
 Queries are already basically done through repositories and/or services, but would require some
 restructuring to be obvious.
 
 ## Transition plan
 
-We will gradually transition to a CQRS pattern over time. If a developer is making changes that use
+We will gradually transition to a CQS pattern over time. If a developer is making changes that use
 or affect a service method, they should consider whether it can be extracted to a query/command, and
 include it in their work as tech debt.
 
@@ -59,3 +67,10 @@ up all at once. It's acceptable to refactor "one level deep" and then leave othe
 are. This may result in the new query/command still being somewhat coupled with other service
 methods. This is acceptable during the transition phase, but these interdependencies should be
 removed over time.
+
+## Further reading
+
+- [Server Architecture](../server/index.md) - Practical guide on implementing CQS in the server
+  codebase
+- [Martin Fowler on CQS](https://martinfowler.com/bliki/CommandQuerySeparation.html) - High-level
+  summary of the CQS principle

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,22 @@ 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.
+We adopted this pattern in order to break up large services focused on a single entity (e.g.
+`CipherService`) into smaller classes based on discrete actions (e.g. `CreateCipherCommand`). This
+results in smaller classes with fewer interdependencies that are easier to change and test.
 
 ### 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 have no return value, or may return the operation result only (e.g. the updated
+object or an error message).
 
-**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 +28,15 @@ 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.
+### Structure of a command
 
-The class, interface and public method should be named after the action. For example:
+A command is just a class. The class, interface and public method should be named after the action.
+For example:
 
 ```csharp
-namespace Bit.Core.OrganizationFeatures.OrganizationApiKeys;
-
 public class RotateOrganizationApiKeyCommand : IRotateOrganizationApiKeyCommand
 {
   public async Task<OrganizationApiKey> RotateApiKeyAsync(OrganizationApiKey organizationApiKey)
@@ -49,51 +46,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)
+2. Validate the request
+3. Perform the action (state change)
+4. Perform any side effects (e.g. sending emails or push notifications)
+5. 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 +92,8 @@ 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-0008: Server CQS Pattern](../adr/0008-server-CQRS-pattern.md) - Architectural decision to
+  adopt CQS for breaking up large service classes

docs/contributing/code-style/web/angular.md

@@ -212,7 +212,7 @@ component "Organization Reports Module" {
 @enduml
 ```
 
-## Reactivity ([ADR-0003](../../../architecture/adr/0003-observable-data-services.md) & ADR-0028)
+## Reactivity ([ADR-0003](../../../architecture/adr/0003-observable-data-services.md))
 
 We make heavy use of reactive programming using [Angular Signals][signals] & [RxJS][rxjs]. Generally
 components should always derive their state reactively from services whenever possible.