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

Validate response json in [apm appveyor cdnjs clojars gem] also affects [npm] #1883

Merged
merged 11 commits into from
Aug 10, 2018
49 changes: 21 additions & 28 deletions services/apm/apm.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,22 @@ const { BaseJsonService } = require('../base')
const { InvalidResponse } = require('../errors')
const { version: versionColor } = require('../../lib/color-formatters')
const { metric, addv } = require('../../lib/text-formatters')
const { positiveInteger } = require('../validators.js')

const apmSchema = Joi.object({
downloads: positiveInteger,
releases: Joi.object({
latest: Joi.string().required(),
}),
metadata: Joi.object({
license: Joi.string().required(),
}),
})

class BaseAPMService extends BaseJsonService {
async fetch(repo) {
return this._requestJson({
schema: Joi.object(),
schema: apmSchema,
url: `https://atom.io/api/packages/${repo}`,
notFoundMessage: 'package not found',
})
Expand All @@ -18,6 +29,15 @@ class BaseAPMService extends BaseJsonService {
static get defaultBadgeData() {
return { label: 'apm' }
}

static get examples() {
return [
{
previewUrl: 'vim-mode',
keywords: ['atom'],
},
]
}
Copy link
Member

Choose a reason for hiding this comment

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

❤️

}

class APMDownloads extends BaseAPMService {
Expand All @@ -43,15 +63,6 @@ class APMDownloads extends BaseAPMService {
capture: ['repo'],
}
}

static get examples() {
return [
{
previewUrl: 'dm/vim-mode',
keywords: ['atom'],
},
]
}
}

class APMVersion extends BaseAPMService {
Expand All @@ -77,15 +88,6 @@ class APMVersion extends BaseAPMService {
capture: ['repo'],
}
}

static get examples() {
return [
{
previewUrl: 'v/vim-mode',
keywords: ['atom'],
},
]
}
}

class APMLicense extends BaseAPMService {
Expand Down Expand Up @@ -115,15 +117,6 @@ class APMLicense extends BaseAPMService {
capture: ['repo'],
}
}

static get examples() {
return [
{
previewUrl: 'l/vim-mode',
keywords: ['atom'],
},
]
}
}

