-
Notifications
You must be signed in to change notification settings - Fork 14
CLOUDP-288860: Support IPA exception and validate exception extensions #321
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 all commits
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 |
---|---|---|
|
@@ -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
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. Expect no errors when component has exception |
||
]); |
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, | ||
}, | ||
], | ||
}, | ||
]); |
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); | ||
}); | ||
}); | ||
}); |
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 |
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 |
---|---|---|
@@ -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) && | ||
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: Should we check if the 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. 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 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. Yes, I think you would need another helper function which loads 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. I think it is fine by now, it can be an improvement for later 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. Cool, I can create a ticket for consideration as a nice to have later |
||
exceptionObjectKeys.length === 1 && | ||
exceptionObjectKeys.includes(REASON_KEY) && | ||
exception[REASON_KEY] !== '' | ||
); | ||
} |
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) { | ||
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: Do we accept if the exception is not in the correct format? 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. 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; | ||
} |
There was a problem hiding this comment.
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