From 826608b4fcb93323e33c22573f4f4f45da95ed03 Mon Sep 17 00:00:00 2001 From: Rachael Sewell Date: Thu, 12 Oct 2023 13:45:54 -0700 Subject: [PATCH] Add liquid syntax linter rule (#44150) Co-authored-by: Peter Bengtsson --- lib/render-with-fallback.js | 2 +- src/content-linter/lib/linting-rules/index.js | 3 + .../lib/linting-rules/liquid-syntax.js | 97 +++++++++++++++++++ src/content-linter/style/github-docs.js | 9 ++ src/content-linter/tests/lint-files.js | 45 --------- .../tests/unit/liquid-syntax.js | 84 ++++++++++++++++ 6 files changed, 194 insertions(+), 46 deletions(-) create mode 100644 src/content-linter/lib/linting-rules/liquid-syntax.js create mode 100644 src/content-linter/tests/unit/liquid-syntax.js diff --git a/lib/render-with-fallback.js b/lib/render-with-fallback.js index a2def19648..c187eefef9 100644 --- a/lib/render-with-fallback.js +++ b/lib/render-with-fallback.js @@ -5,7 +5,7 @@ import { TitleFromAutotitleError } from '#src/content-render/unified/rewrite-loc class EmptyTitleError extends Error {} const LIQUID_ERROR_NAMES = new Set(['RenderError', 'ParseError', 'TokenizationError']) -const isLiquidError = (error) => +export const isLiquidError = (error) => error instanceof Error && error.name && LIQUID_ERROR_NAMES.has(error.name) const isAutotitleError = (error) => error instanceof TitleFromAutotitleError diff --git a/src/content-linter/lib/linting-rules/index.js b/src/content-linter/lib/linting-rules/index.js index b60b807de1..e51fc87e2d 100644 --- a/src/content-linter/lib/linting-rules/index.js +++ b/src/content-linter/lib/linting-rules/index.js @@ -24,6 +24,7 @@ import { liquidQuotedConditionalArg } from './liquid-quoted-conditional-arg.js' import { liquidDataReferencesDefined, liquidDataTagFormat } from './liquid-data-tags.js' import { frontmatterFormat } from './frontmatter-format.js' import { annotateFrontmatter } from './annotate-frontmatter.js' +import { frontmatterLiquidSyntax, liquidSyntax } from './liquid-syntax.js' import { liquidIfTags, liquidIfVersionTags } from './liquid-versioning.js' const noDefaultAltText = markdownlintGitHub.find((elem) => @@ -60,6 +61,8 @@ export const gitHubDocsMarkdownlint = { frontmatterVideoTranscripts, frontmatterFormat, annotateFrontmatter, + frontmatterLiquidSyntax, + liquidSyntax, liquidIfTags, liquidIfVersionTags, ], diff --git a/src/content-linter/lib/linting-rules/liquid-syntax.js b/src/content-linter/lib/linting-rules/liquid-syntax.js new file mode 100644 index 0000000000..adf4ad09cf --- /dev/null +++ b/src/content-linter/lib/linting-rules/liquid-syntax.js @@ -0,0 +1,97 @@ +import { addError } from 'markdownlint-rule-helpers' + +import { getFrontmatter } from '../helpers/utils.js' +import { liquid } from '#src/content-render/index.js' +import { isLiquidError } from '../../../../lib/render-with-fallback.js' + +/* + Attempts to parse all liquid in the frontmatter of a file + to verify the syntax is correct. +*/ +export const frontmatterLiquidSyntax = { + names: ['LQ115', 'frontmatter-liquid-syntax'], + description: + 'Frontmatter properties that contain Markdown (e.g., translatable strings) must use valid liquid.', + tags: ['liquid', 'frontmatter'], + function: function LQ115(params, onError) { + const fm = getFrontmatter(params.lines) + if (!fm) return + + // Currently this list is hardcoded, but in the future we plan to + // use a custom key in the frontmatter to determine which keys + // contain Liquid. + const keysWithLiquid = ['title', 'shortTitle', 'intro', 'product', 'permissions'].filter( + (key) => Boolean(fm[key]), + ) + + for (const key of keysWithLiquid) { + const value = fm[key] + try { + liquid.parse(value) + } catch (error) { + // If the error source is not a Liquid error but rather a + // ReferenceError or bad type we should allow that error to be thrown + if (!isLiquidError(error)) throw error + const { errorDescription, columnNumber } = getErrorMessageInfo(error.message) + const lineNumber = params.lines.findIndex((line) => line.trim().startsWith(`${key}:`)) + 1 + // Add the key length plus 3 to the column number to account colon and + // for the space after the key and column number starting at 1. + // If there is no space after the colon, a YAMLException will be thrown. + const range = [columnNumber + key.length + 3, value.length] + addError( + onError, + lineNumber, + 'Liquid syntax error: ' + errorDescription, + value, + range, + null, // No fix possible + ) + } + } + }, +} + +/* + Attempts to parse all liquid in the Markdown content of a file + to verify the syntax is correct. +*/ +export const liquidSyntax = { + names: ['GHD90', 'liquid-syntax'], + description: 'Markdown content must have valid liquid.', + tags: ['liquid'], + function: function GHD90(params, onError) { + try { + liquid.parse(params.lines.join('\n')) + } catch (error) { + // If the error source is not a Liquid error but rather a + // ReferenceError or bad type we should allow that error to be thrown + if (!isLiquidError(error)) throw error + const { errorDescription, lineNumber, columnNumber } = getErrorMessageInfo(error.message) + const line = params.lines[lineNumber - 1] + // We don't have enough information to know the length of the full + // liquid tag without doing some regex testing and making assumptions + // about if the end tag is correctly formed, so we just give a + // range from the start of the tag to the end of the line. + const range = [columnNumber, line.slice(columnNumber - 1).length] + addError( + onError, + lineNumber, + 'Liquid syntax error: ' + errorDescription, + line, + range, + null, // No fix possible + ) + } + }, +} + +function getErrorMessageInfo(message) { + const [errorDescription, lineString, columnString] = message.split(',') + // There has to be a line number so we'll default to line 1 if the message + // doesn't contain a line number. + if (!columnString || !lineString) + throw new Error('Liquid error message does not contain line or column number') + const lineNumber = parseInt(lineString.trim().replace('line:', ''), 10) + const columnNumber = parseInt(columnString.trim().replace('col:', ''), 10) + return { errorDescription, lineNumber, columnNumber } +} diff --git a/src/content-linter/style/github-docs.js b/src/content-linter/style/github-docs.js index 7f72f3384b..42e2083edf 100644 --- a/src/content-linter/style/github-docs.js +++ b/src/content-linter/style/github-docs.js @@ -86,6 +86,10 @@ const githubDocsConfig = { severity: 'error', 'partial-markdown-files': false, }, + 'liquid-syntax': { + severity: 'error', + 'partial-markdown-files': true, + }, 'liquid-if-tags': { // LQ114 severity: 'error', @@ -118,6 +122,11 @@ export const githubDocsFrontmatterConfig = { severity: 'error', 'partial-markdown-files': false, }, + 'frontmatter-liquid-syntax': { + // LQ115 + severity: 'error', + 'partial-markdown-files': false, + }, } // Configures rules from the `github/markdownlint-github` repo diff --git a/src/content-linter/tests/lint-files.js b/src/content-linter/tests/lint-files.js index a2f19c3866..67399e53d8 100755 --- a/src/content-linter/tests/lint-files.js +++ b/src/content-linter/tests/lint-files.js @@ -4,15 +4,11 @@ import slash from 'slash' import walk from 'walk-sync' import { zip } from 'lodash-es' import yaml from 'js-yaml' -import { fromMarkdown } from 'mdast-util-from-markdown' -import { visit } from 'unist-util-visit' import fs from 'fs/promises' import { existsSync } from 'fs' import { jest } from '@jest/globals' -import { frontmatter } from '../../../lib/frontmatter.js' import languages from '#src/languages/lib/languages.js' -import { liquid } from '#src/content-render/index.js' import { getDiffFiles } from '../lib/diff-files.js' jest.useFakeTimers({ legacyFakeTimers: true }) @@ -313,47 +309,6 @@ if (mdToLint.length + ymlToLint.length < 1) { }) } -describe('lint markdown content', () => { - if (mdToLint.length < 1) return - - describe.each(mdToLint)('%s', (markdownRelPath, markdownAbsPath) => { - let content, ast, links, frontmatterData - - beforeAll(async () => { - const fileContents = await fs.readFile(markdownAbsPath, 'utf8') - const { data, content: bodyContent } = frontmatter(fileContents) - - content = bodyContent - frontmatterData = data - ast = fromMarkdown(content) - - links = [] - visit(ast, ['link', 'definition'], (node) => { - links.push(node.url) - }) - }) - - test('contains valid Liquid', async () => { - // If Liquid can't parse the file, it'll throw an error. - // For example, the following is invalid and will fail this test: - // {% if currentVersion ! "github-ae@latest" %} - expect(() => liquid.parse(content)).not.toThrow() - }) - - if (!markdownRelPath.includes('data/reusables')) { - test('frontmatter contains valid liquid', async () => { - const fmKeysWithLiquid = ['title', 'shortTitle', 'intro', 'product', 'permission'].filter( - (key) => Boolean(frontmatterData[key]), - ) - - for (const key of fmKeysWithLiquid) { - expect(() => liquid.parse(frontmatterData[key])).not.toThrow() - } - }) - } - }) -}) - describe('lint yaml content', () => { if (ymlToLint.length < 1) return describe.each(ymlToLint)('%s', (yamlRelPath, yamlAbsPath) => { diff --git a/src/content-linter/tests/unit/liquid-syntax.js b/src/content-linter/tests/unit/liquid-syntax.js new file mode 100644 index 0000000000..f1e4109ab6 --- /dev/null +++ b/src/content-linter/tests/unit/liquid-syntax.js @@ -0,0 +1,84 @@ +import { runRule } from '../../lib/init-test.js' +import { frontmatterLiquidSyntax, liquidSyntax } from '../../lib/linting-rules/liquid-syntax.js' + +// Configure the test figure to not split frontmatter and content +const fmOptions = { markdownlintOptions: { frontMatter: null } } + +describe(frontmatterLiquidSyntax.names.join(' - '), () => { + test('Frontmatter that contains invalid syntax fails', async () => { + const markdown = [ + '---', + 'title: "{% ata variables.product.product_name %}"', + 'shortTitle: "{% "', + 'intro: "{% data reusables.foo.bar }"', + 'permissions: "{% if true %}Permission statement"', + 'showMiniToc: "{% if true %}Permission statement"', + '---', + ].join('\n') + const result = await runRule(frontmatterLiquidSyntax, { strings: { markdown }, ...fmOptions }) + const errors = result.markdown + expect(errors.length).toBe(4) + expect(errors.map((error) => error.lineNumber)).toEqual([2, 3, 4, 5]) + expect(errors[0].errorRange).toEqual([9, 40]) + }) + test('Frontmatter that contains valid Liquid passes', async () => { + const markdown = [ + '---', + "title: '{% data variables.product.product_name %}'", + 'shortTitle:', + "intro: '{% data reusables.foo.bar %}'", + "permissions: '{% if true %}Permission statement{% endif %}'", + 'showMiniToc: true', + '---', + ].join('\n') + const result = await runRule(liquidSyntax, { strings: { markdown }, ...fmOptions }) + const errors = result.markdown + expect(errors.length).toBe(0) + }) +}) + +describe(liquidSyntax.names.join(' - '), () => { + test('Missing closing tag syntax in Markdown content fails', async () => { + const markdown = ['---', 'title: Title', '---', '{% data reusables.foo.bar }'].join('\n') + const result = await runRule(liquidSyntax, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(4) + expect(errors[0].errorRange).toEqual([1, 27]) + }) + test('Misspelled data tag in Markdown content fails', async () => { + const markdown = [ + '---', + 'title: Title', + '---', + '{% ata variables.product.product_name %}', + ].join('\n') + const result = await runRule(liquidSyntax, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(4) + expect(errors[0].errorRange).toEqual([1, 40]) + }) + test('Missing endif tag in Markdown content fails', async () => { + const markdown = ['---', 'title: Title', '---', '{% if true %}Permission statement'].join('\n') + const result = await runRule(liquidSyntax, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(1) + expect(errors[0].lineNumber).toBe(4) + expect(errors[0].errorRange).toEqual([1, 33]) + }) + test('Valid Liquid syntax in Markdown content passes', async () => { + const markdown = [ + '---', + 'title: "Title"', + '---', + '{% data reusables.foo.bar %}', + '{% if true %}Permission statement{% endif %}', + // Not correct, but not caught by this rule. See liquid-ifversion-tags. + '{% ifversion ghhes %}bla{%endif%}', + ].join('\n') + const result = await runRule(liquidSyntax, { strings: { markdown } }) + const errors = result.markdown + expect(errors.length).toBe(0) + }) +})