[Stylelint] Add invalid properties rule (opensearch-project#4374)

* Add invalid properties rule Signed-off-by: Matt Provost <[email protected]> * Update changelog Signed-off-by: Matt Provost <[email protected]> * Rename old variable Signed-off-by: Matt Provost <[email protected]> * Add types for configs Signed-off-by: Matt Provost <[email protected]> * Rename rule to no_restricted_properties Signed-off-by: Matt Provost <[email protected]> * Refactor duplicate functions into generic one Signed-off-by: Matt Provost <[email protected]> * Add type definitions Signed-off-by: Matt Provost <[email protected]> * Add some documentation about supported config types Signed-off-by: Matt Provost <[email protected]> * Update changelog Signed-off-by: Matt Provost <[email protected]> * Optchain instead of unwrapping source file Signed-off-by: Matt Provost <[email protected]> --------- Signed-off-by: Matt Provost <[email protected]>
joshuali925 · Jun 27, 2023 · d285ecb · d285ecb
1 parent 1ef3e9f
commit d285ecb
Show file tree

Hide file tree

Showing 10 changed files with 190 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -39,6 +39,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Add an achievement badger to the PR ([#3721](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3721))
 - Upgrade the backport workflow ([#4343](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4343))
 - [Lint] Add custom stylelint rules and config to prevent unintended style overrides ([#4290](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4290))
+- [Lint] Add stylelint rule to define properties that are restricted from being used ([#4374](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4374))
 
 ### 📝 Documentation
 

diff --git a/packages/osd-stylelint-config/.stylelintrc.js b/packages/osd-stylelint-config/.stylelintrc.js
@@ -15,6 +15,14 @@ module.exports = {
   ],
 
   rules: {
+    '@osd/stylelint/no_restricted_properties': [
+      {
+        config: "./../../../osd-stylelint-config/config/restricted_properties.json"
+      },
+      {
+        severity: "error"
+      }
+    ],
     '@osd/stylelint/no_modifying_global_selectors': [
       {
         config: "./../../../osd-stylelint-config/config/global_selectors.json"

diff --git a/packages/osd-stylelint-config/config/restricted_properties.json b/packages/osd-stylelint-config/config/restricted_properties.json
@@ -0,0 +1,17 @@
+{
+  "font-family": {
+    "approved": [
+      "src/plugins/discover/public/application/_discover.scss",
+      "src/plugins/maps_legacy/public/map/_leaflet_overrides.scss",
+      "src/plugins/maps_legacy/public/map/_legend.scss",
+      "src/plugins/opensearch_dashboards_legacy/public/font_awesome/font_awesome.scss",
+      "src/plugins/opensearch_dashboards_react/public/markdown/_markdown.scss",
+      "packages/osd-ui-framework/src/components/tool_bar/_tool_bar_search.scss",
+      "packages/osd-ui-framework/src/global_styling/mixins/_global_mixins.scss",
+      "src/plugins/data/public/ui/typeahead/_suggestion.scss",
+      "src/plugins/vis_type_timeseries/public/application/components/_error.scss",
+      "packages/osd-ui-framework/src/components/form/check_box/_check_box.scss",
+      "src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss"
+    ]
+  }
+}
diff --git a/packages/osd-stylelint-plugin-stylelint/README.md b/packages/osd-stylelint-plugin-stylelint/README.md
@@ -16,3 +16,53 @@ Enables the ability to capture styling that potentially can be subject to compli
 For example, if a style attempts to set a button to have a `background-color: $incorrectVariable()` and the JSON config passed to the compliance engine does not explicitly reject the `$incorrectVariable()` being applied to a button's background color then the linter will pass. But it will output this if the environment variable is set to `true`.
 
 The goal being that setting this environment variable to output a list that can then be added to the JSON config which we can feed back into the compliance engine.
+
+## Supported config formats
+
+Currently, this package supports 2 formats for config: file based and value based.
+
+### File based config
+
+Sample:
+```json
+{
+  "#global-id": {
+    "approved": [
+      "src/foo/bar.scss"
+    ]
+  },
+  ".component-class": {
+    "approved": [
+      "src/bar/baz.scss"
+    ]
+  }
+}
+```
+
+Allows specifying a selector, such as a CSS selector (class, id, etc.), to be caught by the rule, as well as an allowlist of files where the selector is allowed.
+
+### Value based config
+
+Sample:
+```json
+{
+  "color": {
+    "button": [
+      {
+        "approved": "$primaryColor",
+        "rejected": [
+          "blue"
+        ]
+      },
+      {
+        "approved": "$errorColor",
+        "rejected": [
+          "red"
+        ]
+      }
+    ]
+  }
+}
+```
+
+Allows specifying multiple selectors for finding values. Supports complex configurations of variants with both an approved value and set of rejected values.
diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts
@@ -9,11 +9,13 @@
  * GitHub history for details.
  */
 
+import noRestrictedProperties from './no_restricted_properties';
 import noCustomColors from './no_custom_colors';
 import noModifyingGlobalSelectors from './no_modifying_global_selectors';
 
 // eslint-disable-next-line import/no-default-export
 export default {
   no_custom_colors: noCustomColors,
   no_modifying_global_selectors: noModifyingGlobalSelectors,
+  no_restricted_properties: noRestrictedProperties,
 };
diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts
@@ -19,6 +19,7 @@ import {
   getRulesFromConfig,
   isColorProperty,
   isValidOptions,
+  ValueBasedConfig,
 } from '../../utils';
 
 const isOuiAuditEnabled = Boolean(process.env.OUI_AUDIT_ENABLED);
@@ -42,7 +43,7 @@ const ruleFunction = (
       return;
     }
 
-    const rules = getRulesFromConfig(primaryOption.config);
+    const rules: ValueBasedConfig = getRulesFromConfig(primaryOption.config);
 
     const isAutoFixing = Boolean(context.fix);
 
@@ -84,7 +85,7 @@ const ruleFunction = (
 
       shouldReport = !ruleObject.isComplaint;
 
-      if (shouldReport && isAutoFixing) {
+      if (shouldReport && isAutoFixing && ruleObject.expected) {
         decl.value = ruleObject.expected;
         return;
       }

diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts
@@ -15,7 +15,8 @@ import {
   getNotCompliantMessage,
   getRulesFromConfig,
   isValidOptions,
-  getSelectorRule,
+  getRuleFromConfig,
+  FileBasedConfig,
 } from '../../utils';
 
 const { ruleMessages, report } = stylelint.utils;
@@ -36,12 +37,12 @@ const ruleFunction = (
       return;
     }
 
-    const rules = getRulesFromConfig(primaryOption.config);
+    const rules: FileBasedConfig = getRulesFromConfig(primaryOption.config);
 
     const isAutoFixing = Boolean(context.fix);
 
     postcssRoot.walkRules((rule: any) => {
-      const selectorRule = getSelectorRule(rules, rule);
+      const selectorRule = getRuleFromConfig(rules, rule.selector);
       if (!selectorRule) {
         return;
       }

diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts
@@ -0,0 +1,87 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import stylelint from 'stylelint';
+import { NAMESPACE } from '../..';
+import {
+  getNotCompliantMessage,
+  getRuleFromConfig,
+  getRulesFromConfig,
+  isValidOptions,
+  FileBasedConfig,
+} from '../../utils';
+
+const { ruleMessages, report } = stylelint.utils;
+
+const ruleName = 'no_restricted_properties';
+const messages = ruleMessages(ruleName, {
+  expected: (message) => `${message}`,
+});
+
+const ruleFunction: stylelint.Rule = (
+  primaryOption: Record<string, any>,
+  secondaryOptionObject: Record<string, any>,
+  context
+) => {
+  return (postcssRoot, postcssResult) => {
+    const validOptions = isValidOptions(postcssResult, ruleName, primaryOption);
+    if (!validOptions) {
+      return;
+    }
+
+    const rules: FileBasedConfig = getRulesFromConfig(primaryOption.config);
+
+    const isAutoFixing = Boolean(context.fix);
+
+    postcssRoot.walkDecls((decl) => {
+      const propertyRule = getRuleFromConfig(rules, decl.prop);
+      if (!propertyRule) {
+        return;
+      }
+
+      let shouldReport = false;
+
+      const file = postcssRoot.source?.input.file;
+      if (!file) {
+        return;
+      }
+
+      const approvedFiles = propertyRule.approved;
+
+      const reportInfo = {
+        ruleName: `${NAMESPACE}/${ruleName}`,
+        result: postcssResult,
+        node: decl,
+        message: '',
+      };
+
+      if (approvedFiles) {
+        shouldReport = !approvedFiles.some((inspectedFile) => {
+          return file.includes(inspectedFile);
+        });
+      }
+
+      if (shouldReport && isAutoFixing) {
+        decl.remove();
+        return;
+      }
+
+      if (!shouldReport) {
+        return;
+      }
+
+      reportInfo.message = messages.expected(
+        getNotCompliantMessage(`Usage of property "${decl.prop}" is not allowed.`)
+      );
+      report(reportInfo);
+    });
+  };
+};
+
+ruleFunction.ruleName = ruleName;
+ruleFunction.messages = messages;
+
+// eslint-disable-next-line import/no-default-export
+export default ruleFunction;
diff --git a/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts b/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts
@@ -9,14 +9,16 @@
  * GitHub history for details.
  */
 
+import { ValueBasedConfig } from './get_rules_from_config';
+
 export interface ComplianceRule {
   isComplaint: boolean;
   actual: string;
-  expected: string;
+  expected: string | undefined;
   message: string;
 }
 
-const getRule = (rules: JSON, nodeInfo: { selector: string; prop: string }) => {
+const getRule = (rules: ValueBasedConfig, nodeInfo: { selector: string; prop: string }) => {
   const rule = rules[nodeInfo.prop];
   if (!rule) {
     return undefined;
@@ -38,12 +40,12 @@ const getRule = (rules: JSON, nodeInfo: { selector: string; prop: string }) => {
   return rule[ruleKey];
 };
 
-const isTracked = (rules: JSON, nodeInfo: { selector: string; prop: string }) => {
+const isTracked = (rules: ValueBasedConfig, nodeInfo: { selector: string; prop: string }) => {
   return getRule(rules, nodeInfo) !== undefined;
 };
 
 const getComplianceRule = (
-  rules: JSON,
+  rules: ValueBasedConfig,
   nodeInfo: { selector: string; prop: string; value: string }
 ): ComplianceRule | undefined => {
   const rule = getRule(rules, nodeInfo);
@@ -53,7 +55,7 @@ const getComplianceRule = (
   }
 
   const ruleObject = rule.find((object) => {
-    if (object.approved.includes(nodeInfo.value) || object.rejected.includes(nodeInfo.value)) {
+    if (object.approved?.includes(nodeInfo.value) || object.rejected?.includes(nodeInfo.value)) {
       return object;
     }
   });
@@ -63,7 +65,7 @@ const getComplianceRule = (
   }
 
   return {
-    isComplaint: !ruleObject.rejected.includes(nodeInfo.value),
+    isComplaint: !ruleObject.rejected?.includes(nodeInfo.value),
     actual: nodeInfo.value,
     expected: ruleObject.approved,
     message: `${nodeInfo.selector} expected: ${ruleObject.approved} - actual: ${nodeInfo.value}`,

diff --git a/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts b/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts
@@ -13,15 +13,21 @@ import path from 'path';
 import { readFileSync } from 'fs';
 import { matches } from './matches';
 
+export type FileBasedConfig = Record<string, { approved?: string[] }>;
+export type ValueBasedConfig = Record<
+  string,
+  Record<string, Array<{ approved?: string; rejected?: string[] }>>
+>;
+
 export const getRulesFromConfig = (configPath: string) => {
   const filePath = path.resolve(__dirname, configPath);
   return JSON.parse(readFileSync(filePath, 'utf-8'));
 };
 
-export const getSelectorRule = (rules: any, rule: any) => {
-  for (const configRule of Object.keys(rules)) {
-    if (matches(configRule, rule.selector)) {
-      return rules[configRule];
+export const getRuleFromConfig = (rules: FileBasedConfig, value: string) => {
+  for (const key of Object.keys(rules)) {
+    if (matches(key, value)) {
+      return rules[key];
     }
   }