From c23c1a96af56e04120db062c5098157dcc0196b3 Mon Sep 17 00:00:00 2001 From: Benjamin Nassler Date: Mon, 4 Mar 2024 21:52:38 +0100 Subject: [PATCH 1/4] fixed missing parameter causing crash --- Sources/LeafKit/LeafError.swift | 4 ++++ Sources/LeafKit/LeafParser/LeafParser.swift | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/Sources/LeafKit/LeafError.swift b/Sources/LeafKit/LeafError.swift index a03f0f81..c9d17a69 100644 --- a/Sources/LeafKit/LeafError.swift +++ b/Sources/LeafKit/LeafError.swift @@ -31,6 +31,8 @@ public struct LeafError: Error { /// Attempt to render an AST with cyclical external references /// - Provide template name & ordered array of template names that causes the cycle path case cyclicalReference(String, [String]) + /// Parameter missing + case missingParameter(String) // MARK: Wrapped Errors related to Lexing or Parsing /// Errors due to malformed template syntax or grammar @@ -84,6 +86,8 @@ public struct LeafError: Error { return "\(src) - \(key) cyclically referenced in [\(chain.joined(separator: " -> "))]" case .lexerError(let e): return "Lexing error - \(e.localizedDescription)" + case .missingParameter(let message): + return message } } diff --git a/Sources/LeafKit/LeafParser/LeafParser.swift b/Sources/LeafKit/LeafParser/LeafParser.swift index 45a906e9..9a0bd994 100644 --- a/Sources/LeafKit/LeafParser/LeafParser.swift +++ b/Sources/LeafKit/LeafParser/LeafParser.swift @@ -200,7 +200,13 @@ internal struct LeafParser { let params = try readParameters() // parameter tags not permitted to have bodies if params.count > 1 { group.append(.expression(params)) } - else { group.append(params.first!) } + ///// else { group.append(params.first!) } + else { + guard let firstParam = params.first else { + throw LeafError(.missingParameter("Expected a parameter but found none")) + } + group.append(firstParam) + } case .parameter(let p): pop() switch p { From 63b3bef2c23d3e12357f0b863cc8d453806bbf98 Mon Sep 17 00:00:00 2001 From: Benjamin Nassler Date: Tue, 5 Mar 2024 21:55:57 +0100 Subject: [PATCH 2/4] incorporated feedback --- Sources/LeafKit/LeafError.swift | 6 ++--- Sources/LeafKit/LeafParser/LeafParser.swift | 28 +++++++++++++-------- Tests/LeafKitTests/LeafErrorTests.swift | 22 ++++++++++++++++ 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/Sources/LeafKit/LeafError.swift b/Sources/LeafKit/LeafError.swift index c9d17a69..0914e48f 100644 --- a/Sources/LeafKit/LeafError.swift +++ b/Sources/LeafKit/LeafError.swift @@ -32,7 +32,7 @@ public struct LeafError: Error { /// - Provide template name & ordered array of template names that causes the cycle path case cyclicalReference(String, [String]) /// Parameter missing - case missingParameter(String) + case missingParameter // MARK: Wrapped Errors related to Lexing or Parsing /// Errors due to malformed template syntax or grammar @@ -86,8 +86,8 @@ public struct LeafError: Error { return "\(src) - \(key) cyclically referenced in [\(chain.joined(separator: " -> "))]" case .lexerError(let e): return "Lexing error - \(e.localizedDescription)" - case .missingParameter(let message): - return message + case .missingParameter: + return "Expected a parameter but found none" } } diff --git a/Sources/LeafKit/LeafParser/LeafParser.swift b/Sources/LeafKit/LeafParser/LeafParser.swift index 9a0bd994..99561959 100644 --- a/Sources/LeafKit/LeafParser/LeafParser.swift +++ b/Sources/LeafKit/LeafParser/LeafParser.swift @@ -183,13 +183,20 @@ internal struct LeafParser { var group = [ParameterDeclaration]() var paramsList = [ParameterDeclaration]() - - func dump() { - defer { group = [] } - if group.isEmpty { return } - group.evaluate() - if group.count > 1 { paramsList.append(.expression(group)) } - else { paramsList.append(group.first!) } + + func dump() throws { + defer { group = [] } + if group.isEmpty { return } + group.evaluate() + if group.count > 1 { paramsList.append(.expression(group)) } + else { + guard let first = group.first else { + // It's better to handle this case as well, even though logically it might never happen + // since you're checking if group.isEmpty before. + throw LeafError(.missingParameter, file: #file, function: #function, line: #line, column: #column) + } + paramsList.append(first) + } } outer: while let next = peek() { @@ -200,10 +207,9 @@ internal struct LeafParser { let params = try readParameters() // parameter tags not permitted to have bodies if params.count > 1 { group.append(.expression(params)) } - ///// else { group.append(params.first!) } else { guard let firstParam = params.first else { - throw LeafError(.missingParameter("Expected a parameter but found none")) + throw LeafError(.missingParameter) } group.append(firstParam) } @@ -220,11 +226,11 @@ internal struct LeafParser { } case .parametersEnd: pop() - dump() + try dump() break outer case .parameterDelimiter: pop() - dump() + try dump() case .whitespace: pop() continue diff --git a/Tests/LeafKitTests/LeafErrorTests.swift b/Tests/LeafKitTests/LeafErrorTests.swift index 257c31d3..caac992d 100644 --- a/Tests/LeafKitTests/LeafErrorTests.swift +++ b/Tests/LeafKitTests/LeafErrorTests.swift @@ -45,4 +45,26 @@ final class LeafErrorTests: XCTestCase { XCTFail("Wrong error: \(error.localizedDescription)") } } + + /// Verify that rendering a template with a missing required parameter will throw `LeafError.missingParameter` + func testMissingParameterError() { + var test = TestFiles() + // Assuming "/missingParam.leaf" is a template that requires a parameter we intentionally don't provide + test.files["/missingParam.leaf"] = """ + #(foo.bar.trim()) + """ + do { + _ = try TestRenderer(sources: .singleSource(test)).render(path: "missingParam", context: [:]).wait() + XCTFail("Should have thrown LeafError.missingParameter") + } catch let error as LeafError { + switch error.reason { + case .missingParameter: + return + default: + XCTFail("Wrong error: \(error.localizedDescription)") + } + } catch { + XCTFail("Wrong error: \(error.localizedDescription)") + } + } } From cf6b9c0aa88040d87d8009df1a67ca0fe03be91c Mon Sep 17 00:00:00 2001 From: b-nassler <162194776+b-nassler@users.noreply.github.com> Date: Fri, 8 Mar 2024 14:21:58 +0100 Subject: [PATCH 3/4] Update Tests/LeafKitTests/LeafErrorTests.swift Co-authored-by: Gwynne Raskind --- Tests/LeafKitTests/LeafErrorTests.swift | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/Tests/LeafKitTests/LeafErrorTests.swift b/Tests/LeafKitTests/LeafErrorTests.swift index caac992d..285e1db8 100644 --- a/Tests/LeafKitTests/LeafErrorTests.swift +++ b/Tests/LeafKitTests/LeafErrorTests.swift @@ -53,18 +53,12 @@ final class LeafErrorTests: XCTestCase { test.files["/missingParam.leaf"] = """ #(foo.bar.trim()) """ - do { - _ = try TestRenderer(sources: .singleSource(test)).render(path: "missingParam", context: [:]).wait() - XCTFail("Should have thrown LeafError.missingParameter") - } catch let error as LeafError { - switch error.reason { - case .missingParameter: - return - default: - XCTFail("Wrong error: \(error.localizedDescription)") + XCTAssertThrowsError(try TestRenderer(sources: .singleSource(test)) + .render(path: "missingParam", context: [:]) + .wait() + ) { + guard case .missingParameter = ($0 as? LeafError)?.reason else { + return XCTFail("Expected LeafError.missingParameter, got \(String(reflecting: $0))") + } } - } catch { - XCTFail("Wrong error: \(error.localizedDescription)") - } - } } From 515e8bb0dcb0021cee8ee2f189bbcceda0aa364c Mon Sep 17 00:00:00 2001 From: Benjamin Nassler Date: Thu, 14 Mar 2024 08:05:06 +0100 Subject: [PATCH 4/4] updated to throw unknownError in favor of new err --- Sources/LeafKit/LeafError.swift | 4 ---- Sources/LeafKit/LeafParser/LeafParser.swift | 4 ++-- Tests/LeafKitTests/LeafErrorTests.swift | 5 +++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Sources/LeafKit/LeafError.swift b/Sources/LeafKit/LeafError.swift index 0914e48f..a03f0f81 100644 --- a/Sources/LeafKit/LeafError.swift +++ b/Sources/LeafKit/LeafError.swift @@ -31,8 +31,6 @@ public struct LeafError: Error { /// Attempt to render an AST with cyclical external references /// - Provide template name & ordered array of template names that causes the cycle path case cyclicalReference(String, [String]) - /// Parameter missing - case missingParameter // MARK: Wrapped Errors related to Lexing or Parsing /// Errors due to malformed template syntax or grammar @@ -86,8 +84,6 @@ public struct LeafError: Error { return "\(src) - \(key) cyclically referenced in [\(chain.joined(separator: " -> "))]" case .lexerError(let e): return "Lexing error - \(e.localizedDescription)" - case .missingParameter: - return "Expected a parameter but found none" } } diff --git a/Sources/LeafKit/LeafParser/LeafParser.swift b/Sources/LeafKit/LeafParser/LeafParser.swift index 99561959..722559f3 100644 --- a/Sources/LeafKit/LeafParser/LeafParser.swift +++ b/Sources/LeafKit/LeafParser/LeafParser.swift @@ -193,7 +193,7 @@ internal struct LeafParser { guard let first = group.first else { // It's better to handle this case as well, even though logically it might never happen // since you're checking if group.isEmpty before. - throw LeafError(.missingParameter, file: #file, function: #function, line: #line, column: #column) + throw LeafError(.unknownError("Found nil while iterating through params"), file: #file, function: #function, line: #line, column: #column) } paramsList.append(first) } @@ -209,7 +209,7 @@ internal struct LeafParser { if params.count > 1 { group.append(.expression(params)) } else { guard let firstParam = params.first else { - throw LeafError(.missingParameter) + throw LeafError(.unknownError("Found nil while iterating through params")) } group.append(firstParam) } diff --git a/Tests/LeafKitTests/LeafErrorTests.swift b/Tests/LeafKitTests/LeafErrorTests.swift index 285e1db8..4da3a302 100644 --- a/Tests/LeafKitTests/LeafErrorTests.swift +++ b/Tests/LeafKitTests/LeafErrorTests.swift @@ -57,8 +57,9 @@ final class LeafErrorTests: XCTestCase { .render(path: "missingParam", context: [:]) .wait() ) { - guard case .missingParameter = ($0 as? LeafError)?.reason else { - return XCTFail("Expected LeafError.missingParameter, got \(String(reflecting: $0))") + guard case .unknownError("Found nil while iterating through params") = ($0 as? LeafError)?.reason else { + return XCTFail("Expected LeafError.unknownError(\"Found nil while iterating through params\"), got \(String(reflecting: $0))") } } + } }