deptrac icon indicating copy to clipboard operation
deptrac copied to clipboard

Enhancement: Ignored/Exempt Layers

Open MGatner opened this issue 3 years ago • 17 comments

I would like a way to define a layer that was available for dependency checks to other layers but otherwise exempt from validation. See also the conversation at: https://github.com/qossmic/deptrac/issues/506

This might be beyond me for my first PR but I would be willing to try. I imagine it working with a special modifier like ~ (maybe + as a nod to transitive dependencies?). Those this would be a valid depfile:

layers:
  - name: Core
    collectors:
      - type: className
        regex: Core
  - name: Module
    collectors:
      - type: className
        regex: Module

ruleset:
  Core: +
  Module:
    - Core

MGatner avatar Jul 03 '21 15:07 MGatner

What do you mean by exempt from validation? Core may use any other layer without having to specify them all in the ruleset?

dbrumann avatar Jul 04 '21 18:07 dbrumann

Core may use any other layer without having to specify them all in the ruleset?

At the very least. Ideally I think these rules would also no show up in "Allowed" as well. A semi-real scenario...

I want to make a file upload applet: simple web interface and CURL uploads to some remote source the user specifies. I'm going to use third-party packages for my Cache and CURL classes, and a basic Controller and View to handle the interface. I want my Controller to use the third-party tools but make sure View uses nothing, so:

layers:
  - name: Cache
    collectors:
      - type: directory
        regex: vendor/cacher/cache/.*
  - name: CURL
    collectors:
      - type: directory
        regex: vendor/curler/curl/.*
  - name: Controller
    collectors:
      - type: className
        regex: Controller
  - name: View
    collectors:
      - type: className
        regex: View
ruleset:
  Controller:
    - Cache
    - CURL
  View: ~

This works great - but wait! Turns out my CURL library actually uses the same Cache library so now I have a violation. In this simple example I can account for it:

ruleset:
  CURL:
    - Cache
  Controller:
    - Cache
    - CURL
  View: ~

But two issues here:

  1. Now I am defining rules for third-party libraries, which is a problem given I don't control the code at all.
  2. This works fine for a single overlap but for instances like frameworks there will be tons of inter-dependency.

The workaround is to list out every layer and just blindly allow all other layers, but this doesn't solve issue 1 and also introduces a falsely heightened "Allowed" count.

Which brings us to my proposal: a way to define a dependency yet exempt it from rulesets.

MGatner avatar Jul 04 '21 22:07 MGatner

This issue has not seen activity in a while and will be closed automatically soon.

github-actions[bot] avatar Sep 10 '21 14:09 github-actions[bot]

Still keen on this.

MGatner avatar Sep 10 '21 14:09 MGatner

@MGatner can I make rule: any module must not use classes from another module without defined each of module ?

ta-tikoma avatar Oct 15 '21 09:10 ta-tikoma

@ta-tikoma That is the default behavior, unless I have misunderstood.

MGatner avatar Oct 15 '21 14:10 MGatner

For example:

app/
    Module1
    Module2
    Module3
...
    Module20

Can I prohibit use classes from another module without defined each module as layer ?

ta-tikoma avatar Oct 15 '21 15:10 ta-tikoma

For my scenario I want the Infrastructure layer to be able to implement interfaces from the Application layer, but not extend or import any other classes from the Application layer.

Tried achieving this by:

  • using a wildcard on the implements collector, but it does not support wildcards
  • using a className collector will not work as we do not want our classnames to explicitly end with *Interface.

So, for the above scenario, having a layer that only includes all interfaces from the application layer would also be great.

rvanlaak avatar Feb 16 '22 09:02 rvanlaak

IMO these last two comments are unrelated to this feature request and should become their own discussions.

I would still like this feature and I am willing to help develop it if a maintainer will specify viability and precise behavior. I can also offer a bounty if anyone else would like to take in on.

