[Packagist] dependency version #8371

PaulaBarszcz · 2022-09-02T16:59:18Z

[Updated description]

Related to: #6963

We can check the dependencies' versions against the require section of https://packagist.org/p2/symfony/symfony.json

Below are some badge screenshots for a couple of scenarios.

dependency's name contains only one part (no slash needed), e.g. php.
For the php case, I added a redirect from an old endpoint (packagist-php-version.service.js), as @chris48s advised:

http://localhost:8080/packagist/php-v/symfony/symfony/v3.2.8?server=https://packagist.org =>
http://localhost:8080/packagist/dependency-v/symfony/symfony/php.svg?server=https%3A%2F%2Fpackagist.org&version=v3.2.8

dependency's name contains two parts, e.g. twig/twig.

both given correctly

http://localhost:8080/packagist/dependency-v/symfony/symfony/twig/twig

one of them given incorrectly

http://localhost:8080/packagist/dependency-v/symfony/symfony/twiiiiiig/twig

when dependency's name is the same as the package name

http://localhost:8080/packagist/dependency-v/symfony/symfony/symfony/contracts

name of the dependency contains a hyphen

http://localhost:8080/packagist/dependency-v/symfony/symfony/ext-xml

with specified version of the package

with correct version

http://localhost:8080/packagist/dependency-v/symfony/symfony/twig/twig?version=v3.2.8

with incorrect version

http://localhost:8080/packagist/dependency-v/symfony/symfony/twig/twig?version=v3.2.8000

with custom server given (plus version given)

with a correct server

http://localhost:8080/packagist/dependency-v/symfony/symfony/twig/twig?version=v3.2.8&server=https://packagist.org

with an incorrect server

http://localhost:8080/packagist/dependency-v/symfony/symfony/twig/twig?version=v3.2.8&server=https://packagiiiist.org

not specified dependency's name

http://localhost:8080/packagist/dependency-v/symfony/symfony/

…dencyVendor and dependencyRepo

…p-ver

…ress

services/packagist/packagist-base.js

shields-ci · 2022-09-02T17:01:23Z

	Messages
📖	✨ Thanks for your contribution to Shields, @PaulaBarszcz!

Generated by 🚫 dangerJS against becfc0e

lgtm-com · 2022-09-02T17:03:26Z

This pull request introduces 1 alert when merging 623b442 into b624179 - view on LGTM.com

new alerts:

1 for Wrong use of 'this' for static method

services/packagist/packagist-dependency-version.service.js

lgtm-com · 2022-09-02T17:12:12Z

This pull request introduces 1 alert when merging fd96f9e into b624179 - view on LGTM.com

new alerts:

1 for Wrong use of 'this' for static method

PaulaBarszcz · 2022-09-03T13:59:35Z

This PR is ready to be reviewed :)

chris48s · 2022-09-04T19:19:02Z

Thanks for picking this one up. I'm just having a look through this, and it seems to me like given the require key for a packagist API response is something like:

"require": {
    "php": ">=8.1",
    "composer-runtime-api": ">=2.1",
    "ext-xml": "*",
    "friendsofphp/proxy-manager-lts": "^1.0.2",
    "doctrine/event-manager": "~1.0",
    "doctrine/persistence": "^2|^3",
    "twig/twig": "^2.13|^3.0.4",
    "psr/cache": "^2.0|^3.0",
    "psr/container": "^1.1|^2.0",
    "psr/event-dispatcher": "^1.0",
    "psr/link": "^1.1|^2.0",
    "psr/log": "^1|^2|^3",
    "symfony/contracts": "^2.5|^3.0",
    "symfony/polyfill-ctype": "~1.8",
    "symfony/polyfill-intl-grapheme": "~1.0",
    "symfony/polyfill-intl-icu": "~1.0",
    "symfony/polyfill-intl-idn": "^1.10",
    "symfony/polyfill-intl-normalizer": "~1.0",
    "symfony/polyfill-mbstring": "~1.0",
    "symfony/polyfill-uuid": "^1.15"
}

the distinction between a "packagist PHP version" badge ( /packagist/php-v ) and a "packagist generic dependency version" badge ( packagist/dependency-v ) is somewhat artificial - right?

