docs(database): add ADR 0010 for project-wide Repository pattern adoption #11452
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,262 @@ | ||
| # Repository Pattern | ||
|
|
||
| This guide defines the repository API conventions used by the project. It supports | ||
| [ADR-0010](../engineering/adr/0010-adopt-project-wide-repository-pattern.md). | ||
|
|
||
| ## Responsibility and ownership | ||
|
|
||
| A repository is a domain-facing contract that hides how data is read, stored, or synchronized. Its implementation may | ||
| coordinate local storage and remote services, but UI state and UI-specific business logic belong elsewhere. | ||
|
|
||
| Split contracts by responsibility before grouping them by aggregate. Do not create one interface per database table or | ||
| one interface per SQL operation. An optional aggregate-named facade may compose focused contracts, but it must not add | ||
| methods. A caller depends on the narrowest contract it needs. | ||
|
|
||
| An aggregate is a group of related domain data that changes together. A facade is an interface that combines focused | ||
| contracts for callers that need all of them. | ||
|
|
||
| Expose a repository contract from an `:api` module only when another area needs that stable contract. Keep | ||
| implementations, data sources, mappers, and storage details in `:internal`. Bind them in an application composition | ||
| module as required by [ADR-0009](../engineering/adr/0009-api-internal-split.md). | ||
|
|
||
| Place shared repository contracts, criteria, errors, and domain models in the area's `:api` module. Place repository | ||
| implementations, data sources, mappers, and storage models in its `:internal` module. Bind contracts to implementations | ||
| in `:app-common` or an app-specific composition module. | ||
|
|
||
| ## Scope and identifiers | ||
|
|
||
| Make the identifiers that scope every operation explicit. For example, an operation on a folder within an account takes | ||
| both `AccountId` and `FolderId`. An account-scoped operation continues to take `AccountId`, even if a folder identifier | ||
| is also supplied. Do not create a repository instance that is permanently bound to one account, folder, or other entity. | ||
|
|
||
| Use domain-specific identifier types such as `AccountId` and `FolderId` in new contracts rather than raw primitives. | ||
| This makes the scope visible to callers and supports a consolidated database with one connection for all accounts. | ||
|
|
||
| ## Asynchronous work and results | ||
|
|
||
| Repository I/O uses coroutines: | ||
|
|
||
| - A one-shot operation is a `suspend fun`. | ||
| - A reactive operation returns a `Flow`. | ||
|
|
||
| Choose the return type based on the contract rather than the current implementation. An expected recoverable failure is | ||
|
Contributor
There was a problem hiding this comment. This seems to be about a specific case where modifying the api to change the contract, making the new implementation match, rather than basing it off of existing repo functions, right? |
||
| a normal condition that the caller can handle, such as unavailable storage, a network failure, a rejected operation, or | ||
| a missing required entity. Return a value directly only when the contract can always provide a meaningful result, | ||
| possibly by using a default. | ||
|
Contributor
There was a problem hiding this comment. Would we want a default over an error, even a generic one? Unless the return type specifically only uses a default on error, it could lead to misleading returned data. Unless I'm misunderstanding the intention here.
Contributor
There was a problem hiding this comment. Seeing the default example below, with it loading default settings if none are available, does make sense. I suppose that, with the below context, this may be clear enough. |
||
|
|
||
| Use [`net.thunderbird.core.outcome.Outcome`](../../core/outcome/src/commonMain/kotlin/net/thunderbird/core/outcome/Outcome.kt) | ||
| for fallible repository results. `ERROR` is a domain-specific sealed type. Do not expose database, HTTP, or platform | ||
| exceptions from a contract. | ||
|
|
||
| Fallible contracts use `Outcome`: | ||
|
|
||
| | Contract | Return type | | ||
| |----------------|------------------------------| | ||
| | Required value | `Outcome<Type, Error>` | | ||
| | Optional value | `Outcome<Type?, Error>` | | ||
| | Collection | `Outcome<List<Type>, Error>` | | ||
| | No value | `Outcome<Unit, Error>` | | ||
| | Reactive value | `Flow<Outcome<Type, Error>>` | | ||
|
|
||
| Non-fallible contracts return values directly: | ||
|
|
||
| | Contract | Return type | | ||
| |----------------|--------------| | ||
| | Required value | `Type` | | ||
| | Optional value | `Type?` | | ||
| | Collection | `List<Type>` | | ||
| | No value | `Unit` | | ||
| | Reactive value | `Flow<Type>` | | ||
|
|
||
| For fallible operations, use `Outcome.Success(Unit)` when an operation completes successfully and has no value to | ||
| return. Use `Outcome.Success(null)` when a lookup completes successfully but no matching entity exists. Use | ||
| `Outcome.Failure(...)` when the operation cannot produce the required result. An API that requires an entity represents | ||
| a missing entity as a domain failure. | ||
|
|
||
| For example, this repository always returns settings by falling back to defaults, while an update can fail: | ||
|
|
||
| ```kotlin | ||
| interface DisplaySettingsRepository { | ||
| suspend fun getById( | ||
| accountId: AccountId, | ||
| ): DisplaySettings | ||
|
|
||
| fun observeById( | ||
| accountId: AccountId, | ||
| ): Flow<DisplaySettings> | ||
|
|
||
| suspend fun update( | ||
| accountId: AccountId, | ||
| settings: DisplaySettings, | ||
| ): Outcome<Unit, DisplaySettingsError> | ||
| } | ||
| ``` | ||
|
|
||
| ## Read method naming | ||
|
|
||
| Use these names for read methods: | ||
|
|
||
| - Reactive reads: | ||
| - `observeById()` reads one entity by the aggregate's main identifier. | ||
| - `observeBy<Identifier>()` reads one entity by another identifier, such as `observeByServerId()`. | ||
| - `observeAll()` reads all entities in scope. | ||
| - `observeByCriteria()` reads at most one entity for an immutable, domain-specific criteria type. | ||
| - `observeAllByCriteria()` reads multiple entities for an immutable, domain-specific criteria type. | ||
| - One-shot reads: | ||
| - Optional singular reads use `findById()`, `findBy<Identifier>()`, or `findByCriteria()`. They always return a | ||
| nullable entity, either as `Type?` or `Outcome<Type?, Error>`. Return `null` directly or `Outcome.Success(null)` | ||
| when no entity exists. | ||
| - Collection reads use `findAll()` or `findAllByCriteria()`. Return an empty list directly or | ||
| `Outcome.Success(emptyList())` when no entities exist. | ||
| - Required singular reads use `getById()` or `getBy<Identifier>()`. They always return a non-null entity, either as | ||
| `Type` or `Outcome<Type, Error>`. A direct return guarantees a meaningful value. A fallible return represents absence | ||
| as a domain failure. | ||
| - Criteria methods accept an immutable, domain-specific criteria type. | ||
|
|
||
| Reactive reads use the same absence semantics as one-shot reads. An optional singular read emits `null` directly or | ||
| `Outcome.Success(null)`. A required singular read emits a value directly or a domain failure. A collection read emits an | ||
| empty list directly or `Outcome.Success(emptyList())` when no entities exist. | ||
|
|
||
| ## Mutation methods | ||
|
|
||
| Use `create()`, `update()`, and `delete()` for aggregate lifecycle operations. Do not add a generic `save()` method to | ||
| new contracts. Use `clear<Scope>()` only for explicitly scoped bulk removal, such as `clearCache()`. | ||
|
|
||
| Within a focused repository contract, method names describe the operation and must not repeat the aggregate named by the | ||
| repository. For example, use `DraftRepository.create()`, `FolderQueryRepository.findById()`, and | ||
| `FolderQueryRepository.findByServerId()`, not `createDraft()`, `findByFolderId()`, or `findFolderByServerId()`. Use | ||
| `FolderQueryRepository.getById()` or `FolderQueryRepository.getByServerId()` for required lookups. | ||
|
|
||
| Avoid repository methods whose names start with `set`. Multiple independent setters expose invalid intermediate state | ||
|
Contributor
There was a problem hiding this comment. Definitely a good point to make 👍 |
||
| and usually mean the API is acting as a mutable state holder. Represent values that change together as an immutable | ||
| aggregate and update them atomically. A setup draft or UI-only mutable state belongs in a `Store` or `StateHolder`, not | ||
| a repository. | ||
|
|
||
| ```kotlin | ||
| data class FolderSettings( | ||
| val includeInUnifiedInbox: Boolean, | ||
| val visible: Boolean, | ||
| val syncEnabled: Boolean, | ||
| val notificationsEnabled: Boolean, | ||
| ) | ||
|
|
||
| interface FolderSettingsRepository { | ||
| fun observeById( | ||
| accountId: AccountId, | ||
| folderId: FolderId, | ||
| ): Flow<Outcome<FolderSettings, FolderError>> | ||
|
|
||
| suspend fun update( | ||
| accountId: AccountId, | ||
| folderId: FolderId, | ||
| settings: FolderSettings, | ||
| ): Outcome<Unit, FolderError> | ||
| } | ||
| ``` | ||
|
|
||
| Creation is deliberately different from a generic write. A draft has its own creation lifecycle and identifier, while | ||
| changes to an existing draft use `update()`: | ||
|
|
||
| ```kotlin | ||
| interface DraftRepository { | ||
| suspend fun create( | ||
| accountId: AccountId, | ||
| ): Outcome<Draft, DraftError> | ||
|
|
||
| suspend fun update( | ||
| accountId: AccountId, | ||
| draftId: DraftId, | ||
| draft: Draft, | ||
| ): Outcome<Unit, DraftError> | ||
|
|
||
| suspend fun delete( | ||
| accountId: AccountId, | ||
| draftId: DraftId, | ||
| ): Outcome<Unit, DraftError> | ||
| } | ||
| ``` | ||
|
|
||
| ## Folder contract example | ||
|
|
||
| The following example starts with the focused contracts new code should depend on. The existing `FolderRepository` | ||
| legacy facade is shown last for callers that genuinely need every contract. | ||
|
|
||
| ```kotlin | ||
| enum class UnifiedInboxFilter { | ||
| ANY, | ||
| INCLUDED, | ||
| EXCLUDED, | ||
| } | ||
|
|
||
| data class FolderCriteria( | ||
| val unifiedInbox: UnifiedInboxFilter = UnifiedInboxFilter.ANY, | ||
| val excludeLocalOnly: Boolean = false, | ||
| ) | ||
|
|
||
| sealed interface FolderError { | ||
| data object NotFound : FolderError | ||
| data object Unavailable : FolderError | ||
| } | ||
|
|
||
| interface FolderQueryRepository { | ||
| fun observeById( | ||
| accountId: AccountId, | ||
| folderId: FolderId, | ||
| ): Flow<Outcome<Folder?, FolderError>> | ||
|
|
||
| fun observeAllByCriteria( | ||
| accountId: AccountId, | ||
| criteria: FolderCriteria, | ||
| ): Flow<Outcome<List<FolderDetails>, FolderError>> | ||
|
|
||
| suspend fun findById( | ||
| accountId: AccountId, | ||
| folderId: FolderId, | ||
| ): Outcome<Folder?, FolderError> | ||
|
|
||
| suspend fun getById( | ||
| accountId: AccountId, | ||
| folderId: FolderId, | ||
| ): Outcome<Folder, FolderError> | ||
|
|
||
| suspend fun findByServerId( | ||
| accountId: AccountId, | ||
| folderServerId: String, | ||
| ): Outcome<Folder?, FolderError> | ||
| } | ||
|
|
||
| interface RemoteFolderRepository { | ||
| suspend fun findAll( | ||
| accountId: AccountId, | ||
| ): Outcome<List<RemoteFolder>, FolderError> | ||
| } | ||
|
|
||
| interface FolderPushTrackingRepository { | ||
| fun observeEnabled( | ||
| accountId: AccountId, | ||
| ): Flow<Outcome<Boolean, FolderError>> | ||
|
|
||
| suspend fun disable( | ||
| accountId: AccountId, | ||
| ): Outcome<Unit, FolderError> | ||
| } | ||
|
|
||
| interface FolderRepository : | ||
| FolderQueryRepository, | ||
| RemoteFolderRepository, | ||
| FolderPushTrackingRepository, | ||
| FolderSettingsRepository | ||
| ``` | ||
|
|
||
| In `FolderCriteria`, `UnifiedInboxFilter.ANY` means that the query does not filter on unified inbox inclusion. The | ||
| criteria contains filters only. Scope identifiers such as `AccountId` and `FolderId` remain explicit parameters rather | ||
| than becoming part of the criteria. | ||
|
|
||
| For example, a screen that renders folder preferences depends only on `FolderSettingsRepository`, not on | ||
| `FolderRepository`. The implementation may use any storage technology, as long as it maintains the contract. | ||
|
|
||
| ## Testing repository consumers | ||
|
|
||
| Test consumers with fakes that implement the focused contract. Cover successful values, nullable absence, empty | ||
| collections, and each expected domain failure. Repository implementations must test account filtering and the mapping | ||
| of expected data-source failures to domain errors. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # ADR 0010: Adopt project-wide Repository pattern | ||
|
|
||
| - Issue: [#11440](https://github.com/thunderbird/thunderbird-android/issues/11440) | ||
| - Status: **Proposed** | ||
|
|
||
| ## Context | ||
|
|
||
| Data access is spread across the codebase and is often coupled to business logic. The current per-account database | ||
| model makes cross-account features, including unified inbox pagination and unified folders, difficult to implement and | ||
| prevents a gradual migration to a consolidated database. | ||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. This really does lay out why this is such an important guideline succinctly 👍 |
||
|
|
||
| Repository-shaped APIs are inconsistent. Some expose mutable state through setters, and `FolderRepository` combines | ||
| local queries, remote queries, push tracking, and folder preference mutation in one interface. New work must not | ||
| replicate these broad, data-source-shaped interfaces. | ||
|
|
||
| ## Decision | ||
|
|
||
| Adopt focused, KMP-safe repository contracts project-wide. Repositories isolate domain code from data-source | ||
| implementations and may coordinate local storage, remote services, and synchronization. They do not contain UI logic. | ||
|
|
||
| Each repository contract must: | ||
|
|
||
| 1. Have one coherent responsibility and one reason to change. | ||
| 2. Be independent of data-source implementation details and platform types such as `android.*` and `Cursor`. | ||
| 3. Receive all identifiers required to scope an operation explicitly. An account-scoped operation always receives an | ||
| `AccountId`, even when it also receives an aggregate identifier such as `FolderId`. Repositories must not be bound | ||
| to a single account or entity instance. | ||
| 4. Be exposed from an `:api` module only when it is an intentionally shared, stable contract. Implementations, data | ||
| sources, mappers, and database details remain internal, as defined by [ADR-0009](0009-api-internal-split.md). | ||
|
|
||
| Repository APIs use Coroutines. The project `Outcome` type and a domain-specific error type represent expected, | ||
| recoverable failures. Repository contracts do not expose database, HTTP, or platform exceptions. | ||
|
|
||
| The detailed [Repository Pattern guide](../../architecture/repository-pattern.md) defines naming, reactive and one-shot | ||
| API shapes, aggregate updates, and examples. Migrate legacy access one use case at a time, introducing a focused | ||
| contract and error type first. Implementations or `:app-common` adapters preserve the contract while the backing data | ||
| moves to the consolidated schema. | ||
|
|
||
| ## Outcomes | ||
|
|
||
| ### Positive Outcomes | ||
|
|
||
| - Domain code can evolve independently from the current per-account database and its eventual consolidated replacement. | ||
| - Focused contracts avoid overly broad interfaces, make dependencies clearer, and are easier to fake in tests. | ||
| - Explicit account and aggregate identifiers make account filtering reviewable and support safe database consolidation. | ||
| - KMP-safe contracts can move to shared source sets without exposing Android implementation details. | ||
|
|
||
| ### Negative Outcomes | ||
|
|
||
| - The migration touches many call sites and requires old and new data-access paths to coexist temporarily. | ||
| - `Outcome` and domain error types add API and call-site verbosity. | ||
| - A missed `account_id` filter can still fail silently. Implementation-level safeguards and tests remain necessary. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if the more precise definition of aggregate and facade should appear before the instructions on how to properly use them.
Some definition of contracts and how we use them between the API and Implementation could also be useful. Not everyone who may wish to contribute to this project will come from years of software engineering experience, we'll likely get a few curious beginners eager to learn.