feat: no-depends-on-boolean-flag rule (#447)

* feat: no-depends-on-boolean-flag rule * test: fix UTs * test: skip ts-dep-check * test: fix workflow ref * chore: retry * test: back to main * test: tmp fail * Update no-depends-on-boolean-flag.md * chore: back to warn * chore: add link to doc * test: skipTsCheck bool=true * test: skipTsCheck bool=false * test: skipTsCheck bool=true * chore: back to main * pr review feedback (#448) * refactor: pr review * refactor: min/max/default * chore: update msg * chore: wrong doc --------- Co-authored-by: Shane McLaughlin <[email protected]>
salesforcecli · Jul 26, 2024 · bb0f816 · bb0f816
1 parent e5352fa
commit bb0f816
Show file tree

Hide file tree

Showing 16 changed files with 291 additions and 80 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -10,6 +10,8 @@ jobs:
   linux-unit-tests:
     needs: yarn-lockfile-check
     uses: salesforcecli/github-workflows/.github/workflows/unitTestsLinux.yml@main
+    with:
+      skipTsDepCheck: true
   windows-unit-tests:
     needs: yarn-lockfile-check
     uses: salesforcecli/github-workflows/.github/workflows/unitTestsWindows.yml@main

diff --git a/README.md b/README.md
@@ -72,6 +72,7 @@ module.exports = {
 | [no-builtin-flags](docs/rules/no-builtin-flags.md)                                     | Handling for sfdxCommand's flags.builtin                                                                                         | ✈️      |      |         | 🔧 |    |
 | [no-classes-in-command-return-type](docs/rules/no-classes-in-command-return-type.md)   | The return type of the run method should not contain a class.                                                                    | ✈️ ✅    |      |         | 🔧 |    |
 | [no-default-and-depends-on-flags](docs/rules/no-default-and-depends-on-flags.md)       | Do not allow creation of a flag with default value and dependsOn                                                                 | ✈️ ✅    |      |         |    |    |
+| [no-depends-on-boolean-flag](docs/rules/no-depends-on-boolean-flag.md)                 | Do not allow flags to depend on boolean flags                                                                                    |         | ✈️ ✅ |         |    |    |
 | [no-deprecated-properties](docs/rules/no-deprecated-properties.md)                     | Removes non-existent properties left over from SfdxCommand                                                                       | ✈️      |      |         | 🔧 |    |
 | [no-duplicate-short-characters](docs/rules/no-duplicate-short-characters.md)           | Prevent duplicate use of short characters or conflicts between aliases and flags                                                 | ✈️ ✅    |      |         |    |    |
 | [no-execcmd-double-quotes](docs/rules/no-execcmd-double-quotes.md)                     | Do not use double quotes in NUT examples.  They will not work on windows                                                         |         |      | 📚 ✈️ ✅ | 🔧 |    |

diff --git a/docs/rules/no-depends-on-boolean-flag.md b/docs/rules/no-depends-on-boolean-flag.md
@@ -0,0 +1,81 @@
+# Do not allow flags to depend on boolean flags (`sf-plugin/no-depends-on-boolean-flag`)
+
+⚠️ This rule _warns_ in the following configs: ✈️ `migration`, ✅ `recommended`.
+
+<!-- end auto-generated rule header -->
+
+Flags depending on other boolean flags via `dependsOn` can cause unexpected behaviors:
+
+```ts
+// src/commands/data/query.ts
+
+export class DataSoqlQueryCommand extends SfCommand<unknown> {
+  public static readonly flags = {
+    query: Flags.string({
+      char: 'q',
+      summary: messages.getMessage('flags.query.summary'),
+    }),
+    bulk: Flags.boolean({
+      char: 'b',
+      default: false,
+      summary: messages.getMessage('flags.bulk.summary'),
+    }),
+    wait: Flags.duration({
+      unit: 'minutes',
+      char: 'w',
+      summary: messages.getMessage('flags.wait.summary'),
+      dependsOn: ['bulk'],
+    }) 
+  }
+}
+```
+
+This code is supposed to only allow `--wait` to be used when `--bulk` was provided. 
+
+However, because `--bulk` has a default value of `false`, oclif's flag parser will allow `--wait` even without passing in `--bulk` because the `wait.dependsOn` check only ensures that `bulk` has a value, so the following execution would be allowed:
+
+```
+sf data query -q 'select name,id from account limit 1' --wait 10
+```
+
+But even if `--bulk` didn't have a default value, it could still allow a wrong combination if it had `allowNo: true`: 
+
+```ts
+bulk: Flags.boolean({
+  char: 'b',
+  default: false,
+  summary: messages.getMessage('flags.bulk.summary'),
+  allowNo: true // Support reversible boolean flag with `--no-` prefix (e.g. `--no-bulk`).
+}),
+```
+
+The following example would still run because `--no-bulk` sets `bulk` value to `false`:
+
+```
+sf data query -q 'select name,id from account limit 1' --wait 10 --no-bulk
+```
+
+If the desired behavior is to only allow a flag when another boolean flag was provided you should use oclif's relationships feature to verify the boolean flag value is `true`:
+
+```ts
+bulk: Flags.boolean({
+  char: 'b',
+  default: false,
+  summary: messages.getMessage('flags.bulk.summary'),
+  allowNo: true // Support reversible boolean flag with `--no-` prefix (e.g. `--no-bulk`).
+}),
+wait: Flags.duration({
+  unit: 'minutes',
+  char: 'w',
+  summary: messages.getMessage('flags.wait.summary'),
+  relationships: [
+    {
+      type: 'some',
+      // eslint-disable-next-line @typescript-eslint/require-await
+      flags: [{ name: 'bulk', when: async (flags): Promise<boolean> => Promise.resolve(flags['bulk'] === true) }],
+    },
+  ]
+}) 
+```
+
+See: https://oclif.io/docs/flags
diff --git a/src/index.ts b/src/index.ts
@@ -49,6 +49,7 @@ import { esmMessageImport } from './rules/esm-message-import';
 import { noDefaultDependsOnFlags } from './rules/no-default-depends-on-flags';
 import { onlyExtendSfCommand } from './rules/only-extend-sfCommand';
 import { spreadBaseFlags } from './rules/spread-base-flags';
+import { noDependsOnBooleanFlags } from './rules/no-depends-on-boolean-flags';
 
 const library = {
   plugins: ['sf-plugin'],
@@ -90,6 +91,7 @@ const recommended = {
     'sf-plugin/no-default-and-depends-on-flags': 'error',
     'sf-plugin/only-extend-SfCommand': 'warn',
     'sf-plugin/spread-base-flags': 'warn',
+    'sf-plugin/no-depends-on-boolean-flag': 'warn',
   },
 };
 
@@ -124,6 +126,7 @@ export = {
     'no-h-short-char': dashH,
     'no-default-and-depends-on-flags': noDefaultDependsOnFlags,
     'only-extend-SfCommand': onlyExtendSfCommand,
+    'no-depends-on-boolean-flag': noDependsOnBooleanFlags,
     'spread-base-flags': spreadBaseFlags,
     'no-duplicate-short-characters': noDuplicateShortCharacters,
     'run-matches-class-type': runMatchesClassType,

diff --git a/src/rules/dash-h.ts b/src/rules/dash-h.ts
@@ -33,13 +33,9 @@ export const dashH = RuleCreator.withoutDocs({
               node.value?.type === AST_NODE_TYPES.CallExpression &&
               node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression
             ) {
-              const hChar = node.value.arguments[0].properties.find(
-                (property) =>
-                  property.type === AST_NODE_TYPES.Property &&
-                  flagPropertyIsNamed(property, 'char') &&
-                  property.value.type === AST_NODE_TYPES.Literal &&
-                  property.value.value === 'h'
-              );
+              const hChar = node.value.arguments[0].properties
+                .filter(flagPropertyIsNamed('char'))
+                .find((property) => property.value.type === AST_NODE_TYPES.Literal && property.value.value === 'h');
               if (hChar) {
                 context.report({
                   node: hChar,

diff --git a/src/rules/dash-o.ts b/src/rules/dash-o.ts
@@ -38,13 +38,9 @@ export const dashO = RuleCreator.withoutDocs({
               !node.value.callee.property.name.toLowerCase().includes('hub') &&
               node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression
             ) {
-              const hChar = node.value.arguments[0].properties.find(
-                (property) =>
-                  property.type === AST_NODE_TYPES.Property &&
-                  flagPropertyIsNamed(property, 'char') &&
-                  property.value.type === AST_NODE_TYPES.Literal &&
-                  property.value.value === 'o'
-              );
+              const hChar = node.value.arguments[0].properties
+                .filter(flagPropertyIsNamed('char'))
+                .find((property) => property.value.type === AST_NODE_TYPES.Literal && property.value.value === 'o');
               if (hChar) {
                 context.report({
                   node: hChar,

diff --git a/src/rules/flag-min-max-default.ts b/src/rules/flag-min-max-default.ts
@@ -4,7 +4,7 @@
  * Licensed under the BSD 3-Clause license.
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
-import { AST_NODE_TYPES } from '@typescript-eslint/utils';
+import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils';
 import { RuleCreator } from '@typescript-eslint/utils/eslint-utils';
 
 import { ancestorsContainsSfCommand, isInCommandDirectory } from '../shared/commands';
@@ -30,24 +30,18 @@ export const flagMinMaxDefault = RuleCreator.withoutDocs({
             if (isFlag(node) && ancestorsContainsSfCommand(context)) {
               if (
                 node.value?.type === AST_NODE_TYPES.CallExpression &&
-                node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression &&
-                // has min/max
-                node.value.arguments[0].properties.some(
-                  (property) =>
-                    property.type === AST_NODE_TYPES.Property &&
-                    (flagPropertyIsNamed(property, 'min') || flagPropertyIsNamed(property, 'max'))
-                ) &&
-                !node.value.arguments[0].properties.some(
-                  (property) =>
-                    property.type === AST_NODE_TYPES.Property &&
-                    // defaultValue for DurationFlags
-                    (flagPropertyIsNamed(property, 'default') || flagPropertyIsNamed(property, 'defaultValue'))
-                )
+                node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression
               ) {
-                context.report({
-                  node,
-                  messageId: 'message',
-                });
+                const props = node.value.arguments[0].properties.filter(ASTUtils.isNodeOfType(AST_NODE_TYPES.Property));
+                if (
+                  props.some((p) => flagPropertyIsNamed('min')(p) || flagPropertyIsNamed('max')(p)) &&
+                  !props.some((p) => flagPropertyIsNamed('default')(p) || flagPropertyIsNamed('defaultValue')(p))
+                ) {
+                  context.report({
+                    node,
+                    messageId: 'message',
+                  });
+                }
               }
             }
           },

diff --git a/src/rules/flag-summary.ts b/src/rules/flag-summary.ts
@@ -38,10 +38,8 @@ export const flagSummary = RuleCreator.withoutDocs({
                 ASTUtils.isNodeOfType(AST_NODE_TYPES.Property)
               );
 
-              if (!propertyArguments.some((property) => flagPropertyIsNamed(property, 'summary'))) {
-                const descriptionProp = propertyArguments.find((property) =>
-                  flagPropertyIsNamed(property, 'description')
-                );
+              if (!propertyArguments.some(flagPropertyIsNamed('summary'))) {
+                const descriptionProp = propertyArguments.find(flagPropertyIsNamed('description'));
 
                 const range = descriptionProp && 'key' in descriptionProp ? descriptionProp?.key.range : undefined;
                 return context.report({
@@ -55,11 +53,9 @@ export const flagSummary = RuleCreator.withoutDocs({
                     : {}),
                 });
               }
-              if (!propertyArguments.some((property) => flagPropertyIsNamed(property, 'description'))) {
+              if (!propertyArguments.some(flagPropertyIsNamed('description'))) {
                 // if there is no description, but there is a longDescription, turn that into the description
-                const longDescriptionProp = propertyArguments.find((property) =>
-                  flagPropertyIsNamed(property, 'longDescription')
-                );
+                const longDescriptionProp = propertyArguments.find(flagPropertyIsNamed('longDescription'));
                 if (!longDescriptionProp) {
                   return;
                 }

diff --git a/src/rules/id-flag-suggestions.ts b/src/rules/id-flag-suggestions.ts
@@ -44,8 +44,8 @@ export const idFlagSuggestions = RuleCreator.withoutDocs({
                 const argProps = node.value.arguments[0].properties.filter(
                   ASTUtils.isNodeOfType(AST_NODE_TYPES.Property)
                 );
-                const hasStartsWith = argProps.some((property) => flagPropertyIsNamed(property, 'startsWith'));
-                const hasLength = argProps.some((property) => flagPropertyIsNamed(property, 'length'));
+                const hasStartsWith = argProps.some(flagPropertyIsNamed('startsWith'));
+                const hasLength = argProps.some(flagPropertyIsNamed('length'));
 
                 if (!hasStartsWith || !hasLength) {
                   const existing = context.sourceCode.getText(node);

diff --git a/src/rules/migration/encourage-alias-deprecation.ts b/src/rules/migration/encourage-alias-deprecation.ts
@@ -32,30 +32,29 @@ export const encourageAliasDeprecation = RuleCreator.withoutDocs({
     return isInCommandDirectory(context)
       ? {
           PropertyDefinition(node): void {
-            if (ancestorsContainsSfCommand(context)) {
-              if (node.key.type === AST_NODE_TYPES.Identifier && node.key.name === 'aliases') {
-                // but you don't have deprecateAliases = true then add id
-                if (
-                  node.parent?.type === AST_NODE_TYPES.ClassBody &&
-                  !node.parent.body.some(
-                    (n) =>
-                      n.type === AST_NODE_TYPES.PropertyDefinition &&
-                      n.key.type === AST_NODE_TYPES.Identifier &&
-                      n.key.name === 'deprecateAliases'
-                  )
-                ) {
-                  context.report({
-                    node,
+            if (
+              ancestorsContainsSfCommand(context) &&
+              node.key.type === AST_NODE_TYPES.Identifier &&
+              node.key.name === 'aliases' &&
+              node.parent?.type === AST_NODE_TYPES.ClassBody &&
+              !node.parent.body.some(
+                (n) =>
+                  n.type === AST_NODE_TYPES.PropertyDefinition &&
+                  n.key.type === AST_NODE_TYPES.Identifier &&
+                  n.key.name === 'deprecateAliases'
+              )
+            ) {
+              // but you don't have deprecateAliases = true then add id
+              context.report({
+                node,
+                messageId: 'command',
+                suggest: [
+                  {
                     messageId: 'command',
-                    suggest: [
-                      {
-                        messageId: 'command',
-                        fix: (fixer) => fixer.insertTextBefore(node, 'public static readonly deprecateAliases = true;'),
-                      },
-                    ],
-                  });
-                }
-              }
+                    fix: (fixer) => fixer.insertTextBefore(node, 'public static readonly deprecateAliases = true;'),
+                  },
+                ],
+              });
             }
           },
           Property(node): void {
@@ -69,8 +68,8 @@ export const encourageAliasDeprecation = RuleCreator.withoutDocs({
                 ASTUtils.isNodeOfType(AST_NODE_TYPES.Property)
               );
 
-              const aliasesProperty = argProps.find((property) => flagPropertyIsNamed(property, 'aliases'));
-              if (aliasesProperty && !argProps.some((property) => flagPropertyIsNamed(property, 'deprecateAliases'))) {
+              const aliasesProperty = argProps.find(flagPropertyIsNamed('aliases'));
+              if (aliasesProperty && !argProps.some(flagPropertyIsNamed('deprecateAliases'))) {
                 context.report({
                   node: aliasesProperty,
                   messageId: 'flag',

diff --git a/src/rules/no-default-depends-on-flags.ts b/src/rules/no-default-depends-on-flags.ts
@@ -4,7 +4,7 @@
  * Licensed under the BSD 3-Clause license.
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
-import { AST_NODE_TYPES } from '@typescript-eslint/utils';
+import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils';
 import { RuleCreator } from '@typescript-eslint/utils/eslint-utils';
 import { ancestorsContainsSfCommand, isInCommandDirectory } from '../shared/commands';
 import { flagPropertyIsNamed, isFlag } from '../shared/flags';
@@ -33,12 +33,9 @@ export const noDefaultDependsOnFlags = RuleCreator.withoutDocs({
               node.value?.type === AST_NODE_TYPES.CallExpression &&
               node.value.arguments?.[0]?.type === AST_NODE_TYPES.ObjectExpression
             ) {
-              const dependsOnProperty = node.value.arguments[0].properties.find(
-                (property) => property.type === AST_NODE_TYPES.Property && flagPropertyIsNamed(property, 'dependsOn')
-              );
-              const defaultValueProperty = node.value.arguments[0].properties.find(
-                (property) => property.type === AST_NODE_TYPES.Property && flagPropertyIsNamed(property, 'default')
-              );
+              const props = node.value.arguments[0].properties.filter(ASTUtils.isNodeOfType(AST_NODE_TYPES.Property));
+              const dependsOnProperty = props.find(flagPropertyIsNamed('dependsOn'));
+              const defaultValueProperty = props.find(flagPropertyIsNamed('default'));
 
               // @ts-expect-error from the node (flag), go up a level (parent) and find the dependsOn flag definition, see if it has a default
               const dependsOnFlagDefaultValue = node.parent.properties