search

mediact / dependency-guard

Static production dependency analysis.
MIT License
83 stars 5 forks source link

1.0.8 Issue/33 proposal fix #34

Closed marcelmediact closed 5 years ago

marcelmediact commented 5 years ago

This PR contains a proposal for a fix, for the infinite loop issue #33.

Instead of using the built-in composer getDependents function with the recurse option on true, a new function will be introduced. This function will prevent an inifinite loop from occuring. It does so by storing already resolved dependency information in a local property.

The test-case has also been adjusted, because with this change, there are no errors returned from the test-case.

A dependency on the package symfony/options-resolver has also been added to the dependency guard package itself. The package was missing and dependency-guard was warning about this missing dependency.

janmartenjongerius commented 5 years ago

@marcelmediact can you show us a list of the dependents you eventually ended up with?

janmartenjongerius commented 5 years ago

A dependency on the package symfony/options-resolver has also been added to the dependency guard package itself. The package was missing and dependency-guard was warning about this missing dependency.

@marcelmediact I do not get this on the master branch. This may be a sign that the filter is fixed incorrectly.

marcelmediact commented 5 years ago

The list of dependencies:

array (
  0 => 'composer/composer',
  1 => 'magento/framework',
  2 => 'magento/module-authorization',
  3 => 'magento/module-backend',
  4 => 'magento/module-backup',
  5 => 'magento/module-bundle',
  6 => 'magento/module-catalog',
  7 => 'magento/module-catalog-import-export',
  8 => 'magento/module-catalog-inventory',
  9 => 'magento/module-catalog-rule',
  10 => 'magento/module-catalog-url-rewrite',
  11 => 'magento/module-checkout',
  12 => 'magento/module-cms',
  13 => 'magento/module-cms-url-rewrite',
  14 => 'magento/module-config',
  15 => 'magento/module-contact',
  16 => 'magento/module-cron',
  17 => 'magento/module-customer',
  18 => 'magento/module-deploy',
  19 => 'magento/module-developer',
  20 => 'magento/module-directory',
  21 => 'magento/module-downloadable',
  22 => 'magento/module-eav',
  23 => 'magento/module-email',
  24 => 'magento/module-gift-message',
  25 => 'magento/module-grouped-product',
  26 => 'magento/module-import-export',
  27 => 'magento/module-indexer',
  28 => 'magento/module-integration',
  29 => 'magento/module-media-storage',
  30 => 'magento/module-msrp',
  31 => 'magento/module-newsletter',
  32 => 'magento/module-page-cache',
  33 => 'magento/module-payment',
  34 => 'magento/module-product-alert',
  35 => 'magento/module-quote',
  36 => 'magento/module-reports',
  37 => 'magento/module-require-js',
  38 => 'magento/module-review',
  39 => 'magento/module-rss',
  40 => 'magento/module-rule',
  41 => 'magento/module-sales',
  42 => 'magento/module-sales-rule',
  43 => 'magento/module-sales-sequence',
  44 => 'magento/module-security',
  45 => 'magento/module-shipping',
  46 => 'magento/module-store',
  47 => 'magento/module-tax',
  48 => 'magento/module-theme',
  49 => 'magento/module-translation',
  50 => 'magento/module-ui',
  51 => 'magento/module-url-rewrite',
  52 => 'magento/module-user',
  53 => 'magento/module-variable',
  54 => 'magento/module-widget',
  55 => 'magento/module-wishlist',
)
janmartenjongerius commented 5 years ago

The list of dependencies:

