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

Convert examples arrays to openApi objects (part 1) #9320

Merged
merged 18 commits into from
Jul 31, 2023
Merged

Convert examples arrays to openApi objects (part 1) #9320

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
0987a0f
add helper functions for generating Open API path/query params with d…
chris48s Jun 26, 2023
86b893a
tweak Open API schema
chris48s Jun 26, 2023
f6f201e
convert examples --> openApi in amo
chris48s Jun 26, 2023
db7a92b
convert examples --> openApi in ansible
chris48s Jun 26, 2023
8cec565
convert examples --> openApi in appveyor build/job
chris48s Jun 26, 2023
f886470
add re-usable Open API query param for test-results badges
chris48s Jun 26, 2023
d3d62c8
convert examples --> openApi in appveyor tests
chris48s Jun 26, 2023
f0fdd6d
DRY up existing dynamic/endpoint param definitions
chris48s Jun 26, 2023
5a5ceb4
DRY up queryParam
chris48s Jul 10, 2023
ecef6c8
allow enum param in serviceDefinition schema
chris48s Jul 10, 2023
a5cc620
improve misleading param name
chris48s Jul 10, 2023
5be537a
check route and openApi are consistent on service load
chris48s Jul 10, 2023
0f770a9
fix mistake in ansible role route
chris48s Jul 10, 2023
b1de47f
documentation --> description
chris48s Jul 29, 2023
8c669dd
add pathParams and queryParams helpers +docstrings
chris48s Jul 29, 2023
2d40770
give everything a search-friendly summary, check for duplicate summary
chris48s Jul 29, 2023
f06f0b1
Merge branch 'master' into 9285-part1
chris48s Jul 29, 2023
7d05fb6
prettier fixup
chris48s Jul 29, 2023
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
3 changes: 3 additions & 0 deletions core/base-service/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
Deprecated,
ImproperlyConfigured,
} from './errors.js'
import { pathParam, queryParam } from './openapi.js'

export {
BaseService,
Expand All @@ -32,4 +33,6 @@ export {
InvalidParameter,
ImproperlyConfigured,
Deprecated,
pathParam,
queryParam,
}
32 changes: 31 additions & 1 deletion core/base-service/openapi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,4 +332,34 @@ function category2openapi(category, services) {
return spec
}

export { category2openapi }
function pathParam({
name,
example,
schema = { type: 'string' },
description,
}) {
return { name, in: 'path', required: true, schema, example, description }
}

function queryParam({
name,
example,
schema = { type: 'string' },
required = false,
description,
}) {
chris48s marked this conversation as resolved.
Show resolved Hide resolved
if (example === null && schema.type === 'boolean') {
return {
name,
in: 'query',
required,
schema,
allowEmptyValue: true,
example,
description,
}
}
return { name, in: 'query', required, schema, example, description }
chris48s marked this conversation as resolved.
Show resolved Hide resolved
}

export { category2openapi, pathParam, queryParam }
82 changes: 81 additions & 1 deletion core/base-service/openapi.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import chai from 'chai'
import { category2openapi } from './openapi.js'
import { category2openapi, pathParam, queryParam } from './openapi.js'
import BaseJsonService from './base-json.js'
const { expect } = chai

Expand Down Expand Up @@ -376,3 +376,83 @@ describe('category2openapi', function () {
).to.deep.equal(expected)
})
})

describe('pathParam', function () {
it('generates a pathParam with defaults', function () {
expect(pathParam({ name: 'name', example: 'example' })).to.deep.equal({
name: 'name',
in: 'path',
required: true,
schema: {
type: 'string',
},
example: 'example',
description: undefined,
})
})

it('generates a pathParam with custom args', function () {
expect(
pathParam({
name: 'name',
example: true,
schema: { type: 'boolean' },
description: 'long desc',
})
).to.deep.equal({
name: 'name',
in: 'path',
required: true,
schema: {
type: 'boolean',
},
example: true,
description: 'long desc',
})
})
})

