submissionXmlToFieldStream: handle stream end properly #1256
Conversation
Previously if poorly-structured or undefined/null XML was passed to this function, it would hang.
| @@ -58,18 +66,15 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, | |||
| textBuffer += text; | |||
| }, | |||
| onclosetag: () => { | |||
| if (stack.hasExited()) throw new Error('Stack has already exited'); | |||
There was a problem hiding this comment.
I don't feel totally sure about throwing here. Is it impossible for there to be XML after the last tag for the fields in the SchemaStack? Could this function (as well as onopentag and ontext) just early-return if the stack has exited?
If it is the case that we should throw, I feel like the throw should happen in onopentag. Rather than waiting until the close tag, if we can tell at the open tag that there will be a problem, I think it'd be better to throw sooner.
There was a problem hiding this comment.
I don't feel totally sure about throwing here. Is it impossible for there to be XML after the last tag for the fields in the
SchemaStack?
I'm not sure, but I expect having a close tag after the stack is empty represents something very unexpected happening.
If it is the case that we should throw, I feel like the throw should happen in
onopentag.
I don't think there's any guarantee there will be an opentag after a dangling closetag, but maybe both cases should throw?
| if (!stack.hasExited()) { | ||
| outStream.destroy(new Error('Stack has not exited yet')); | ||
| } else { | ||
| parser.reset(); |
There was a problem hiding this comment.
I don't feel quite sure about what parser.reset() is doing here. I don't see reset() mentioned in the docs for htmlparser2 (though I know we're using an old version). If the parser has ended, and we're not reusing the parser, why do we need to reset it? Part of the reason I'm asking is that I wonder whether reset() had more of an effect before. Could reset() have had more of an effect when it was called in onclosetag, like causing the parser to stop parsing early?
There was a problem hiding this comment.
I don't feel quite sure about what
parser.reset()is doing here.
Copied from current line 70. I'm also not sure why it's required, but my guess is similar to yours. Would you rather remove it?
Previously if poorly-structured or undefined/null XML was passed to this function, the iterator this function returns would hang.