Skip to content

Commit

Permalink
Merge branch 'master' into issue2510-part4
Browse files Browse the repository at this point in the history
  • Loading branch information
@chris48s
chris48s committed Jan 26, 2019
2 parents eb1aeed + 47e8cc3 commit c90bd9d
Show file tree
Hide file tree
Showing 55 changed files with 283 additions and 214 deletions.
6 changes: 4 additions & 2 deletions core/base-service/base-non-memory-caching.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const BaseService = require('./base')
const { setCacheHeaders } = require('./cache-headers')
const { makeSend } = require('./legacy-result-sender')
const coalesceBadge = require('./coalesce-badge')
const { prepareRoute, namedParamsForMatch } = require('./route')

// Badges are subject to two independent types of caching: in-memory and
// downstream.
Expand All @@ -26,9 +27,10 @@ module.exports = class NonMemoryCachingBaseService extends BaseService {
static register({ camp }, serviceConfig) {
const { cacheHeaders: cacheHeaderConfig } = serviceConfig
const { _cacheLength: serviceDefaultCacheLengthSeconds } = this
const { regex, captureNames } = prepareRoute(this.route)

camp.route(this._regex, async (queryParams, match, end, ask) => {
const namedParams = this._namedParamsForMatch(match)
camp.route(regex, async (queryParams, match, end, ask) => {
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
{},
serviceConfig,
Expand Down
6 changes: 4 additions & 2 deletions core/base-service/base-static.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ const {
} = require('./cache-headers')
const { makeSend } = require('./legacy-result-sender')
const coalesceBadge = require('./coalesce-badge')
const { prepareRoute, namedParamsForMatch } = require('./route')

module.exports = class BaseStaticService extends BaseService {
static register({ camp }, serviceConfig) {
const {
profiling: { makeBadge: shouldProfileMakeBadge },
} = serviceConfig
const { regex, captureNames } = prepareRoute(this.route)

camp.route(this._regex, async (queryParams, match, end, ask) => {
camp.route(regex, async (queryParams, match, end, ask) => {
analytics.noteRequest(queryParams, match)

if (serverHasBeenUpSinceResourceCached(ask.req)) {
Expand All @@ -26,7 +28,7 @@ module.exports = class BaseStaticService extends BaseService {
return
}

const namedParams = this._namedParamsForMatch(match)
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
{},
serviceConfig,
Expand Down
85 changes: 15 additions & 70 deletions core/base-service/base.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

// See available emoji at http://emoji.muan.co/
const emojic = require('emojic')
const pathToRegexp = require('path-to-regexp')
const Joi = require('joi')
const { checkErrorResponse } = require('../../lib/error-helper')
const { assertValidCategory } = require('../../services/categories')
Expand All @@ -14,6 +13,12 @@ const {
InvalidParameter,
Deprecated,
} = require('./errors')
const {
makeFullUrl,
assertValidRoute,
prepareRoute,
namedParamsForMatch,
} = require('./route')
const { assertValidServiceDefinition } = require('./service-definitions')
const trace = require('./trace')
const { validateExample, transformExample } = require('./transform-example')
Expand Down Expand Up @@ -150,13 +155,11 @@ class BaseService {
return []
}

static _makeFullUrl(partialUrl) {
return `/${[this.route.base, partialUrl].filter(Boolean).join('/')}`
}

static validateDefinition() {
assertValidCategory(this.category, `Category for ${this.name}`)

assertValidRoute(this.route, `Route for ${this.name}`)

Joi.assert(
this.defaultBadgeData,
defaultBadgeDataSchema,
Expand All @@ -171,9 +174,9 @@ class BaseService {
static getDefinition() {
const { category, name, isDeprecated } = this

let format, pattern, queryParams
let base, format, pattern, queryParams
try {
;({ format, pattern, query: queryParams = [] } = this.route)
;({ base, format, pattern, query: queryParams = [] } = this.route)
} catch (e) {
// Legacy services do not have a route.
}
Expand All @@ -184,7 +187,7 @@ class BaseService {

let route
if (pattern) {
route = { pattern: this._makeFullUrl(pattern), queryParams }
route = { pattern: makeFullUrl(base, pattern), queryParams }
} else if (format) {
route = { format, queryParams }
} else {
Expand All @@ -198,44 +201,6 @@ class BaseService {
return result
}

static get _regexFromPath() {
const { pattern } = this.route
const fullPattern = `${this._makeFullUrl(
pattern
)}.:ext(svg|png|gif|jpg|json)`

const keys = []
const regex = pathToRegexp(fullPattern, keys, {
strict: true,
sensitive: true,
})
const capture = keys.map(item => item.name).slice(0, -1)

return { regex, capture }
}

static get _regex() {
const { pattern, format, capture } = this.route
if (
pattern !== undefined &&
(format !== undefined || capture !== undefined)
) {
throw Error(
`Since the route for ${
this.name
} includes a pattern, it should not include a format or capture`
)
} else if (pattern !== undefined) {
return this._regexFromPath.regex
} else if (format !== undefined) {
return new RegExp(
`^${this._makeFullUrl(this.route.format)}\\.(svg|png|gif|jpg|json)$`
)
} else {
throw Error(`The route for ${this.name} has neither pattern nor format`)
}
}

static get _cacheLength() {
const cacheLengths = {
build: 30,
Expand All @@ -246,28 +211,6 @@ class BaseService {
return cacheLengths[this.category]
}

static _namedParamsForMatch(match) {
const { pattern, capture } = this.route
const names = pattern ? this._regexFromPath.capture : capture || []

// Assume the last match is the format, and drop match[0], which is the
// entire match.
const captures = match.slice(1, -1)

if (names.length !== captures.length) {
throw new Error(
`Service ${this.name} declares incorrect number of capture groups ` +
`(expected ${names.length}, got ${captures.length})`
)
}

const result = {}
names.forEach((name, index) => {
result[name] = captures[index]
})
return result
}

_handleError(error) {
if (error instanceof NotFound || error instanceof InvalidParameter) {
trace.logTrace('outbound', emojic.noGoodWoman, 'Handled error', error)
Expand Down Expand Up @@ -344,12 +287,14 @@ class BaseService {

static register({ camp, handleRequest, githubApiProvider }, serviceConfig) {
const { cacheHeaders: cacheHeaderConfig, fetchLimitBytes } = serviceConfig
const { regex, captureNames } = prepareRoute(this.route)

camp.route(
this._regex,
regex,
handleRequest(cacheHeaderConfig, {
queryParams: this.route.queryParams,
handler: async (queryParams, match, sendBadge, request) => {
const namedParams = this._namedParamsForMatch(match)
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
{
sendAndCacheRequest: request.asPromise,
Expand Down
124 changes: 1 addition & 123 deletions core/base-service/base.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const Joi = require('joi')
const { expect } = require('chai')
const { test, given, forCases } = require('sazerac')
const sinon = require('sinon')
const trace = require('./trace')

Expand Down Expand Up @@ -60,6 +59,7 @@ class DummyService extends BaseService {
},
]
}

static get route() {
return {
base: 'foo',
Expand All @@ -72,128 +72,6 @@ class DummyService extends BaseService {
describe('BaseService', function() {
const defaultConfig = { handleInternalErrors: false }

describe('URL pattern matching', function() {
context('A `pattern` with a named param is declared', function() {
const regexExec = str => DummyService._regex.exec(str)
const getNamedParamA = str => {
const [, namedParamA] = regexExec(str)
return namedParamA
}
const namedParams = str => {
const match = regexExec(str)
return DummyService._namedParamsForMatch(match)
}

test(regexExec, () => {
forCases([
given('/foo/bar.bar.bar.zip'),
given('/foo/bar/bar.svg'),
// This is a valid example with the wrong extension separator, to
// test that we only accept a `.`.
given('/foo/bar.bar.bar_svg'),
]).expect(null)
})

test(getNamedParamA, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect('bar.bar.bar')
})

test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({ namedParamA: 'bar.bar.bar' })
})
})

context('A `format` with a named param is declared', function() {
class ServiceWithFormat extends BaseService {
static get route() {
return {
base: 'foo',
format: '([^/]+)',
capture: ['namedParamA'],
}
}
}

const regexExec = str => ServiceWithFormat._regex.exec(str)
const getNamedParamA = str => {
const [, namedParamA] = regexExec(str)
return namedParamA
}
const namedParams = str => {
const match = regexExec(str)
return ServiceWithFormat._namedParamsForMatch(match)
}

test(regexExec, () => {
forCases([
given('/foo/bar.bar.bar.zip'),
given('/foo/bar/bar.svg'),
// This is a valid example with the wrong extension separator, to
// test that we only accept a `.`.
given('/foo/bar.bar.bar_svg'),
]).expect(null)
})

test(getNamedParamA, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect('bar.bar.bar')
})

test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({ namedParamA: 'bar.bar.bar' })
})
})

context('No named params are declared', function() {
class ServiceWithZeroNamedParams extends BaseService {
static get route() {
return {
base: 'foo',
format: '(?:[^/]+)',
}
}
}

const namedParams = str => {
const match = ServiceWithZeroNamedParams._regex.exec(str)
return ServiceWithZeroNamedParams._namedParamsForMatch(match)
}

test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({})
})
})
})

it('Invokes the handler as expected', async function() {
expect(
await DummyService.invoke(
Expand Down

0 comments on commit c90bd9d

Please sign in to comment.