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.

Sign up for GitHub

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

Jump to bottom

[Packagist] Update Packagist service to use v2 api #6508

Closed
wants to merge 5 commits into from
Closed

[Packagist] Update Packagist service to use v2 api #6508

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0993750
Update Packagist service to use v2 api
GeoffSelby May 15, 2021
83935ed
Adjust validation schema
GeoffSelby May 16, 2021
ffaa723
Fix typo
GeoffSelby May 18, 2021
c8692bc
Throw NotFound when license not found
GeoffSelby May 18, 2021
4d3c20b
Expand response and find the correct release
GeoffSelby Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions services/packagist/packagist-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@
const Joi = require('joi')
const { BaseJsonService } = require('..')

const packageSchema = Joi.object()
.pattern(
/^/,
Joi.object({
'default-branch': Joi.bool(),
version: Joi.string(),
require: Joi.object({
php: Joi.string(),
}),
}).required()
)
.required()
const packageSchema = Joi.array().items(
Joi.object({
version: Joi.string(),
require: Joi.object({
php: Joi.string(),
}),
})
)

const allVersionsSchema = Joi.object({
packages: Joi.object().pattern(/^/, packageSchema).required(),
Expand All @@ -38,7 +34,7 @@ class BasePackagistService extends BaseJsonService {
* @returns {object} Parsed response
*/
async fetch({ user, repo, schema, server = 'https://packagist.org' }) {
const url = `${server}/p/${user.toLowerCase()}/${repo.toLowerCase()}.json`
const url = `${server}/p2/${user.toLowerCase()}/${repo.toLowerCase()}.json`

return this._requestJson({
schema,
Expand Down
21 changes: 12 additions & 9 deletions services/packagist/packagist-license.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@ const {
customServerDocumentationFragment,
} = require('./packagist-base')

const packageSchema = Joi.object()
.pattern(
/^/,
const packageSchema = Joi.array()
.items(
Joi.object({
'default-branch': Joi.bool(),
license: Joi.array().required(),
license: Joi.array(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to drop .required() here? Is this because the new schema may not always define the key? If that's the case then I think in the transform function we need to account for that fact and throw a not found/invalid response error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new api endpoint now only returns the license array with the first item in the response (most recent release). At least that's what I've found in my testing. because of this, the schema validation would fail when the response contains more than 1 version. I do think that throwing a not found error would be good just in case there isn't a license at all for some reason. I'll add that in shortly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally you would run the MetadataMinifier::expand logic on the response before validating the schema, and then you can restore the required() I'd say. Note that packagist.org does not enforce the presence of a license key, so it may well be missing if the package author did not include it. I'd not expect they'd display a shields.io license badge if they haven't defined a license though but who knows :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Seldaek i thought about that too but I don’t think it would be possible with the way Shields makes a request since it’s shared with other services. Maybe with some sort of event hook system but that seems like overkill for this one situation.

}).required()
)
.required()
Expand Down Expand Up @@ -59,17 +57,22 @@ module.exports = class PackagistLicense extends BasePackagistService {
}

transform({ json, user, repo }) {
const branch = this.getDefaultBranch(json, user, repo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can delete getDefaultBranch() from the base class now. It is not being called anywhere.

if (!branch) {
throw new NotFound({ prettyMessage: 'default branch not found' })
const packageName = this.getPackageName(user, repo)

const license = json.packages[packageName][0].license

if (!license) {
throw new NotFound({ prettyMessage: 'license not found' })
}
const { license } = branch

return { license }
}

async handle({ user, repo }, { server }) {
const json = await this.fetch({ user, repo, schema, server })

const { license } = this.transform({ json, user, repo })

return renderLicenseBadge({ license })
}
}
49 changes: 27 additions & 22 deletions services/packagist/packagist-license.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,50 +5,55 @@ const { NotFound } = require('..')
const PackagistLicense = require('./packagist-license.service')

describe('PackagistLicense', function () {
it('should throw NotFound when default branch is missing', function () {
it('should return the license of the most recent release', function () {
const json = {
packages: {
'frodo/the-one-package': {
'1.0.x-dev': { license: 'MIT' },
'1.1.x-dev': { license: 'MIT' },
'2.0.x-dev': { license: 'MIT' },
'2.1.x-dev': { license: 'MIT' },
},
'frodo/the-one-package': [
{
version: '1.2.4',
license: 'MIT-latest',
},
{
version: '1.2.3',
license: 'MIT',
},
],
},
}
expect(() =>

expect(
PackagistLicense.prototype.transform({
json,
user: 'frodo',
repo: 'the-one-package',
})
)
.to.throw(NotFound)
.with.property('prettyMessage', 'default branch not found')
.to.have.property('license')
.that.equals('MIT-latest')
})

it('should return default branch when default branch is found', function () {
it('should throw NotFound when license key not in response', function () {
const json = {
packages: {
'frodo/the-one-package': {
'1.0.x-dev': { license: 'MIT' },
'1.1.x-dev': { license: 'MIT' },
'2.0.x-dev': {
license: 'MIT-default-branch',
'default-branch': true,
'frodo/the-one-package': [
{
version: '1.2.4',
},
'2.1.x-dev': { license: 'MIT' },
},
{
version: '1.2.3',
},
],
},
}
expect(

expect(() =>
PackagistLicense.prototype.transform({
json,
user: 'frodo',
repo: 'the-one-package',
})
)
.to.have.property('license')
.that.equals('MIT-default-branch')
.to.throw(NotFound)
.with.property('prettyMessage', 'license not found')
})
})
12 changes: 10 additions & 2 deletions services/packagist/packagist-php-version.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,19 @@ module.exports = class PackagistPhpVersion extends BasePackagistService {
}
}

findVersionIndex(json, user, repo, version) {
const packageArr = json.packages[this.getPackageName(user, repo)]

return packageArr.findIndex(v => v.version === version)
}

transform({ json, user, repo, version = '' }) {
const packageVersion =
version === ''
? this.getDefaultBranch(json, user, repo)
: json.packages[this.getPackageName(user, repo)][version]
? json.packages[this.getPackageName(user, repo)][0]
: json.packages[this.getPackageName(user, repo)][
this.findVersionIndex(json, user, repo, version)
]

if (!packageVersion) {
throw new NotFound({ prettyMessage: 'invalid version' })
Expand Down
100 changes: 61 additions & 39 deletions services/packagist/packagist-php-version.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,20 @@ const PackagistPhpVersion = require('./packagist-php-version.service')
describe('PackagistPhpVersion', function () {
const json = {
packages: {
'frodo/the-one-package': {
'1.0.0': { require: { php: '^5.6 || ^7' } },
'2.0.0': { require: { php: '^7.2' } },
'3.0.0': { require: { php: '^7.4 || 8' } },
'dev-main': { require: { php: '^8' }, 'default-branch': true },
},
'samwise/gardening': {
'1.0.x-dev': {},
'2.0.x-dev': {},
},
'pippin/mischief': {
'1.0.0': {},
'dev-main': { require: {}, 'default-branch': true },
},
Comment on lines -11 to -23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it typically concerns me when we have to modify tests as part of some other change. I realize that some updates are inevitable here since the new api has a different response structure, but I feel like there's more going on or at least that I'm missing some context (though I presume this is all branch related)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is solely because of the change in response structure as seen in the diff.

'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: { php: '^5.6 || ^7' },
},
],
},
}

Expand All @@ -37,44 +37,66 @@ describe('PackagistPhpVersion', function () {
.with.property('prettyMessage', 'invalid version')
})

it('should throw NotFound when version not specified and no default branch found', function () {
expect(() =>
PackagistPhpVersion.prototype.transform({
json,
user: 'samwise',
repo: 'gardening',
})
)
.to.throw(NotFound)
.with.property('prettyMessage', 'invalid version')
})

it('should throw NotFound when PHP version not found on package when using default branch', function () {
it('should throw NotFound when PHP version not found on package when using default release', function () {
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' },
},
],
},
}
expect(() =>
PackagistPhpVersion.prototype.transform({
json,
user: 'pippin',
repo: 'mischief',
json: specJson,
user: 'frodo',
repo: 'the-one-package',
})
)
.to.throw(NotFound)
.with.property('prettyMessage', 'version requirement not found')
})

it('should throw NotFound when PHP version not found on package when using specified version', function () {
it('should throw NotFound when PHP version not found on package when using specified release', function () {
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',
},
],
},
}
expect(() =>
PackagistPhpVersion.prototype.transform({
json,
user: 'pippin',
repo: 'mischief',
json: specJson,
user: 'frodo',
repo: 'the-one-package',
version: '1.0.0',
})
)
.to.throw(NotFound)
.with.property('prettyMessage', 'version requirement not found')
})

it('should return PHP version for the default branch', function () {
it('should return PHP version for the default release', function () {
expect(
PackagistPhpVersion.prototype.transform({
json,
Expand All @@ -83,19 +105,19 @@ describe('PackagistPhpVersion', function () {
})
)
.to.have.property('phpVersion')
.that.equals('^8')
.that.equals('^7.4 || 8')
})

it('should return PHP version for the specified branch', function () {
it('should return PHP version for the specified release', function () {
expect(
PackagistPhpVersion.prototype.transform({
json,
user: 'frodo',
repo: 'the-one-package',
version: '3.0.0',
version: '2.0.0',
})
)
.to.have.property('phpVersion')
.that.equals('^7.4 || 8')
.that.equals('^7.2')
})
})
4 changes: 2 additions & 2 deletions services/packagist/packagist-php-version.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ t.create('gets the package version of symfony')
.get('/symfony/symfony.json')
.expectBadge({ label: 'php', message: isComposerVersion })

t.create('gets the package version of symfony 2.8')
.get('/symfony/symfony/v2.8.0.json')
t.create('gets the package version of symfony 5.2.3')
.get('/symfony/symfony/v5.2.3.json')
.expectBadge({ label: 'php', message: isComposerVersion })

t.create('package with no requirements')
Expand Down
39 changes: 18 additions & 21 deletions services/packagist/packagist-version.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,12 @@ const {
customServerDocumentationFragment,
} = require('./packagist-base')

const packageSchema = Joi.object()
.pattern(
/^/,
Joi.object({
version: Joi.string(),
extra: Joi.object({
'branch-alias': Joi.object().pattern(/^/, Joi.string()),
}),
}).required()
)
.required()
const packageSchema = Joi.array().items(
Joi.object({
version: Joi.string(),
extra: Joi.any(),
})
)

const schema = Joi.object({
packages: Joi.object().pattern(/^/, packageSchema).required(),
Expand Down Expand Up @@ -90,25 +85,27 @@ class PackagistVersion extends BasePackagistService {

transform({ includePrereleases, json, user, repo }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted on findRelease(), we should have one method for working out the latest (stable) version. I think most of the logic in this function was there to handle dev branches being mixed in with the releases. Now that they are on a different endpoint ..~dev.json, I think we can ditch all this can't we (do correct me if I'm wrong)? If so, this simplifies to something like:

const packageSchema = Joi.array().items(
  Joi.object({version: Joi.string().required()})
)

transform({ includePrereleases, json, user, repo }) {
  const versionsData = json.packages[this.getPackageName(user, repo)]
  const versions = versionsData.map( version => version.version)

  if (includePrereleases) {
    return { version: latest(versions) }
  } else {
    const stableVersion = latest(versions.filter(isStable))
    return { version: stableVersion || latest(versions) }
  }
}

Then at that point, the logic for identifying the latest release is the same logic as in your findRelease() (which we should rename) but with a includePrereleases flag.. so we can move this to the base class and call with includePrereleases=false when we use it for the license/PHP version badges.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packagist do not have separate tagged version on different branch. Even the main branch is considered a development branch. The ~dev.json endpoint is useful for accessing information of branch HEAD, regardless if it is tagged or not. The current version of documentation stated:

The p2/$vendor/$package.json file contains only tagged releases. If you want to fetch information about branches (i.e. dev versions) you need to download p2/$vendor/$package~dev.json.

Sometimes the tagged pre-release versions (e.g. v0.1.2) of libraries are published and used in production by others. In which case, the latest version would be "not stable" but is not shown on the ~dev.json endpoint but is still relevant.

const versionsData = json.packages[this.getPackageName(user, repo)]
let versions = Object.keys(versionsData)

let versions = []
const aliasesMap = {}
versions.forEach(version => {
const versionData = versionsData[version]
if (
versionData.extra &&
versionData.extra['branch-alias'] &&
versionData.extra['branch-alias'][version]
) {

versionsData.forEach(version => {
if (version.extra && version.extra['branch-alias']) {
// eg, version is 'dev-master', mapped to '2.0.x-dev'.
const validVersion = versionData.extra['branch-alias'][version]
const validVersion =
version.extra['branch-alias'][
Object.keys(version.extra['branch-alias'])
]
if (
aliasesMap[validVersion] === undefined ||
compare(aliasesMap[validVersion], validVersion) < 0
) {
versions.push(validVersion)
aliasesMap[validVersion] = version
aliasesMap[validVersion] = version.version
}
}

versions.push(version.version)
})

versions = versions.filter(version => !/^dev-/.test(version))
Expand Down