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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
84 changes: 71 additions & 13 deletions services/packagist/packagist-base.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,16 @@

const Joi = require('joi')
const { BaseJsonService } = require('..')
const { isStable, latest } = require('../php-version')

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 +35,31 @@ 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,
url,
})
}

/**
* Fetch dev releases method.
*
* This method utilize composer metadata API which
* "... is the preferred way to access the data as it is always up to date,
* and dumped to static files so it is very efficient on our end." (comment from official documentation).
* For more information please refer to https://packagist.org/apidoc#get-package-data.
*
* @param {object} attrs Refer to individual attrs
* @param {string} attrs.user package user
* @param {string} attrs.repo package repository
* @param {Joi} attrs.schema Joi schema to validate the response transformed to JSON
* @param {string} attrs.server URL for the packagist registry server (Optional)
* @returns {object} Parsed response
*/
async fetchDev({ user, repo, schema, server = 'https://packagist.org' }) {
const url = `${server}/p2/${user.toLowerCase()}/${repo.toLowerCase()}~dev.json`

return this._requestJson({
schema,
Expand Down Expand Up @@ -85,6 +106,43 @@ class BasePackagistService extends BaseJsonService {
getPackageName(user, repo) {
return `${user.toLowerCase()}/${repo.toLowerCase()}`
}

decompressResponse(json, packageName) {
const versions = json.packages[packageName]
const expanded = []
let expandedVersion = null

versions.forEach(versionData => {
if (!expandedVersion) {
expandedVersion = versionData
expanded.push(expandedVersion)
}

Object.entries(versionData).forEach(([key, value]) => {
if (value === '__unset') {
delete expandedVersion[key]
} else {
expandedVersion[key] = value
}
})

expandedVersion = { ...expandedVersion }

expanded.push(expandedVersion)
})

return expanded
}

findRelease(json, versions = []) {
json.forEach(version => {
versions.push(version.version)
})
Comment on lines +137 to +140
Copy link
Member

Choose a reason for hiding this comment

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

This function is not that clearly named. How about findLatestRelease()?

Also, we should have one method of determining the latest (stable) version that we use consistently - we shouldn't have 2 different methods for different badges. More commentry on this on the packagist-version.js file..

I don't think versions needs to be a function param here. We never pass anything in when we call it. How about we make this just

const versions = json.map(version => version.version)


const release = latest(versions.filter(isStable)) || latest(versions)

return json.filter(version => version.version === release)[0]
}
}

const customServerDocumentationFragment = `
Expand Down
26 changes: 17 additions & 9 deletions services/packagist/packagist-license.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ 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(),
version: Joi.string(),
license: Joi.array(),
}).required()
)
.required()
Expand Down Expand Up @@ -59,17 +58,26 @@ 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 decompressed = this.decompressResponse(json, packageName)

const version = this.findRelease(decompressed)

const license = version.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 })
}
}
101 changes: 80 additions & 21 deletions services/packagist/packagist-license.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,42 +5,76 @@ 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 the license of the most recent stable release', function () {
const json = {
packages: {
'frodo/the-one-package': [
{
version: '1.2.4-RC1', // Pre-release
license: 'MIT-latest',
},
{
version: '1.2.3', // Stable release
license: 'MIT',
},
],
},
}

expect(
PackagistLicense.prototype.transform({
json,
user: 'frodo',
repo: 'the-one-package',
})
)
.to.have.property('license')
.that.equals('MIT')
})

it('should return default branch when default branch is found', function () {
it('should return the license of the most recent pre-release if no stable releases', 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,
},
'2.1.x-dev': { license: 'MIT' },
},
'frodo/the-one-package': [
{
version: '1.2.4-RC2',
license: 'MIT-latest',
},
{
version: '1.2.4-RC1',
license: 'MIT',
},
],
},
}

expect(
PackagistLicense.prototype.transform({
json,
Expand All @@ -49,6 +83,31 @@ describe('PackagistLicense', function () {
})
)
.to.have.property('license')
.that.equals('MIT-default-branch')
.that.equals('MIT-latest')
})

it('should throw NotFound when license key not in response', function () {
const json = {
packages: {
'frodo/the-one-package': [
{
version: '1.2.4',
},
{
version: '1.2.3',
},
],
},
}

expect(() =>
PackagistLicense.prototype.transform({
json,
user: 'frodo',
repo: 'the-one-package',
})
)
.to.throw(NotFound)
.with.property('prettyMessage', 'license not found')
})
})
66 changes: 59 additions & 7 deletions services/packagist/packagist-php-version.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,68 @@ module.exports = class PackagistPhpVersion extends BasePackagistService {
}
}

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

async findSpecifiedVersion(json, user, repo, version, server) {
let release

if ((release = json[this.findVersionIndex(json, version)])) {
return release
} else {
try {
const allData = await this.fetchDev({
user,
repo,
schema: allVersionsSchema,
server,
})

const decompressed = this.decompressResponse(
allData,
this.getPackageName(user, repo)
)

return decompressed[this.findVersionIndex(decompressed, version)]
} catch (e) {
return release
}
}
}

async transform({ json, user, repo, version = '', server }) {
Copy link
Member

Choose a reason for hiding this comment

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

The logic path for this badge is a bit muddled
Our sequence of execution is fetch --> transform --> render (or sometimes just fetch --> render)

  • Fetch gets data from APIs
  • Transform re-shapes the data (if necessary)
  • Render turns it into a badge

Here we are making an API call in transform, so that's a good indicator we've split the logic up incorrectly or named things poorly.

In this situation, calling fetch(), trying to find the latest version in there, falling back to calling fetchDev() if necessary is all part of the "fetch" stage.

The "transform" stage (to the extent there is one for this badge) is just extracting .require.php

let packageVersion
const decompressed = this.decompressResponse(
json,
this.getPackageName(user, repo)
)

if (version === '') {
packageVersion = this.findRelease(decompressed)
} else {
try {
packageVersion = await this.findSpecifiedVersion(
decompressed,
user,
repo,
version,
server
)
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

stylistically we always use } catch (e) {
we should probably have an eslint rule for that

packageVersion = null
}
}

if (!packageVersion) {
throw new NotFound({ prettyMessage: 'invalid version' })
}

if (!packageVersion.require || !packageVersion.require.php) {
if (
!packageVersion.require ||
!packageVersion.require.php ||
packageVersion.require.php === '__unset'
) {
throw new NotFound({ prettyMessage: 'version requirement not found' })
}

Expand All @@ -92,11 +143,12 @@ module.exports = class PackagistPhpVersion extends BasePackagistService {
schema: allVersionsSchema,
server,
})
const { phpVersion } = this.transform({
const { phpVersion } = await this.transform({
json: allData,
user,
repo,
version,
server,
})
return this.constructor.render({ php: phpVersion })
}
Expand Down
Loading