describe('queryParam', function () {
it('generates a queryParam with defaults', function () {
expect(queryParam({ name: 'name', example: 'example' })).to.deep.equal({
name: 'name',
in: 'query',
required: false,
schema: { type: 'string' },
example: 'example',
description: undefined,
})
})

it('generates queryParam with custom args', function () {
expect(
queryParam({
name: 'name',
example: 'example',
required: true,
description: 'long desc',
})
).to.deep.equal({
name: 'name',
in: 'query',
required: true,
schema: { type: 'string' },
example: 'example',
description: 'long desc',
})
})

it('generates a queryParam with boolean/null example', function () {
expect(
queryParam({ name: 'name', example: null, schema: { type: 'boolean' } })
).to.deep.equal({
name: 'name',
in: 'query',
required: false,
schema: { type: 'boolean' },
allowEmptyValue: true,
example: null,
description: undefined,
})
})
})
5 changes: 3 additions & 2 deletions core/base-service/service-definitions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const serviceDefinition = Joi.object({
Joi.object({
get: Joi.object({
summary: Joi.string().required(),
description: Joi.string().required(),
description: Joi.string(),
parameters: Joi.array()
.items(
Joi.object({
Expand All @@ -57,7 +57,8 @@ const serviceDefinition = Joi.object({
in: Joi.string().valid('query', 'path').required(),
required: Joi.boolean().required(),
schema: Joi.object({ type: Joi.string().required() }).required(),
example: Joi.string(),
allowEmptyValue: Joi.boolean(),
example: Joi.string().allow(null),
})
)
.min(1)
Expand Down
20 changes: 10 additions & 10 deletions services/amo/amo-downloads.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { renderDownloadsBadge } from '../downloads.js'
import { redirector } from '../index.js'
import { BaseAmoService, keywords } from './amo-base.js'
import { redirector, pathParam } from '../index.js'
import { BaseAmoService } from './amo-base.js'

const documentation = `
Previously \`amo/d\` provided a “total downloads” badge. However,
Expand All @@ -12,15 +12,15 @@ class AmoWeeklyDownloads extends BaseAmoService {
static category = 'downloads'
static route = { base: 'amo/dw', pattern: ':addonId' }

static examples = [
{
title: 'Mozilla Add-on',
namedParams: { addonId: 'dustman' },
staticPreview: this.render({ downloads: 120 }),
keywords,
documentation,
static openApi = {
'/amo/dw/{addonId}': {
get: {
summary: 'Mozilla Add-on Downloads',
description: documentation,
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
]
}

static _cacheLength = 21600

Expand Down
29 changes: 14 additions & 15 deletions services/amo/amo-rating.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,26 @@
import { starRating } from '../text-formatters.js'
import { floorCount as floorCountColor } from '../color-formatters.js'
import { BaseAmoService, keywords } from './amo-base.js'
import { pathParam } from '../index.js'
import { BaseAmoService } from './amo-base.js'

export default class AmoRating extends BaseAmoService {
static category = 'rating'
static route = { base: 'amo', pattern: ':format(stars|rating)/:addonId' }

static examples = [
{
title: 'Mozilla Add-on',
pattern: 'rating/:addonId',
namedParams: { addonId: 'dustman' },
staticPreview: this.render({ format: 'rating', rating: 4 }),
keywords,
static openApi = {
'/amo/rating/{addonId}': {
Copy link
Member Author

Choose a reason for hiding this comment

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

There is some duplication here between the route definition and the object keys here. My assumption was this should be fixable (refs #8966 (comment) ). Unfortunately this proved hard to fix because we can't use an expression here. These object keys can only be a literal. So no function calls, template strings, etc here. I think if we want to use bits of the route definition, we need to make openApi a function rather than a static class property. My gut instinct is that accepting the duplication is actually the lesser evil here, but open to thoughts on it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm currently ambivalent on it. In cases like this where we had an enum-like route param in the pattern it doesn't seem like much of a change from what we were doing before.

However, in cases where we have examples relying on the previously defined route it does feel like we're introducing duplication. I think I can live with that though, so long as we've still got something automated that can be used to validate the validity of these routes, and be easy to check both locally and CI.

I.e. If we ever have a small PR that updates a route path, I don't want to have to rely on human reviewers to remember to check the spec (which may or may not be part of the diff)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. So maybe this wasn't the right place to open this discussion point.

If we take format(stars|rating) out of the equation for a moment:
What I'm really getting at here is that writing something like

static openApi = {
  '/' + this.route.base + '/{param}': //some object
}

or

static openApi = {
  `/${this.route.base}/{param}`: //some object
}

is not legal javascript. We'd have to make the openApi object something that is returned from a function call to do something like that. That was the original point.

Then if we re-introduce format(stars|rating) to the conversation.

Basically the reason why I've done this like this is because that's how it was before: It was a one-to-one replacement. Also partly because I've not really fully tackled enums yet.

However, instead of writing:

static openApi = {
  '/amo/rating/{addonId}': {
    get: {
      summary: 'Mozilla Add-on Rating',
      parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
    },
  },
  '/amo/stars/{addonId}': {
    get: {
      summary: 'Mozilla Add-on Stars',
      parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
    },
  },
}

We could write this instead:

static openApi = {
  '/amo/{format}/{addonId}': {
    get: {
      summary: 'Mozilla Add-on Rating',
      parameters: [
        pathParam({
          name: 'format',
          example: 'stars',
          schema: { type: 'string', enum: ['stars', 'rating'] },
        }),
        pathParam({ name: 'addonId', example: 'dustman' }),
      ],
    },
  },
}

..and then I think (untested) we should be able to add another helper to derive the allowed enum values from the route. Tbc. As I say, I've not really tackled this case yet.

We could say we always represent a :param(this|that) as an enum and only use multiple examples when we have an optional param (e.g: :branch*). I guess which of those options we prefer is partly a stylistic decision though, or what is most useful to show the user. As opposed to always one-to-one mapping with the route.

Copy link
Member

Choose a reason for hiding this comment

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

Do you feel like the latter option might make it easier for us to more have a single example in the badge listing (e.g. a single listing for a downloads badge with the interval param vs. a listing per interval) if/when we want?

Or, do you anticipate we'd like to maintain individual listings per variant and then keep that component of the route static in each of the corresponding builders?

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 think in most cases, probably optimising for fewer different pages/menu items in the menu on the left with more options to customise is going to be the better option.

get: {
summary: 'Mozilla Add-on Rating',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
{
title: 'Mozilla Add-on',
pattern: 'stars/:addonId',
namedParams: { addonId: 'dustman' },
staticPreview: this.render({ format: 'stars', rating: 4 }),
keywords,
'/amo/stars/{addonId}': {
get: {
summary: 'Mozilla Add-on Stars',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
]
}

static _cacheLength = 7200

Expand Down
17 changes: 9 additions & 8 deletions services/amo/amo-users.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import { renderDownloadsBadge } from '../downloads.js'
import { BaseAmoService, keywords } from './amo-base.js'
import { pathParam } from '../index.js'
import { BaseAmoService } from './amo-base.js'

export default class AmoUsers extends BaseAmoService {
static category = 'downloads'
static route = { base: 'amo/users', pattern: ':addonId' }

static examples = [
{
title: 'Mozilla Add-on',
namedParams: { addonId: 'dustman' },
staticPreview: this.render({ users: 750 }),
keywords,
static openApi = {
'/amo/users/{addonId}': {
get: {
summary: 'Mozilla Add-on Users',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
]
}

static _cacheLength = 21600

Expand Down
17 changes: 9 additions & 8 deletions services/amo/amo-version.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import { renderVersionBadge } from '../version.js'
import { BaseAmoService, keywords } from './amo-base.js'
import { pathParam } from '../index.js'
import { BaseAmoService } from './amo-base.js'

export default class AmoVersion extends BaseAmoService {
static category = 'version'
static route = { base: 'amo/v', pattern: ':addonId' }

static examples = [
{
title: 'Mozilla Add-on',
namedParams: { addonId: 'dustman' },
staticPreview: renderVersionBadge({ version: '2.1.0' }),
keywords,
static openApi = {
'/amo/v/{addonId}': {
get: {
summary: 'Mozilla Add-on',
parameters: [pathParam({ name: 'addonId', example: 'dustman' })],
},
},
]
}

async handle({ addonId }) {
const data = await this.fetch({ addonId })
Expand Down
17 changes: 8 additions & 9 deletions services/ansible/ansible-collection.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Joi from 'joi'
import { BaseJsonService } from '../index.js'
import { BaseJsonService, pathParam } from '../index.js'

const ansibleCollectionSchema = Joi.object({
name: Joi.string().required(),
Expand All @@ -12,15 +12,14 @@ class AnsibleGalaxyCollectionName extends BaseJsonService {
static category = 'other'
static route = { base: 'ansible/collection', pattern: ':collectionId' }

static examples = [
{
title: 'Ansible Collection',
namedParams: { collectionId: '278' },
staticPreview: this.render({
name: 'community.general',
}),
static openApi = {
'/ansible/collection/{collectionId}': {
get: {
summary: 'Ansible Collection',
parameters: [pathParam({ name: 'collectionId', example: '278' })],
},
},
]
}

static defaultBadgeData = { label: 'collection' }

Expand Down
15 changes: 7 additions & 8 deletions services/ansible/ansible-quality.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Joi from 'joi'
import { floorCount } from '../color-formatters.js'
import { BaseJsonService, InvalidResponse } from '../index.js'
import { BaseJsonService, InvalidResponse, pathParam } from '../index.js'

const ansibleContentSchema = Joi.object({
quality_score: Joi.number().allow(null).required(),
Expand All @@ -20,15 +20,14 @@ export default class AnsibleGalaxyContentQualityScore extends AnsibleGalaxyConte
static category = 'analysis'
static route = { base: 'ansible/quality', pattern: ':projectId' }

static examples = [
{
title: 'Ansible Quality Score',
namedParams: {
projectId: '432',
static openApi = {
'/ansible/quality/{projectId}': {
get: {
summary: 'Ansible Quality Score',
parameters: [pathParam({ name: 'projectId', example: '432' })],
},
staticPreview: this.render({ qualityScore: 4.125 }),
},
]
}

static defaultBadgeData = { label: 'quality' }

Expand Down
Loading