Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,26 @@ testRule('xgen-IPA-102-path-alternate-resource-name-path-param', [
},
],
},
{
name: 'invalid paths with exceptions',
document: {
paths: {
'/api/atlas/v2/unauth/resourceName1/resourceName2': {
'x-xgen-IPA-exception': {
'xgen-IPA-102-path-alternate-resource-name-path-param': {
reason: 'test',
},
},
},
'/api/atlas/v2/resourceName/{pathParam1}/{pathParam2}': {
'x-xgen-IPA-exception': {
'xgen-IPA-102-path-alternate-resource-name-path-param': {
reason: 'test',
},
},
},
},
},
errors: [],
Comment on lines +137 to +157
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expect no errors when component has exception

},
]);
21 changes: 21 additions & 0 deletions tools/spectral/ipa/__tests__/eachResourceHasGetMethod.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,25 @@ testRule('xgen-IPA-104-resource-has-GET', [
},
],
},
{
name: 'invalid method with exception',
document: {
paths: {
'/standard': {
post: {},
get: {},
'x-xgen-IPA-exception': {
'xgen-IPA-104-resource-has-GET': {
reason: 'test',
},
},
},
'/standard/{exampleId}': {
patch: {},
delete: {},
},
},
},
errors: [],
},
Comment on lines +128 to +148
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expect no errors when component has exception

]);
122 changes: 122 additions & 0 deletions tools/spectral/ipa/__tests__/exceptionExtensionFormat.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import testRule from './__helpers__/testRule';
import { DiagnosticSeverity } from '@stoplight/types';

testRule('xgen-IPA-005-exception-extension-format', [
{
name: 'valid exceptions',
document: {
paths: {
'/path': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {
reason: 'Exception',
},
},
},
'/nested': {
post: {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {
reason: 'Exception',
},
},
},
},
},
},
errors: [],
},
{
name: 'invalid exceptions',
document: {
paths: {
'/path1': {
'x-xgen-IPA-exception': 'Exception',
},
'/path2': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': 'Exception',
},
},
'/path3': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {
reason: '',
},
},
},
'/path4': {
'x-xgen-IPA-exception': {
'invalid-rule-name': {
reason: 'Exception',
},
},
},
'/path5': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {
wrongKey: 'Exception',
},
},
},
'/path6': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {
reason: 'Exception',
excessKey: 'Exception',
},
},
},
'/path7': {
'x-xgen-IPA-exception': {
'xgen-IPA-100-rule-name': {},
},
},
},
},
errors: [
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path1', 'x-xgen-IPA-exception'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path2', 'x-xgen-IPA-exception', 'xgen-IPA-100-rule-name'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path3', 'x-xgen-IPA-exception', 'xgen-IPA-100-rule-name'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path4', 'x-xgen-IPA-exception', 'invalid-rule-name'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path5', 'x-xgen-IPA-exception', 'xgen-IPA-100-rule-name'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path6', 'x-xgen-IPA-exception', 'xgen-IPA-100-rule-name'],
severity: DiagnosticSeverity.Warning,
},
{
code: 'xgen-IPA-005-exception-extension-format',
message: 'IPA exceptions must have a valid rule name and a reason. http://go/ipa/5',
path: ['paths', '/path7', 'x-xgen-IPA-exception', 'xgen-IPA-100-rule-name'],
severity: DiagnosticSeverity.Warning,
},
],
},
]);
67 changes: 67 additions & 0 deletions tools/spectral/ipa/__tests__/utils/exceptions.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { describe, expect, it } from '@jest/globals';
import { hasException } from '../../rulesets/functions/utils/exceptions';

const TEST_RULE_NAME_100 = 'xgen-IPA-100';

const objectWithIpa100Exception = {
'x-xgen-IPA-exception': {
'xgen-IPA-100': {
reason: 'test',
},
},
};

const objectWithNestedIpa100Exception = {
get: {
'x-xgen-IPA-exception': {
'xgen-IPA-100': {
reason: 'test',
},
},
},
};

const objectWithIpa100ExceptionAndOwnerExtension = {
'x-xgen-IPA-exception': {
'xgen-IPA-100': {
reason: 'test',
},
},
'x-xgen-owner-team': 'apix',
};

const objectWithIpa101Exception = {
'x-xgen-IPA-exception': {
'xgen-IPA-101': {
reason: 'test',
},
},
};

const objectWithIpa100And101Exception = {
'x-xgen-IPA-exception': {
'xgen-IPA-101': {
reason: 'test',
},
'xgen-IPA-100': {
reason: 'test',
},
},
};

describe('tools/spectral/ipa/rulesets/functions/utils/exceptions.js', () => {
describe('hasException', () => {
it('returns true if object has exception matching the rule name', () => {
expect(hasException(objectWithIpa100Exception, TEST_RULE_NAME_100)).toBe(true);
expect(hasException(objectWithIpa100ExceptionAndOwnerExtension, TEST_RULE_NAME_100)).toBe(true);
expect(hasException(objectWithIpa100And101Exception, TEST_RULE_NAME_100)).toBe(true);
});
it('returns false if object does not have exception matching the rule name', () => {
expect(hasException({}, TEST_RULE_NAME_100)).toBe(false);
expect(hasException(objectWithIpa101Exception, TEST_RULE_NAME_100)).toBe(false);
});
it('returns false if object has nested exception matching the rule name', () => {
expect(hasException(objectWithNestedIpa100Exception, TEST_RULE_NAME_100)).toBe(false);
});
});
});
1 change: 1 addition & 0 deletions tools/spectral/ipa/ipa-spectral.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
extends:
- ./rulesets/IPA-102.yaml
- ./rulesets/IPA-104.yaml
- ./rulesets/IPA-005.yaml
14 changes: 14 additions & 0 deletions tools/spectral/ipa/rulesets/IPA-005.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# IPA-5: Documenting Exceptions to IPAs
# http://go/ipa/5

