feat: improve gradle plugin accuracy

muscar commented 3 years ago

[x] Ready for review
[x] Follows CONTRIBUTING rules
[ ] Reviewed by Snyk internal team

What does this PR do?

This patch set improves the accuracy of the gradle plugin.

Newer version of Gradle added two attributes to configs for controlling the resolution process: canBeResolved and canBeConsumed (see https://docs.gradle.org/current/userguide/declaring_dependencies.html#sec:resolvable-consumable-configs for details). A recent change to the plugin brought it in line with Gradle best practices, i.e. prefer canBeResolved=true and canBeConsumed=false. If no configs match the criteria, it falls back to configs with both attributes set to true. While this is a good approach, the implementation is a bit too strict: if any true- false configs are found, the true-true ones won't be considered. This misses some configs, and hence some dependencies that users expect to see.

The first change is to relax the strategy, and accept all the configs that have canBeResolved=true. This is equivalent to taking the union of true-false and true-true configs.

The second change fixes a bug in the plugin where sub-projects with the same name at different levels of a multi-project hierarchy would overwrite each other. To patch now uses the full path of the sub-project to disambiguate between sub-projects with the same name.

In addition to the above changes, the patch refactors the plugin code.

Where should the reviewer start?

The init.gradle file.

How should this be manually tested?

Pull the plugin locally, change your local copy of snyk to use this version of the plugin, run it on a gradle project.

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Before:

After:

Additional questions

CLAassistant commented 3 years ago

All committers have signed the CLA.

muscar commented 3 years ago

Thanks @anthogez! I'll have a look.

shanman190 commented 3 years ago

So variants should be fine under canBeResolved only. The best way to think about the different combinations of those flags can be as such:

canBeResolved	canBeConsumed	Configuration role	Purpose
false	false	Bucket of dependencies	Contains first-order dependency declarations with no transitive information (metadata, dependency graph, artifacts, etc)
true	false	Resolve for certain usages	These represent actual resolved dependency graphs complete with variant awareness. Both resolved and unresolved dependencies will be visible in the list as there is extra information on the Configuration, ResolvedConfiguration, and ResolvedDependency that indicate failure to download.
false	true	Exposed to consumers	These configurations are outputs generated by the build to later be consumed by another project within the current multi-module one or an external project. I.e. The jar file generated by the Java plugin's jar task is in the outgoing configuration apiElements. This contains the code of the built library for a future consumer to consume. If they do consume it, it will make an appearance in a canBeResolved configuration. Note: this has no bearing on if the configuration was successfully resolved. That is found via ResolvedConfiguration#hasError instead.
true	true	Legacy, don't use	Configurations with these settings tend to be either of the middle two categories and in modern Gradle versions have been removed from existence as the above configurations help to define a tighter purpose than this mode.

Note: this table is taken and enhanced from the Gradle link above.

So I think that you should be fine by having the canBeResolved only as that's going to capture all configurations that have the purpose of being a dependency graph.

muscar commented 3 years ago

Thanks for the very detailed comment @shanman190 :).

muscar commented 3 years ago

@anthogez agreed on the fact that the PR does too many things. And that if we have to roll back the resolution change it'll take with it other fix. We are going to address this shortly in a followup PR since there is some urgency to makign the new behaviour available as soon as possible.

anthogez commented 3 years ago

These represent actual resolved dependency graphs complete with variant awareness. Both resolved and unresolved dependencies will be visible in the list as there is extra information on the Configuration, ResolvedConfiguration, and ResolvedDependency that indicate failure to download.

Thanks @shanman190 is always good to have your input here :)

snyksec commented 3 years ago

:tada: This PR is included in version 3.17.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

snyk / snyk-gradle-plugin