Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions embedding/embedding_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not mention resolved (after we merge this PR) issues in the doc.

Same in all other places.

//
// 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.
Comment on lines +148 to +150

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backticks don’t prevent the link from being clickable in IntelliJ IDEA. Let’s keep them, as they are necessary to visually separate the link from the default text.

Example:

Image

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"}

Expand Down Expand Up @@ -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 `<embed-code>` tag is not closed",
))
Expand Down Expand Up @@ -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 <unexpected> closed by </embed-code>",
))
Expand All @@ -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",
))
})
Expand All @@ -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",
))
})
Expand Down
13 changes: 11 additions & 2 deletions embedding/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +47 to +50

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we need to show column number, as it's always hardcoded to 1.

I'm sure that link would be clickable even without column number.


// 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,
)
}
Expand Down
43 changes: 40 additions & 3 deletions embedding/parsing/instruction_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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="<br/>"` does not end the instruction early.
func instructionClosed(instructionBody []string) bool {
instruction := strings.Join(instructionBody, " ")
closingTag := "</" + EmbeddingTag + ">"
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, "</"+EmbeddingTag+">") {
if !instructionClosed(instructionBody) {
return fmt.Sprintf("the `<%s>` tag is not closed",
EmbeddingTag,
)
Expand Down
120 changes: 120 additions & 0 deletions embedding/parsing/state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<embed-code .../>` 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(`<embed-code file="Example.java" start="["/>`)
})

It("should not consume the rest of the document when the comments mode is invalid", func() {
assertBoundedMalformedInstruction(`<embed-code file="Example.java" comments="bogus"/>`)
})

It("should not consume the rest of the document when attributes are mutually exclusive", func() {
assertBoundedMalformedInstruction(`<embed-code file="Example.java" fragment="f" line="l"/>`)
})

// 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(
"<embed-code file=\"Example.java\" line=\"if (a < b)\"/>",
"```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{
"<embed-code file=\"Example.java\" line=\"if (a < b)\"/>",
}))
})

// 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(
"<embed-code",
` line="<br/>"`,
` file="Example.java"/>`,
"```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{
"<embed-code",
` line="<br/>"`,
` file="Example.java"/>`,
}))
})

It("should render source and close the embedding fence when the end state is accepted", func() {
sourceRoot := GinkgoT().TempDir()
Expect(os.WriteFile(
Expand Down Expand Up @@ -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")
Expand Down
Loading
Loading