i.e: if we've got a badge where we can use that data structure to show the version of twig/twig or composer-runtime-api then using that data structure to show the version of php should just be that same badge, but passing it php - right? ..or is there something subtly different we need to deal with if the thing we're reporting the version of PHP specifically?

I can also see that in that case things are a bit fiddly because of maintaining compatibility with the current URL structure, but I think I can see where I think we should go with this to streamline things a bit. I just want to check that assumption before I start making suggestions - you've probably spent more time thinking about this lately than I have.

PaulaBarszcz · 2022-09-04T19:26:44Z

Thanks for picking this one up. I'm just having a look through this, and it seems to me like given the require key for a packagist API response is something like:
"require": {
    "php": ">=8.1",
    "composer-runtime-api": ">=2.1",
    "ext-xml": "*",
    "friendsofphp/proxy-manager-lts": "^1.0.2",
    "doctrine/event-manager": "~1.0",
    "doctrine/persistence": "^2|^3",
    "twig/twig": "^2.13|^3.0.4",
    "psr/cache": "^2.0|^3.0",
    "psr/container": "^1.1|^2.0",
    "psr/event-dispatcher": "^1.0",
    "psr/link": "^1.1|^2.0",
    "psr/log": "^1|^2|^3",
    "symfony/contracts": "^2.5|^3.0",
    "symfony/polyfill-ctype": "~1.8",
    "symfony/polyfill-intl-grapheme": "~1.0",
    "symfony/polyfill-intl-icu": "~1.0",
    "symfony/polyfill-intl-idn": "^1.10",
    "symfony/polyfill-intl-normalizer": "~1.0",
    "symfony/polyfill-mbstring": "~1.0",
    "symfony/polyfill-uuid": "^1.15"
}
the distinction between a "packagist PHP version" badge ( /packagist/php-v ) and a "packagist generic dependency version" badge ( packagist/dependency-v ) is somewhat artificial - right?

i.e: if we've got a badge where we can use that data structure to show the version of twig/twig or composer-runtime-api then using that data structure to show the version of php should just be that same badge, but passing it php - right? ..or is there something subtly different we need to deal with if the thing we're reporting the version of PHP specifically?

I can also see that in that case things are a bit fiddly because of maintaining compatibility with the current URL structure, but I think I can see where I think we should go with this to streamline things a bit. I just want to check that assumption before I start making suggestions - you've probably spent more time thinking about this lately than I have.

Yes, packagist/dependency-v is nearly a copy of packagist/php-v. I did not want to change php-v endpoint for the sake of backwards compatibility. But if you think that we should follow a different approach than what I wrote in this PR, I am definitely open to suggestions :)

PaulaBarszcz

I modified a regex in services/test-validators for checking the dependecy’s version so that it would accept also 2 options present in the response from packagist’s API.
An asterisk and 2 alternative versions (separated by a single |):

I checked with https://regexr.com/ to confirm that the previously accepted strings are still valid.

Edit: only now did I notice that I have to check also if these also pass the regex validation check (three option separated by a single pipe; version number starting with a ~):

psr/log": "^1|^2|^3",
symfony/polyfill-intl-grapheme": "~1.0",

Edit2:
I confirmed that all of these pass the regex validation:
https://regexr.com/
Expression:

\*|(\s*(>=|>|<|<=|!=|\^|~)?\d+(\.(\*|(\d+(\.(\d+|\*))?)))?((\s*\|*)?\s*(>=|>|<|<=|!=|\^|~)?\d+(\.(\*|(\d+(\.(\d+|\*))?)))?)*\s*)

~1.28|~2.0
7.* || 5.6.*
7.1
>=5.6
>1.0 <2.0
!=1.0 <1.1 || >=1.2
7.1.*
7.* || 5.6.*
*
^1|^2|^3
~1.0

chris48s · 2022-09-04T20:23:32Z

Yes, packagist/dependency-v is nearly a copy of packagist/php-v. I did not want to change php-v endpoint for the sake of backwards compatibility. But if you think that we should follow a different approach than what I wrote in this PR, I am definitely open to suggestions :)