array (
  0 => 'composer/composer',
  1 => 'magento/framework',
  2 => 'magento/module-authorization',
  3 => 'magento/module-backend',
  4 => 'magento/module-backup',
  5 => 'magento/module-bundle',
  6 => 'magento/module-catalog',
  7 => 'magento/module-catalog-import-export',
  8 => 'magento/module-catalog-inventory',
  9 => 'magento/module-catalog-rule',
  10 => 'magento/module-catalog-url-rewrite',
  11 => 'magento/module-checkout',
  12 => 'magento/module-cms',
  13 => 'magento/module-cms-url-rewrite',
  14 => 'magento/module-config',
  15 => 'magento/module-contact',
  16 => 'magento/module-cron',
  17 => 'magento/module-customer',
  18 => 'magento/module-deploy',
  19 => 'magento/module-developer',
  20 => 'magento/module-directory',
  21 => 'magento/module-downloadable',
  22 => 'magento/module-eav',
  23 => 'magento/module-email',
  24 => 'magento/module-gift-message',
  25 => 'magento/module-grouped-product',
  26 => 'magento/module-import-export',
  27 => 'magento/module-indexer',
  28 => 'magento/module-integration',
  29 => 'magento/module-media-storage',
  30 => 'magento/module-msrp',
  31 => 'magento/module-newsletter',
  32 => 'magento/module-page-cache',
  33 => 'magento/module-payment',
  34 => 'magento/module-product-alert',
  35 => 'magento/module-quote',
  36 => 'magento/module-reports',
  37 => 'magento/module-require-js',
  38 => 'magento/module-review',
  39 => 'magento/module-rss',
  40 => 'magento/module-rule',
  41 => 'magento/module-sales',
  42 => 'magento/module-sales-rule',
  43 => 'magento/module-sales-sequence',
  44 => 'magento/module-security',
  45 => 'magento/module-shipping',
  46 => 'magento/module-store',
  47 => 'magento/module-tax',
  48 => 'magento/module-theme',
  49 => 'magento/module-translation',
  50 => 'magento/module-ui',
  51 => 'magento/module-url-rewrite',
  52 => 'magento/module-user',
  53 => 'magento/module-variable',
  54 => 'magento/module-widget',
  55 => 'magento/module-wishlist',
)

I'm sorry to say this, but the packages in this list cannot be right. The method you are patching is supposed to find the dependents. I cannot imagine composer/composer having a dependency on a magento/* package, which is what is being analyzed if I remember correctly. See it like this:

  1. DependencyGuard analyzes the symbols in the code
  2. It encounters symbols from symfony/console that aren't available because of an explicit requirement
  3. The package symfony/console is installed because one of the packages, foo/bar, has a requirement on it.
  4. An ignore rule for foo/bar is added in composer.json.
  5. All violations are now filtered away.

Apply all the rules above, but then with no ignore rule for that package. Then the violation is deemed valid, because one of the dependents, not dependencies caused the installation of a package you explicitly depend on.

This whole DependencyFilter has the purpose to detect that if I use a code symbol Foo\Bar from the foo/bar package, you need to explicitly require foo/bar in your composer.json and can't depend on bar/baz always requiring foo/bar for you instead.

marcelmediact commented 5 years ago

composer/composer is dependent on symfony/console, which is in the composer.json require list of the regression test.

janmartenjongerius commented 5 years ago

@marcelmediact and @ksangers,

composer/composer is dependent on symfony/console, which is in the composer.json require list of the regression test.

But then it would mean symfony/console has a dependency on a magento/* package, or somewhere down the line that remains the case. This list can't be right for hard requirements, but I found out that Composer repositories mark something as dependent based on Package links. However, package links can be more than just the entries in require of a package.

As I think the code has never really solved it correctly by asking the composer repository for its dependents, I will write down a few pointers as to how I believe the solution should be implemented.

The DependencyFilter has been historically imprecise. Therefore it should be marked with @deprecated at the class level and point to a new filter that should be introduced. I am saying new filter, because I believe we should introduce our own resolution of the dependency graph, to ensure only the package requirements are used in the dependency graph. To achieve that, we can use the available instance of Composer\Package\Locker that is set on the Composer\Composer instance, available in Mediact\DependencyGuard\Violation\Filter\ViolationFilterFactory::create.

By using the locker, we can use the lock data to look at the require section of each locked package to determine a dependency graph. Because the lock data is already flattened, this should perform better and prevent circular dependency resolution.

I suggest a new symbol called Mediact\DependencyGuard\Violation\Filter\PackageRequirementsFilter or similar will be introduced to solve the problem correctly. Additionally, it is probably best to at this stage accompany the new class, not only with a unit test, but also with an integration test, to ensure it resolves packages as expected. In order to do a proper integration test, I suggest an additional class should be introduced that focuses on resolving the dependency graph and then the filter is only responsible for filter logic. That way you can test the resolved packages that cause the installation of the package you supply more easily. I envision something like:

/** @var Composer\Package\Locker $locker */
$resolver = new PackageRequirementsResolver($locker);

