From 2cbfe58654266a4aedab8f7acac00182d9f8ab9e Mon Sep 17 00:00:00 2001
From: Karolin Varner
Date: Fri, 19 Apr 2019 13:50:37 +0200
Subject: [PATCH] feat(pipe): Simplify pipeline step executor
BREAKING CHANGE: return value from pipeline functions are no longer merged into the context
NOTE: Most of the functional changes live in src/pipeline.js;
most other changes are just refactoring the rest of the code
base to utilize the changes there.
feat(pipe): Drop write protection in pipeline fixes #228
feat(pipe): Get rid of parameter merging fixes #223
This specific change necessitated the bulk of the changes to the code
bases; most of the code that was refactored relied on parameter
merging before.
feat(pipe): Fuller error reporting in pipeline
Specifically, all errors are now reported by printing an
error message the stack trace with additional information
identifying the specific pipeline step.
On the http level a 500 status code is transmitted.
Co-authored-by: Tobias Bocanegra
---
.eslintrc.js | 8 +-
src/defaults/html.pipe.js | 5 +-
src/defaults/json.pipe.js | 1 -
src/defaults/xml.pipe.js | 5 +-
src/helper.js | 25 -----
src/html/fetch-markdown.js | 49 ++++----
src/html/find-embeds.js | 3 -
src/html/flag-esi.js | 11 +-
src/html/get-metadata.js | 62 +++++------
src/html/html-to-hast.js | 11 +-
src/html/make-html.js | 5 +-
src/html/output-debug.js | 35 +++---
src/html/parse-frontmatter.js | 10 +-
src/html/parse-markdown.js | 10 +-
src/html/responsify-images.js | 3 -
src/html/set-status.js | 70 +++++-------
src/html/set-surrogate-key.js | 35 +++---
src/html/shared-cache.js | 15 +--
src/html/smartypants.js | 11 +-
src/html/split-sections.js | 18 ++-
src/html/static-asset-links.js | 16 +--
src/html/stringify-hast.js | 10 +-
src/json/emit-json.js | 29 +++--
src/pipeline.js | 136 +++++++++++++----------
src/utils/coerce-secrets.js | 1 -
src/utils/conditional-sections.js | 107 ++++++------------
src/utils/dump-context.js | 2 -
src/utils/is-production.js | 2 +-
src/utils/set-content-type.js | 18 +--
src/xml/check-xml.js | 20 ++--
src/xml/emit-xml.js | 33 +++---
src/xml/set-xml-status.js | 66 +++++------
test/testCheckXML.js | 4 +-
test/testConditionalSections.js | 55 +++++----
test/testDebugTmp.js | 11 +-
test/testDefault.js | 6 +-
test/testEmbedHandler.js | 8 +-
test/testEmitJSON.js | 54 +++------
test/testEmitXML.js | 29 ++---
test/testFetchMarkdown.js | 90 ++++++++-------
test/testFindEmbeds.js | 12 +-
test/testFrontmatter.js | 11 +-
test/testGetMetadata.js | 101 ++++++++---------
test/testHTML.js | 135 +++++++++++-----------
test/testHTMLProduction.js | 10 +-
test/testHelper.js | 31 ------
test/testJSON.js | 31 +++---
test/testOutputDebug.js | 10 +-
test/testParseMarkdown.js | 7 +-
test/testPipeline.js | 159 ++++++++++++++------------
test/testRewriteStatic.js | 44 ++++----
test/testSetContentType.js | 17 +--
test/testSetStatus.js | 62 ++++-------
test/testSetXMLStatus.js | 78 ++++++-------
test/testSmartypants.js | 14 ++-
test/testSplitSections.js | 8 +-
test/testStringifyHast.js | 166 ++++++++++++++--------------
test/testToHast.js | 178 +++++++++++++++---------------
test/testXML.js | 78 +++++++------
59 files changed, 1038 insertions(+), 1203 deletions(-)
delete mode 100644 src/helper.js
delete mode 100644 test/testHelper.js
diff --git a/.eslintrc.js b/.eslintrc.js
index fb5237fd5..6312a379b 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -42,8 +42,12 @@ module.exports = {
// Allow return before else & redundant else statements
'no-else-return': 'off',
- // allow dangling underscores for 'fields'
- 'no-underscore-dangle': ['error', {'allowAfterThis': true}],
+ // Quite useful to mark values as unused
+ 'no-underscore-dangle': 'off',
+
+ // We have quite a lot of use cases where assignment to function
+ // parameters is definitely desirable
+ 'no-param-reassign': 'off',
// enforce license header
'header/header': [2, 'block', ['',
diff --git a/src/defaults/html.pipe.js b/src/defaults/html.pipe.js
index 738c036f9..e70ac9014 100644
--- a/src/defaults/html.pipe.js
+++ b/src/defaults/html.pipe.js
@@ -18,7 +18,7 @@ const meta = require('../html/get-metadata.js');
const html = require('../html/make-html.js');
const responsive = require('../html/responsify-images.js');
const type = require('../utils/set-content-type.js');
-const { selectStatus } = require('../html/set-status.js');
+const selectStatus = require('../html/set-status.js');
const smartypants = require('../html/smartypants');
const sections = require('../html/split-sections');
const { selectstrain, selecttest } = require('../utils/conditional-sections');
@@ -36,7 +36,6 @@ const tohast = require('../html/html-to-hast');
const tohtml = require('../html/stringify-hast');
const addHeaders = require('../html/add-headers');
-/* eslint no-param-reassign: off */
/* eslint newline-per-chained-call: off */
function hascontent({ content }) {
@@ -71,7 +70,7 @@ const htmlpipe = (cont, payload, action) => {
.after(addHeaders)
.after(tohtml) // end HTML post-processing
.after(flag).expose('esi').when(esi) // flag ESI when there is ESI in the response
- .error(selectStatus(production()));
+ .error(selectStatus);
action.logger.log('debug', 'Running HTML pipeline');
return pipe.run(payload);
diff --git a/src/defaults/json.pipe.js b/src/defaults/json.pipe.js
index eda48b3de..1bab113df 100644
--- a/src/defaults/json.pipe.js
+++ b/src/defaults/json.pipe.js
@@ -25,7 +25,6 @@ const emit = require('../json/emit-json.js');
const { selectStatus } = require('../json/set-json-status.js');
const parseFrontmatter = require('../html/parse-frontmatter.js');
-/* eslint no-param-reassign: off */
/* eslint newline-per-chained-call: off */
const jsonpipe = (cont, payload, action) => {
diff --git a/src/defaults/xml.pipe.js b/src/defaults/xml.pipe.js
index 1947b4c63..ab638c76b 100644
--- a/src/defaults/xml.pipe.js
+++ b/src/defaults/xml.pipe.js
@@ -25,11 +25,10 @@ const dump = require('../utils/dump-context.js');
const validate = require('../utils/validate');
const type = require('../utils/set-content-type.js');
const emit = require('../xml/emit-xml.js');
-const { selectStatus } = require('../xml/set-xml-status.js');
+const selectStatus = require('../xml/set-xml-status.js');
const check = require('../xml/check-xml');
const parseFrontmatter = require('../html/parse-frontmatter');
-/* eslint no-param-reassign: off */
/* eslint newline-per-chained-call: off */
const xmlpipe = (cont, payload, action) => {
@@ -53,7 +52,7 @@ const xmlpipe = (cont, payload, action) => {
.when(uncached)
.after(key)
.after(flag).expose('esi').when(esi) // flag ESI when there is ESI in the response
- .error(selectStatus(production()));
+ .error(selectStatus);
action.logger.log('debug', 'Running XML pipeline');
return pipe.run(payload);
diff --git a/src/helper.js b/src/helper.js
deleted file mode 100644
index 413994ba9..000000000
--- a/src/helper.js
+++ /dev/null
@@ -1,25 +0,0 @@
-/*
- * Copyright 2018 Adobe. All rights reserved.
- * This file is licensed to you under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License. You may obtain a copy
- * of the License at http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under
- * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
- * OF ANY KIND, either express or implied. See the License for the specific language
- * governing permissions and limitations under the License.
- */
-
-function bail(logger, message, error, status = 500) {
- logger.error(message);
- return {
- error: Object.assign({ message }, error),
- response: {
- status,
- },
- };
-}
-
-module.exports = {
- bail,
-};
diff --git a/src/html/fetch-markdown.js b/src/html/fetch-markdown.js
index 9db56ba9d..438a4ca80 100644
--- a/src/html/fetch-markdown.js
+++ b/src/html/fetch-markdown.js
@@ -9,9 +9,10 @@
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/
+const { inspect } = require('util');
const client = require('request-promise-native');
const URI = require('uri-js');
-const { bail } = require('../helper');
+const { setdefault } = require('@adobe/helix-shared').types;
function uri(root, owner, repo, ref, path) {
const rootURI = URI.parse(root);
@@ -36,16 +37,11 @@ function uri(root, owner, repo, ref, path) {
* @param {import("../context").Context} ctx some param
* @param {import("../context").Action} action some other param
*/
-async function fetch(
- { content: { sources = [] } = {} },
- {
- secrets = {},
- request,
- logger,
- },
-) {
+async function fetch(context, { secrets = {}, request, logger }) {
+ const content = setdefault(context, 'content', {});
+
if (!request || !request.params) {
- return bail(logger, 'Request parameters are missing');
+ throw new Error('Request parameters missing');
}
let timeout;
@@ -64,13 +60,13 @@ async function fetch(
// bail if a required parameter cannot be found
if (!owner) {
- return bail(logger, 'Unknown owner, cannot fetch content');
+ throw new Error('Unknown owner, cannot fetch content');
}
if (!repo) {
- return bail(logger, 'Unknown repo, cannot fetch content');
+ throw new Error('Unknown repo, cannot fetch content');
}
if (!path) {
- return bail(logger, 'Unknown path, cannot fetch content');
+ throw new Error('Unknown path, cannot fetch content');
}
if (!ref) {
logger.warn(`Recoverable error: no ref given for ${repo}/${owner}.git${path}, falling back to master`);
@@ -86,24 +82,25 @@ async function fetch(
timeout,
time: true,
};
+
logger.debug(`fetching Markdown from ${options.uri}`);
try {
- const response = await client(options);
- return {
- content: {
- body: response,
- sources: [...sources, options.uri],
- },
- };
+ content.body = await client(options);
+ setdefault(content, 'sources', []).push(options.uri);
} catch (e) {
- if (e && e.statusCode && e.statusCode === 404) {
- return bail(logger, `Could not find Markdown at ${options.uri}`, e, 404);
- } else if ((e && e.response && e.response.elapsedTime && e.response.elapsedTime > timeout) || (e && e.cause && e.cause.code && (e.cause.code === 'ESOCKETTIMEDOUT' || e.cause.code === 'ETIMEDOUT'))) {
+ if (e.statusCode === 404) {
+ logger.error(`Could not find Markdown at ${options.uri}`);
+ setdefault(context, 'response', {}).status = 404;
+ } else if ((e.response && e.response.elapsedTime && e.response.elapsedTime > timeout) || (e.cause && e.cause.code && (e.cause.code === 'ESOCKETTIMEDOUT' || e.cause.code === 'ETIMEDOUT'))) {
// return gateway timeout
- return bail(logger, `Gateway timout of ${timeout} milliseconds exceeded for ${options.uri}`, e, 504);
+ logger.error(`Gateway timout of ${timeout} milliseconds exceeded for ${options.uri}`);
+ setdefault(context, 'response', {}).status = 504;
+ } else {
+ logger.error(`Error while fetching Markdown from ${options.uri} with the following `
+ + `options:\n${inspect(options, { depth: null })}`);
+ setdefault(context, 'response', {}).status = 502;
}
- // return a bad gateway for all other errors
- return bail(logger, `Could not fetch Markdown from ${options.uri}`, e, 502);
+ context.error = e;
}
}
diff --git a/src/html/find-embeds.js b/src/html/find-embeds.js
index 41e97de40..93b27d8d8 100644
--- a/src/html/find-embeds.js
+++ b/src/html/find-embeds.js
@@ -9,7 +9,6 @@
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/
-/* eslint-disable no-param-reassign */
const map = require('unist-util-map');
const URI = require('uri-js');
const mm = require('micromatch');
@@ -135,8 +134,6 @@ function find({ content: { mdast }, request: { extension, url } },
internalembed(internalImgEmbed(node, url, contentext, resourceext), node, `.${EMBED_SELECTOR}.${extension}`);
}
});
-
- return { content: { mdast } };
}
module.exports = find;
diff --git a/src/html/flag-esi.js b/src/html/flag-esi.js
index adeb250ce..455b7122b 100644
--- a/src/html/flag-esi.js
+++ b/src/html/flag-esi.js
@@ -9,26 +9,29 @@
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/
+
+const { merge } = require('lodash');
+
/**
* Detects if ESI tags are used in the repose body. Intended to be used as
* a predicate in the pipeline construction.
* @param {Context} param0 the pipeline payload
*/
function esi({ response }) {
- return response && response.body && / payload)));
return section;
}
function title(section) {
const header = select('heading', section);
- return header ? Object.assign({ title: plain(header) }, section) : section;
+ section.title = header ? plain(header) : '';
}
function intro(section) {
@@ -34,14 +36,16 @@ function intro(section) {
}
return true;
})[0];
- return para ? Object.assign({ intro: plain(para) }, section) : section;
+ section.intro = para ? plain(para) : '';
}
function image(section) {
// selects the most prominent image of the section
// TODO: get a better measure of prominence than "first"
const img = select('image', section);
- return img ? Object.assign({ image: img.url }, section) : section;
+ if (img) {
+ section.image = img.url;
+ }
}
/**
@@ -77,7 +81,6 @@ function sectiontype(section) {
function reducer(counter, node) {
const { type, children: pChildren } = node;
- // eslint-disable-next-line no-param-reassign
node.data = Object.assign({ types: [] }, node.data);
if (type === 'yaml') {
@@ -129,43 +132,36 @@ function sectiontype(section) {
}
const typecounter = children.reduce(reducer, {});
-
- const types = constructTypes(typecounter);
-
- return Object.assign({ types }, section);
+ section.types = constructTypes(typecounter);
}
function fallback(section) {
if (section.intro && !section.title) {
- return Object.assign({ title: section.intro }, section);
+ section.title = section.intro;
} else if (section.title && !section.intro) {
- return Object.assign({ intro: section.title }, section);
+ section.intro = section.title;
}
- return section;
}
-function getmetadata({ content: { sections = [] } }, { logger }) {
+function getmetadata({ content }, { logger }) {
+ const { sections } = content;
+ if (!sections) {
+ content.meta = {};
+ return;
+ }
+
logger.debug(`Parsing Markdown Metadata from ${sections.length} sections`);
- const retsections = sections
- .map(yaml)
- .map(title)
- .map(intro)
- .map(image)
- .map(sectiontype)
- .map(fallback);
- const img = retsections.filter(section => section.image)[0];
- if (retsections[0]) {
- const retcontent = {
- sections: retsections,
- meta: retsections[0].meta,
- title: retsections[0].title,
- intro: retsections[0].intro,
- image: img ? img.image : undefined,
- };
- return { content: retcontent };
- }
- return { content: { meta: {} } };
+ each([yaml, title, intro, image, sectiontype, fallback], (fn) => {
+ each(sections, fn);
+ });
+
+ const img = sections.filter(section => section.image)[0];
+
+ content.meta = empty(sections) ? {} : sections[0].meta;
+ content.title = empty(sections) ? '' : sections[0].title;
+ content.intro = empty(sections) ? '' : sections[0].intro;
+ content.image = img ? img.image : undefined;
}
module.exports = getmetadata;
diff --git a/src/html/html-to-hast.js b/src/html/html-to-hast.js
index b15e90ac3..083a70d45 100644
--- a/src/html/html-to-hast.js
+++ b/src/html/html-to-hast.js
@@ -12,14 +12,9 @@
const unified = require('unified');
const parse = require('rehype-parse');
-function tohast({ response: { body } }) {
- const fragment = !body.match(/';
+const DEBUG_TEMPLATE = '';
-function debug(payload, { logger }) {
- const isDebug = payload.request && payload.request.params && (payload.request.params.debug === true || payload.request.params.debug === 'true');
- const hasBody = payload.response && payload.response.body;
- if (isDebug && hasBody) {
- logger.debug('Adding debug script');
- const p = payload;
- // backup body
- const { body } = p.response;
- // remove body because that would be the response content
- // and causes rendering issues of the script
- delete p.response.body;
+function debug(context, { logger }) {
+ const isDebug = context.request && context.request.params && (context.request.params.debug === true || context.request.params.debug === 'true');
+ const hasBody = context.response && context.response.body;
- const debugScript = DEBUG_TEMPLATE.replace(/PAYLOAD_JSON/, JSON.stringify(p));
- // inject debug script before the closing body tag
- p.response.body = body.replace(/<\/body>/i, `${debugScript}`);
- return p;
+ if (!isDebug || !hasBody) {
+ return;
}
- return payload;
+
+ logger.debug('Adding debug script');
+ // backup body
+ const { body } = context.response;
+
+ // remove body because that would be the response content
+ // and causes rendering issues of the script
+ delete context.response.body;
+
+ const debugScript = DEBUG_TEMPLATE.replace(/PAYLOAD_JSON/, JSON.stringify(context));
+ // inject debug script before the closing body tag
+ context.response.body = body.replace(/<\/body>/i, `${debugScript}
500
${error}