OK cool. I think we can usefully tidy up and reduce some duplication here then :)

In terms of the URL schema for the new packagist dependency version badge we've got a few requirements:

In general, params which are required to render a badge should be positional params in the URL rather than passed in the query string. There are some docs on badge URLs in https://github.com/badges/shields/blob/master/doc/badge-urls.md
We would like the ability for the 'dependency' to either inlcude a / character or not include a / character. i.e: php and twig/twig should both be valid
There is also this optional :version param (for specifying we want the to query dependencies for a particular package version instead of the latest) that currently exists on the php-v badge which is uncommonly used and not required but we need to maintain compatibility with it.

so here's my suggestion:

Lets make the URL pattern for the new badge /packagist/dependency-v/:user/:repo/:dependency+. This will make the :dependency param required and also allow it to contain slash characters. Then the value of dependency can be either php or twig/twig and we can just look for dependency in require and use it as the badge label. That should simplify a lot of the dependencyVendor/dependencyRepo stuff you are doing here.
Using + does mean that we then can't make the version param positional in the URL, so lets convert that to a query param instead, like server.
We completely get rid of the existing PackagistPhpVersion class, make sure any test cases are covered in the tests for the new badge and we convert the php-v route to a redirect:
- /packagist/php-v/symfony/symfony should redirect to /packagist/dependency-v/symfony/symfony/php
- /packagist/php-v/symfony/symfony/v2.8.0 should redirect to /packagist/dependency-v/symfony/symfony/php?version=v2.8.0
- We can have a specific example on the class for /packagist/dependency-v showing PHP version as it is a reasonably important special-case

There are lots of places where we have replaced an old route with a redirect to a new one you can refer to, but here is one example. https://github.com/badges/shields/blob/master/services/pypi/pypi-django-versions.service.js

services/packagist/packagist-php-version.spec.js

…/php-v redirects to the new url

PaulaBarszcz · 2022-09-08T21:20:45Z

I added toLowerCase() to prevent the below case:

http://localhost:8080/packagist/dependency-v/symfony/symfony/PHP

Now we're good both with PHP and php as a dependency's name:
http://localhost:8080/packagist/dependency-v/symfony/symfony/php

http://localhost:8080/packagist/dependency-v/symfony/symfony/PHP

PaulaBarszcz · 2022-09-08T21:26:58Z

@chris48s
Thanks a lot for your suggestions. I applied them. This PR is ready to be re-reviewed.

chris48s

The bulk of this PR is good to go. I've commented some suggestions, but its all minor tweaks now.

services/packagist/packagist-dependency-version.service.js

chris48s · 2022-09-11T18:19:28Z

services/packagist/packagist-dependency-version.service.js

+      throw new NotFound({ prettyMessage: 'version requirement not found' })
+    }
+
+    const depLowerCase = dependency.toLowerCase()


Are we sure no packagist vendor name, package name or extension name can contain an upper-case character?