/** @var Mediact\DependencyGuard\Violation\ViolationInterface $violation */
$parents = $resolver->__invoke($violation->getPackage()->getName());

I want to stress once more that the list of packages you've shown before cannot be correct, as you are only looking for the packages that cause the installation of the package within the violation.

marcelmediact commented 5 years ago

list of dependents package in require of composer.json > resulting list of getDependents

jyxon/gdpr-cookie-compliance > 
magento/framework > magento/module-authorization,magento/module-backend,magento/module-backup,magento/module-bundle,magento/module-catalog,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-cms,magento/module-cms-url-rewrite,magento/module-config,magento/module-contact,magento/module-cron,magento/module-customer,magento/module-deploy,magento/module-developer,magento/module-directory,magento/module-downloadable,magento/module-eav,magento/module-email,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-msrp,magento/module-newsletter,magento/module-page-cache,magento/module-payment,magento/module-product-alert,magento/module-quote,magento/module-reports,magento/module-require-js,magento/module-review,magento/module-rss,magento/module-rule,magento/module-sales,magento/module-sales-rule,magento/module-sales-sequence,magento/module-security,magento/module-shipping,magento/module-store,magento/module-tax,magento/module-theme,magento/module-translation,magento/module-ui,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-wishlist
magento/module-backend > magento/module-authorization,magento/module-backup,magento/module-bundle,magento/module-catalog,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-cms,magento/module-config,magento/module-customer,magento/module-directory,magento/module-downloadable,magento/module-eav,magento/module-email,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-newsletter,magento/module-page-cache,magento/module-product-alert,magento/module-quote,magento/module-reports,magento/module-review,magento/module-rss,magento/module-rule,magento/module-sales,magento/module-sales-rule,magento/module-security,magento/module-shipping,magento/module-tax,magento/module-theme,magento/module-translation,magento/module-ui,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-wishlist,magento/module-backend,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-checkout,magento/module-contact,magento/module-payment,magento/module-msrp,magento/module-store,magento/module-cms-url-rewrite,magento/module-deploy,magento/module-developer,magento/module-cron
magento/module-cms > magento/module-catalog,magento/module-cms-url-rewrite,magento/module-contact,magento/module-email,magento/module-newsletter,magento/module-reports,magento/module-theme,magento/module-url-rewrite,magento/module-widget,magento/module-backend,magento/module-bundle,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-cms,magento/module-customer,magento/module-downloadable,magento/module-eav,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-msrp,magento/module-product-alert,magento/module-quote,magento/module-review,magento/module-rule,magento/module-sales,magento/module-sales-rule,magento/module-shipping,magento/module-store,magento/module-tax,magento/module-wishlist,magento/module-authorization,magento/module-backup,magento/module-config,magento/module-directory,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-page-cache,magento/module-rss,magento/module-security,magento/module-translation,magento/module-ui,magento/module-user,magento/module-variable,magento/module-payment,magento/module-deploy,magento/module-developer,magento/module-cron
magento/module-config > magento/module-backend,magento/module-bundle,magento/module-catalog,magento/module-catalog-inventory,magento/module-checkout,magento/module-contact,magento/module-customer,magento/module-deploy,magento/module-developer,magento/module-directory,magento/module-downloadable,magento/module-eav,magento/module-email,magento/module-media-storage,magento/module-page-cache,magento/module-payment,magento/module-reports,magento/module-sales,magento/module-sales-rule,magento/module-store,magento/module-tax,magento/module-theme,magento/module-authorization,magento/module-backup,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-cms,magento/module-config,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-indexer,magento/module-integration,magento/module-newsletter,magento/module-product-alert,magento/module-quote,magento/module-review,magento/module-rss,magento/module-rule,magento/module-security,magento/module-shipping,magento/module-translation,magento/module-ui,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-wishlist,magento/module-catalog-import-export,magento/module-msrp,magento/module-cms-url-rewrite,magento/module-cron
magento/module-store > magento/module-backend,magento/module-backup,magento/module-bundle,magento/module-catalog,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-cms,magento/module-cms-url-rewrite,magento/module-config,magento/module-contact,magento/module-cron,magento/module-customer,magento/module-deploy,magento/module-developer,magento/module-directory,magento/module-downloadable,magento/module-eav,magento/module-email,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-integration,magento/module-media-storage,magento/module-msrp,magento/module-newsletter,magento/module-page-cache,magento/module-payment,magento/module-product-alert,magento/module-quote,magento/module-reports,magento/module-review,magento/module-rss,magento/module-rule,magento/module-sales,magento/module-sales-rule,magento/module-security,magento/module-shipping,magento/module-tax,magento/module-theme,magento/module-translation,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-wishlist,magento/module-authorization,magento/module-indexer,magento/module-ui,magento/module-store
magento/module-theme > magento/module-catalog,magento/module-checkout,magento/module-cms,magento/module-customer,magento/module-downloadable,magento/module-email,magento/module-review,magento/module-sales,magento/module-widget,magento/module-backend,magento/module-bundle,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-eav,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-msrp,magento/module-product-alert,magento/module-quote,magento/module-reports,magento/module-rule,magento/module-sales-rule,magento/module-shipping,magento/module-store,magento/module-tax,magento/module-url-rewrite,magento/module-wishlist,magento/module-authorization,magento/module-backup,magento/module-config,magento/module-directory,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-newsletter,magento/module-page-cache,magento/module-rss,magento/module-security,magento/module-theme,magento/module-translation,magento/module-ui,magento/module-user,magento/module-variable,magento/module-contact,magento/module-payment,magento/module-cms-url-rewrite,magento/module-deploy,magento/module-developer,magento/module-cron
magento/module-ui > magento/module-bundle,magento/module-catalog,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-cms,magento/module-customer,magento/module-downloadable,magento/module-gift-message,magento/module-grouped-product,magento/module-review,magento/module-sales,magento/module-sales-rule,magento/module-shipping,magento/module-store,magento/module-theme,magento/module-wishlist,magento/module-backend,magento/module-payment,magento/module-quote,magento/module-reports,magento/module-tax,magento/module-authorization,magento/module-backup,magento/module-config,magento/module-directory,magento/module-eav,magento/module-email,magento/module-import-export,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-newsletter,magento/module-page-cache,magento/module-product-alert,magento/module-rss,magento/module-rule,magento/module-security,magento/module-translation,magento/module-ui,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-catalog-import-export,magento/module-contact,magento/module-msrp,magento/module-cms-url-rewrite,magento/module-deploy,magento/module-developer,magento/module-cron
magento/module-widget > magento/module-catalog,magento/module-cms,magento/module-newsletter,magento/module-reports,magento/module-sales,magento/module-sales-rule,magento/module-theme,magento/module-backend,magento/module-bundle,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-customer,magento/module-downloadable,magento/module-eav,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-msrp,magento/module-product-alert,magento/module-quote,magento/module-review,magento/module-rule,magento/module-shipping,magento/module-store,magento/module-tax,magento/module-url-rewrite,magento/module-widget,magento/module-wishlist,magento/module-authorization,magento/module-backup,magento/module-config,magento/module-directory,magento/module-email,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-page-cache,magento/module-rss,magento/module-security,magento/module-translation,magento/module-ui,magento/module-user,magento/module-variable,magento/module-contact,magento/module-payment,magento/module-cms-url-rewrite,magento/module-deploy,magento/module-developer,magento/module-cron
symfony/console > composer/composer,magento/framework,magento/module-authorization,magento/module-backend,magento/module-backup,magento/module-bundle,magento/module-catalog,magento/module-catalog-import-export,magento/module-catalog-inventory,magento/module-catalog-rule,magento/module-catalog-url-rewrite,magento/module-checkout,magento/module-cms,magento/module-cms-url-rewrite,magento/module-config,magento/module-contact,magento/module-cron,magento/module-customer,magento/module-deploy,magento/module-developer,magento/module-directory,magento/module-downloadable,magento/module-eav,magento/module-email,magento/module-gift-message,magento/module-grouped-product,magento/module-import-export,magento/module-indexer,magento/module-integration,magento/module-media-storage,magento/module-msrp,magento/module-newsletter,magento/module-page-cache,magento/module-payment,magento/module-product-alert,magento/module-quote,magento/module-reports,magento/module-require-js,magento/module-review,magento/module-rss,magento/module-rule,magento/module-sales,magento/module-sales-rule,magento/module-sales-sequence,magento/module-security,magento/module-shipping,magento/module-store,magento/module-tax,magento/module-theme,magento/module-translation,magento/module-ui,magento/module-url-rewrite,magento/module-user,magento/module-variable,magento/module-widget,magento/module-wishlist

