Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compose Metadata Analyzer: Use v2 URL #4470

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

valentijnscholten
Copy link
Contributor

@valentijnscholten valentijnscholten commented Dec 18, 2024

Description

Use Composer Repository V2 url for metadata as v1 is deprecated and becoming readonly soon. V1 endpoint will be removed from packagist later this year. This PR removes V1 metadata handling from DT.

Addressed Issue

Fixes #2337

Additional Details

The metadata being returned for V2 has the same format as V1, except that the releases for a package are returned as an Array instead of a JSONObject.

One thing that changed is how non-existing (or no longer existing) packages are being handled:

I added a new unit test for the 404 scenario. I also removed the V1 unit test because I don't have a real world example of this happening in V2. Or should that unit test remain in place with some dummy package just to make sure we don't regress on this code path in the future?

Checklist

  • I have read and understand the contributing guidelines
  • This PR fixes a defect, and I have provided tests to verify that the fix is effective
  • This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • This PR introduces changes to the database model, and I have added corresponding update logic
  • This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

Copy link

codacy-production bot commented Dec 18, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%) 100.00% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (ee5cbce) 22927 18194 79.36%
Head commit (eb80260) 22927 (+0) 18194 (+0) 79.36% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4470) 9 9 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@valentijnscholten valentijnscholten marked this pull request as ready for review December 18, 2024 10:31
@valentijnscholten
Copy link
Contributor Author

The current code only works with repositories that use the default url pattern for package metadata:

    /**
     * @see <a href="https://packagist.org/apidoc#get-package-data">Packagist's API doc for "Getting package data - Using the Composer v2 metadata"</a>
     * Example: https://repo.packagist.org/p2/monolog/monolog.json
     */
    private static final String PACKAGE_META_DATA_URL = "/p2/%s/%s.json";

More extensive changes are needed to support repositories that use a different url pattern, or provide the package metadata inline. Those changes should go into a different PR so we can keep this PR here simple. Hopefully that helpts to get this PR merged/released before Composer V1 stops providing new metadata (February 1st 2025)

@nscuro
Copy link
Member

nscuro commented Dec 20, 2024

Hopefully that helpts to get this PR merged/released before Composer V1 stops providing new metadata (February 1st 2025)

So this really could (should!) go out in the 4.12.3 bugfix release then. Even if we get 4.13 out before Feb, not everyone will upgrade non-bugfix releases immediately. So better play it safe and get it out sooner.

@nscuro nscuro added enhancement New feature or request backport/4.12.3 PRs to be backported to version 4.12.3 labels Dec 20, 2024
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
@valentijnscholten
Copy link
Contributor Author

Even better. Didn't know if you'd want it in a bugfix release, but it's not too big of a change.

@valentijnscholten
Copy link
Contributor Author

valentijnscholten commented Dec 20, 2024

Looks like some third party repositories still expose their (meta)data in V1 format. Example https://packages.shopware.com.
And there's no field indicating which version. So we'll need to support both versions in DT for now by just trying both.

@valentijnscholten valentijnscholten marked this pull request as draft December 20, 2024 21:26
@valentijnscholten
Copy link
Contributor Author

valentijnscholten commented Dec 21, 2024

After a good nights sleep I realized we should just look at https://github.com/composer/composer/blob/main/src/Composer/Repository/ComposerRepository.php and mimic what is done there. The approach in #4435 may work for the short term, but it will result in lots of failing (404) requests for V2 meatada to repositories that are still V1.

Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
@valentijnscholten
Copy link
Contributor Author

Turns out that do it right, we need to retrieve the packages.json for the repository and check for the metadata-url. If present, it's V2. Otherwise it's V1. The approach in #4435 may work for the short term, but I decided to implement the "correct" solution here by mimicking what Composer itself does. We need that anyway if we want to do Vulnerability Analysis as well from these repositories.

Some talking points:

  • I added a simple in memory cache to cache the packages.json response. This may need something persistent/shared Hyades.
  • The ComposerMetaAnalyzer doesn't have enough information to make decent cache key. Any thoughts on providing more fields to the Analyzers besides just the Url. We could provide the ID as well, which should be unique? Or can we just provide the full reposiroy entity? For now the code works with the repositories I have seen, but I had to add a protected method to clean the cache before the start of each unit test.
  • Some features of the V2 repository are not implemented yet, but there were not implemented for V1 either. These are includes , packages, available-packages and available-packages-patterns.

Valentijn

Copy link

codacy-production bot commented Dec 21, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.01% (target: -1.00%) 77.17% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (ee5cbce) 22927 18194 79.36%
Head commit (c0ed867) 22987 (+60) 18240 (+46) 79.35% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4470) 92 71 77.17%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/4.12.3 PRs to be backported to version 4.12.3 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to Packagist api v2 for Composer package metadata
2 participants