test(architecture): a rule that selects nothing fails instead of passing - #1165
Merged
Conversation
ArchitecturePackages declared 32 globs that matched no class after the ADR-003 flattening, and thirteen of them were still read by rules, on the far side where ArchUnit never looks. Repoint what still means something, delete what the flattening or a stronger sibling made redundant, and assert every constant matches at least one class. Closes #1159.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1159.
Why
ArchitecturePackagesdeclared 51 globs. Measured with ArchUnit's own matcher against theimported classes, 32 matched nothing, and thirteen of those were still read by rules.
The issue's table was measured by expanding the globs by hand and overcounts the damage in one
direction while missing where it actually sits.
..matches zero packages as well as many, sonet.blueshell.api..web..does matchnet.blueshell.api.user.web—WEB,PERSISTENCE,DOMAINand
DTOare live, not dead, and the four rule files the table blames them for were mostly fine.The dead ones are the
applicationlayer, thedomain.model/domain.servicesplit,persistence.repository,web.dtoand the wholedomain.<feature>prefix.Where the dead globs sat matters more than how many there were. ArchUnit already fails a rule
whose selection is empty —
failOnEmptyShouldis on by default, which is why several rules hereexplicitly opt out with
allowEmptyShould(true). Almost none of these thirteen were on that side.They sat in
dependOnClassesThat, inonlyBeAccessed().byAnyPackage, in aresideOutsideOfPackagesexemption — positions where an empty match makes the rule permissiveand ArchUnit is structurally unable to notice.
persistence must not depend on application layerbuilt its entire target predicate out of five dead globs and passed.
What this achieves
A controller taking a repository, an input type without
@Schema, aQueryfiled in persistence— all three now fail, and none of them did. A glob that stops matching fails the build with the
constant's name in the message rather than turning its rules green.
How
Rule by rule. Five rules are gone, four are repointed, three had dead entries pruned from a list
that was quietly widening them.
dto only accessed at api boundarycontrollers do not access repositories directlypersistence must not depend on application layerweb DTOs must not be entities(ApiBoundary)controllers must not depend on Spring Data repositoriesrepositories do not depend on servicesapplication layer does not depend on controllersSERVICE_LAYER+ persistenceresponse DTOs are decorated with Schema<module>/web+ name suffixrequest DTOs are decorated with Schema<module>/web+ name suffixdtos must not be entities/must not be Spring components<module>/webquery objects in application layer not persistencerepository only accessed by.../jobs only accessed from...transactional annotations in application layer onlyThe three deletions worth arguing about:
persistence must not depend on application layercannot be restated. It split a module'sapplicationpackage into query objects, which persistence may use under ADR-015, and everythingelse, which it may not. The flattening put both in
<module>/domain. There is no package boundaryleft that draws that line, and inventing one would be a rule nobody asked for. Its nameable half —
persistence must not touch a
*Service— isrepositories do not depend on services, repointedand widened from repositories to the whole
persistencefolder so nothing is lost.Two controller-to-repository rules, one in each of
AccessArchitectureTestandApiBoundaryArchitectureTest, are both retired. I repointed theApiBoundaryone first, plantedthe probe, and it fired — but so did
DataOwnershipArchitectureTest'sweb validators should use services not repositories, which was already live against today's packages and is strictlystronger: every class in a
webpackage, not just the*Controllerones. Keeping a repointedthird copy would have been a rule kept alive for its own sake.
dto only accessed at api boundaryguardedweb/dto, a folder the flattening emptied.Input and response types sit directly in
<module>/web, whereapplication services do not depend on DTOscovers the inside of a module andCrossModuleWebAccessArchitectureTestcovers the outside.Every deletion is recorded in the class header next to the two #1158 already retired, so the file
says why a rule is absent rather than leaving a gap.
PackageConstantsArchitectureTestis the mechanism. It reflects over every constant — arrayscontribute one entry per element — and fails naming any glob that matches no class.
The trade-off.
allowEmptyShould(false)is the direct answer the issue suggests, and it isalready the default; it is not enough, because it only sees the classes a rule selects and these
globs were mostly on the target side. Nor can it simply be forced everywhere: two rules here are
honestly "no class does X" and would become false positives. So the two mechanisms are kept
together. The constant check is the wider net, and I removed the four
allowEmptyShould(true)opt-outs that repointing made unnecessary, which puts ArchUnit's own guard back on those rules.
What the constant check does not catch is a constant that still matches but no longer means what
its readers think —
DTOmatched three platform error DTOs while every module'sweb/dtohadbeen emptied. That one needs a human, and it is why
DTOandDOMAINare deleted here despitematching: they name the pre-flattening shape and had no reader left.
Not in scope
The 32 dead constants are deleted; live-but-unused ones (
SHARED_MODEL,SHARED_SECURITY) stay,since they name today's packages and are the vocabulary a new rule gets written in.
WEBandMODULE_WEBnow match the same 267 classes, as doPERSISTENCEandMODULE_PERSISTENCE;collapsing each pair is a separate tidy.
Worth a reviewer's attention
Repointing
request DTOs are decorated with Schemafound four real violations, so this PRtouches one production file:
CohortSubjectController's four*Requesttypes had no@Schemawhile all six of its
*Responsesiblings did. That is the defect this issue describes doing itsjob on the first run — the rule had been vacuous since the flattening, so nothing objected. Adding
the annotations is the smallest honest way to land the repoint; the alternative was to keep
tolerating an empty selection.
dtos must not be Spring componentspicks its subjects by name (*Request/*Responsein<module>/web) rather than by package, because awebpackage legitimately holds mappers andargument resolvers that are beans.
dtos must not be entitiesdid not need that and selects thewhole
webpackage, which is stronger.repositories do not depend on serviceslost its*Repositoryname filter and now coversentities and specifications too. It passes today, but it is a genuinely wider rule than the one it
replaces, not just a repointed one.
Verification
./gradlew :services:api:test --tests '*ArchitectureTest*' --tests '*ArchTest*'— exit 0, 67passed (71 before, minus 5 deleted rules, plus 1 new).
./gradlew :services:api:test— exit 0, 912 passed.The guard was watched failing. A
PROBE = "$ROOT..application.."constant was added:A repointed rule was watched failing. A
ProbeControllertaking aBlogRepository, with abare
ProbeRequestbeside it, was planted innet.blueshell.api.blog.web:The second message is what settled the controller-repository question:
DataOwnershipArchitectureTestreported the same two violations in the same run, which is what made the repointed rule redundant
rather than useful. Both probes were removed; the runs above are with them gone.
Diff breakdown —
█added░removed, scaled to the largest row.