Good question - I don’t think we can be sure of that.
Should i remove the above line?
It would then have issues with querries containing capital letters
(#8371 (comment))

We can do one of the following:

first check if there is a position in the response in ‚require’ section that contains directly the dependecy’s name (with capital letters)
and if not, transform the queried dependency’s name to lowercase and repeat the comparison

transform every dependency’s name from the reeponse to lowercase - and compare with lowercase depemdency’s name from the query.

What would be your preference?

I modified it, now it looks like this - is it fine?

// All dependencies' names in the 'require' section from the response should be lowercase, // so that we can compare lowercase name of the dependency given via url by the user. Object.keys(packageVersion.require).forEach(dependency => { packageVersion.require[dependency.toLowerCase()] = packageVersion.require[dependency] }) const depLowerCase = dependency.toLowerCase() if (!packageVersion.require[depLowerCase]) { throw new NotFound({ prettyMessage: 'version requirement not found' }) } return { dependencyVersion: packageVersion.require[depLowerCase] }

services/packagist/packagist-dependency-version.service.js

services/packagist/packagist-dependency-version.tester.js

chris48s · 2022-09-11T18:37:00Z

services/packagist/packagist-dependency-version.tester.js

+t.create(
+  `gets the package version of symfony 5.2.3 - incorrectly with missing part of dependency's name`
+)
+  .get('/symfony/symfony/twig.json')
+  .expectBadge({
+    label: 'dependency version',
+    message: 'version requirement not found',
+  })


In general, the naming of tests in this file is a bit jumbled. For example, this one is called gets the package version of symfony 5.2.3... but you're not passing ?version=5.2.3. There's another one further down with the same problem that I've suggested deleting as a duplicate. Then you've got another one further up where you're actually explicitly passing ?version=v3.2.8 in the URL but the test name is just "gets the package version of symfony...", and so on.

Can you have a look over the tests and make sure:

you're passing the ?version= param when that is actually part of the test, and leave it out if it doesn't make any difference to the test

if you want to explicitly describe the test params in the name of the test, make sure the test names and the params in the test body match up consistently.

That said, I know the number of cases we need to cover is now a bit larger and naming things is hard, but if you look at the test names that are deleted elsewhere in this PR (e.g: "invalid package name", "invalid version", "custom server", "invalid custom server", etc) those test names are actually pretty good. They explain what we are actually trying to test (i.e: what behaviour are we trying to trigger by providing these inputs, and what are we making an assertion about) rather than describing all of the inputs in each test. You might get more mileage out of just simplifying the test names a bit. I'll leave it with you to have a think about it.

I removed the version query param, where it was not needed.
I modified the tests' names, I hope that now they are ok.

PaulaBarszcz · 2022-09-12T21:35:11Z

Suggested changes were implemented. This PR is ready to be re-reviewed.

chris48s · 2022-09-13T20:25:16Z

I'm just having a look over the tests in services/packagist/packagist-dependency-version.spec.js and it seems like we have dropped some important test cases that exist in https://github.com/badges/shields/blob/master/services/packagist/packagist-php-version.spec.js

There's the case where the require key is missing:

shields/services/packagist/packagist-php-version.spec.js

Lines 36 to 52 in e3c938b

    
           const specJson = { 
        
             packages: { 
        
               'frodo/the-one-package': [ 
        
                 { 
        
                   version: '3.0.0', 
        
                 }, 
        
                 { 
        
                   version: '2.0.0', 
        
                   require: { php: '^7.2' }, 
        
                 }, 
        
                 { 
        
                   version: '1.0.0', 
        
                   require: { php: '^5.6 || ^7' }, 
        
                 }, 
        
               ], 
        
             }, 
        
           }

and the case where the require key is the string '__unset':

shields/services/packagist/packagist-php-version.spec.js

Lines 63 to 80 in e3c938b

    
           const specJson = { 
        
             packages: { 
        
               'frodo/the-one-package': [ 
        
                 { 
        
                   version: '3.0.0', 
        
                   require: { php: '^7.4 || 8' }, 
        
                 }, 
        
                 { 
        
                   version: '2.0.0', 
        
                   require: { php: '^7.2' }, 
        
                 }, 
        
                 { 
        
                   version: '1.0.0', 
        
                   require: '__unset', 
        
                 }, 
        
               ], 
        
             }, 
        
           }

#7779 gives a good example of a response including both the missing and __unset case: https://packagist.org/p2/hyde/framework.json

The packagist API has some surprising behaviours and edge cases. Most of this is handled by calling expandPackageVersions() to 'decompress' the API response. I'd like to ensure we are still covering these cases in the tests. It is quite easy to accidentally break the packagist code due to these gotchas (I actually have an open issue about refactoring this to make it a bit easier at #7783 but lets leave that one for another day).

PaulaBarszcz · 2022-09-14T13:39:59Z

There's the case where the require key is missing:
[...]
and the case where the require key is the string '__unset':
[...]

#7779 gives a good example of a response including both the missing and __unset case: https://packagist.org/p2/hyde/framework.json

The packagist API has some surprising behaviours and edge cases. Most of this is handled by calling expandPackageVersions() to 'decompress' the API response. I'd like to ensure we are still covering these cases in the tests. It is quite easy to accidentally break the packagist code due to these gotchas (I actually have an open issue about refactoring this to make it a bit easier at #7783 but lets leave that one for another day).

You're right:)
I added the missing cases to unit tests (missing require key and require key with the value of the __unset string).

chris48s

Nearly done. There is one final thing to get this over the line: In a6fd477 the captions and version are the wrong way round: v2.5.0 is the __unset case and v2.4.0 is the missing case. I've suggested 2 corrections to make the inputs match the descriptions.

services/packagist/packagist-dependency-version.spec.js

chris48s · 2022-09-14T19:51:37Z

services/packagist/packagist-dependency-version.spec.js

+        {
+          version: 'v2.5.0',
+          require: '__unset',
+        },
+        {
+          version: 'v2.4.0',
+        },


I was initially confused by this, but I see what you did with the ordering to fit all 3 cases into a single input.

Co-authored-by: chris48s <chris48s@users.noreply.github.com>

PaulaBarszcz · 2022-09-14T19:58:46Z

Nearly done. There is one final thing to get this over the line: In a6fd477 the captions and version are the wrong way round: v2.5.0 is the __unset case and v2.4.0 is the missing case. I've suggested 2 corrections to make the inputs match the descriptions.

Thanks for spotting this, you’re right 😅 Committed.

chris48s

Got there in the end. Packagist is definitely one of the trickier service families to work on.

🚀

PaulaBarszcz added 12 commits August 22, 2022 23:56

happy path is done: packagist depenency version is returned for depen…

4448d3f

…dencyVendor and dependencyRepo

Merge branch 'badges:master' into packagist-dependency-version

7db6ba2

Merge branch 'badges:master' into packagist-dependency-version

411ec19

changes in error handling in packagist dependency version service

2b97033

Move to packagist-base common parts of packagist-php and packagist-de…

77717e4

…p-ver

Merge branch 'badges:master' into packagist-dependency-version

94474a9

the label now shows the name of the dependency

623b787

add comments,slightly modify if statement

6de21c3

fix unit tests in services/packagist/packagist-php-version.spec.js

905667c

add unit tests for services/packagist/packagist-dependency-version.js

6fe762e

service tests for packagist-dependency-version.service.js are in prog…

55f50ce

…ress

Add additional service tests for packagist-dependency-version.tester.js

623b442

PaulaBarszcz commented Sep 2, 2022

View reviewed changes

services/packagist/packagist-base.js Show resolved Hide resolved

remove toLowerCase()

fd96f9e

PaulaBarszcz commented Sep 2, 2022

View reviewed changes

services/packagist/packagist-dependency-version.service.js Show resolved Hide resolved

PaulaBarszcz added 2 commits September 2, 2022 19:13

resolve from lgtm: 1 for Wrong use of 'this' for static method

de9aee1

DRY determining the name of the dependency (depVen/depRepo)

9b0e9cc

PaulaBarszcz changed the title ~~Packagist dependency version~~ [Packagist] dependency version Sep 3, 2022

PaulaBarszcz marked this pull request as ready for review September 3, 2022 13:59

calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Sep 3, 2022

PaulaBarszcz commented Sep 4, 2022

View reviewed changes

Merge branch 'master' into packagist-dependency-version

1602710

chris48s reviewed Sep 4, 2022

View reviewed changes

services/packagist/packagist-php-version.spec.js Outdated Show resolved Hide resolved

PaulaBarszcz added 6 commits September 7, 2022 21:10

Merge branch 'master' into packagist-dependency-version

55c36df

url change in strategy is in progress

c1a7786

basic examples of packagist/dependency-v work with new url; packagist…

b3c8014

…/php-v redirects to the new url

service tests should be green

46bc9b0

Merge branch 'master' into packagist-dependency-version

8eb8437

unit tests for packagist should be fixed

ed15ffb

add toLowerCase()

a0a99ad

PaulaBarszcz requested a review from chris48s September 8, 2022 21:33

chris48s reviewed Sep 11, 2022

View reviewed changes

PaulaBarszcz changed the title ~~[Packagist] dependency version~~ WIP [Packagist] dependency version Sep 12, 2022

PaulaBarszcz added 4 commits September 12, 2022 22:44

part of suggestions from the PR is implemented

af35f84

updated tests for packagist dependency version

e356b1b

Merge branch 'master' into packagist-dependency-version

d78c83d

fix packagist dependency version spec

4071359

PaulaBarszcz changed the title ~~WIP [Packagist] dependency version~~ [Packagist] dependency version Sep 12, 2022

PaulaBarszcz requested a review from chris48s September 12, 2022 21:35

PaulaBarszcz added 2 commits September 14, 2022 15:09

Merge branch 'master' into packagist-dependency-version

3a2fce2

add missing unit tests

a6fd477

chris48s reviewed Sep 14, 2022

View reviewed changes

PaulaBarszcz and others added 2 commits September 14, 2022 21:57

Update services/packagist/packagist-dependency-version.spec.js

126eb42

Co-authored-by: chris48s <chris48s@users.noreply.github.com>

Update services/packagist/packagist-dependency-version.spec.js

becfc0e

Co-authored-by: chris48s <chris48s@users.noreply.github.com>

chris48s approved these changes Sep 14, 2022

View reviewed changes

chris48s merged commit 93fa955 into badges:master Sep 14, 2022

PaulaBarszcz deleted the packagist-dependency-version branch September 14, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Packagist] dependency version #8371

