qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.64k stars 134 forks source link

Private collectors no longer work #1435

Closed ndench closed 1 month ago

ndench commented 3 months ago

We've just started using Deptrac to help reduce architectural complexity and I think it's a really excellent idea!

Our platform has a lot of different modules all depending on each other and we want to use Deptrac to enforce that modules only depend on "Public" aspects of other modules. This allows each module to keep their internal workings private.

The private: true collector would allow us to do that really easily, but it seems to have stopped working.

We can see in the PR that added private collectors the dependency would be allowed if:

!$isPublic && $dependerLayer !== $dependentLayer

However, this logic seems to have been changed when the violations were improved, now it will allow the dependency if:

$event->dependerLayer === $dependentLayer && !$isPublic

I'm more than happy to do a PR to resolve this issue unless there's a reason this change was made that I'm unaware of?

Reproducer

I've created a simple reproducer to illustrate this: https://github.com/ndench/deptrac-private-layer-bug.

It contains 2 Deptrac configuration files:

  1. deptrac-private.yaml
    • Configuration using the private parameter
  2. deptrac-public.yaml
    • Alternative configuration which achives the desired result without using the private parameter
    • This configuration becomes very hard to maintain when the number of layers increases

I'm expecting both of these configuration files to return the same errors. However deptrac-private.yaml returns no errors at all. Despite clear violations in the controller.

Making the following small change to DependsOnPrivateLayer makes both configuration files work as expected:

public function invoke(ProcessEvent $event) : void
{
    $ruleset = $event->getResult();
    foreach ($event->dependentLayers as $dependentLayer => $isPublic) {
-       if ($event->dependerLayer === $dependentLayer && !$isPublic) {
+       if ($event->dependerLayer !== $dependentLayer && !$isPublic) {
            $this->eventHelper->addSkippableViolation($event, $ruleset, $dependentLayer, $this);
            $event->stopPropagation();
        }
    }
}
gennadigennadigennadi commented 3 months ago

Thank you for pointing this out! I’m right now on vacation and can’t spend time on going through all of this.

But I remember that this changed seemed off to me too.

I would appreciate a PR and a unittest in the meantime :-).

ndench commented 3 months ago

No worries @gennadigennadigennadi, thanks for replying whilst on vacation.

I've created qossmic/deptrac-src#69 with the fix and some new tests.

Enjoy your vacation!