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

Added new rule to detect assertions where statusCode is in the parameter #97

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Added new rule to detect assertions where statusCode is in the parameter #97

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
efc924c
Added new rule to detect assertions where statusCode is in the parameter
jpolavar Nov 25, 2024
7c24b6b
Updated description
jpolavar Nov 25, 2024
cf98fa1
Updated rule messages
jpolavar Nov 25, 2024
6a0fa5b
Updated rule messages
jpolavar Nov 26, 2024
77b0ed7
updated PR comments
jpolavar Dec 5, 2024
d7e6061
Added a valid test case
jpolavar Dec 5, 2024
4a2a2a9
Merge from main
jpolavar Dec 19, 2024
7681452
Updated to report errors for any StatusCodes in asserts
jpolavar Dec 20, 2024
fb7a181
Updated to report errors for any StatusCodes in asserts
jpolavar Dec 20, 2024
0ac0fef
Updated to report errors for any StatusCodes in asserts
jpolavar Dec 20, 2024
2ab6ac9
Merged latest changes from main
jpolavar Dec 20, 2024
4ff304a
Remove unused numeric codes check
jpolavar Dec 23, 2024
d626c8f
Remove unused numeric codes check
jpolavar Dec 23, 2024
2306703
merged conflicts
jpolavar Jan 2, 2025
2590d5e
Added functionality to report error if found any numeric status codes
jpolavar Jan 6, 2025
8855abb
Added functionality to report error if found any numeric status codes
jpolavar Jan 7, 2025
7b575ed
Merge branch 'main' into no-status-assert-rule
jpolavar Jan 8, 2025
2af864a
Merged main
jpolavar Jan 8, 2025
d238a68
Merged main
jpolavar Jan 8, 2025
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
15 changes: 15 additions & 0 deletions docs/rules/no-status-code-assert.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Disallow the use of `statusCode` as a parameter in assert.

## Fail

```js
assert(statusCode === 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

update to reflect the latest implementation?

Copy link
Contributor Author

@jpolavar jpolavar Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@carlansley should we have rule to auto fix the prod code with statusCode if we have numbers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I can answer that, can we add a little more description to the issue/PR about what this entire rule is needed for? I'm sure this was discussed at some point but tbh I don't recall what the original problem was.

Copy link
Contributor Author

@jpolavar jpolavar Dec 24, 2024 •
edited by carlansley
Loading

Choose a reason for hiding this comment

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

We need to monitor for assertion failures in production and also improve how assertions are used in production code. Currently, assertions are being used for status checks, which isn’t ideal. A good first step is to implement an ESLint rule that will detect when assertions are made using a statusCode as one of the parameters. This will help identify and clean up improper use of assertions in the code. notify service could look for assertion errors, but we need to clean up usage of assertions for status checks.

As we are checking for statusCode, should we have another rule to auto fix the prod code with statusCode if we have numbers some thing like
assert(statusCode === 200);
would be assert(statusCode === StatusCodes.OK);

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a mistake in the documentation wrt what is "pass" and what is "fail"? That was what confused me initially.

Copy link
Contributor Author

@jpolavar jpolavar Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But initially it was like

Fail

assert(statusCode === 200);
assert.equal(statusCode, 200);

Pass

assert(response.status === 200);
assert.equal(response.status, 200);

Updated the rule to handle just statusCodes.

Fail

assert(statusCode === StatusCodes.OK);
assert.equal(statusCode, StatusCodes.OK);

Pass

assert(response.status === 200);
assert.equal(response.status, 200);

Pass wouldn't report as StatusCodes aren't used. This was the reason if we need to have another rule to auto fix all the numbers to statusCodes?

assert.equal(statusCode, 200);
```

## Pass