[Packagist] dependency version #8371

PaulaBarszcz commented Sep 2, 2022 •

edited

shields-ci commented Sep 2, 2022 •

edited

lgtm-com bot commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

PaulaBarszcz commented Sep 3, 2022

chris48s commented Sep 4, 2022

PaulaBarszcz commented Sep 4, 2022

PaulaBarszcz left a comment •

edited

chris48s commented Sep 4, 2022

PaulaBarszcz commented Sep 8, 2022

PaulaBarszcz commented Sep 8, 2022

chris48s left a comment

chris48s Sep 11, 2022

PaulaBarszcz Sep 12, 2022

PaulaBarszcz Sep 12, 2022

chris48s Sep 11, 2022

PaulaBarszcz Sep 12, 2022

PaulaBarszcz commented Sep 12, 2022

chris48s commented Sep 13, 2022 •

edited

PaulaBarszcz commented Sep 14, 2022

chris48s left a comment

chris48s Sep 14, 2022

PaulaBarszcz Sep 14, 2022

PaulaBarszcz commented Sep 14, 2022

chris48s left a comment

[Packagist] dependency version #8371

[Packagist] dependency version #8371

Conversation

PaulaBarszcz commented Sep 2, 2022 • edited

shields-ci commented Sep 2, 2022 • edited