This should visualize it a bit better.

janmartenjongerius commented 5 years ago

@marcelmediact the list you provide in this comment is simply not showing correct data. If the violation is about magento/module-widget, you are looking for the packages that cause magento/module-widget to get installed. symfony/console will never cause magento/module-widget to be installed and therefore it should not be in the list, but it is. I'm not saying you implemented a wrong solution against the circular dependents resolution, but I'm saying that it turns out it has been imprecise all along and should be solved differently. Please consult with @sjokki and @ksangers and have a look at the pointers I made earlier.

marcelmediact commented 5 years ago

So just for clarification. You want the "new" dependency filter to actually return dependencies opposed to dependents? For making everything clear:

Dependent = Package A relies on Package B, so when Package B is checked with the function, Package A is returned (this is what is happening now). When Package A is used as a parameter for this function, it will not return anything.

Dependency = Package A relies on Package B, so when Package A is checked with the function, Package B is returned. When Package B is analysed, nothing will be returned.

janmartenjongerius commented 5 years ago

So just for clarification. You want the "new" dependency filter to actually return dependencies opposed to dependents? For making everything clear:

Dependent = Package A relies on Package B, so when Package B is checked with the function, Package A is returned (this is what is happening now). When Package A is used as a parameter for this function, it will not return anything.

