Throw an error if a 'data' Liquid tag fails (#25730)

Author: peterbe

lib/liquid-tags/data.js

@@ -1,5 +1,7 @@
 import { TokenizationError } from 'liquidjs'
 
+import { THROW_ON_EMPTY, DataReferenceError } from './error-handling.js'
+
 const Syntax = /([a-z0-9/\\_.\-[\]]+)/i
 const SyntaxHelp = "Syntax Error in 'data' - Valid syntax: data [path]"
 
@@ -14,7 +16,14 @@ export default {
 
   async render(scope) {
     const value = await this.liquid.evalValue(`site.data.${this.path}`, scope)
-    if (typeof value !== 'string') return value
+    if (typeof value !== 'string') {
+      const message = `Can't find the key 'site.data.${this.path}' in the scope.`
+      if (THROW_ON_EMPTY) {
+        throw new DataReferenceError(message)
+      }
+      console.warn(message)
+      return
+    }
     return this.liquid.parseAndRender(value, scope.environments)
   },
 }

lib/liquid-tags/error-handling.js

@@ -0,0 +1,10 @@
+// If 'THROW_ON_EMPTY' is set and it's value is '0' or 'false' it becomes
+// false. Or true if it's 'true' or '1'.
+export const THROW_ON_EMPTY = Boolean(
+  process.env.THROW_ON_EMPTY
+    ? JSON.parse(process.env.THROW_ON_EMPTY)
+    : JSON.parse(process.env.CI || process.env.NODE_ENV !== 'production')
+)
+
+export class DataReferenceError extends Error {}
+export class IndentedDataReferenceError extends DataReferenceError {}

lib/liquid-tags/indented-data-reference.js

@@ -1,14 +1,6 @@
 import assert from 'assert'
 
-// If 'THROW_ON_EMPTY' is set and it's value is '0' or 'false' it becomes
-// false. Or true if it's 'true' or '1'.
-const THROW_ON_EMPTY = Boolean(
-  process.env.THROW_ON_EMPTY
-    ? JSON.parse(process.env.THROW_ON_EMPTY)
-    : JSON.parse(process.env.CI || process.env.NODE_ENV !== 'production')
-)
-
-class IndentedDataReferenceError extends Error {}
+import { THROW_ON_EMPTY, IndentedDataReferenceError } from './error-handling.js'
 
 // This class supports a tag that expects two parameters, a data reference and `spaces=NUMBER`:
 //

tests/fixtures/liquid-tags/bad-data-variable.md

@@ -0,0 +1,6 @@
+---
+title: Sample page
+versions: '*'
+---
+
+{% data foo.bar.tipu %}

tests/fixtures/liquid-tags/good-data-variable.md

@@ -0,0 +1,6 @@
+---
+title: Good sample page
+versions: '*'
+---
+
+{% data variables.foo %}

tests/unit/liquid-tags/data.js

@@ -0,0 +1,53 @@
+import { expect } from '@jest/globals'
+import path from 'path'
+import Page from '../../../lib/page.js'
+import nonEnterpriseDefaultVersion from '../../../lib/non-enterprise-default-version.js'
+import { fileURLToPath } from 'url'
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url))
+
+describe('data tag', () => {
+  it('should render fine if data is found', async () => {
+    const page = await Page.init({
+      relativePath: 'liquid-tags/good-data-variable.md',
+      basePath: path.join(__dirname, '../../fixtures'),
+      languageCode: 'en',
+    })
+    const context = {
+      currentVersion: nonEnterpriseDefaultVersion,
+      currentLanguage: 'en',
+      currentPath: '/en/liquid-tags/good-data-variable',
+    }
+    const rendered = await page.render(
+      Object.assign(
+        {
+          site: {
+            data: {
+              variables: {
+                foo: 'Foo',
+              },
+            },
+          },
+        },
+        context
+      )
+    )
+    // The test fixture contains:
+    //   {% data variables.foo %}
+    // which we control the value of here in the test.
+    expect(rendered.includes('Foo')).toBeTruthy()
+  })
+  it('should throw if the data tag is used with something unrecognized', async () => {
+    const page = await Page.init({
+      relativePath: 'liquid-tags/bad-data-variable.md',
+      basePath: path.join(__dirname, '../../fixtures'),
+      languageCode: 'en',
+    })
+    const context = {
+      currentPath: '/en/liquid-tags/bad-data-variable',
+    }
+    await expect(page.render(context)).rejects.toThrow(
+      "Can't find the key 'site.data.foo.bar.tipu' in the scope., line:2, col:1"
+    )
+  })
+})