MGatner avatar Feb 19 '22 13:02 MGatner

@MGatner you are right, so I created issue https://github.com/qossmic/deptrac/issues/823 for that.

rvanlaak avatar Feb 21 '22 12:02 rvanlaak

@MGatner I think this is what you want:

class ProcessEventListenerIgnoreLayerDependencies
{
    /**
     * @var array<string>
     */
    private array $ignoredLayerNames = [];

    /**
     * @param  array<string, array{=attributes: string}>  $layers
     */
    public function __construct(array $layers, string $ignoreAttributeName)
    {
        foreach ($layers as $layerName => $layer) {
            if (array_key_exists('attributes', $layer) && in_array($ignoreAttributeName, $layer['attributes'], true)) {
                $this->ignoredLayerNames[] = $layerName;
            }
        }
    }

    public function onProcessEvent(ProcessEvent $event): void
    {
        foreach ($this->ignoredLayerNames as $ignoredLayerName) {
            if($ignoredLayerName === $event->getDependerLayer()) {
                $event->stopPropagation();
            }
        }
    }
}

register it as an event listener, plus yaml config for the layer in question:

deptrac:
  layers:
    - name: File
      attributes:
        - <$ignoreAttributeName>
      collectors:

patrickkusebauch avatar Jun 06 '22 14:06 patrickkusebauch

Thanks @patrickkusebauch! I will give wit a try. In your example below, this would require me to declare dependencies on File in all other layers, but ignore anything File might depend on - correct?

MGatner avatar Jun 07 '22 10:06 MGatner

Thanks @patrickkusebauch! I will give wit a try. In your example below, this would require me to declare dependencies on File in all other layers, but ignore anything File might depend on - correct?

Yes, exactly.

patrickkusebauch avatar Jun 07 '22 10:06 patrickkusebauch

Hello,

I recently got the same issue as @MGatner and I have tested the provided solution by @patrickkusebauch in this comment.

Although I'm not familiar with the code base, these are the changes I've done to get this working:

Register the new listener In services.php:

  $services
        ->set(ProcessEventListenerIgnoreLayerDependencies::class)
        ->args([
            '$layers' => param('layers'),
            '$ignoreAttributeName' => 'ignore-dependencies',
        ])
        ->tag('event_listener', ['method' => 'onProcessEvent', 'priority' => 64]);

Slight change in ProcessEventListenerIgnoreLayerDependencies constructor's foreach loop -> I got the layer name from $layer['name']. Finally I made the necessary updates to layers definition as suggested in the solution comment.

It seems to work great!

Looking forward to get this released 😊

m-nic avatar Jun 30 '22 10:06 m-nic

@m-nic that's awesome you got it working. One improvement I would suggest is to register the service in the deptrac.yaml file instead. That way it won't get overwritten when a new version of deptrac is released:

services:
  - class: ProcessEventListenerIgnoreLayerDependencies
    arguments:
      $layers: '%layers%'
      $ignoreAttributeName: 'ignore-dependencies'
    tags:
     - { name: event_listener, event: Qossmic\Deptrac\Analyser\Event\ProcessEvent }

patrickkusebauch avatar Jun 30 '22 11:06 patrickkusebauch

Hey @patrickkusebauch, I did not realize the deptrac yaml config file is built on top of symfony configuration. I got it working as you suggested. Maybe this should also get documented?

Many thanks!

m-nic avatar Jun 30 '22 12:06 m-nic

Maybe this should also get documented?

We are working on it. Right now this is still somewhat experimental. For example, it doesn't work quite right when using the deptrac-shim binary.

Once we have ironed out a few kinks we will add it to the official docs.

I am also working on a way to ignore layers without custom code for one of the next releases.

dbrumann avatar Jul 01 '22 12:07 dbrumann

There was no activity in a while. I will consider this done for now. Feel free to reopen.

dbrumann avatar Sep 08 '22 15:09 dbrumann