chore(eslint-plugin): rule against invalid paths by kaizencc · Pull Request #28828 · aws/aws-cdk

kaizencc · 2024-01-23T18:01:15Z

This is a follow up to #28658, #28772, and #28760. We had to fix multiple places where that file path extended beyond the package itself into other areas of the local repository (that would not be available after packaging). This caused myriad issues at synth time with file not found errors.

This PR introduces a linter rule with the following specifications:

no inefficient paths, i.e. no going backwards multiple times. Ex. path.join(__dirname, '..', 'folder', '..', 'another-folder'). This should and can be easily simplified
no paths that go backwards past a package.json file. This should catch the instances we faced next time.

The yarn lint command on aws-cdk-lib took 51.47s seconds without this new rule and 53.32s seconds with the rule enabled. The difference of ~2 seconds shouldn't be a hindrance in this case but I am happy to look for additional efficiencies in the rule I've written.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

paulhcsun

Looks good, just one minor comment.

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

rix0rrr · 2024-01-24T07:43:25Z

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

+      }
+
+      if (isPathJoinFuncCall(node)) {
+        // Confirm path does not have unnecessary '..'


First: is this check against oscillating worth the squeeze? (Or is this just to make the subsequent checks easier? If so, I can dig that. Might want to write that down.)

Second: can this not be simplified in the following way:

.. may only occur in the path prefix.

So any .. whose index is after the first non-.. is incorrect:

const components = confirmFirstArgumentIsDirnameAndRemove(node.arguments); const firstDownDir = components.findIndex((p) => p !== '..'); if (components.some((p, i) => p === '..' && i > firstDownDir) { // ERROR: Oscillating paths return; }

Then we can observe that package.json may only exist in the very last directory that we ended up with by doing ..: if it occurs anywhere earlier that's an error:

// Note the - 1 to exclude the last directory from the check // Exclude the case where there are no '..' at all in the path -- those are never invalid if (firstDownDir > 0) { for (let i = 0; i < firstDownDir - 1; i++) { const pjFile = path.join(...[path.dirname(currentFile), ...components.slice(0, i), 'package.json']); if (existsSync(pjFile)) { // ERROR: this path will end up going out of the package.json directory return; } } }

Take a moment to reason through this table to see how this works:

// components | firstDownDir | closest invalid package.json ['x'] 0 /* N/A */ ['..', 'x'] 1 'package.json' ['..', '..', 'x'] 2 '../package.json'

Oh you should also add a check that / doesn't occur anywhere inside any of the components, otherwise our checks are going to fail:

path.join(__dirname, '../../../..', 'banana')

Is bad style but works, and anything that accidentally works will occur in our code base 😅. We need to guard against it.

rix0rrr · 2024-01-24T07:44:34Z

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

+        let forward = false; // Determines if the path is officially going forward
+        let validPath = true;
+        for (let i=1; i<node.arguments.length; i++) {
+          const path = node.arguments[i].value;


Are we confirming anywhere that the first argument to path.join() is __dirname ? I can't find it

Yes, in isPathJoinFuncCall:

function isPathJoinFuncCall(node: any): boolean { return ( node.callee?.property?.name === 'join' && node.arguments.length > 1 && node.arguments[0].name && node.arguments[0].name === '__dirname' ) }

I assume that we don't have any rule that says path.join must have __dirname as the first argument, but rereading your comment suggests that that may be indeed what you are suggesting

No, but just saying that we don't need to do any checks on path.joins where the first argument ISN'T __dirname.

kaizencc · 2024-01-24T23:03:46Z

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

+            // Note i + 1 here to include the current '..' in the path, since Array.slice excludes the end index.
+            const pjFile = path.join(...[path.dirname(currentFile), ...args.slice(0, i + 1), 'package.json']);


I think this is the correct implementation of your algo @rix0rrr

Yeah, you are right. Good catch!

I suppose we could also do i in [1..firstDownDir) as opposed to [0..firstDownDir - 1), and that would have avoided this i + 1.

tools/@aws-cdk/eslint-plugin/test/rules/fixtures/no-invalid-path/variable-path.ts

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

rix0rrr

Conditionally approved! I'll leave you to decide what to do with the i + 1/i - 1 business

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

rix0rrr · 2024-01-25T07:37:14Z

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts

+            // Note i + 1 here to include the current '..' in the path, since Array.slice excludes the end index.
+            const pjFile = path.join(...[path.dirname(currentFile), ...args.slice(0, i + 1), 'package.json']);


Yeah, you are right. Good catch!

I suppose we could also do i in [1..firstDownDir) as opposed to [0..firstDownDir - 1), and that would have avoided this i + 1.

mrgrain · 2024-01-29T14:13:50Z

@Mergifyio update

mergify · 2024-01-29T14:13:55Z

update

✅ Branch has been successfully updated

…conroy/eslint4

packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts

…ng-synthesizer.test.ts

mergify · 2024-02-02T19:24:44Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-02-02T20:19:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a8e6610
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-02-02T20:19:56Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This is a follow up to #28658, #28772, and #28760. We had to fix multiple places where that file path extended beyond the package itself into other areas of the local repository (that would not be available after packaging). This caused myriad issues at synth time with `file not found` errors. This PR introduces a linter rule with the following specifications: - no inefficient paths, i.e. no going backwards multiple times. Ex. `path.join(__dirname, '..', 'folder', '..', 'another-folder')`. This should and can be easily simplified - no paths that go backwards past a `package.json` file. This should catch the instances we faced next time. The `yarn lint` command on `aws-cdk-lib` took 51.47s seconds without this new rule and 53.32s seconds with the rule enabled. The difference of ~2 seconds shouldn't be a hindrance in this case but I am happy to look for additional efficiencies in the rule I've written. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

kaizencc added 2 commits January 23, 2024 12:02

feat(eslint-plugin): rule against invalid paths

0dc442f

finishing touches

d1b16b3

aws-cdk-automation requested a review from a team January 23, 2024 18:01

github-actions bot added the p2 label Jan 23, 2024

Merge branch 'main' into conroy/eslint4

f4f3f4e

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 23, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 23, 2024

Merge branch 'main' into conroy/eslint4

83853cf

paulhcsun reviewed Jan 24, 2024

View reviewed changes

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts Outdated Show resolved Hide resolved

rix0rrr requested changes Jan 24, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 24, 2024

pr feedback, updated algo

e6d7442

kaizencc commented Jan 24, 2024

View reviewed changes

tools/@aws-cdk/eslint-plugin/lib/rules/no-invalid-path.ts Show resolved Hide resolved

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jan 25, 2024

rix0rrr approved these changes Jan 25, 2024

View reviewed changes

Merge branch 'main' into conroy/eslint4

9b7e5ff

kaizencc and others added 2 commits January 30, 2024 11:10

fix minor things

c593348

Merge branch 'main' into conroy/eslint4

094a39b

kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2024

kaizencc added 4 commits January 30, 2024 13:04

fix linter failures in cdk

2bd6ae6

Merge branch 'conroy/eslint4' of https://github.com/aws/aws-cdk into …

8856d9a

…conroy/eslint4

fix paths

b4dd790

fix joins

accd8f3

kaizencc commented Jan 30, 2024

View reviewed changes

packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts Outdated Show resolved Hide resolved

kaizencc and others added 2 commits January 30, 2024 14:24

Update packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-stagi…

8bf339c

…ng-synthesizer.test.ts

updates

cf69834

kaizencc had a problem deploying to test-pipeline January 30, 2024 20:37 — with GitHub Actions Failure

last couple

09dc55b

kaizencc had a problem deploying to test-pipeline January 30, 2024 21:07 — with GitHub Actions Failure

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

final fixes

dafa4b7

kaizencc added the pr/do-not-merge This PR should not be merged at this time. label Feb 1, 2024

Merge branch 'main' into conroy/eslint4

2f99a1f

kaizencc had a problem deploying to test-pipeline February 1, 2024 21:47 — with GitHub Actions Failure

Merge branch 'main' into conroy/eslint4

72ade51

kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Feb 2, 2024

kaizencc had a problem deploying to test-pipeline February 2, 2024 18:59 — with GitHub Actions Failure

Merge branch 'main' into conroy/eslint4

a8e6610

mergify bot had a problem deploying to test-pipeline February 2, 2024 19:54 Failure

mergify bot merged commit e5b7813 into main Feb 2, 2024

mergify bot deleted the conroy/eslint4 branch February 2, 2024 20:19

		// Note i + 1 here to include the current '..' in the path, since Array.slice excludes the end index.
		const pjFile = path.join(...[path.dirname(currentFile), ...args.slice(0, i + 1), 'package.json']);

Conversation

kaizencc commented Jan 23, 2024

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrgrain commented Jan 29, 2024

Uh oh!

mergify bot commented Jan 29, 2024

✅ Branch has been successfully updated

Uh oh!

Uh oh!

mergify bot commented Feb 2, 2024

Uh oh!

aws-cdk-automation commented Feb 2, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaizencc Jan 24, 2024 •

edited

Loading