module.exports = {
Expand Down
8 changes: 7 additions & 1 deletion services/appveyor/appveyor.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
const Joi = require('joi')
const { BaseJsonService } = require('../base')

const appVeyorSchema = Joi.object({
build: Joi.object({
status: Joi.string().required(),
}),
}).required()

module.exports = class AppVeyor extends BaseJsonService {
async handle({ repo, branch }) {
let url = `https://ci.appveyor.com/api/projects/${repo}`
Expand All @@ -12,7 +18,7 @@ module.exports = class AppVeyor extends BaseJsonService {
const {
build: { status },
} = await this._requestJson({
schema: Joi.object(),
schema: appVeyorSchema,
url,
notFoundMessage: 'project not found or access denied',
})
Expand Down
11 changes: 7 additions & 4 deletions services/cdnjs/cdnjs.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,27 @@ const { NotFound } = require('../errors')
const { addv: versionText } = require('../../lib/text-formatters')
const { version: versionColor } = require('../../lib/color-formatters')

const cdnjsSchema = Joi.object({
version: Joi.string(),
Copy link
Member

Choose a reason for hiding this comment

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

I think you want version: Joi.string().required().

Copy link
Member Author

Choose a reason for hiding this comment

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

On both CDNJS and Clojars the version key is intentionally optional because {} should also pass validation.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment in the schema explaining why?

}).required()

module.exports = class Cdnjs extends BaseJsonService {
async handle({ library }) {
const url = `https://api.cdnjs.com/libraries/${library}?fields=version`
const json = await this._requestJson({
url,
schema: Joi.any(),
schema: cdnjsSchema,
})

if (Object.keys(json).length === 0) {
/* Note the 'not found' response from cdnjs is:
status code = 200, body = {} */
throw new NotFound()
}
const version = json.version || 0

return {
message: versionText(version),
color: versionColor(version),
message: versionText(json.version),
color: versionColor(json.version),
}
}

Expand Down
6 changes: 5 additions & 1 deletion services/clojars/clojars.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@ const { BaseJsonService } = require('../base')
const { NotFound } = require('../errors')
const { version: versionColor } = require('../../lib/color-formatters')

const clojarsSchema = Joi.object({
version: Joi.string(),
}).required()

module.exports = class Clojars extends BaseJsonService {
async handle({ clojar }) {
const url = `https://clojars.org/${clojar}/latest-version.json`
const json = await this._requestJson({
url,
schema: Joi.any(),
schema: clojarsSchema,
})

if (Object.keys(json).length === 0) {
Expand Down
149 changes: 149 additions & 0 deletions services/gem/gem-downloads.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
'use strict'

const semver = require('semver')
const Joi = require('joi')

const { BaseJsonService } = require('../base')
const { InvalidResponse } = require('../errors')
const {
downloadCount: downloadCountColor,
} = require('../../lib/color-formatters')
const { metric } = require('../../lib/text-formatters')
const { latest: latestVersion } = require('../../lib/version')
const { positiveInteger } = require('../validators.js')

const downloadsSchema = Joi.alternatives().try(
Joi.object({
downloads: positiveInteger,
version_downloads: positiveInteger,
}).required(),
Joi.array()
.items(
Joi.object({
prerelease: Joi.boolean().required(),
number: Joi.string().required(),
downloads_count: positiveInteger,
})
)
.min(1)
.required()
Copy link
Member Author

Choose a reason for hiding this comment

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

I hit an edge case here. Depending on which downloads badge we are building (dt/dtv or dv) we request a different endpoint on rubygems.org. This means there is more than one possible response schema. We can cover it by using Joi.alternatives().try() like I've done here, but I wonder if it could be better to think in terms of a URL pattern/schema pair here instead of a one-to-one badge/schema relationship? Any thoughts? Am I just over-complicating things? Are there other situations where we might hit a more complicated version of this?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, the npm downloads badge had the same issue. There I handled it with a switch:

const schema = isRange ? rangeResponseSchema : pointResponseSchema

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - that's a much better way of doing it

)

module.exports = class GemDownloads extends BaseJsonService {
async fetch(repo, info) {
const endpoint = info === 'dv' ? 'versions/' : 'gems/'
const url = `https://rubygems.org/api/v1/${endpoint}${repo}.json`
return this._requestJson({
url,
schema: downloadsSchema,
})
}

_getLabel(version, info) {
if (version) {
return 'downloads@' + version
} else {
if (info === 'dtv') {
return 'downloads@latest'
} else {
return 'downloads'
}
}
}

async handle({ info, rubygem }) {
const splitRubygem = rubygem.split('/')
const repo = splitRubygem[0]
let version =
splitRubygem.length > 1 ? splitRubygem[splitRubygem.length - 1] : null
version = version === 'stable' ? version : semver.valid(version)
const label = this._getLabel(version, info)
const json = await this.fetch(repo, info)

let downloads
if (info === 'dt') {
downloads = metric(json.downloads)
} else if (info === 'dtv') {
downloads = metric(json.version_downloads)
} else if (info === 'dv') {
let versionData
if (version !== null && version === 'stable') {
const versions = json
.filter(function(ver) {
return ver.prerelease === false
})
.map(function(ver) {
return ver.number
})
// Found latest stable version.
const stableVersion = latestVersion(versions)
versionData = json.filter(function(ver) {
return ver.number === stableVersion
})[0]
downloads = metric(versionData.downloads_count)
} else if (version !== null) {
versionData = json.filter(function(ver) {
return ver.number === version
})[0]

downloads = metric(versionData.downloads_count)
} else {
throw new InvalidResponse({
underlyingError: new Error('version is null'),
})
}
} else {
throw new InvalidResponse({
underlyingError: new Error('info is invalid'),
})
}

return {
label: label,
message: downloads,
color: downloadCountColor(downloads),
}
}

// Metadata
static get defaultBadgeData() {
return { label: 'downloads' }
}

static get category() {
return 'downloads'
}

static get url() {
return {
base: 'gem',
format: '(dt|dtv|dv)/(.+)',
capture: ['info', 'rubygem'],
}
}

static get examples() {
return [
{
title: 'Gem',
previewUrl: 'dv/rails/stable',
keywords: ['ruby'],
},
{
title: 'Gem',
previewUrl: 'dv/rails/4.1.0',
keywords: ['ruby'],
},
{
title: 'Gem',
previewUrl: 'dtv/rails',
keywords: ['ruby'],
},
{
title: 'Gem',
previewUrl: 'dt/rails',
keywords: ['ruby'],
},
]
}
}
49 changes: 49 additions & 0 deletions services/gem/gem-owner.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict'

const Joi = require('joi')

const { BaseJsonService } = require('../base')
const { floorCount: floorCountColor } = require('../../lib/color-formatters')

module.exports = class GemOwner extends BaseJsonService {
async handle({ user }) {
const url = `https://rubygems.org/api/v1/owners/${user}/gems.json`
const json = await this._requestJson({
url,
schema: 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.

Looks like this one is pending ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally left this because in this badge we are not using any of the data in the array - we are just counting the number of elements in it. As long the response is an array we're fine. I've moved the declaration out of the class and made it required() in 410b329 though. I don't think we need to be any more specific

})
const count = json.length

return {
message: count,
color: floorCountColor(count, 10, 50, 100),
}
}

// Metadata
static get defaultBadgeData() {
return { label: 'gems' }
}

static get category() {
return 'other'
}

static get url() {
return {
base: 'gem/u',
format: '(.+)',
capture: ['user'],
}
}

static get examples() {
return [
{
title: 'Gems',
previewUrl: 'raphink',
keywords: ['ruby'],
},
]
}
}
Loading