Dependency = Package A relies on Package B, so when Package A is checked with the function, Package B is returned. When Package B is analysed, nothing will be returned.

I'm not looking for dependencies. Not in the current filter and not in the new filter. This will still be about dependents. What we define as a dependent is a package that has another package as requirement. So if package A requires package B, then A is a dependent of B and B is a dependency of A. However, when you ask a Composer repository what the dependents of a package are, it goes beyond the require section of a package and includes all package links, like suggests, replaces, conflicts, etc. It was previously not clear that DependencyGuard was using all those in this filter.

I'm looking for 2 new classes. One that can get a list of depentents, purely using the require sections of the Composer lock. The second class would be basically the current DependencyFilter, but then using the aforementioned class to determine the dependents, instead of using a Composer repository to determine the dependents.

Sadly we cannot reuse the DependencyFilter when using the Composer lock, because of the signature of the constructor. Otherwise we could have patched it. Now we'll have to deprecate the old class, introduce a new one and then update the factory that creates an instance of that filter.

@marcelmediact this bug requires a lot of in-depth knowledge of how both Composer and DependencyGuard function, so it is only to be expected we didn't get it right the first time. What you have done is fixing the bug that was reported, however, in encountering this bug, we actually found a deeper underlying bug. Accepting your patch as-is will only mean we have to immediately deprecate it and create a new bug report and patch that. I'm suggesting we do it now without the extra steps.

Please, once more, I ask of you to discuss a good approach for this with @sjokki and @ksangers, because I'm actually indisposed and should not be the one guiding you through this pull request.