[MediaWiki-commits] [Gerrit] mediawiki...mobileapps[master]: Revert "Revert "Update section validation logic""
jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/398183 )
Change subject: Revert "Revert "Update section validation logic""
..
Revert "Revert "Update section validation logic""
This reverts commit ebb87374b3c69ff6a3a71dffaf4e1e64af9b.
It's easier to see what has changed from the previous solution
if we split up the changes into two commits. I'm also going to add
a unit test for the log call.
Change-Id: I2394ee5d14b7cb83f329c7f17e735a8f8d6342f9
---
M lib/parsoid-access.js
M lib/parsoidSections.js
M lib/parsoidSectionsUsingSectionTags.js
M test/lib/parsoid/parsoid-sections-section-elements-tests.js
4 files changed, 57 insertions(+), 31 deletions(-)
Approvals:
jenkins-bot: Verified
Mholloway: Looks good to me, approved
diff --git a/lib/parsoid-access.js b/lib/parsoid-access.js
index 338ce33..3ae3308 100644
--- a/lib/parsoid-access.js
+++ b/lib/parsoid-access.js
@@ -119,7 +119,7 @@
transforms.stripUnneededMarkup(doc, legacy);
parsoidSections.addSectionDivs(doc);
-page.sections = parsoidSections.getSectionsText(doc);
+page.sections = parsoidSections.getSectionsText(doc, req.logger);
return page;
});
}
diff --git a/lib/parsoidSections.js b/lib/parsoidSections.js
index dcdbb72..f8e21d6 100644
--- a/lib/parsoidSections.js
+++ b/lib/parsoidSections.js
@@ -27,13 +27,14 @@
* Gets the sections array, including HTML string and metadata for all sections
* (id, anchor, line, toclevel).
* @param {!document} doc the parsed DOM Document of the Parsoid output
+ * @param {!Logger} logger the app's bunyan logger
* @return {!sections[]} an array of section JSON elements
*/
-function getSectionsText(doc) {
+function getSectionsText(doc, logger) {
if (!hasParsoidSections(doc)) {
return parsoidSectionsUsingDivs.getSectionsText(doc);
} else {
-return parsoidSectionsUsingSectionTags.getSectionsText(doc);
+return parsoidSectionsUsingSectionTags.getSectionsText(doc, logger);
}
}
diff --git a/lib/parsoidSectionsUsingSectionTags.js
b/lib/parsoidSectionsUsingSectionTags.js
index 76e471d..ce27178 100644
--- a/lib/parsoidSectionsUsingSectionTags.js
+++ b/lib/parsoidSectionsUsingSectionTags.js
@@ -1,8 +1,6 @@
'use strict';
const parsoidDomUtils = require('parsoid-dom-utils');
-// const sUtil = require('./util');
-// const HTTPError = sUtil.HTTPError;
const NodeType = require('./nodeType');
/**
@@ -39,7 +37,13 @@
}
}
-/* function throwIfPreviousSectionIsIncomplete(allSections, sectionObj) {
+// skip lead sections since they don't have headings;
+// no line and no anchor means incomplete section
+function shouldLogInvalidSectionWarning(sectionObj) {
+return sectionObj.id >= 1 && !sectionObj.line && !sectionObj.anchor;
+}
+
+function validatePreviousSection(logger, allSections, sectionObj) {
if (allSections.length === 0) {
return;
}
@@ -47,30 +51,29 @@
if (!sectionObj) {
sectionObj = allSections[allSections.length - 1];
}
-// skip lead sections since they don't have headings;
-// no line and no anchor means incomplete section
-if (allSections.length > 1 && !sectionObj.line && !sectionObj.anchor) {
-throw new HTTPError({
-status: 502,
-type: 'unsupported_section',
-title: 'Section structure not supported',
+
+if (shouldLogInvalidSectionWarning(sectionObj)) {
+logger.warn({
+warning: 'invalid_section',
+title: 'Found section without expected heading',
detail: `Cannot find heading for section number ${sectionObj.id}.`
});
}
-} */
+}
/**
* Visits one DOM node. Do the stuff that needs to be done when a single DOM
node is handled.
* In this case, starts a new section object when a tag is
encountered that is a
* direct descendant
+ * @param {!Logger} logger the app's bunyan logger
* @param {!DOMNode} node the node to visit
* @param {!Object[]} allSections the array containing the results
* @param {!Object} state some state to pass around
*/
-function visit(node, allSections, state) {
+function visit(logger, node, allSections, state) {
let sectionObj = allSections.length > 0 ? allSections[allSections.length -
1] : undefined;
if (node.tagName === 'SECTION') {
-// throwIfPreviousSectionIsIncomplete(allSections, sectionObj);
+validatePreviousSection(logger, allSections, sectionObj);
sectionObj = { id: getSectionNumber(node), text: '' };
allSections.push(sectionObj);
@@ -99,16 +102,17 @@
* This started out with a traditional iterative DFS algorithm but added a
boolean to stop
* descending so we don't duplicate content since elsewhere outerHTML is used
(which contains
* content of sub-nodes).
+ * @param {!Logger} logger the app's bunyan logger
* @param
[MediaWiki-commits] [Gerrit] mediawiki...mobileapps[master]: Revert "Revert "Update section validation logic""
Hello jenkins-bot, Mholloway,
I'd like you to do a code review. Please visit
https://gerrit.wikimedia.org/r/398183
to review the following change.
Change subject: Revert "Revert "Update section validation logic""
..
Revert "Revert "Update section validation logic""
This reverts commit ebb87374b3c69ff6a3a71dffaf4e1e64af9b.
It's easier to see what has changed from the previous solution
if we split up the changes into two commits. I'm also going to add
a unit test for the log call.
Change-Id: I2394ee5d14b7cb83f329c7f17e735a8f8d6342f9
---
M lib/parsoid-access.js
M lib/parsoidSections.js
M lib/parsoidSectionsUsingSectionTags.js
M test/lib/parsoid/parsoid-sections-section-elements-tests.js
4 files changed, 57 insertions(+), 31 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps
refs/changes/83/398183/1
diff --git a/lib/parsoid-access.js b/lib/parsoid-access.js
index 338ce33..3ae3308 100644
--- a/lib/parsoid-access.js
+++ b/lib/parsoid-access.js
@@ -119,7 +119,7 @@
transforms.stripUnneededMarkup(doc, legacy);
parsoidSections.addSectionDivs(doc);
-page.sections = parsoidSections.getSectionsText(doc);
+page.sections = parsoidSections.getSectionsText(doc, req.logger);
return page;
});
}
diff --git a/lib/parsoidSections.js b/lib/parsoidSections.js
index dcdbb72..f8e21d6 100644
--- a/lib/parsoidSections.js
+++ b/lib/parsoidSections.js
@@ -27,13 +27,14 @@
* Gets the sections array, including HTML string and metadata for all sections
* (id, anchor, line, toclevel).
* @param {!document} doc the parsed DOM Document of the Parsoid output
+ * @param {!Logger} logger the app's bunyan logger
* @return {!sections[]} an array of section JSON elements
*/
-function getSectionsText(doc) {
+function getSectionsText(doc, logger) {
if (!hasParsoidSections(doc)) {
return parsoidSectionsUsingDivs.getSectionsText(doc);
} else {
-return parsoidSectionsUsingSectionTags.getSectionsText(doc);
+return parsoidSectionsUsingSectionTags.getSectionsText(doc, logger);
}
}
diff --git a/lib/parsoidSectionsUsingSectionTags.js
b/lib/parsoidSectionsUsingSectionTags.js
index 76e471d..ce27178 100644
--- a/lib/parsoidSectionsUsingSectionTags.js
+++ b/lib/parsoidSectionsUsingSectionTags.js
@@ -1,8 +1,6 @@
'use strict';
const parsoidDomUtils = require('parsoid-dom-utils');
-// const sUtil = require('./util');
-// const HTTPError = sUtil.HTTPError;
const NodeType = require('./nodeType');
/**
@@ -39,7 +37,13 @@
}
}
-/* function throwIfPreviousSectionIsIncomplete(allSections, sectionObj) {
+// skip lead sections since they don't have headings;
+// no line and no anchor means incomplete section
+function shouldLogInvalidSectionWarning(sectionObj) {
+return sectionObj.id >= 1 && !sectionObj.line && !sectionObj.anchor;
+}
+
+function validatePreviousSection(logger, allSections, sectionObj) {
if (allSections.length === 0) {
return;
}
@@ -47,30 +51,29 @@
if (!sectionObj) {
sectionObj = allSections[allSections.length - 1];
}
-// skip lead sections since they don't have headings;
-// no line and no anchor means incomplete section
-if (allSections.length > 1 && !sectionObj.line && !sectionObj.anchor) {
-throw new HTTPError({
-status: 502,
-type: 'unsupported_section',
-title: 'Section structure not supported',
+
+if (shouldLogInvalidSectionWarning(sectionObj)) {
+logger.warn({
+warning: 'invalid_section',
+title: 'Found section without expected heading',
detail: `Cannot find heading for section number ${sectionObj.id}.`
});
}
-} */
+}
/**
* Visits one DOM node. Do the stuff that needs to be done when a single DOM
node is handled.
* In this case, starts a new section object when a tag is
encountered that is a
* direct descendant
+ * @param {!Logger} logger the app's bunyan logger
* @param {!DOMNode} node the node to visit
* @param {!Object[]} allSections the array containing the results
* @param {!Object} state some state to pass around
*/
-function visit(node, allSections, state) {
+function visit(logger, node, allSections, state) {
let sectionObj = allSections.length > 0 ? allSections[allSections.length -
1] : undefined;
if (node.tagName === 'SECTION') {
-// throwIfPreviousSectionIsIncomplete(allSections, sectionObj);
+validatePreviousSection(logger, allSections, sectionObj);
sectionObj = { id: getSectionNumber(node), text: '' };
allSections.push(sectionObj);
@@ -99,16 +102,17 @@
* This started out with a traditional iterative DFS algorithm but added a
boolean to stop
* descending so we don't duplicate content since elsewhere outerHTML is used
(which contains
* content
