TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.17k stars 287 forks source link

Why the onion architecture ArchUnit test detects one violation when there are none? #1111

Open Tristan107 opened 1 year ago

Tristan107 commented 1 year ago

Here is the formatted SO question : https://stackoverflow.com/questions/76252658/why-does-this-archunit-test-fails

Here is a raw copy :

I have a demo project which tries to respect strictly clean/onion/hexagonal architecture. Here is how I configure ArchUnit test :

    @AnalyzeClasses(packages ="fr.tristan.demoassurance", importOptions = {ImportOption.DoNotIncludeTests.class, ExcludeConfig.class})
public class ArchitectureTest {

    @ArchTest
    public static final ArchRule onionArchitectureRule = onionArchitecture()
        .domainModels("fr.tristan.demoassurance.domain.entity..")
        .domainModels("fr.tristan.demoassurance.domain.event..")
        .domainServices("fr.tristan.demoassurance.domain.api..")
        .applicationServices("fr.tristan.demoassurance.application.controller")
        .adapter("message", "fr.tristan.demoassurance.infrastructure.message..")
        .adapter("persistence-mongodb", "fr.tristan.demoassurance.infrastructure.repository.mongodb..")
        .adapter("persistence-mysql", "fr.tristan.demoassurance.infrastructure.repository.mysql..")
        ;
}

ArchUnit complains about one parameter :

was violated (1 times):
Method <fr.tristan.demoassurance.domain.spi.message.MessageQueueSpi.send(fr.tristan.demoassurance.domain.event.PoliceAssuranceEvent)> has parameter of type <fr.tristan.demoassurance.domain.event.PoliceAssuranceEvent> in (MessageQueueSpi.java:0)

Here is the "guilty" method :

package fr.tristan.demoassurance.domain.spi.message;

import fr.tristan.demoassurance.domain.event.PoliceAssuranceEvent;

public interface MessageQueueSpi {
    void send(PoliceAssuranceEvent policeAssuranceCreatedEvent);
}

This makes no sense for me because :

  1. I don't see which rule is violated, it's just not explained why a SPI interface should not have a parameter of a domain model type.

  2. I have an other interface in the same fr.tristan.demoassurance.domain.spi parent package with a savemethod which also have a parameter PoliceAssurance from "domain models" and ArchUnit doesn't complain about it :

package fr.tristan.demoassurance.domain.spi.repository;

import java.util.List;

import fr.tristan.demoassurance.domain.entity.PoliceAssurance;
import fr.tristan.demoassurance.domain.exception.PoliceAssuranceException;

public interface PoliceAssuranceRepositorySpi {

    List<PoliceAssurance> findAll();
    PoliceAssurance save(PoliceAssurance policeAssurance) throws PoliceAssuranceException;
    PoliceAssurance findById(Integer id) throws PoliceAssuranceException;
    void deleteById(Integer id) throws PoliceAssuranceException;    
}
marco-sanjuan commented 1 year ago

Same issue here, in my test, domain ports (interfaces) methods can not have domain object as parameter.

Am I missing something?

hankem commented 1 year ago

domainModels behaves like a setter, i.e. does not extend the internal domainModelPredicate (and likewiese for domainServices and applicationServices). That is, when calling the methods multiple times, previous package identifiers are simply overwritten.

The solution for @Tristan107 was to

  1. pass all domainModels in one method call, and
  2. add mappers and spi interfaces to applicationServices and domainServices, respectively.
     @ArchTest
     public static final ArchRule onionArchitectureRule = onionArchitecture()
    -        .domainModels("fr.tristan.demoassurance.domain.entity..")
    -        .domainModels("fr.tristan.demoassurance.domain.event..")
    -        .domainServices("fr.tristan.demoassurance.domain.api..")
    -        .applicationServices("fr.tristan.demoassurance.application.controller")
    +        .domainModels("fr.tristan.demoassurance.domain.entity..", "fr.tristan.demoassurance.domain.event..")
    +        .domainServices("fr.tristan.demoassurance.domain.api..", "fr.tristan.demoassurance.domain.spi..")
    +        .applicationServices("fr.tristan.demoassurance.application.controller..", "fr.tristan.demoassurance.application.mapper..")
         .adapter("message", "fr.tristan.demoassurance.infrastructure.message..")
         .adapter("persistence-mongodb", "fr.tristan.demoassurance.infrastructure.repository.mongodb..")
         .adapter("persistence-mysql", "fr.tristan.demoassurance.infrastructure.repository.mysql..")
hankem commented 1 year ago

A discussion was started in comments on StackOverflow (which is probably better continued here)

  1. whether the second invocation of those methods should log a warning (or even throw an exception, but that might break intentional usages 🤔), and
  2. whether service provider interfaces should have their own category (and not be "forced" to be declared as "domain services" or "adapters").
Tristan107 commented 1 year ago

Yes but like I said there should be more categories, like "serviceProviderInterfaces" to ensure this is actual onion architecture and not layered architecture with domain depending on persistence like in your example.