Baseline for needs migration

[Laimiux]

This is a feature suggestion that could help existing projects that have many violations. Instead of having big aliases such as NeedsMigration, I wonder if we could have violation baseline which we can generate (similarly to detekt/lint)

[jraska]

Hey, thanks for the suggestion.

Please what are your thoughts and motivation where would you find this useful? :)

[Laimiux]

Let's say we have a project with impl and public(called api in one of your talks) types of modules. When migrating an existing codebase to this structure there will be cases that break the categorization rules and need to be migrated over time. The NeedsMigration alias mentioned in the docs can help, but it's not fine-grained enough - once you mark a module with it, new violations can be added without triggering a failure.

Instead, I'm proposing a violation baseline where you can record all the existing violations.

{
    ":module-a:impl": [
        ":module-y:impl"
    ],
    "module-b:impl:": [
        ":module-y:impl",
    ],
}

This way we can fail the build if new violations are added (ex: :module-c:impl includes :module-y:impl)

[jraska]

Thanks for sharing more context :) it is a similar topic to what was discussed at: #157.

Using more entries within the allowed array as a whitelist would not work for your case?
Like allowed = [:module-a:impl -> module-y:impl, module-b:impl -> module-y:impl].

Another option I can share is that the plugin can be used on each module, not only at the :app module so another option is using more granular allowed/restricted in each particular module. -the plugin can be applied for each module subgraph.

Basically I'm trying to see the gaps which are truly there and the ones which can be done by current feature set as I try to keep the feature set minimal to keep the plugin lean. :)

[Laimiux]

Thanks for sharing more context :) it is a similar topic to what was discussed at: #157.

Indeed, it's a very similar issue.

Using more entries within the allowed array as a whitelist would not work for your case? Like allowed = [:module-a:impl -> module-y:impl, module-b:impl -> module-y:impl].

Another option I can share is that the plugin can be used on each module, not only at the :app module so another option is using more granular allowed/restricted in each particular module. -the plugin can be applied for each module subgraph.

Basically I'm trying to see the gaps which are truly there and the ones which can be done by current feature set as I try to keep the feature set minimal to keep the plugin lean. :)

I was able to get a more fine-grained allowed ruleset to work, however it was a bit tricky with some custom logic and then labor intensive to manually write each violation. I think having violation baseline generated by the plugin would have been valuable.

[jraska]

Hey @Laimiux, sorry for not getting back to you earlier - end of the year was busy.

One option I was investigating is to use the error message as the simple way to generate the rules as when there is an error, all violations are printed.

E.g.

... GradleException: [':module-a:impl' -> ':module-y:impl', ':module-b:impl' -> ':module-y:impl'] not allowed...

Though it has to be modified as the format required would be allowed = [':module-a:impl -> :module-y:impl', ':module-b:impl -> :module-y:impl']

So not ideal, but some start 🤔

Anyway about the whole feature you ask for I feel like I won't be able to get to this soon and probably the preferred solution would be unifying the error message format with the configuration so there is a simple way of copy/paste and use this as the baseline.

Thanks a lot for raising the issue in any case :)

[Laimiux]

I was able to work around this using a hacky solution, but it's becoming harder to scale this. The hacky solution to enable fine-grained violations requires us to define moduleNameAssertAlias dynamically and generate a very complicated allowed list. Instead, we can add an allowedViolations property to the API and implement the same functionality with a few simple lines of code. My colleague implemented this API here: #285

The API to allow certain violations looks like

allowedViolations = [
    ":a:impl": [
        ":b:impl",
        ":c:impl"   
     ],
     ":b:impl": [
        ":c:impl"
    ]
]

[jraska]

Hey, I commented here.

At the end it seems just as a change from List<Entry<String, String>> into Entry<String, List<String>> do I get it correctly?

You can achieve the same Entry<String, List<String>> by regex "a:impl -> b:impl|c:impl|d:impl..." using existing API 🤔

[Laimiux]

The problem is that the library only matches on the alias and not the project path. Allowing impl -> :b:impl is too broad and if you use project path as alias, you have to rebuild all other rules a:impl -> b:impl and a:impl -> public etc. Why was regex chosen as the API? I wonder if a lambda (Project Path, Alias, Dependency) -> Boolean would be a more flexible API that enabled much more customization by the consumer. This would get rid of the need for the API to support separate allowed/rejected properties (and would enable us to define our rules in a simpler manner).

[jraska]

the library only matches on the alias and not the project path

1. It matches alias if alias is present. You can choose a unique alias like LegacyImpl and then have rule for that or just not have alias for these modules?

why was regex chosen as the API?

2. Simple API with greater flexibillity. Hard to find something more flexible for names matching.
   I agree (Project Path, Alias, Dependency) -> Boolean might allow extra logic, but it would also expose all these APIs as public API to be maintained.

3. Even with alias you can still rewrite your code above as

LegacyImpl -> BAlias|CAlias

4. At the moment we go path of you needing a code and having something that specific, I would propose a fork - the main guiding policy of this plugin is simplicity, even for a cost of not satisfying specific use cases.
   The whole plugin doesn't do much in the end :) Pulling dependencies and matching pairs against regex rules.

Opening public Lambda API effectively doubles what this plugin actually does in my eyes. People would like to access Gradle there, which would break cacheability and configuration cache. Boolean will likely not be enough - what if we need warnings or later exclusions etc. etc. :)

5. I think a possible solution is to align the error messaging with the allowed/restricted format to be abel to use the error messaging as a generation fo the baseline.

[scana]

Anyway about the whole feature you ask for I feel like I won't be able to get to this soon and probably the preferred solution would be unifying the error message format with the configuration so there is a simple way of copy/paste and use this as the baseline.

Opening public Lambda API effectively doubles what this plugin actually does in my eyes. People would like to access Gradle there, which would break cacheability and configuration cache. Boolean will likely not be enough - what if we need warnings or later exclusions etc. etc. :)

Would it make sense to just have an file handle API in the plugin and let it generate the baseline on its own, so there's no manual work required? That's exactly how Android Lint / Detekt / any other static code analysis work. I agree that opening up API further is too complicated for what the plugin is supposed to do.

moduleGraphAssert {
  baseline = "config/graph-assert-baseline.xml" // or whatever extension seems fit
}

with a task <module>:generateAssertModuleGraphBaseline.

I think I could take a look into it during the holiday break if you were open for such a change.

EDIT:
Main reasoning behind is that while allowlisting technically works, it's hard to keep it up to date with devs fixing the violations.
My understanding is also that if something is put within allowlist it will still fail if I put a more generic rule under restricted:

// this will still fail if app has any .*:implementation dependencies?
moduleGraphAssert {
  allowed = [
    ':app -> .*:implementation',
  ]
  restricted = [
    '.* -> .*:implementation',
  ]
}