diff --git a/embedding/embedding_test.go b/embedding/embedding_test.go index b0ddb12..3de2ad1 100644 --- a/embedding/embedding_test.go +++ b/embedding/embedding_test.go @@ -36,6 +36,7 @@ import ( "embed-code/embed-code-go/configuration" "embed-code/embed-code-go/embedding" "embed-code/embed-code-go/embedding/parsing" + "embed-code/embed-code-go/logging" _type "embed-code/embed-code-go/type" . "github.com/onsi/ginkgo/v2" @@ -142,6 +143,27 @@ var _ = Describe("Embedding", func() { Expect(errors.As(err, &parseErr)).Should(BeTrue()) }) + // Regresses https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // The failure location must be a bare `file://...:line:column` URL so an + // IDE such as IntelliJ IDEA can open it from the console. Backticks around + // the URL and a missing column component prevent that navigation. + It("should format the failure location as a bare file URL with line and column", func() { + docPath := filepath.Join(GinkgoT().TempDir(), "doc.md") + processingErr := embedding.ProcessingError{ + DocFilePath: docPath, + Line: 2, + Err: errors.New("boom"), + } + + message := processingErr.Error() + + Expect(message).Should(ContainSubstring( + logging.FileReferenceWithPosition(docPath, 2, 1))) + Expect(message).Should(ContainSubstring(":2:1")) + Expect(message).ShouldNot(ContainSubstring("`")) + }) + It("should report all pattern matching errors", func() { config.DocIncludes = []string{"missing-start-pattern.md", "missing-end-pattern.md"} @@ -243,7 +265,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "missing-closing-tag.md:3`: " + + "missing-closing-tag.md:3:1: " + "failed to parse an embedding instruction: " + "the `` tag is not closed", )) @@ -275,7 +297,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "unclosed-nested-tag.md:3`: " + + "unclosed-nested-tag.md:3:1: " + "failed to parse an embedding instruction: " + "element closed by ", )) @@ -289,7 +311,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "missing-code-fence.md:3`: " + + "missing-code-fence.md:3:1: " + "expected a markdown code fence after the embedding instruction", )) }) @@ -302,7 +324,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "unclosed-code-fence.md:3`: " + + "unclosed-code-fence.md:3:1: " + "the markdown code fence after the embedding instruction is not closed", )) }) diff --git a/embedding/error.go b/embedding/error.go index 4842d79..32f9402 100644 --- a/embedding/error.go +++ b/embedding/error.go @@ -44,13 +44,22 @@ type ProcessingError struct { Err error } +// errorLocationColumn is the column reported for a processing failure. The +// parser tracks line granularity only, so the location points at the beginning +// of the offending line. +const errorLocationColumn = 1 + // Error returns a user-facing description of the failed documentation processing operation. // +// The documentation location is emitted as a bare `file://...:line:column` +// URL, without surrounding backticks, so IDEs such as IntelliJ IDEA can open +// it from the console. +// // Returns formatted processing error text. func (e ProcessingError) Error() string { return fmt.Sprintf( - "failed to embed code fragment into doc file `%s`: %s", - logging.FileReferenceWithLine(e.DocFilePath, e.Line), + "failed to embed code fragment into doc file %s: %s", + logging.FileReferenceWithPosition(e.DocFilePath, e.Line, errorLocationColumn), e.Err, ) } diff --git a/embedding/parsing/instruction_token.go b/embedding/parsing/instruction_token.go index ab4c3b6..5ceff49 100644 --- a/embedding/parsing/instruction_token.go +++ b/embedding/parsing/instruction_token.go @@ -129,6 +129,13 @@ func (e EmbedInstructionTokenState) Accept(context *Context, context.Result = append(context.Result, line) context.ToNextLine() + + // Once the tag is syntactically closed, the following lines are not + // part of the instruction. Stop here instead of consuming the rest of + // the document trying to parse a complete but invalid instruction. + if context.EmbeddingInstruction == nil && instructionClosed(instructionBody) { + break + } } if context.EmbeddingInstruction == nil { return InstructionParseError{ @@ -140,11 +147,41 @@ func (e EmbedInstructionTokenState) Accept(context *Context, return nil } +// instructionClosed reports whether the accumulated instruction body contains a +// closing tag, meaning any following lines are not part of the instruction. +// +// The terminator is only recognized outside quoted attribute values, so a +// value such as `line="
"` does not end the instruction early. +func instructionClosed(instructionBody []string) bool { + instruction := strings.Join(instructionBody, " ") + closingTag := "" + insideValue := false + for index := 0; index < len(instruction); index++ { + if isEscapedQuote(instruction, index) { + index++ + + continue + } + if instruction[index] == '"' { + insideValue = !insideValue + + continue + } + if insideValue { + continue + } + if strings.HasPrefix(instruction[index:], "/>") || + strings.HasPrefix(instruction[index:], closingTag) { + return true + } + } + + return false +} + // parseFailureReason explains why an embedding instruction could not be parsed. func parseFailureReason(instructionBody []string, parseErr error) string { - instruction := strings.TrimSpace(strings.Join(instructionBody, " ")) - if !strings.Contains(instruction, "/>") && - !strings.Contains(instruction, "") { + if !instructionClosed(instructionBody) { return fmt.Sprintf("the `<%s>` tag is not closed", EmbeddingTag, ) diff --git a/embedding/parsing/state_test.go b/embedding/parsing/state_test.go index c2452b0..cc766e8 100644 --- a/embedding/parsing/state_test.go +++ b/embedding/parsing/state_test.go @@ -120,6 +120,100 @@ var _ = Describe("Parser states", func() { })) }) + // The following specs reproduce + // https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // A self-closed `` instruction that fails validation is a + // complete, single-line instruction, yet `Accept` keeps re-parsing the + // joined body and consumes every following line up to EOF before reporting + // the failure. That is why a single malformed instruction near the top of a + // document surfaces its error many lines later (line 76 -> line 404 in the + // original report) and destroys the parse of everything after it. + // + // The parser should report the InstructionParseError but stop right after + // the instruction's `/>` terminator, leaving the code fence and the rest of + // the document intact. These specs assert that bounded behavior and + // therefore FAIL until the over-consumption is fixed. + It("should not consume the rest of the document when a start pattern is invalid", func() { + assertBoundedMalformedInstruction(``) + }) + + It("should not consume the rest of the document when the comments mode is invalid", func() { + assertBoundedMalformedInstruction(``) + }) + + It("should not consume the rest of the document when attributes are mutually exclusive", func() { + assertBoundedMalformedInstruction(``) + }) + + // Reproduces https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // A self-closed instruction whose start/end/line pattern contains raw XML + // metacharacters (`<`, `&`) is legitimate user input (e.g. matching a Java + // generic or comparison), yet `quoteEscapedXMLLine` only escapes `\"`, so + // `xml.Unmarshal` rejects it. Because the tag is self-closed, this is NOT + // the "tag is not closed" case; instead `Accept` keeps re-parsing and + // consumes every following line up to EOF, then fails with + // InstructionParseError -- swallowing the code fence and the rest of the + // document, exactly the behavior reported in the issue. + // + // This test asserts the desired behavior and therefore FAILS until the XML + // metacharacters in attribute values are escaped before unmarshalling. + It("should parse an instruction whose pattern contains XML metacharacters", func() { + config := configuration.NewConfiguration() + context := newStateContext( + "", + "```java", + "old source", + "```", + "text after the fence", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + + err := parsing.EmbedInstruction.Accept(&context, config) + + Expect(err).ShouldNot(HaveOccurred()) + Expect(context.EmbeddingInstruction).ShouldNot(BeNil()) + Expect(context.EmbeddingInstruction.LinePattern).ShouldNot(BeNil()) + // The parser must stop right after the instruction line rather than + // swallowing the code fence and the rest of the document. + Expect(context.ReachedEOF()).Should(BeFalse()) + Expect(context.GetResult()).Should(Equal([]string{ + "", + })) + }) + + // Regresses https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // The `/>` inside the `line` value must not be mistaken for the tag + // terminator: the instruction spans several lines and only closes on the + // last one. Recognizing the inner `/>` would stop accumulation early and + // fail with an "unexpected EOF" before the real closing `/>`. + It("should accept a multiline instruction whose value contains a slash-close sequence", func() { + config := configuration.NewConfiguration() + context := newStateContext( + "`, + "```java", + "old source", + "```", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + Expect(parsing.EmbedInstruction.Accept(&context, config)).Should(Succeed()) + + Expect(context.EmbeddingInstruction).ShouldNot(BeNil()) + Expect(context.EmbeddingInstruction.CodeFile).Should(Equal("Example.java")) + Expect(context.EmbeddingInstruction.LinePattern).ShouldNot(BeNil()) + Expect(context.GetResult()).Should(Equal([]string{ + "`, + })) + }) + It("should render source and close the embedding fence when the end state is accepted", func() { sourceRoot := GinkgoT().TempDir() Expect(os.WriteFile( @@ -181,6 +275,32 @@ var _ = Describe("Parser states", func() { }) }) +// assertBoundedMalformedInstruction drives EmbedInstruction.Accept over a +// document whose first line is a self-closed but invalid instruction, followed +// by a code fence and trailing content. +// +// It asserts that the parser reports the failure as an InstructionParseError +// without consuming past the instruction line. See issue #19. +func assertBoundedMalformedInstruction(instruction string) { + config := configuration.NewConfiguration() + context := newStateContext( + instruction, + "```java", + "old source", + "```", + "text after the fence", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + + err := parsing.EmbedInstruction.Accept(&context, config) + + var parseErr parsing.InstructionParseError + Expect(errors.As(err, &parseErr)).Should(BeTrue()) + Expect(context.ReachedEOF()).Should(BeFalse()) + Expect(context.GetResult()).Should(Equal([]string{instruction})) +} + // newStateContext builds a parser context from in-memory source lines. func newStateContext(lines ...string) parsing.Context { docPath := filepath.Join(GinkgoT().TempDir(), "doc.md") diff --git a/embedding/parsing/xml_parse.go b/embedding/parsing/xml_parse.go index d7ab6c8..fd3719c 100644 --- a/embedding/parsing/xml_parse.go +++ b/embedding/parsing/xml_parse.go @@ -101,7 +101,136 @@ func ParseXMLLine(xmlLine string) (map[string]string, error) { return attributes, nil } -// quoteEscapedXMLLine converts backslash-escaped quotes into XML entities. +// quoteEscapedXMLLine escapes characters that XML forbids inside attribute +// values, so instruction patterns may contain raw source-code characters. +// +// Backslash-escaped quotes become the `"` entity, and the XML +// metacharacters `&`, `<`, and `>` appearing inside a quoted attribute value +// are escaped to their entities. Source-line patterns such as +// `line="if (a < b)"` or `end="while (x && y)"` routinely contain these +// characters; without escaping, xml.Unmarshal rejects the whole instruction +// and the parser fails with an InstructionParseError. Markup outside quoted +// values, such as the `` terminator, is left +// untouched. func quoteEscapedXMLLine(xmlLine string) string { - return strings.ReplaceAll(xmlLine, `\"`, """) + var builder strings.Builder + builder.Grow(len(xmlLine)) + insideValue := false + for index := 0; index < len(xmlLine); index++ { + if isEscapedQuote(xmlLine, index) { + builder.WriteString(""") + index++ + + continue + } + char := xmlLine[index] + if char == '"' { + insideValue = !insideValue + builder.WriteByte(char) + + continue + } + if escaped, consumed := escapeValueByte(xmlLine[index:], insideValue); consumed > 0 { + builder.WriteString(escaped) + index += consumed - 1 + + continue + } + builder.WriteByte(char) + } + + return builder.String() +} + +// isEscapedQuote reports whether a backslash-escaped quote (`\"`) starts at index. +func isEscapedQuote(text string, index int) bool { + return text[index] == '\\' && index+1 < len(text) && text[index+1] == '"' +} + +// escapeValueByte returns the XML escaping for the metacharacter that starts +// value together with the number of source bytes it consumes. +// +// It escapes only when insideValue is set. A pre-existing entity such as +// `"` is preserved rather than re-escaped; a raw ampersand becomes +// `&`. A consumed count of zero means the leading byte needs no escaping. +func escapeValueByte(value string, insideValue bool) (string, int) { + if !insideValue { + return "", 0 + } + switch value[0] { + case '<': + return "<", 1 + case '>': + return ">", 1 + case '&': + if entity := xmlEntityPrefix(value); entity != "" { + return entity, len(entity) + } + + return "&", 1 + default: + return "", 0 + } +} + +// xmlEntityPrefix returns the leading XML character entity reference in text, +// or an empty string when text does not begin with one. +// +// It recognizes the predefined entities (`&`, `<`, `>`, `"`, +// `'`) and numeric character references such as `'` or ``, so a +// pattern author may include a pre-escaped entity without it being re-escaped. +func xmlEntityPrefix(text string) string { + semicolon := strings.IndexByte(text, ';') + if semicolon <= 0 { + return "" + } + name := text[1:semicolon] + if name == "" { + return "" + } + switch name { + case "amp", "lt", "gt", "quot", "apos": + return text[:semicolon+1] + } + if isNumericCharRef(name) { + return text[:semicolon+1] + } + + return "" +} + +// isNumericCharRef reports whether name is the body of an XML numeric character +// reference, such as `#39` (decimal) or `#x1F` (hexadecimal). +func isNumericCharRef(name string) bool { + if len(name) < 2 || name[0] != '#' { + return false + } + digits := name[1:] + hexadecimal := digits[0] == 'x' || digits[0] == 'X' + if hexadecimal { + digits = digits[1:] + } + if digits == "" { + return false + } + for i := 0; i < len(digits); i++ { + if !isReferenceDigit(digits[i], hexadecimal) { + return false + } + } + + return true +} + +// isReferenceDigit reports whether char is a valid digit for a numeric +// character reference, allowing hexadecimal digits when hexadecimal is set. +func isReferenceDigit(char byte, hexadecimal bool) bool { + if char >= '0' && char <= '9' { + return true + } + if !hexadecimal { + return false + } + + return (char >= 'a' && char <= 'f') || (char >= 'A' && char <= 'F') } diff --git a/logging/logger.go b/logging/logger.go index dfb6929..4965638 100644 --- a/logging/logger.go +++ b/logging/logger.go @@ -159,6 +159,31 @@ func FileReferenceWithLine(path string, line int) string { return reference + ":" + strconv.Itoa(line) } +// FileReferenceWithPosition returns a clickable file URL with line and column +// suffixes. +// +// The `file://...:line:column` form is the one IDEs such as IntelliJ IDEA +// recognize for navigation; a line-only suffix is not enough. When no column +// is known, pass 1 to point at the beginning of the line. +// +// Parameters: +// path - provides a local file path. +// line - provides an optional one-based line number. +// column - provides an optional one-based column number. +// +// Returns file reference with line and column suffixes when both are positive. +func FileReferenceWithPosition(path string, line int, column int) string { + if column <= 0 { + return FileReferenceWithLine(path, line) + } + reference := FileReferenceWithLine(path, line) + if line <= 0 { + return reference + } + + return reference + ":" + strconv.Itoa(column) +} + // fileURLFromAbsolutePath formats an absolute local path as an OS-neutral file URL. func fileURLFromAbsolutePath(path string) string { normalizedPath := filepath.ToSlash(strings.ReplaceAll(path, "\\", "/"))