```js
assert(response.status === 200);
assert.equal(response.status, 200);
Copy link

Choose a reason for hiding this comment

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

shouldn't these be failures as well? In a nutshell, we want to disallow the usage of assert to check for the desired response status, whether by using status or statusCode, in production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

status/code are widely used in none response status code related assertions, e.g. things like account.status/message.code can be incorrectly labelled. Should we check .status, .code in the params of assert instead

Copy link

Choose a reason for hiding this comment

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

not sure I understand, but all I'm saying is, just like how you're disallowing assert.ok(response.statusCode === StatusCodes.OK), we should also disallow assert.ok(response.status === StatusCodes.OK)

Copy link
Contributor Author

@jpolavar jpolavar Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

assert.ok(response.status === StatusCodes.OK) is disallowed and would be reported as error but assert.equal(response.status, 200); is allowed since the rule check is going by StatusCodes. We can add that to disallow

Copy link
Contributor

Choose a reason for hiding this comment

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

created #109 to address the usage of magic number instead of StatusCodes.xxx

Copy link

Choose a reason for hiding this comment

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

@le-cong, why do we need a separate rule? if 200/201/400/500 etc., are used, it will fail with a no-magic-numbers correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, the only thing is that no-magic-number rule is turned off for test codes, thought we might want to be consistent with status code handling all across targeting a specific subset of numbers. thoughts?

Copy link

Choose a reason for hiding this comment

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

no-magic-number is turned off for https://github.com/checkdigit/eslint-config/blob/main/index.mjs#L208. So, the only HTTP response status codes that will be allowed would be 256 and 512, which are uncommon and I can't think of any services that use these.

Copy link
Contributor

Choose a reason for hiding this comment

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

L208 is for default setting including production code, but it's overridden by the next section in https://github.com/checkdigit/eslint-config/blob/5eea1c576703f1ba1663bd6bb73542d19ccbce5d/index.mjs#L438, which disables this rule completely for test codes.

Copy link

Choose a reason for hiding this comment

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

got it. the new GitHub issue you created makes sense to me then

```
103 changes: 54 additions & 49 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@checkdigit/eslint-plugin",
"version": "7.7.0",
"version": "7.8.0",
"description": "Check Digit eslint plugins",
"keywords": [
"eslint",
Expand Down
4 changes: 4 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import invalidJsonStringify, { ruleId as invalidJsonStringifyRuleId } from './in
import noDuplicatedImports, { ruleId as noDuplicatedImportsRuleId } from './no-duplicated-imports.ts';
import noLegacyServiceTyping, { ruleId as noLegacyServiceTypingRuleId } from './no-legacy-service-typing.ts';
import noPromiseInstanceMethod, { ruleId as noPromiseInstanceMethodRuleId } from './no-promise-instance-method.ts';
import noStatusCodeAssert from './no-status-code-assert.ts';
import requireFixedServicesImport, {
ruleId as requireFixedServicesImportRuleId,
} from './require-fixed-services-import.ts';
Expand Down Expand Up @@ -39,6 +40,7 @@ const rules: Record<string, TSESLint.LooseRuleDefinition> = {
'file-path-comment': filePathComment,
'no-card-numbers': noCardNumbers,
'no-random-v4-uuid': noRandomV4UUID,
'no-status-code-assert': noStatusCodeAssert,
'no-uuid': noUuid,
'require-strict-assert': requireStrictAssert,
'require-ts-extension-imports': requireTsExtensionImports,
Expand Down Expand Up @@ -73,6 +75,7 @@ const configs: Record<string, TSESLint.FlatConfig.Config[]> = {
'@checkdigit/no-card-numbers': 'error',
'@checkdigit/file-path-comment': 'error',
'@checkdigit/no-random-v4-uuid': 'error',
'@checkdigit/no-status-code-assert': 'error',
'@checkdigit/no-uuid': 'error',
'@checkdigit/require-strict-assert': 'error',
'@checkdigit/require-ts-extension-imports': 'error',
Expand Down Expand Up @@ -103,6 +106,7 @@ const configs: Record<string, TSESLint.FlatConfig.Config[]> = {
'@checkdigit/no-card-numbers': 'error',
'@checkdigit/file-path-comment': 'off',
'@checkdigit/no-random-v4-uuid': 'error',
'@checkdigit/no-status-code-assert': 'error',
'@checkdigit/no-uuid': 'error',
'@checkdigit/require-strict-assert': 'error',
'@checkdigit/require-ts-extension-imports': 'error',
Expand Down
Loading
Loading