functions:
- exceptionExtensionFormat

rules:
xgen-IPA-005-exception-extension-format:
description: 'IPA exception extensions must follow the correct format. http://go/ipa/5'
message: '{{error}} http://go/ipa/5'
severity: warn
given: '$..x-xgen-IPA-exception'
then:
function: 'exceptionExtensionFormat'
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import { isPathParam } from './utils/pathUtils.js';
import { hasException } from './utils/exceptions';

const RULE_NAME = 'xgen-IPA-102-path-alternate-resource-name-path-param';
const ERROR_MESSAGE = 'API paths must alternate between resource name and path params.';
const ERROR_RESULT = [{ message: ERROR_MESSAGE }];
const AUTH_PREFIX = '/api/atlas/v2';
const UNAUTH_PREFIX = '/api/atlas/v2/unauth';

const getPrefix = (path) => {
if (path.includes(UNAUTH_PREFIX)) return UNAUTH_PREFIX;
if (path.includes(AUTH_PREFIX)) return AUTH_PREFIX;
if (path.includes(UNAUTH_PREFIX)) {
return UNAUTH_PREFIX;
}
if (path.includes(AUTH_PREFIX)) {
return AUTH_PREFIX;
}
return null;
};

Expand All @@ -18,9 +24,16 @@ const validatePathStructure = (elements) => {
});
};

export default (input) => {
export default (input, _, { documentInventory }) => {
const oas = documentInventory.resolved;
if (hasException(oas.paths[input], RULE_NAME)) {
return;
}

const prefix = getPrefix(input);
if (!prefix) return;
if (!prefix) {
return;
}

let suffixWithLeadingSlash = input.slice(prefix.length);
if (suffixWithLeadingSlash.length === 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
isSingletonResource,
getResourcePaths,
} from './utils/resourceEvaluation.js';
import { hasException } from './utils/exceptions';

const RULE_NAME = 'xgen-IPA-104-resource-has-GET';
const ERROR_MESSAGE = 'APIs must provide a get method for resources.';

export default (input, _, { documentInventory }) => {
Expand All @@ -15,6 +17,11 @@ export default (input, _, { documentInventory }) => {
}

const oas = documentInventory.resolved;

if (hasException(oas.paths[input], RULE_NAME)) {
return;
}

const resourcePaths = getResourcePaths(input, Object.keys(oas.paths));

if (isSingletonResource(resourcePaths)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const ERROR_MESSAGE = 'IPA exceptions must have a valid rule name and a reason.';
const RULE_NAME_PREFIX = 'xgen-IPA-';
const REASON_KEY = 'reason';

// Note: This rule does not allow exceptions
export default (input, _, { path }) => {
const exemptedRules = Object.keys(input);
const errors = [];

exemptedRules.forEach((ruleName) => {
const exception = input[ruleName];
if (!isValidException(ruleName, exception)) {
errors.push({
path: path.concat([ruleName]),
message: ERROR_MESSAGE,
});
}
});

return errors;
};

function isValidException(ruleName, exception) {
const exceptionObjectKeys = Object.keys(exception);
return (
ruleName.startsWith(RULE_NAME_PREFIX) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Should we check if the ruleName is among the rules defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was thinking about it but couldn't figure out how to do it without having to introduce a list of some sorts with the rules names, that would be manually updated any time there are new rules, I'll have a think if we can achieve this in some way

Copy link
Collaborator

@yelizhenden-mdb yelizhenden-mdb Dec 18, 2024

Choose a reason for hiding this comment

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

Yes, I think you would need another helper function which loads ipa-spectral.yaml to a Spectral object and fetch the rule names from there

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine by now, it can be an improvement for later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, I can create a ticket for consideration as a nice to have later

exceptionObjectKeys.length === 1 &&
exceptionObjectKeys.includes(REASON_KEY) &&
exception[REASON_KEY] !== ''
);
}
15 changes: 15 additions & 0 deletions tools/spectral/ipa/rulesets/functions/utils/exceptions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const EXCEPTION_EXTENSION = 'x-xgen-IPA-exception';

/**
* Checks if the object has an exception extension "x-xgen-IPA-exception"
*
* @param object the object to evaluate
* @param ruleName the name of the exempted rule
* @returns {boolean} true if the object has an exception named ruleName, otherwise false
*/
export function hasException(object, ruleName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Do we accept if the exception is not in the correct format?
Because the exception format will be evaluated within the scope of another rule, and it will not affect the acceptance of the rule exception. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I decided not to check the format here since we have a rule that covers it already, so here we assume the extension has keys with the rule names

if (object[EXCEPTION_EXTENSION]) {
return Object.keys(object[EXCEPTION_EXTENSION]).includes(ruleName);
}
return false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export function hasGetMethod(pathObject) {
*
* @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
* @returns {string[]} all paths for a resource, including the parent
*/
export function getResourcePaths(parent, allPaths) {
const childPathPattern = new RegExp(`^${parent}/{[a-zA-Z]+}$`);
Expand Down
Loading