From ee7627dfce2515eb7724bb18a6915e8e1092cc45 Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers Date: Sun, 6 Sep 2026 10:25:57 +0200 Subject: [PATCH] refactor(cohort): one port over the list, with the vendor behind contact CohortPort and TargetStrategy abstracted the same Brevo list, each with one production implementation delegating to the same ContactListAdapter. The richer of the two survives; the other, its registry, its Brevo adapter and its mock are gone. BrevoTargetStrategy no longer injects the generated ContactsApi: folder and list paging moved into BrevoListAdapter and is published through contact :: api, so cohort reaches Brevo only through the wrapper and the pinned ADR-019 violation in VendorClientArchitectureTest is deleted rather than edited. --- .../cohort/domain/CohortLedgerAutoflushIT.kt | 10 +- .../CohortProviderTransactionBoundaryIT.kt | 8 +- .../blueshell/api/cohort/ModuleMetadata.kt | 6 +- .../api/cohort/domain/BrevoCohortAdapter.kt | 57 -------- .../api/cohort/domain/BrevoTargetStrategy.kt | 83 +++-------- .../domain/CohortMembershipSyncService.kt | 18 +-- .../blueshell/api/cohort/domain/CohortPort.kt | 41 ------ .../api/cohort/domain/CohortPortRegistry.kt | 22 --- .../cohort/domain/CohortRemediationService.kt | 14 +- .../api/cohort/domain/CohortTargeting.kt | 2 +- .../api/cohort/domain/TargetStrategies.kt | 9 ++ .../api/cohort/domain/TargetStrategy.kt | 13 ++ .../api/contact/api/ContactListAdapter.kt | 17 +++ .../api/contact/domain/BrevoListAdapter.kt | 56 ++++++-- .../integration/mock/MockCohortPort.kt | 101 -------------- .../integration/mock/MockTargetStrategy.kt | 45 +++++- .../VendorClientArchitectureTest.kt | 23 +--- .../cohort/domain/BrevoCohortAdapterTest.kt | 130 ------------------ .../cohort/domain/BrevoTargetStrategyTest.kt | 85 ++---------- .../domain/CohortMembershipSyncServiceTest.kt | 34 ++--- .../domain/CohortRemediationServiceTest.kt | 39 ++++-- .../api/cohort/domain/TargetFolderMoveTest.kt | 4 +- .../contact/domain/BrevoListAdapterTest.kt | 47 +++++++ 23 files changed, 288 insertions(+), 576 deletions(-) delete mode 100644 services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt delete mode 100644 services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt delete mode 100644 services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt delete mode 100644 services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt delete mode 100644 services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt diff --git a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt index a56a4d089..0b1a57c6e 100644 --- a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt +++ b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt @@ -10,7 +10,7 @@ import net.blueshell.api.cohort.persistence.CohortMemberRepository import net.blueshell.api.cohort.persistence.CohortRepository import net.blueshell.api.cohort.persistence.CohortSubjectRepository import net.blueshell.api.cohort.persistence.state -import net.blueshell.api.platform.integration.mock.MockCohortPort +import net.blueshell.api.platform.integration.mock.MockTargetStrategy import net.blueshell.api.sync.persistence.ExternalIdMapping import net.blueshell.api.sync.persistence.ExternalIdMappingRepository import net.blueshell.api.shared.enums.Role @@ -41,11 +41,11 @@ class CohortLedgerAutoflushIT : UserTestSupport() { private lateinit var remediation: CohortRemediationService @Autowired - private lateinit var mockCohortPort: MockCohortPort + private lateinit var mockTarget: MockTargetStrategy @BeforeEach - fun resetCohortPort() { - mockCohortPort.clear() + fun resetTarget() { + mockTarget.clear() } @Test @@ -65,7 +65,7 @@ class CohortLedgerAutoflushIT : UserTestSupport() { ), ) externalIds.saveAndFlush(ExternalIdMapping("USER", user.id!!, TargetSystem.BREVO.name, "ext-1")) - mockCohortPort.seedMember("ext-1", "list-99", "Ada Remote") + mockTarget.seedMember("ext-1", "list-99", "Ada Remote") assertThatCode { remediation.verifyCohort(cohort.id!!) }.doesNotThrowAnyException() diff --git a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt index 188e9c498..a564d4a36 100644 --- a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt +++ b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt @@ -6,7 +6,7 @@ import net.blueshell.api.cohort.persistence.CohortSubject import net.blueshell.api.cohort.persistence.CohortSubjectType import net.blueshell.api.cohort.persistence.CohortRepository import net.blueshell.api.cohort.persistence.CohortSubjectRepository -import net.blueshell.api.platform.integration.mock.MockCohortPort +import net.blueshell.api.platform.integration.mock.MockTargetStrategy import net.blueshell.api.sync.api.ExternalIdMappingService.Companion.USER_AGGREGATE import net.blueshell.api.sync.persistence.ExternalIdMapping import net.blueshell.api.sync.persistence.ExternalIdMappingRepository @@ -24,8 +24,8 @@ import tools.jackson.databind.ObjectMapper * path. [AbstractJsonJobHandler.handle][net.blueshell.api.jobs.api.AbstractJsonJobHandler] * is `@Transactional`, so a job is always dispatched with a transaction * active. The application services suspend it (`PROPAGATION_NOT_SUPPORTED`) - * around every [CohortPort][net.blueshell.api.cohort.domain.CohortPort] - * call; [MockCohortPort] records whether a transaction was actually active + * around every [TargetStrategy][net.blueshell.api.cohort.domain.TargetStrategy] + * call; [MockTargetStrategy] records whether a transaction was actually active * at each call so we can assert it never is. */ @SpringBootTest @@ -41,7 +41,7 @@ class CohortProviderTransactionBoundaryIT : UserTestSupport() { @Autowired private lateinit var externalIds: ExternalIdMappingRepository - @Autowired private lateinit var port: MockCohortPort + @Autowired private lateinit var port: MockTargetStrategy @Autowired private lateinit var objectMapper: ObjectMapper diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt index 8fd46791e..ed6523ce6 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt @@ -7,8 +7,8 @@ import org.springframework.modulith.PackageInfo * Code-defined audiences — "active members", "paid for this period" — evaluated into `CohortMember` * rows and reconciled against the mailing list or group that stands for them in an external system. * - * `port/in` is what other modules may call and `port/out` is the driven side, one `CohortPort` per - * target system; neither is a REST surface, the controllers under `adapter/web` are. + * One `TargetStrategy` per target system stands between the module and that system; the REST + * surface is the controllers under `web`. */ @PackageInfo @ApplicationModule( @@ -26,7 +26,7 @@ import org.springframework.modulith.PackageInfo // committees — this is a definition reaching into persistence, and it // wants a projection published through committee :: api instead. "committee :: entities", - // List membership is pushed through ContactListAdapter. + // The list catalogue, its folders and its membership all go through ContactListAdapter. "contact :: api", // Period cohorts are built from ContributionService and // ContributionPeriodService, and react to ContributionChanged. diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt deleted file mode 100644 index 2752f9609..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt +++ /dev/null @@ -1,57 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.enums.ContactSystem -import org.springframework.context.annotation.Profile -import org.springframework.stereotype.Service - -/** - * Brevo's [CohortPort] implementation. Delegates to the existing - * Brevo [ContactListAdapter] bean for the actual HTTP calls so we - * inherit its recovery semantics (idempotent add, contact-gone - * disambiguation, ...) without duplicating client wiring. - */ -@Service -@Profile("!test & !dev") -class BrevoCohortAdapter( - contactListAdapters: List, -) : CohortPort { - - private val delegate: ContactListAdapter = contactListAdapters.single { it.system == ContactSystem.BREVO } - - override val system: TargetSystem = TargetSystem.BREVO - - override fun createCohort(label: String, hint: String?): String = - delegate.createList(label, hint).toString() - - override fun addMember(externalUserId: String, externalCohortId: String) { - delegate.addToList( - externalUserId.toBrevoId("externalUserId", "addMember"), - externalCohortId.toBrevoId("externalCohortId", "addMember"), - ) - } - - override fun removeMember(externalUserId: String, externalCohortId: String) { - delegate.removeFromList( - externalUserId.toBrevoId("externalUserId", "removeMember"), - externalCohortId.toBrevoId("externalCohortId", "removeMember"), - ) - } - - override fun deleteCohort(externalCohortId: String) { - delegate.deleteList(externalCohortId.toBrevoId("externalCohortId", "deleteCohort")) - } - - override fun listMembers(externalCohortId: String): List = - delegate.listMembers(externalCohortId.toBrevoId("externalCohortId", "listMembers")) - .map { MemberRef(it.externalUserId.toString(), it.email) } - // Never surface a blank external id: it would classify as an - // INVALID ledger row downstream (see CohortMemberState). - .filter { it.externalUserId.isNotBlank() } - - private fun String.toBrevoId(field: String, operation: String): Long = - toLongOrNull() ?: throw InvalidExternalIdException( - "Brevo $operation: $field \"$this\" is not a valid numeric id" - ) -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt index 48b443825..93fcd0636 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt @@ -2,22 +2,21 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.cohort.persistence.CohortKind import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.contact.api.ContactServiceException +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.shared.enums.ContactSystem import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.clients.brevo.api.ContactsApi -import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner -import net.blueshell.clients.brevo.model.GetContactsSortParameter -import org.slf4j.LoggerFactory import org.springframework.context.annotation.Profile import org.springframework.stereotype.Service -import org.springframework.web.client.RestClientResponseException +/** + * Brevo's [TargetStrategy], stated entirely in terms of [ContactListAdapter]: the catalogue, the + * folders and the membership writes are all that module's, so nothing Brevo-shaped reaches here + * beyond the numeric id its lists are keyed by (API ADR-019). + */ @Service @Profile("!test & !dev") class BrevoTargetStrategy( contactListAdapters: List, - private val contactsApi: ContactsApi, ) : TargetStrategy { private val lists = contactListAdapters.single { it.system == ContactSystem.BREVO } @@ -40,7 +39,9 @@ class BrevoTargetStrategy( override fun catalog(query: String?): List { val q = query?.trim()?.lowercase().orEmpty() - return listTargets(folderNames()) + val folderNames = lists.listFolders() + return lists.listAll() + .map { it.toTarget(folderNames[it.folderId]) } .filter { it.matches(q) } .sortedWith(compareBy({ it.folderLabel.orEmpty() }, { it.label })) } @@ -99,36 +100,15 @@ class BrevoTargetStrategy( private fun pathTo(folder: String?): List = listOfNotNull(descriptor.systemLabel, folder?.takeIf { it.isNotBlank() }) - private fun folderNames(): Map = - page("folders") { limit, offset -> - contactsApi.getFolders(limit, offset, GetContactsSortParameter.ASC).let { page -> - Page(page.count, page.folders.orEmpty().associate { it.id.toString() to it.name }.entries.toList()) - } - }.associate { it.key to it.value } - - private fun listTargets(folderNames: Map): List = - page("lists") { limit, offset -> - contactsApi.getLists(limit, offset, GetContactsSortParameter.ASC).let { page -> - Page(page.count, page.lists.orEmpty().map { it.toTarget(folderNames, ::pathTo) }) - } - } - - private fun page(kind: String, fetch: (Long, Long) -> Page): List { - val results = mutableListOf() - var offset = 0L - while (true) { - val page = try { - fetch(PAGE_SIZE, offset) - } catch (e: RestClientResponseException) { - if (e.statusCode.value() == 429) log.warn("Brevo target catalog {} fetch was rate limited", kind) - throw ContactServiceException("Failed to fetch Brevo $kind catalog", e) - } - results += page.items - if (page.items.size < PAGE_SIZE || page.count != null && results.size >= page.count) break - offset += PAGE_SIZE - } - return results - } + private fun ContactListRef.toTarget(folder: String?): ExternalTarget = ExternalTarget( + system = system, + externalId = externalListId.toString(), + kind = descriptor.kind, + label = name, + folderLabel = folder, + memberCount = memberCount, + path = pathTo(folder), + ) private fun ExternalTarget.matches(query: String): Boolean = query.isBlank() || @@ -136,29 +116,10 @@ class BrevoTargetStrategy( label.lowercase().contains(query) || folderLabel.orEmpty().lowercase().contains(query) + /** + * Brevo keys lists and contacts by number, so an id that is not one names nothing there. + * The sole conversion between the port's [String] ids and Brevo's own. + */ private fun String.toBrevoId(field: String, operation: String): Long = toLongOrNull() ?: throw InvalidExternalIdException("Brevo $operation: $field \"$this\" is not numeric") - - private data class Page(val count: Long?, val items: List) - - companion object { - const val PAGE_SIZE: Long = 50 - private val log = LoggerFactory.getLogger(BrevoTargetStrategy::class.java) - } -} - -private fun GetLists200ResponseListsInner.toTarget( - folderNames: Map, - pathTo: (String?) -> List, -): ExternalTarget { - val folder = folderNames[folderId.toString()] - return ExternalTarget( - system = TargetSystem.BREVO, - externalId = id.toString(), - kind = CohortKind.LIST, - label = name, - folderLabel = folder, - memberCount = uniqueSubscribers, - path = pathTo(folder), - ) } diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt index a8f1242cf..4e549d2bc 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt @@ -18,7 +18,7 @@ import java.time.LocalDateTime /** * Drives one `(user, cohort)` sync end to end: resolves both external ids, picks the - * [CohortPort] for the cohort's system and asks it to apply the change. + * [TargetStrategy] for the cohort's system and asks it to apply the change. * * An ADD with no user external id enqueues `SyncContact` and throws retryably, so the retry * lands once the contact exists. An ADD with no cohort target id fails terminally — linking a @@ -28,7 +28,7 @@ import java.time.LocalDateTime class CohortMembershipSyncService( private val cohorts: CohortRepository, private val ledger: CohortLedger, - private val registry: CohortPortRegistry, + private val strategies: TargetStrategies, private val externalIds: ExternalIdMappingService, private val targetIds: CohortTargetIds, private val jobs: TrackedJobDispatcher, @@ -52,15 +52,15 @@ class CohortMembershipSyncService( val system = runCatching { TargetSystem.valueOf(cohort.system) }.getOrElse { throw NonRetryableJobException("Cohort $cohortId has unknown system '${cohort.system}'") } - val port = registry.require(system) + val strategy = strategies.requireForJob(system) when (intent) { - SyncCohortMembershipIntent.ADD -> add(userId, cohort, port) - SyncCohortMembershipIntent.REMOVE -> remove(userId, cohort, port) + SyncCohortMembershipIntent.ADD -> add(userId, cohort, strategy) + SyncCohortMembershipIntent.REMOVE -> remove(userId, cohort, strategy) } } - private fun add(userId: Long, cohort: Cohort, port: CohortPort) { + private fun add(userId: Long, cohort: Cohort, strategy: TargetStrategy) { val cohortId = cohort.id!! val system = cohort.system val externalUserId = externalIds.find(USER_AGGREGATE, userId, system)?.externalId @@ -74,7 +74,7 @@ class CohortMembershipSyncService( if (externalCohortId == null) { throw CohortTargetNotLinkedException(cohortId, system) } - outsideTransaction.executeWithoutResult { port.addMember(externalUserId, externalCohortId) } + outsideTransaction.executeWithoutResult { strategy.add(strategy.handle(externalCohortId), externalUserId) } // Stamp the ledger so the desired row reads as synced. This is the // primary path to healthy; reconcile only verifies afterwards. @@ -84,7 +84,7 @@ class CohortMembershipSyncService( log.debug("Added user {} to {} cohort {} (ext={})", userId, system, cohortId, externalCohortId) } - private fun remove(userId: Long, cohort: Cohort, port: CohortPort) { + private fun remove(userId: Long, cohort: Cohort, strategy: TargetStrategy) { val cohortId = cohort.id!! val system = cohort.system val externalUserId = externalIds.find(USER_AGGREGATE, userId, system)?.externalId @@ -96,7 +96,7 @@ class CohortMembershipSyncService( ) return } - outsideTransaction.executeWithoutResult { port.removeMember(externalUserId, externalCohortId) } + outsideTransaction.executeWithoutResult { strategy.remove(strategy.handle(externalCohortId), externalUserId) } log.debug("Removed user {} from {} cohort {} (ext={})", userId, system, cohortId, externalCohortId) } diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt deleted file mode 100644 index ba1f9b80d..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt +++ /dev/null @@ -1,41 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.shared.enums.TargetSystem - -/** - * Outbound port for cohort membership sync, one implementation per [TargetSystem], chosen out - * of `List` by the cohort's system. - * - * Operation-based rather than state-replacing: a caller adds or removes one `(user, cohort)` - * pair by external id, and the cohort's target must already be linked — creating one is an - * operator's act. Ids are `String` so Discord snowflakes and Google group emails sit alongside - * Brevo's numeric list ids without every adapter coercing to `Long`. - */ -interface CohortPort { - val system: TargetSystem - - /** - * Creates the cohort on the external system and returns its native - * id (the snowflake / list id / group address). The `hint` may be - * used by the adapter as a folder/parent classification when the - * external system supports it (e.g. Brevo folder id); adapters that - * do not need a hint may ignore it. - */ - fun createCohort(label: String, hint: String? = null): String - - fun addMember(externalUserId: String, externalCohortId: String) - - fun removeMember(externalUserId: String, externalCohortId: String) - - fun deleteCohort(externalCohortId: String) - - /** - * Lists all members currently present in the external target. - * Returns a bounded list; the largest live cohort is ~350 rows. - * If a cohort ever grows past ~2 000, switch to cursor-based pages. - */ - fun listMembers(externalCohortId: String): List -} - -/** One member as the external system knows them: a native id and an optional human-readable label. */ -data class MemberRef(val externalUserId: String, val label: String?) diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt deleted file mode 100644 index bb6950833..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt +++ /dev/null @@ -1,22 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.job.NonRetryableJobException -import org.springframework.stereotype.Component - -/** - * Central registry of all registered [CohortPort] implementations. - * Replaces ad-hoc `ports.single { it.system == system }` lookups. - * A missing port becomes a [NonRetryableJobException] (terminal FAILED) - * rather than an infinite retry. - */ -@Component -class CohortPortRegistry(ports: List) { - private val bySystem: Map = ports.associateBy { it.system } - - fun require(system: TargetSystem): CohortPort = - bySystem[system] - ?: throw NonRetryableJobException("No CohortPort registered for $system") - - fun systems(): Set = bySystem.keys -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt index f3782bee7..2163e712f 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt @@ -38,7 +38,7 @@ class CohortRemediationService( private val ledger: CohortLedger, private val externalIds: ExternalIdMappingService, private val targetIds: CohortTargetIds, - private val registry: CohortPortRegistry, + private val strategies: TargetStrategies, private val jobs: TrackedJobDispatcher, transactionManager: PlatformTransactionManager, ) : CohortRemediation { @@ -72,7 +72,8 @@ class CohortRemediationService( val system = TargetSystem.valueOf(cohort.system) val externalCohortId = targetIds.require(cohort) - outsideTransaction.executeWithoutResult { registry.require(system).removeMember(externalUserId, externalCohortId) } + val strategy = strategies.requireForJob(system) + outsideTransaction.executeWithoutResult { strategy.remove(strategy.handle(externalCohortId), externalUserId) } ledger.removeStranger(cohortId, externalUserId) } @@ -83,7 +84,8 @@ class CohortRemediationService( */ override fun verifyCohort(cohortId: Long) { val plan = readOnlyTransaction.execute { loadPlan(cohortId) } - val remote = outsideTransaction.execute { registry.require(plan.system).listMembers(plan.externalCohortId) } + val strategy = strategies.requireForJob(plan.system) + val remote = outsideTransaction.execute { strategy.members(strategy.handle(plan.externalCohortId)) } writeTransaction.executeWithoutResult { applySnapshot(plan, remote) } } @@ -120,7 +122,7 @@ class CohortRemediationService( return ReconcilePlan(cohortId, subjectId, system, externalCohortId) } - private fun applySnapshot(plan: ReconcilePlan, remote: List) { + private fun applySnapshot(plan: ReconcilePlan, remote: List) { val cohort = cohortRepo.findById(plan.cohortId).orElseThrow { NonRetryableJobException("Cohort ${plan.cohortId} not found") } @@ -169,7 +171,7 @@ class CohortRemediationService( plan: ReconcilePlan, desiredRows: List, externalIdByUserId: Map, - remoteByExtId: Map, + remoteByExtId: Map, now: LocalDateTime, ): Set { val confirmations = desiredRows.mapNotNull { row -> @@ -220,7 +222,7 @@ class CohortRemediationService( private fun reconcileStrangers( cohort: net.blueshell.api.cohort.persistence.Cohort, subject: net.blueshell.api.cohort.persistence.CohortSubject, - remoteByExtId: Map, + remoteByExtId: Map, confirmedExtIds: Set, now: LocalDateTime, ) { diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt index 13018b6df..7b3e1ad13 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt @@ -6,7 +6,7 @@ import net.blueshell.api.shared.enums.TargetSystem * Inbound port for admin management of a subject's external targets: linking an existing one, * creating one, and repointing a mapping at another. * - * External writes go through [CohortPort]. A removal after a switch is handed to the + * External writes go through [TargetStrategy]. A removal after a switch is handed to the * `cohort.delete-external-target` job rather than run inline. */ interface CohortTargeting { diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt index 4468ec94f..36aab342c 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt @@ -1,6 +1,7 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.shared.enums.TargetSystem +import net.blueshell.api.shared.job.NonRetryableJobException import org.springframework.http.HttpStatus import org.springframework.stereotype.Component import org.springframework.web.server.ResponseStatusException @@ -13,6 +14,14 @@ class TargetStrategies(strategies: List) { bySystem[system] ?: throw ResponseStatusException(HttpStatus.BAD_REQUEST, "$system is not a cohort target") + /** + * The strategy a job needs. A missing one is terminal rather than retried: no amount of + * backoff registers a bean, where [require] answers a request with a bad request. + */ + fun requireForJob(system: TargetSystem): TargetStrategy = + bySystem[system] + ?: throw NonRetryableJobException("No TargetStrategy registered for $system") + fun descriptor(system: TargetSystem): TargetDescriptor = require(system).descriptor fun descriptors(): List = diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt index 830483d49..1f4de9d64 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt @@ -38,10 +38,23 @@ data class ExternalTarget( data class ExternalMember(val externalUserId: String, val label: String?) +/** + * The one port over a cohort's external target: its catalogue, its folders, and who is on it. + * + * Ids are [String] so a Discord snowflake or a Google group address sits beside Brevo's numeric + * list id; an adapter that needs another shape converts at its own edge and nowhere else. + */ interface TargetStrategy { val descriptor: TargetDescriptor val system: TargetSystem get() = descriptor.system + /** + * A target known only by its id. The member, move and delete calls key on the id alone, so a + * caller holding one writes without reading the catalogue first. + */ + fun handle(externalId: String): ExternalTarget = + ExternalTarget(system, externalId, descriptor.kind, externalId) + fun catalog(query: String?): List = emptyList() fun resolve(externalId: String): ExternalTarget? = diff --git a/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt index af919427a..3752cf9e7 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt @@ -29,9 +29,26 @@ interface ContactListAdapter { /** Every folder, as id to name. */ fun listFolders(): Map = emptyMap() + /** + * Every list on the system, whoever it belongs to: the catalogue an operator picks from. + * A system that cannot be browsed answers with nothing rather than failing. + */ + fun listAll(): List = emptyList() + /** Lists all members currently present in the given external list. */ fun listMembers(externalListId: Long): List } /** One member as the external system knows them: a native numeric id and optional email label. */ data class ContactListMember(val externalUserId: Long, val email: String?) + +/** + * One list as the external system knows it. [folderId] keys into [ContactListAdapter.listFolders]; + * it is unresolved here so a caller reads the folders once rather than per list. + */ +data class ContactListRef( + val externalListId: Long, + val name: String, + val folderId: Long?, + val memberCount: Long?, +) diff --git a/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt index 6e5362e9f..08ef0e262 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt @@ -1,6 +1,7 @@ package net.blueshell.api.contact.domain import net.blueshell.api.contact.api.ContactListMember +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.contact.api.ContactServiceException import net.blueshell.api.contact.api.ContactListAdapter import net.blueshell.api.shared.enums.ContactSystem @@ -51,15 +52,52 @@ class BrevoListAdapter( } override fun listFolders(): Map = - try { - contactsApi.getFolders(FOLDER_PAGE_LIMIT, 0, GetContactsSortParameter.ASC) - .folders.orEmpty() - .associate { it.id to it.name } - } catch (e: RestClientResponseException) { - log.error("Failed to read Brevo folders", e) - throw ContactServiceException("Failed to read folders", e) + page("folders") { limit, offset -> + contactsApi.getFolders(limit, offset, GetContactsSortParameter.ASC) + .let { Page(it.count, it.folders.orEmpty().map { folder -> folder.id to folder.name }) } + }.toMap() + + override fun listAll(): List = + page("lists") { limit, offset -> + contactsApi.getLists(limit, offset, GetContactsSortParameter.ASC).let { page -> + Page( + page.count, + page.lists.orEmpty().map { + ContactListRef( + externalListId = it.id, + name = it.name, + folderId = it.folderId, + memberCount = it.uniqueSubscribers, + ) + }, + ) + } } + /** + * Walks a Brevo collection until a short page or the reported count ends it. A rate-limited + * page is logged as such and raised like any other fetch failure, so the job retries. + */ + private fun page(kind: String, fetch: (Long, Long) -> Page): List { + val results = mutableListOf() + var offset = 0L + while (true) { + val page = try { + fetch(CATALOG_PAGE_SIZE, offset) + } catch (e: RestClientResponseException) { + if (e.statusCode.value() == 429) log.warn("Brevo {} fetch was rate limited", kind) + log.error("Failed to read Brevo {}", kind, e) + throw ContactServiceException("Failed to read $kind", e) + } + results += page.items + if (page.items.size < CATALOG_PAGE_SIZE || page.count != null && results.size >= page.count) break + offset += CATALOG_PAGE_SIZE + } + return results + } + + private data class Page(val count: Long?, val items: List) + override fun createList(name: String, folderName: String?): Long { val safeName = sanitizeForLog(name) log.info("Creating Brevo list '{}'", safeName) @@ -207,8 +245,8 @@ class BrevoListAdapter( } companion object { - /** Folders are few; one page covers any real account. */ - private const val FOLDER_PAGE_LIMIT = 50L + /** Brevo's own page cap for folders and lists. */ + private const val CATALOG_PAGE_SIZE = 50L private val log = LoggerFactory.getLogger(BrevoListAdapter::class.java) } } diff --git a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt deleted file mode 100644 index 049ec3adb..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt +++ /dev/null @@ -1,101 +0,0 @@ -package net.blueshell.api.platform.integration.mock - -import net.blueshell.api.cohort.domain.CohortPort -import net.blueshell.api.cohort.domain.MemberRef -import net.blueshell.api.shared.enums.TargetSystem -import org.slf4j.LoggerFactory -import org.springframework.context.annotation.Primary -import org.springframework.context.annotation.Profile -import org.springframework.stereotype.Service -import org.springframework.transaction.support.TransactionSynchronizationManager -import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.CopyOnWriteArrayList -import java.util.concurrent.atomic.AtomicLong - -/** - * In-memory [CohortPort] for test and dev profiles. Holds member and - * cohort state so the registry, drift service, and controller ITs - * have a real port to exercise without touching Brevo. - * - * Reported system is [TargetSystem.BREVO] so tests exercise the Brevo - * code path without needing the production adapter. - */ -@Service -@Primary -@Profile("test | dev") -class MockCohortPort : CohortPort { - - override val system: TargetSystem = TargetSystem.BREVO - - // cohortId → label - private val cohorts = ConcurrentHashMap() - - // (externalUserId, externalCohortId) → email hint ("" when unknown; - // ConcurrentHashMap forbids null values). - private val memberships = ConcurrentHashMap, String>() - - private val idSequence = AtomicLong(9000) - - /** - * Records `isActualTransactionActive()` at the entry of every provider - * call, so a boundary test can assert these run outside any DB - * transaction (the no-provider-call-inside-a-transaction rule). - */ - val transactionActiveDuringCalls: MutableList = CopyOnWriteArrayList() - - private fun recordTransactionState() { - transactionActiveDuringCalls += TransactionSynchronizationManager.isActualTransactionActive() - } - - override fun createCohort(label: String, hint: String?): String { - recordTransactionState() - val id = idSequence.getAndIncrement().toString() - cohorts[id] = label - log.info("MockCohort: created cohort id={} label='{}'", id, label) - return id - } - - override fun addMember(externalUserId: String, externalCohortId: String) { - recordTransactionState() - memberships[externalUserId to externalCohortId] = "" - log.info("MockCohort: added member {} to cohort {}", externalUserId, externalCohortId) - } - - override fun removeMember(externalUserId: String, externalCohortId: String) { - recordTransactionState() - memberships.remove(externalUserId to externalCohortId) - log.info("MockCohort: removed member {} from cohort {}", externalUserId, externalCohortId) - } - - override fun deleteCohort(externalCohortId: String) { - recordTransactionState() - cohorts.remove(externalCohortId) - memberships.keys.removeIf { (_, cohortId) -> cohortId == externalCohortId } - log.info("MockCohort: deleted cohort {}", externalCohortId) - } - - override fun listMembers(externalCohortId: String): List { - recordTransactionState() - return memberships.keys - .filter { (_, cohortId) -> cohortId == externalCohortId } - .map { (userId, _) -> MemberRef(externalUserId = userId, label = memberships[userId to externalCohortId]?.ifEmpty { null }) } - } - - /** Directly seeds a member with an optional email label for drift tests. */ - fun seedMember(externalUserId: String, externalCohortId: String, label: String? = null) { - memberships[externalUserId to externalCohortId] = label.orEmpty() - } - - fun getMembers(externalCohortId: String): Set = - memberships.keys.filter { (_, cId) -> cId == externalCohortId }.map { (uId, _) -> uId }.toSet() - - fun clear() { - cohorts.clear() - memberships.clear() - transactionActiveDuringCalls.clear() - } - - companion object { - private val log = LoggerFactory.getLogger(MockCohortPort::class.java) - } -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt index b5a3f79cb..763d3f82f 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt @@ -10,9 +10,17 @@ import net.blueshell.api.shared.enums.TargetSystem import org.springframework.context.annotation.Primary import org.springframework.context.annotation.Profile import org.springframework.stereotype.Service +import org.springframework.transaction.support.TransactionSynchronizationManager import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicLong +/** + * In-memory [TargetStrategy] for the test and dev profiles: the registry, the remediation + * services and the controller ITs get a real target to exercise without touching Brevo. + * + * Reported system is [TargetSystem.BREVO] so those paths run as they do in production. + */ @Service @Primary @Profile("test | dev") @@ -37,12 +45,21 @@ class MockTargetStrategy : TargetStrategy { private val members = ConcurrentHashMap, String>() private val ids = AtomicLong(9000) - override fun catalog(query: String?): List = - targets.values.filter { it.matches(query.orEmpty().trim().lowercase()) } + /** + * `isActualTransactionActive()` at the entry of every provider call, so a boundary test can + * assert these run outside any DB transaction (the no-provider-call-inside-a-transaction rule). + */ + val transactionActiveDuringCalls: MutableList = CopyOnWriteArrayList() + + override fun catalog(query: String?): List { + recordTransactionState() + return targets.values.filter { it.matches(query.orEmpty().trim().lowercase()) } + } override fun resolve(externalId: String): ExternalTarget? = targets[externalId] override fun create(label: String, folder: String?): ExternalTarget { + recordTransactionState() val target = ExternalTarget( system, ids.getAndIncrement().toString(), @@ -55,24 +72,44 @@ class MockTargetStrategy : TargetStrategy { return target } - override fun members(target: ExternalTarget): List = - members.entries + override fun members(target: ExternalTarget): List { + recordTransactionState() + return members.entries .filter { it.key.second == target.externalId } .map { ExternalMember(it.key.first, it.value.ifBlank { null }) } + } override fun add(target: ExternalTarget, externalUserId: String) { + recordTransactionState() members[externalUserId to target.externalId] = "" } override fun remove(target: ExternalTarget, externalUserId: String) { + recordTransactionState() members.remove(externalUserId to target.externalId) } override fun delete(target: ExternalTarget) { + recordTransactionState() targets.remove(target.externalId) members.keys.removeIf { it.second == target.externalId } } + /** Directly seeds a member with an optional label, for the drift tests. */ + fun seedMember(externalUserId: String, externalTargetId: String, label: String? = null) { + members[externalUserId to externalTargetId] = label.orEmpty() + } + + fun clear() { + targets.clear() + members.clear() + transactionActiveDuringCalls.clear() + } + + private fun recordTransactionState() { + transactionActiveDuringCalls += TransactionSynchronizationManager.isActualTransactionActive() + } + private fun ExternalTarget.matches(query: String): Boolean = query.isBlank() || externalId == query || diff --git a/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt b/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt index 3aff97efd..6ca714280 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt @@ -16,9 +16,9 @@ import org.junit.jupiter.api.Test * [VENDOR_OWNERS] states. Modulith polices `net.blueshell.api` and stops at the module boundary, * so nothing but this rule looks at a third-party import. * - * The reaches that exist are pinned in [PINNED] rather than fixed here, the way - * [CrossModuleWebAccessArchitectureTest] pins its own. Each is a line in the file, so dropping one - * is a visible diff. Pinned at six reaches, all made by one class. + * A reach that is being cleaned up separately is pinned in [PINNED] rather than fixed here, the + * way [CrossModuleWebAccessArchitectureTest] pins its own. Each is a line in the file, so dropping + * one is a visible diff. Nothing is pinned: every module speaks to its vendors through a wrapper. */ class VendorClientArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { @@ -35,21 +35,10 @@ class VendorClientArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT ) /** - * Vendor reaches from outside the owning module that existed when this rule landed, as - * ` -> `. + * Vendor reaches from outside the owning module that are being cleaned up separately, as + * ` -> `. Empty, and a new entry needs a reason next to it. */ - val PINNED = setOf( - // DEBT. BrevoTargetStrategy drives ContactsApi for the list catalog and folder names - // while pushing membership through contact's ContactListAdapter — one integration - // behind two ports, one of them raw. Removing these means publishing the catalog side - // through contact :: api so cohort speaks only to the wrapper. Tracked separately. - "cohort -> net.blueshell.clients.brevo.api.ContactsApi", - "cohort -> net.blueshell.clients.brevo.model.GetContactsSortParameter", - "cohort -> net.blueshell.clients.brevo.model.GetFolder", - "cohort -> net.blueshell.clients.brevo.model.GetFolders200Response", - "cohort -> net.blueshell.clients.brevo.model.GetLists200Response", - "cohort -> net.blueshell.clients.brevo.model.GetLists200ResponseListsInner", - ) + val PINNED = emptySet() } @Test diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt deleted file mode 100644 index 3f8c2c3c5..000000000 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt +++ /dev/null @@ -1,130 +0,0 @@ -package net.blueshell.api.cohort.domain - -import io.mockk.every -import io.mockk.mockk -import io.mockk.verify -import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.enums.ContactSystem -import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy -import org.junit.jupiter.api.Test - -class BrevoCohortAdapterTest { - - private val brevoLegacy: ContactListAdapter = mockk { - every { system } returns ContactSystem.BREVO - } - private val adapter = BrevoCohortAdapter(listOf(brevoLegacy)) - - @Test - fun `system is TargetSystem BREVO`() { - assertThat(adapter.system).isEqualTo(TargetSystem.BREVO) - } - - @Test - fun `createCohort delegates to the legacy adapter and stringifies the returned id`() { - every { brevoLegacy.createList("Members", "contributionPeriods") } returns 42L - - val externalId = adapter.createCohort("Members", "contributionPeriods") - - assertThat(externalId).isEqualTo("42") - } - - @Test - fun `createCohort passes a null hint through to the delegate`() { - every { brevoLegacy.createList("Members", null) } returns 7L - - val externalId = adapter.createCohort("Members") - - assertThat(externalId).isEqualTo("7") - verify { brevoLegacy.createList("Members", null) } - } - - @Test - fun `addMember parses string ids and delegates to addToList`() { - every { brevoLegacy.addToList(123L, 456L) } returns Unit - - adapter.addMember("123", "456") - - verify { brevoLegacy.addToList(123L, 456L) } - } - - @Test - fun `removeMember parses string ids and delegates to removeFromList`() { - every { brevoLegacy.removeFromList(123L, 456L) } returns Unit - - adapter.removeMember("123", "456") - - verify { brevoLegacy.removeFromList(123L, 456L) } - } - - @Test - fun `deleteCohort delegates to deleteList`() { - every { brevoLegacy.deleteList(456L) } returns Unit - - adapter.deleteCohort("456") - - verify { brevoLegacy.deleteList(456L) } - } - - @Test - fun `addMember throws InvalidExternalIdException when externalUserId is malformed`() { - assertThatThrownBy { adapter.addMember("not-a-number", "456") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("not-a-number") - .hasMessageContaining("externalUserId") - .hasMessageContaining("addMember") - } - - @Test - fun `addMember throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.addMember("123", "bad-cohort") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("bad-cohort") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("addMember") - } - - @Test - fun `removeMember throws InvalidExternalIdException when externalUserId is malformed`() { - assertThatThrownBy { adapter.removeMember("oops", "456") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("oops") - .hasMessageContaining("externalUserId") - .hasMessageContaining("removeMember") - } - - @Test - fun `removeMember throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.removeMember("123", "oops") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("oops") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("removeMember") - } - - @Test - fun `deleteCohort throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.deleteCohort("not-a-long") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("not-a-long") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("deleteCohort") - } - - @Test - fun `listMembers throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.listMembers("xyz") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("xyz") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("listMembers") - } -} diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt index 18ede6dac..b637223e4 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt @@ -1,57 +1,34 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.contact.api.ContactServiceException +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.shared.enums.ContactSystem -import net.blueshell.clients.brevo.api.ContactsApi -import net.blueshell.clients.brevo.model.GetFolder -import net.blueshell.clients.brevo.model.GetFolders200Response -import net.blueshell.clients.brevo.model.GetLists200Response -import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner -import net.blueshell.clients.brevo.model.GetContactsSortParameter import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test -import org.mockito.kotlin.doThrow -import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.whenever -import org.springframework.web.client.RestClientResponseException class BrevoTargetStrategyTest { - private val contactsApi: ContactsApi = mock() private val lists: ContactListAdapter = mock { whenever(it.system).thenReturn(ContactSystem.BREVO) } - private val strategy = BrevoTargetStrategy(listOf(lists), contactsApi) + private val strategy = BrevoTargetStrategy(listOf(lists)) @Test - fun `pages folders and lists and maps folder names and counts`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 51L, folders = (1L..50L).map { folder(it, "Folder $it") })) - whenever(contactsApi.getFolders(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 51L, folders = listOf(folder(51L, "Contribution periods")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 51L, lists = (1L..50L).map { list(it, "List $it", 1L) })) - whenever(contactsApi.getLists(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 51L, lists = listOf(list(99L, "Paid 2026", 51L, 728L)))) + fun `maps folder names and counts onto the catalog`() { + whenever(lists.listFolders()).thenReturn(mapOf(51L to "Contribution periods")) + whenever(lists.listAll()).thenReturn(listOf(list(99L, "Paid 2026", 51L, 728L))) - val targets = strategy.catalog(null) - val paid = targets.single { it.externalId == "99" } + val paid = strategy.catalog(null).single { it.externalId == "99" } - assertThat(targets).hasSize(51) assertThat(paid.folderLabel).isEqualTo("Contribution periods") assertThat(paid.memberCount).isEqualTo(728L) } @Test fun `says where each list sits, outside in`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 1L, folders = listOf(folder(1L, "Committees")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn( - GetLists200Response(count = 2L, lists = listOf(list(10L, "Web Cmte", 1L), list(11L, "Loose ends", 404L))), - ) + whenever(lists.listFolders()).thenReturn(mapOf(1L to "Committees")) + whenever(lists.listAll()).thenReturn(listOf(list(10L, "Web Cmte", 1L), list(11L, "Loose ends", 404L))) val targets = strategy.catalog(null).associateBy { it.externalId } @@ -64,52 +41,14 @@ class BrevoTargetStrategyTest { @Test fun `filters by query after fetching the bounded catalog`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 1L, folders = listOf(folder(1L, "Members")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 2L, lists = listOf(list(10L, "Guests", 1L), list(11L, "Paid", 1L)))) + whenever(lists.listFolders()).thenReturn(mapOf(1L to "Members")) + whenever(lists.listAll()).thenReturn(listOf(list(10L, "Guests", 1L), list(11L, "Paid", 1L))) val targets = strategy.catalog("paid") assertThat(targets).extracting { it.externalId }.containsExactly("11") } - @Test - fun `treats Brevo rate limiting as retryable`() { - doThrow(error(429)).whenever(contactsApi) - .getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC)) - - assertThatThrownBy { strategy.catalog(null) } - .isInstanceOf(ContactServiceException::class.java) - .hasCauseInstanceOf(RestClientResponseException::class.java) - } - - // The generated models are immutable data classes, so the counts Brevo - // always returns have to be supplied even where the assertions ignore them. - private fun folder(id: Long, name: String): GetFolder = - GetFolder( - id = id, - name = name, - totalBlacklisted = 0L, - totalSubscribers = 0L, - uniqueSubscribers = 0L, - ) - - private fun list( - id: Long, - name: String, - folderId: Long, - unique: Long = 10L + id, - ): GetLists200ResponseListsInner = - GetLists200ResponseListsInner( - id = id, - name = name, - folderId = folderId, - uniqueSubscribers = unique, - totalBlacklisted = 0L, - totalSubscribers = unique, - ) - - private fun error(status: Int): RestClientResponseException = - RestClientResponseException("$status error", status, "error", null, ByteArray(0), null) + private fun list(id: Long, name: String, folderId: Long, unique: Long = 10L + id): ContactListRef = + ContactListRef(externalListId = id, name = name, folderId = folderId, memberCount = unique) } diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt index 8a8d73598..ccded9fcf 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt @@ -20,8 +20,10 @@ class CohortMembershipSyncServiceTest { private val cohorts: CohortRepository = mockk() private val ledger: CohortLedger = mockk(relaxed = true) - private val brevoPort: CohortPort = mockk(relaxed = true) { + private val target42 = ExternalTarget(TargetSystem.BREVO, "42", CohortKind.LIST, "42") + private val brevoTarget: TargetStrategy = mockk(relaxed = true) { every { system } returns TargetSystem.BREVO + every { handle("42") } returns target42 } private val externalIds: ExternalIdMappingService = mockk(relaxed = true) private val targetIds: CohortTargetIds = mockk(relaxed = true) @@ -29,7 +31,7 @@ class CohortMembershipSyncServiceTest { private val service = CohortMembershipSyncService( cohorts = cohorts, ledger = ledger, - registry = CohortPortRegistry(listOf(brevoPort)), + strategies = TargetStrategies(listOf(brevoTarget)), externalIds = externalIds, targetIds = targetIds, jobs = jobs, @@ -44,14 +46,14 @@ class CohortMembershipSyncServiceTest { } @Test - fun `ADD calls port when both external ids exist`() { + fun `ADD calls the strategy when both external ids exist`() { givenCohort(id = 10L, system = "BREVO", label = "Members") every { externalIds.find("USER", 1L, "BREVO") } returns mapping("USER", 1L, "BREVO", "777") every { targetIds.find(any()) } returns "42" service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) - verify { brevoPort.addMember("777", "42") } + verify { brevoTarget.add(target42, "777") } } @Test @@ -68,8 +70,8 @@ class CohortMembershipSyncServiceTest { verify(exactly = 0) { jobs.runAsync(CohortJobs.MaterializeCohortTarget, any()) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } - verify(exactly = 0) { brevoPort.createCohort(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } + verify(exactly = 0) { brevoTarget.create(any(), any()) } } @Test @@ -80,7 +82,7 @@ class CohortMembershipSyncServiceTest { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) - verify { brevoPort.addMember("777", "42") } + verify { brevoTarget.add(target42, "777") } verify { ledger.markPushed(10L, 1L, "777", any()) } } @@ -96,18 +98,18 @@ class CohortMembershipSyncServiceTest { verify { jobs.runAsync(ContactJobs.SyncContact, ContactJobs.SyncContactPayload(1L)) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } } @Test - fun `REMOVE calls port when both external ids exist`() { + fun `REMOVE calls the strategy when both external ids exist`() { givenCohort(id = 10L, system = "BREVO", label = "Members") every { externalIds.find("USER", 1L, "BREVO") } returns mapping("USER", 1L, "BREVO", "777") every { targetIds.find(any()) } returns "42" service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.REMOVE) - verify { brevoPort.removeMember("777", "42") } + verify { brevoTarget.remove(target42, "777") } } @Test @@ -118,8 +120,8 @@ class CohortMembershipSyncServiceTest { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.REMOVE) - verify(exactly = 0) { brevoPort.removeMember(any(), any()) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } + verify(exactly = 0) { brevoTarget.remove(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } } @Test @@ -141,15 +143,15 @@ class CohortMembershipSyncServiceTest { } @Test - fun `cohort whose system has no registered port throws NonRetryableJobException`() { - // Cohort's system is a valid TargetSystem value but no matching CohortPort bean exists - // (GOOGLE_CALENDAR has none yet). + fun `cohort whose system has no registered strategy throws NonRetryableJobException`() { + // Cohort's system is a valid TargetSystem value but no matching TargetStrategy bean + // exists (GOOGLE_CALENDAR has none yet). givenCohort(id = 10L, system = "GOOGLE_CALENDAR", label = "events") assertThatThrownBy { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) }.isInstanceOf(NonRetryableJobException::class.java) - .hasMessageContaining("No CohortPort") + .hasMessageContaining("No TargetStrategy") } private fun givenCohort(id: Long, system: String, label: String) { diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt index 656184f3e..76077504a 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt @@ -33,7 +33,7 @@ class CohortRemediationServiceTest { private val externalIds: ExternalIdMappingService = mockk() private val targetIds: CohortTargetIds = mockk() private val jobs: TrackedJobDispatcher = mockk(relaxed = true) - private val port = RecordingCohortPort() + private val port = RecordingTargetStrategy() private val service = CohortRemediationService( cohortRepo = cohorts, subjectRepo = subjects, @@ -41,7 +41,7 @@ class CohortRemediationServiceTest { ledger = CohortLedger(members), externalIds = externalIds, targetIds = targetIds, - registry = CohortPortRegistry(listOf(port)), + strategies = TargetStrategies(listOf(port)), jobs = jobs, transactionManager = ImmediateTransactionManager(), ) @@ -80,8 +80,8 @@ class CohortRemediationServiceTest { verifiedAt = LocalDateTime.parse("2026-01-03T12:00:00"), ) port.remote = listOf( - MemberRef("ext-1", "Alice Remote"), - MemberRef("ext-extra", "Extra Remote"), + ExternalMember("ext-1", "Alice Remote"), + ExternalMember("ext-extra", "Extra Remote"), ) every { cohorts.findById(99L) } returns Optional.of(cohort) @@ -270,24 +270,35 @@ class CohortRemediationServiceTest { label = label, ) - private class RecordingCohortPort : CohortPort { - override val system: TargetSystem = TargetSystem.BREVO - var remote: List = emptyList() + private class RecordingTargetStrategy : TargetStrategy { + override val descriptor = TargetDescriptor( + system = TargetSystem.BREVO, + kind = CohortKind.LIST, + systemLabel = "Brevo", + targetLabel = "Brevo list", + idLabel = "List id", + capabilities = setOf( + TargetCapability.READ_MEMBERS, + TargetCapability.WRITE_MEMBERS, + TargetCapability.DELETE, + ), + ) + var remote: List = emptyList() var listCalls = 0 var lastExternalCohortId: String? = null var sawTransactionDuringList = false val removeCalls = mutableListOf>() - override fun createCohort(label: String, hint: String?): String = error("not used") - override fun addMember(externalUserId: String, externalCohortId: String) = Unit - override fun removeMember(externalUserId: String, externalCohortId: String) { - removeCalls += externalUserId to externalCohortId + override fun create(label: String, folder: String?): ExternalTarget = error("not used") + override fun add(target: ExternalTarget, externalUserId: String) = Unit + override fun remove(target: ExternalTarget, externalUserId: String) { + removeCalls += externalUserId to target.externalId } - override fun deleteCohort(externalCohortId: String) = Unit + override fun delete(target: ExternalTarget) = Unit - override fun listMembers(externalCohortId: String): List { + override fun members(target: ExternalTarget): List { listCalls += 1 - lastExternalCohortId = externalCohortId + lastExternalCohortId = target.externalId sawTransactionDuringList = TransactionSynchronizationManager.isActualTransactionActive() return remote } diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt index f71861621..2c2a2dda9 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt @@ -7,7 +7,6 @@ import net.blueshell.api.contact.api.ContactListAdapter import net.blueshell.api.shared.enums.ContactSystem import net.blueshell.api.cohort.persistence.CohortKind import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.clients.brevo.api.ContactsApi import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test @@ -22,8 +21,7 @@ class TargetFolderMoveTest { private val lists = mockk(relaxed = true) { every { system } returns ContactSystem.BREVO } - private val contactsApi = mockk(relaxed = true) - private val strategy = BrevoTargetStrategy(listOf(lists), contactsApi) + private val strategy = BrevoTargetStrategy(listOf(lists)) private val target = ExternalTarget( system = TargetSystem.BREVO, diff --git a/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt b/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt index 6cfff9bee..c5808070c 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt @@ -1,10 +1,16 @@ package net.blueshell.api.contact.domain +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.contact.api.ContactServiceException import net.blueshell.clients.brevo.api.ContactsApi import net.blueshell.clients.brevo.model.AddContactToListRequest import net.blueshell.clients.brevo.model.GetContactInfo200Response import net.blueshell.clients.brevo.model.GetContactInfo200ResponseAllOfStatistics +import net.blueshell.clients.brevo.model.GetContactsSortParameter +import net.blueshell.clients.brevo.model.GetFolder +import net.blueshell.clients.brevo.model.GetFolders200Response +import net.blueshell.clients.brevo.model.GetLists200Response +import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner import net.blueshell.clients.brevo.model.RemoveContactFromListRequest import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatThrownBy @@ -82,6 +88,47 @@ class BrevoListAdapterTest { adapter.removeFromList(externalUserId = 100L, externalListId = 200L) } + @Test + fun `pages folders and lists past the first page`() { + whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetFolders200Response(count = 51L, folders = (1L..50L).map { folder(it, "Folder $it") })) + whenever(contactsApi.getFolders(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetFolders200Response(count = 51L, folders = listOf(folder(51L, "Contribution periods")))) + whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetLists200Response(count = 51L, lists = (1L..50L).map { list(it, "List $it", 1L) })) + whenever(contactsApi.getLists(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetLists200Response(count = 51L, lists = listOf(list(99L, "Paid 2026", 51L, 728L)))) + + assertThat(adapter.listFolders()).hasSize(51).containsEntry(51L, "Contribution periods") + assertThat(adapter.listAll()).hasSize(51) + .contains(ContactListRef(externalListId = 99L, name = "Paid 2026", folderId = 51L, memberCount = 728L)) + } + + @Test + fun `surfaces a rate-limited catalog page as a retryable failure`() { + doThrow(error(429, "")).whenever(contactsApi) + .getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC)) + + assertThatThrownBy { adapter.listFolders() } + .isInstanceOf(ContactServiceException::class.java) + .hasCauseInstanceOf(RestClientResponseException::class.java) + } + + // The generated models are immutable data classes, so the counts Brevo always returns have + // to be supplied even where the assertions ignore them. + private fun folder(id: Long, name: String): GetFolder = + GetFolder(id = id, name = name, totalBlacklisted = 0L, totalSubscribers = 0L, uniqueSubscribers = 0L) + + private fun list(id: Long, name: String, folderId: Long, unique: Long = 10L + id): GetLists200ResponseListsInner = + GetLists200ResponseListsInner( + id = id, + name = name, + folderId = folderId, + uniqueSubscribers = unique, + totalBlacklisted = 0L, + totalSubscribers = unique, + ) + private fun alreadyInListOrMissing(): RestClientResponseException = error( 400, """{"code":"invalid_parameter","message":"Contact already in list and/or does not exist"}""",