From 59c83834a6ffaad735937e48f64d97928e59206f Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers <74975850+ExtraToast@users.noreply.github.com> Date: Sun, 6 Sep 2026 00:12:20 +0200 Subject: [PATCH] test(architecture): the rules assert against packages that exist Three constants named `net.blueshell.api.infrastructure`, removed by the architecture ADR-003 flattening, so every rule built on them matched nothing. SECURITY and PERMISSION now name `security` and `security.permission`, which is where ADR-003 puts them; INFRASTRUCTURE has no successor package and its two dependents are answered without it. Adds the missing vendor-import rule: only the module that wraps a generated client under `net.blueshell.clients` may import it. The one existing reach, cohort's BrevoTargetStrategy, is pinned in a decaying allowlist. --- .../architecture/AccessArchitectureTest.kt | 40 +------ .../api/architecture/ArchitecturePackages.kt | 12 +- .../SpringPracticesArchitectureTest.kt | 9 +- .../VendorClientArchitectureTest.kt | 111 ++++++++++++++++++ 4 files changed, 127 insertions(+), 45 deletions(-) create mode 100644 services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt diff --git a/services/api/src/test/kotlin/net/blueshell/api/architecture/AccessArchitectureTest.kt b/services/api/src/test/kotlin/net/blueshell/api/architecture/AccessArchitectureTest.kt index 825a1b043..945d7a150 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/architecture/AccessArchitectureTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/AccessArchitectureTest.kt @@ -9,7 +9,6 @@ import com.tngtech.archunit.lang.SimpleConditionEvent import com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes import com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods import com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses -import com.tngtech.archunit.library.dependencies.SlicesRuleDefinition.slices import net.blueshell.api.architecture.support.ArchJUnitTestBase import org.junit.jupiter.api.Test import org.springframework.security.access.prepost.PreAuthorize @@ -24,6 +23,11 @@ import org.springframework.security.access.prepost.PreAuthorize * the auth module ever had, and architecture ADR-003 gives every module one domain folder * holding both. Its cross-module half is what module verification now checks; what remains * intra-module is a single folder with no layer boundary left to cross. + * + * `only the web layer reaches a domain module's web package` went the same way. It selected + * `net.blueshell.api.domain..web..`, a grouping level the flattening removed, and + * [CrossModuleWebAccessArchitectureTest] now states the stronger rule against today's packages: + * no module reaches another module's web package at all, whatever layer the reach comes from. */ class AccessArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { @@ -155,21 +159,6 @@ class AccessArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { .because("ADR-016: Persistence can depend on query objects (ADR-015), but not services/handlers/validators") } - @Test - fun `only the web layer reaches a domain module's web package`(): Unit = - arch("Domain web packages are reached from the web layer only") { - noClasses() - .that().resideInAnyPackage( - ArchitecturePackages.APPLICATION, - ArchitecturePackages.INFRASTRUCTURE, - ArchitecturePackages.PLATFORM - ) - // The OpenAPI schema customizer documents a response type, so it names one. - .and(DescribedPredicate.not(openApiConfiguration)) - .should().dependOnClassesThat().resideInAnyPackage(ArchitecturePackages.DOMAIN_WEB) - .because("ADR-016: controllers, request/response types and their mappers serve one endpoint; inner layers work with entities and commands") - } - @Test fun `repositories do not depend on DTOs`(): Unit = arch("Repositories must not depend on DTOs") { @@ -180,20 +169,6 @@ class AccessArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { .because("ADR-016: Persistence layer should not know about web DTOs") } - // NOTE: Cyclic dependency test disabled - known exception exists - // shared → domain.user.persistence.User for audit fields is documented in ADR-016 as acceptable - // The cycle is: domain → shared (normal) + shared → domain.user.persistence.User (for audit) - // This is a conscious architectural trade-off for audit field convenience - // TODO: Consider adding test with ignoreDependency() when ArchUnit API is clearer - // @Test - // fun `no cyclic dependencies by top level package`(): Unit = - // arch("No cyclic dependencies between top-level packages") { - // slices() - // .matching("${ArchitecturePackages.ROOT}.(*)..") - // .should().beFreeOfCycles() - // .because("Cyclic dependencies make refactoring risky and coupling invisible") - // } - @Test fun `configuration does not depend on web controllers`(): Unit = arch("Configuration must not depend on controllers") { @@ -288,10 +263,5 @@ class AccessArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { ) ) .`as`("a domain module's application package other than its query objects") - - val openApiConfiguration: DescribedPredicate = - JavaClass.Predicates.resideInAnyPackage(ArchitecturePackages.PLATFORM_CONFIG) - .and(JavaClass.Predicates.simpleNameContaining("OpenApi")) - .`as`("OpenAPI configuration") } } diff --git a/services/api/src/test/kotlin/net/blueshell/api/architecture/ArchitecturePackages.kt b/services/api/src/test/kotlin/net/blueshell/api/architecture/ArchitecturePackages.kt index fb5fe8dd2..1d43fa657 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/architecture/ArchitecturePackages.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/ArchitecturePackages.kt @@ -48,10 +48,14 @@ object ArchitecturePackages { const val REPOSITORY = "$ROOT..persistence.repository.." const val SPECIFICATION = "$ROOT..persistence.spec.." - /** Infrastructure - Cross-cutting concerns */ - const val INFRASTRUCTURE = "$ROOT.infrastructure.." - const val SECURITY = "$ROOT.infrastructure.security.." - const val PERMISSION = "$ROOT.infrastructure.security.permission.." + /** + * Cross-cutting security. Architecture ADR-003 makes this a top-level module of its own; + * the `infrastructure` package these used to name was removed by the flattening. + */ + const val SECURITY = "$ROOT.security.." + + /** ADR-007: only the base and composite evaluator stay here, never a `*Permission`. */ + const val PERMISSION = "$ROOT.security.permission.." /** Platform - Integration with external systems */ const val PLATFORM = "$ROOT.platform.." diff --git a/services/api/src/test/kotlin/net/blueshell/api/architecture/SpringPracticesArchitectureTest.kt b/services/api/src/test/kotlin/net/blueshell/api/architecture/SpringPracticesArchitectureTest.kt index 57661ce47..8dc058ddb 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/architecture/SpringPracticesArchitectureTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/SpringPracticesArchitectureTest.kt @@ -15,13 +15,10 @@ class SpringPracticesArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.R @Test fun `no field injection in application code`(): Unit = arch("No @Autowired field injection - use constructor injection") { + // The old layer list named packages the ADR-003 flattening removed, so it selected + // a fraction of the code; constructor injection is wanted everywhere regardless. noFields() - .that().areDeclaredInClassesThat().resideInAnyPackage( - ArchitecturePackages.WEB, - ArchitecturePackages.APPLICATION, - ArchitecturePackages.INFRASTRUCTURE, - ArchitecturePackages.PLATFORM - ) + .that().areDeclaredInClassesThat().resideInAnyPackage("${ArchitecturePackages.ROOT}..") .should().beAnnotatedWith(Autowired::class.java) .because("Constructor injection is preferred: immutable dependencies, simpler tests, clearer contracts") } 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 new file mode 100644 index 000000000..3aff97efd --- /dev/null +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt @@ -0,0 +1,111 @@ +package net.blueshell.api.architecture + +import net.blueshell.api.architecture.support.ArchJUnitTestBase +import net.blueshell.api.architecture.support.ArchModules +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test + +/** + * API ADR-019: an external model does not enter a domain. A generated vendor client under + * `net.blueshell.clients` is that external model in its rawest form, so exactly one module owns + * each vendor and wraps it behind an interface the rest of the application speaks. Anyone else + * calls that interface. + * + * The record still describes the vendor wrappers as living in `platform/integration/{system}`, + * packages the architecture ADR-003 flattening removed; ownership is by module now, which is what + * [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. + */ +class VendorClientArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { + + private companion object { + const val VENDOR_ROOT = "net.blueshell.clients" + + /** + * The module that owns each vendor client and wraps it. A vendor absent from this map has + * no owner, which is itself a violation — adding a client means deciding who wraps it. + */ + val VENDOR_OWNERS = mapOf( + "brevo" to "contact", + "discord" to "sync", + ) + + /** + * Vendor reaches from outside the owning module that existed when this rule landed, as + * ` -> `. + */ + 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", + ) + } + + @Test + fun `only the owning module imports a vendor client`() { + val offenders = measureReaches() + .filterKeys { it !in PINNED } + .flatMap { (reach, origins) -> origins.map { "$reach from $it" } } + .sorted() + + assertThat(offenders) + .describedAs( + "API ADR-019: a generated vendor client belongs to the module that wraps it. Call " + + "that module's published interface instead, or add the reach to PINNED if it " + + "is being cleaned up separately", + ) + .isEmpty() + } + + @Test + fun `no pinned reach outlives the code that made it`() { + val measured = measureReaches().keys + + val stale = PINNED.filterNot { it in measured }.sorted() + + assertThat(stale) + .describedAs( + "these reaches are gone — drop them from PINNED so the ratchet cannot slip back", + ) + .isEmpty() + } + + /** + * Every ` -> ` reach from outside the owning module, mapped to the classes + * that make it. A type directly under the base package belongs to no module and owns no vendor, + * so its reaches count too. + */ + private fun measureReaches(): Map> { + val reaches = mutableMapOf>() + + importedClasses.forEach { origin -> + val originModule = ArchModules.moduleOf(origin) + origin.directDependenciesFromSelf.forEach { dependency -> + val target = dependency.targetClass + val vendor = vendorOf(target.packageName) ?: return@forEach + if (VENDOR_OWNERS[vendor] == originModule) return@forEach + reaches.getOrPut("${originModule ?: ""} -> ${target.fullName}") { mutableSetOf() } + .add(origin.fullName) + } + } + + return reaches + } + + private fun vendorOf(packageName: String): String? = + packageName.removePrefix("$VENDOR_ROOT.") + .takeIf { packageName.startsWith("$VENDOR_ROOT.") } + ?.substringBefore('.') + ?.takeIf { it.isNotEmpty() } +}