tests/unit/page.js

@@ -8,6 +8,7 @@ import readJsonFile from '../../lib/read-json-file.js'
 import { allVersions } from '../../lib/all-versions.js'
 import enterpriseServerReleases, { latest } from '../../lib/enterprise-server-releases.js'
 import nonEnterpriseDefaultVersion from '../../lib/non-enterprise-default-version.js'
+import loadSiteData from '../../lib/site-data.js'
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url))
 const prerenderedObjects = readJsonFile('./lib/graphql/static/prerendered-objects.json')
@@ -80,6 +81,8 @@ describe('Page class', () => {
   })
 
   describe('page.render(context)', () => {
+    const siteData = loadSiteData()
+
     test('rewrites links to include the current language prefix and version', async () => {
       const page = await Page.init(opts)
       const context = {
@@ -88,6 +91,7 @@ describe('Page class', () => {
         currentPath:
           '/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches',
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       const rendered = await page.render(context)
       const $ = cheerio.load(rendered)
@@ -126,6 +130,7 @@ describe('Page class', () => {
         currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`,
         currentPath: '/en/page-with-deprecated-enterprise-links',
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       const rendered = await page.render(context)
       // That page only contains exactly 2 links. And we can know
@@ -161,6 +166,7 @@ describe('Page class', () => {
         currentPath:
           '/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches',
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       // This is needed because unit tests are weird. The page.render()
       // method is dependent on module global cache.
@@ -192,6 +198,7 @@ describe('Page class', () => {
         currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`,
         currentPath: `/en/enterprise-server@${enterpriseServerReleases.latest}/admin/enterprise-management/migrating-from-github-enterprise-1110x-to-2123`,
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       const rendered = await page.render(context)
       const $ = cheerio.load(rendered)
@@ -219,6 +226,7 @@ describe('Page class', () => {
         currentVersion: nonEnterpriseDefaultVersion,
         currentPath: `/en/${nonEnterpriseDefaultVersion}/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches`,
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       const rendered = await page.render(context)
       const $ = cheerio.load(rendered)
@@ -239,6 +247,7 @@ describe('Page class', () => {
         currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`,
         currentLanguage: 'en',
         enterpriseServerVersions,
+        site: siteData.en.site,
       }
       context.currentPath = `/${context.currentLanguage}/${context.currentVersion}/${page.relativePath}`
       let rendered = await page.render(context)
@@ -282,6 +291,7 @@ describe('Page class', () => {
       const context = {
         currentVersion: 'enterprise-server@3.0',
         currentLanguage: 'en',
+        site: siteData.en.site,
       }
       await expect(() => {
         return page.render(context)

tests/unit/toc-links.js

@@ -2,10 +2,13 @@ import { jest } from '@jest/globals'
 import { loadPages } from '../../lib/page-data.js'
 import renderContent from '../../lib/render-content/index.js'
 import { allVersionKeys } from '../../lib/all-versions.js'
+import loadSiteData from '../../lib/site-data.js'
 
 describe('toc links', () => {
   jest.setTimeout(3 * 60 * 1000)
 
+  const siteData = loadSiteData()
+
   test('every toc link works without redirects', async () => {
     const pages = await loadPages()
 
@@ -27,6 +30,7 @@ describe('toc links', () => {
           redirects: {},
           currentLanguage: 'en',
           currentVersion: pageVersion,
+          site: siteData.en.site,
         }
 
         // ensure all toc pages can render