-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-285964: Adds IPA rule 104 - Resource has get #301
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
Changes from 6 commits
a03356b
d509d03
4cb147f
6e3b711
cd65e45
2884f15
65a79db
99c6d47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"presets": ["@babel/preset-env"] | ||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import * as fs from 'node:fs'; | ||
import * as path from 'node:path'; | ||
import { describe, expect, it } from '@jest/globals'; | ||
import { Spectral, Document } from '@stoplight/spectral-core'; | ||
import { httpAndFileResolver } from '@stoplight/spectral-ref-resolver'; | ||
import { bundleAndLoadRuleset } from '@stoplight/spectral-ruleset-bundler/with-loader'; | ||
|
||
const rulesetPath = path.join(__dirname, '../..', 'ipa-spectral.yaml'); | ||
|
||
export default (ruleName, tests) => { | ||
describe(`Rule ${ruleName}`, () => { | ||
for (const testCase of tests) { | ||
it.concurrent(testCase.name, async () => { | ||
const s = await createSpectral(); | ||
const doc = testCase.document instanceof Document ? testCase.document : JSON.stringify(testCase.document); | ||
const allErrors = await s.run(doc); | ||
|
||
const errors = getErrorsForRule(allErrors, ruleName); | ||
|
||
expect(errors.length).toEqual(testCase.errors.length); | ||
|
||
errors.forEach((error, index) => { | ||
expect(error.code).toEqual(testCase.errors[index].code); | ||
expect(error.message).toEqual(testCase.errors[index].message); | ||
expect(error.path).toEqual(testCase.errors[index].path); | ||
}); | ||
}); | ||
} | ||
}); | ||
}; | ||
|
||
async function createSpectral() { | ||
const s = new Spectral({ resolver: httpAndFileResolver }); | ||
s.setRuleset(await bundleAndLoadRuleset(rulesetPath, { fs, fetch })); | ||
return s; | ||
} | ||
|
||
function getErrorsForRule(errors, rule) { | ||
return errors.filter((e) => e.code === rule); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import testRule from './__helpers__/testRule'; | ||
import { DiagnosticSeverity } from '@stoplight/types'; | ||
|
||
testRule('xgen-IPA-104-resource-has-GET', [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a case with nested resource? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
{ | ||
name: 'valid methods', | ||
document: { | ||
paths: { | ||
'/standard': { | ||
post: {}, | ||
get: {}, | ||
}, | ||
'/standard/{exampleId}': { | ||
get: {}, | ||
patch: {}, | ||
delete: {}, | ||
}, | ||
'/custom': { | ||
post: {}, | ||
get: {}, | ||
}, | ||
'/custom/{exampleId}': { | ||
get: {}, | ||
patch: {}, | ||
delete: {}, | ||
}, | ||
'/custom:method': { | ||
post: {}, | ||
}, | ||
'/singleton': { | ||
get: {}, | ||
}, | ||
}, | ||
}, | ||
errors: [], | ||
}, | ||
{ | ||
name: 'invalid methods', | ||
document: { | ||
paths: { | ||
'/standard': { | ||
post: {}, | ||
get: {}, | ||
}, | ||
'/standard/{exampleId}': { | ||
patch: {}, | ||
delete: {}, | ||
}, | ||
'/custom': { | ||
post: {}, | ||
get: {}, | ||
}, | ||
'/custom/{exampleId}': { | ||
patch: {}, | ||
delete: {}, | ||
}, | ||
'/custom:method': { | ||
post: {}, | ||
}, | ||
'/singleton': { | ||
patch: {}, | ||
}, | ||
}, | ||
}, | ||
errors: [ | ||
{ | ||
code: 'xgen-IPA-104-resource-has-GET', | ||
yelizhenden-mdb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
message: 'APIs must provide a get method for resources. http://go/ipa/117', | ||
path: ['paths', '/standard'], | ||
severity: DiagnosticSeverity.Warning, | ||
}, | ||
{ | ||
code: 'xgen-IPA-104-resource-has-GET', | ||
message: 'APIs must provide a get method for resources. http://go/ipa/117', | ||
path: ['paths', '/custom'], | ||
severity: DiagnosticSeverity.Warning, | ||
}, | ||
{ | ||
code: 'xgen-IPA-104-resource-has-GET', | ||
message: 'APIs must provide a get method for resources. http://go/ipa/117', | ||
path: ['paths', '/singleton'], | ||
severity: DiagnosticSeverity.Warning, | ||
}, | ||
], | ||
}, | ||
]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
extends: | ||
- ./rulesets/IPA-104.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# IPA-104: Get | ||
# http://go/ipa/104 | ||
|
||
functions: | ||
- eachResourceHasGetMethod | ||
|
||
rules: | ||
xgen-IPA-104-resource-has-GET: | ||
description: "APIs must provide a get method for resources. http://go/ipa/104" | ||
message: "{{error}} http://go/ipa/117" | ||
severity: warn | ||
given: "$.paths" | ||
then: | ||
field: "@key" | ||
function: "eachResourceHasGetMethod" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import { | ||
hasGetMethod, | ||
isChild, | ||
isCustomMethod, | ||
isStandardResource, | ||
isSingletonResource, | ||
getResourcePaths, | ||
} from './utils/resourceEvaluation.js'; | ||
|
||
const ERROR_MESSAGE = 'APIs must provide a get method for resources.'; | ||
|
||
export default (input, _, { documentInventory }) => { | ||
if (isChild(input) || isCustomMethod(input)) { | ||
return; | ||
} | ||
|
||
const oas = documentInventory.resolved; | ||
const resourcePaths = getResourcePaths(input, Object.keys(oas.paths)); | ||
|
||
if (isSingletonResource(resourcePaths)) { | ||
if (!hasGetMethod(oas.paths[resourcePaths[0]])) { | ||
return [ | ||
{ | ||
message: ERROR_MESSAGE, | ||
}, | ||
]; | ||
} | ||
} else if (isStandardResource(resourcePaths)) { | ||
if (!hasGetMethod(oas.paths[resourcePaths[1]])) { | ||
return [ | ||
{ | ||
message: ERROR_MESSAGE, | ||
}, | ||
]; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: What if it is not a singleton and standard resource?(The equivalent of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently we have no resources that fall into neither standard nor singleton (with the checks currently implemented) |
||
}; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we unit test these, and if not can we add some examples to things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, makes sense, I'll add unit tests for them as well (getResourcePaths, isStandardResource and isSingletonResource as they are probably the tricky ones) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
export function isChild(path) { | ||
return path.endsWith('}'); | ||
} | ||
|
||
export function isCustomMethod(path) { | ||
return path.includes(':'); | ||
} | ||
|
||
/** | ||
* Checks if a resource is a singleton resource ({@link https://docs.devprod.prod.corp.mongodb.com/ipa/113 IPA-113}) based on the paths for the | ||
* resource. The resource may have custom methods. | ||
* | ||
* @param resourcePaths all paths for the resource as an array of strings | ||
* @returns {boolean} | ||
*/ | ||
export function isSingletonResource(resourcePaths) { | ||
if (resourcePaths.length === 1) { | ||
return true; | ||
} | ||
const additionalPaths = resourcePaths.slice(1); | ||
return !additionalPaths.some((p) => !isCustomMethod(p)); | ||
} | ||
|
||
/** | ||
* Checks if a resource is a standard resource ({@link https://docs.devprod.prod.corp.mongodb.com/ipa/103 IPA-103}) based on the paths for the | ||
* resource. The resource may have custom methods. | ||
* | ||
* @param resourcePaths all paths for the resource as an array of strings | ||
* @returns {boolean} | ||
*/ | ||
export function isStandardResource(resourcePaths) { | ||
if (resourcePaths.length === 2 && isChild(resourcePaths[1])) { | ||
return true; | ||
} | ||
if (resourcePaths.length < 3 || !isChild(resourcePaths[1])) { | ||
return false; | ||
} | ||
const additionalPaths = resourcePaths.slice(2); | ||
return !additionalPaths.some((p) => !isCustomMethod(p)); | ||
} | ||
|
||
/** | ||
* Checks if a path object has a GET method | ||
* | ||
* @param pathObject the path object to evaluate | ||
* @returns {boolean} | ||
*/ | ||
export function hasGetMethod(pathObject) { | ||
return Object.keys(pathObject).some((o) => o === 'get'); | ||
} | ||
|
||
/** | ||
* Get all paths for a resource based on the parent path | ||
* | ||
* @param parent the parent path string | ||
* @param allPaths all paths as an array of strings | ||
* @returns {*} a string array of all paths for a resource, including the parent | ||
*/ | ||
export function getResourcePaths(parent, allPaths) { | ||
const childPathPattern = new RegExp(`^${parent}/{[a-zA-Z]+}$`); | ||
const customMethodPattern = new RegExp(`^${parent}/{[a-zA-Z]+}:+[a-zA-Z]+$`); | ||
return allPaths.filter((p) => parent === p || childPathPattern.test(p) || customMethodPattern.test(p)); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.