diff --git a/custom-words.txt b/custom-words.txt index 0dd332c9c..912b885e1 100644 --- a/custom-words.txt +++ b/custom-words.txt @@ -12,6 +12,7 @@ codebases CODEOWNERS COSE CQRS +CQS CTAP2 deinitializer deinitializers diff --git a/docs/architecture/adr/0008-server-CQRS-pattern.md b/docs/architecture/adr/0008-server-CQRS-pattern.md index 49ff3737b..69dceb6e5 100644 --- a/docs/architecture/adr/0008-server-CQRS-pattern.md +++ b/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 +:::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 `<>Service` pattern to act on our entities. These @@ -30,27 +39,26 @@ method parameters, which goes against our typical DI pattern. - **`<>Services`** -- Discussed above - **Queries and Commands** -- Fundamentally our problem is that the `<>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 `<>Service` into `<>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 diff --git a/docs/architecture/adr/index.mdx b/docs/architecture/adr/index.mdx index b6c780bf7..10f220e9a 100644 --- a/docs/architecture/adr/index.mdx +++ b/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 diff --git a/docs/architecture/server/index.md b/docs/architecture/server/index.md index bfd8533b7..d885a16c3 100644 --- a/docs/architecture/server/index.md +++ b/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 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 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 diff --git a/docs/contributing/code-style/web/angular.md b/docs/contributing/code-style/web/angular.md index cf547ba5a..165ed03ad 100644 --- a/docs/contributing/code-style/web/angular.md +++ b/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.