Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
3 changes: 3 additions & 0 deletions babel.config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"presets": ["@babel/preset-env"]
}
11 changes: 10 additions & 1 deletion eslint.config.mjs
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import globals from 'globals';
import pluginJs from '@eslint/js';
import pluginJest from 'eslint-plugin-jest';
import jest from 'eslint-plugin-jest';

/** @type {import('eslint').Linter.Config[]} */
export default [
{ languageOptions: { globals: globals.browser } },
{
plugins: { jest: pluginJest },
languageOptions: { globals: globals.node },
},
pluginJs.configs.recommended,
{
languageOptions: {
Expand All @@ -14,4 +19,8 @@ export default [
{
ignores: ['node-modules'],
},
{
files: ['**/*.test.js'],
...jest.configs['flat/recommended'],
},
];
8,582 changes: 7,876 additions & 706 deletions package-lock.json

Large diffs are not rendered by default.

20 changes: 19 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,33 @@
"scripts": {
"format": "npx prettier . --write",
"format-check": "npx prettier . --check",
"lint-js": "npx eslint **/*.js"
"lint-js": "npx eslint **/*.js",
"ipa-validation": "spectral lint ./openapi/v2.yaml --ruleset=./tools/spectral/ipa/ipa-spectral.yaml",
"test": "jest"
},
"jest": {
"transform": {
"^.+\\.[t|j]sx?$": "babel-jest"
},
"testPathIgnorePatterns": [
"__helpers__"
]
},
"dependencies": {
"@stoplight/spectral-cli": "^6.14.2",
"@stoplight/spectral-core": "^1.19.4",
"@stoplight/spectral-ref-resolver": "^1.0.5",
"@stoplight/spectral-ruleset-bundler": "^1.6.1",
"eslint-plugin-jest": "^28.9.0",
"openapi-to-postmanv2": "4.24.0"
},
"devDependencies": {
"@babel/preset-env": "^7.26.0",
"@eslint/js": "^9.16.0",
"@jest/globals": "^29.7.0",
"eslint": "^9.16.0",
"globals": "^15.13.0",
"jest": "^29.7.0",
"prettier": "3.4.2"
}
}
40 changes: 40 additions & 0 deletions tools/spectral/ipa/__tests__/__helpers__/testRule.js
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);
}
86 changes: 86 additions & 0 deletions tools/spectral/ipa/__tests__/eachResourceHasGetMethod.test.js
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', [
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a case with nested resource?
/groups/{groupId}/clusters? and a singleton like /groups/{groupId}/settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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',
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,
},
],
},
]);
2 changes: 2 additions & 0 deletions tools/spectral/ipa/ipa-spectral.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
extends:
- ./rulesets/IPA-104.yaml
15 changes: 15 additions & 0 deletions tools/spectral/ipa/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,
},
];
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 /custom in the tests). Don't we check for get method at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For /custom in the tests, we do check these as well. It's basically a standard resource with extra custom methods, in these cases, we ignore the custom methods (since they can be post or get) and only check the children (/custom/{exampleId} in the tests)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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)

};
Copy link
Contributor

Choose a reason for hiding this comment

The 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 isSingletonResource or isStandardResource it's a bit hard to follow the intention of the method and some examples either in text or as actual tests would be of great help in the future if we need to debug or change those functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 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 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));
}
Loading