diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index f655511d268..cb0309364d3 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -22,6 +22,7 @@ generator, in this case, **mdbook**. It defines the structure and navigation of - [Module Organization](architecture/module-organization.md) - [Module Structure](architecture/module-structure.md) - [Feature Modules](architecture/feature-modules.md) + - [Repository Pattern](architecture/repository-pattern.md) - [UI Architecture](architecture/ui-architecture.md) - [Theme System](architecture/theme-system.md) - [Design System](architecture/design-system.md) @@ -48,6 +49,7 @@ generator, in this case, **mdbook**. It defines the structure and navigation of - [Architecture Decision Records](engineering/adr/README.md) - [Template](engineering/adr/0000-adr-template.md) - [Proposed]() + - [0010 - Adopt project-wide Repository pattern](engineering/adr/0010-adopt-project-wide-repository-pattern.md) - [Accepted]() - [0001 - Switch From Java to Kotlin](engineering/adr/0001-switch-from-java-to-kotlin.md) - [0002 - UI - Wrap Material Components in Atomic Design System](engineering/adr/0002-ui-wrap-material-components-in-atomic-design-system.md) @@ -57,6 +59,7 @@ generator, in this case, **mdbook**. It defines the structure and navigation of - [0006 - White Label Architecture](engineering/adr/0006-white-label-architecture.md) - [0007 - Project Structure](engineering/adr/0007-project-structure.md) - [0008 - Change Shared Module package to `net.thunderbird`](engineering/adr/0008-change-shared-modules-package-name.md) + - [0009 - Feature/Core API/Internal split and dependency rules](engineering/adr/0009-api-internal-split.md) - [Rejected]() - [Obsolete]() - [Technical Designs](engineering/technical-designs/README.md) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 84a2dba166e..d62f4094c79 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -176,6 +176,8 @@ The data layer is responsible for data retrieval, storage, and synchronization. - **🔌 Data Sources**: Provide data from specific sources (API, database, preferences) - **📄 Data Transfer Objects**: Represent data at the data layer +See the [Repository Pattern](repository-pattern.md) guide for repository responsibilities and API conventions. + **Pattern: Data Source Pattern** - 🔍 Abstracts data sources behind a clean API - Maps data between domain models and data transfer objects diff --git a/docs/architecture/repository-pattern.md b/docs/architecture/repository-pattern.md new file mode 100644 index 00000000000..8ed9883fef2 --- /dev/null +++ b/docs/architecture/repository-pattern.md @@ -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 +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. + +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` | +| Optional value | `Outcome` | +| Collection | `Outcome, Error>` | +| No value | `Outcome` | +| Reactive value | `Flow>` | + +Non-fallible contracts return values directly: + +| Contract | Return type | +|----------------|--------------| +| Required value | `Type` | +| Optional value | `Type?` | +| Collection | `List` | +| No value | `Unit` | +| Reactive value | `Flow` | + +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 + + suspend fun update( + accountId: AccountId, + settings: DisplaySettings, + ): Outcome +} +``` + +## Read method naming + +Use these names for read methods: + +- Reactive reads: + - `observeById()` reads one entity by the aggregate's main identifier. + - `observeBy()` 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()`, or `findByCriteria()`. They always return a + nullable entity, either as `Type?` or `Outcome`. 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()`. They always return a non-null entity, either as + `Type` or `Outcome`. 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()` 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 +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> + + suspend fun update( + accountId: AccountId, + folderId: FolderId, + settings: FolderSettings, + ): Outcome +} +``` + +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 + + suspend fun update( + accountId: AccountId, + draftId: DraftId, + draft: Draft, + ): Outcome + + suspend fun delete( + accountId: AccountId, + draftId: DraftId, + ): Outcome +} +``` + +## 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> + + fun observeAllByCriteria( + accountId: AccountId, + criteria: FolderCriteria, + ): Flow, FolderError>> + + suspend fun findById( + accountId: AccountId, + folderId: FolderId, + ): Outcome + + suspend fun getById( + accountId: AccountId, + folderId: FolderId, + ): Outcome + + suspend fun findByServerId( + accountId: AccountId, + folderServerId: String, + ): Outcome +} + +interface RemoteFolderRepository { + suspend fun findAll( + accountId: AccountId, + ): Outcome, FolderError> +} + +interface FolderPushTrackingRepository { + fun observeEnabled( + accountId: AccountId, + ): Flow> + + suspend fun disable( + accountId: AccountId, + ): Outcome +} + +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. diff --git a/docs/contributing/development-guide.md b/docs/contributing/development-guide.md index 25d1e471b7f..2d02b045882 100644 --- a/docs/contributing/development-guide.md +++ b/docs/contributing/development-guide.md @@ -21,7 +21,7 @@ For full details, see: - Dependencies must flow in **one direction only** - UI built with **Jetpack Compose** + **MVI pattern** - Domain logic implemented in **Use Cases** -- Data handled via **Repository pattern** +- Data handled via the [**Repository pattern**](../architecture/repository-pattern.md) ## ⚙️ Dependency Injection diff --git a/docs/engineering/adr/0010-adopt-project-wide-repository-pattern.md b/docs/engineering/adr/0010-adopt-project-wide-repository-pattern.md new file mode 100644 index 00000000000..1b6c52b42af --- /dev/null +++ b/docs/engineering/adr/0010-adopt-project-wide-repository-pattern.md @@ -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. + +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. +