lgtm-com bot commented Sep 2, 2022

lgtm-com bot commented Sep 2, 2022

PaulaBarszcz commented Sep 3, 2022

chris48s commented Sep 4, 2022

PaulaBarszcz commented Sep 4, 2022

PaulaBarszcz left a comment • edited

Choose a reason for hiding this comment

chris48s commented Sep 4, 2022

PaulaBarszcz commented Sep 8, 2022

PaulaBarszcz commented Sep 8, 2022

chris48s left a comment

Choose a reason for hiding this comment

chris48s Sep 11, 2022

Choose a reason for hiding this comment

PaulaBarszcz Sep 12, 2022

Choose a reason for hiding this comment

PaulaBarszcz Sep 12, 2022

Choose a reason for hiding this comment

chris48s Sep 11, 2022

Choose a reason for hiding this comment

PaulaBarszcz Sep 12, 2022

Choose a reason for hiding this comment

PaulaBarszcz commented Sep 12, 2022

chris48s commented Sep 13, 2022 • edited

PaulaBarszcz commented Sep 14, 2022

chris48s left a comment

Choose a reason for hiding this comment

chris48s Sep 14, 2022

Choose a reason for hiding this comment

PaulaBarszcz Sep 14, 2022

Choose a reason for hiding this comment

PaulaBarszcz commented Sep 14, 2022

chris48s left a comment

Choose a reason for hiding this comment

PaulaBarszcz commented Sep 2, 2022 •

edited

shields-ci commented Sep 2, 2022 •

edited

PaulaBarszcz left a comment •

edited

chris48s commented Sep 13, 2022 •

edited