Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the option to throw when variable not found in context #289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ _None_
, i.e. `myfilter = "uppercase"` and then use it to invoke this filter with `{{ string|filter:myfilter }}`.
[Ilya Puchka](https://github.com/ilyapuchka)
[#203](https://github.com/stencilproject/Stencil/pull/203)
- Added option to throw when a variable used in a template could not be resolved.
[Andres Cecilia Luque](https://github.com/acecilia)
[#289](https://github.com/stencilproject/Stencil/pull/289)

### Deprecations

Expand Down
3 changes: 3 additions & 0 deletions Sources/Environment.swift
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
public struct Environment {
public let templateClass: Template.Type
public var extensions: [Extension]
public let throwOnUnresolvedVariable: Bool

public var loader: Loader?

public init(loader: Loader? = nil,
extensions: [Extension] = [],
throwOnUnresolvedVariable: Bool = false,
templateClass: Template.Type = Template.self) {

self.templateClass = templateClass
self.loader = loader
self.extensions = extensions + [DefaultExtension()]
self.throwOnUnresolvedVariable = throwOnUnresolvedVariable
}

public func loadTemplate(name: String) throws -> Template {
Expand Down
1 change: 1 addition & 0 deletions Sources/Errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public protocol ErrorReporter: AnyObject {
}

open class SimpleErrorReporter: ErrorReporter {
public init() { }

open func renderError(_ error: Error) -> String {
guard let templateError = error as? TemplateSyntaxError else { return error.localizedDescription }
Expand Down
8 changes: 7 additions & 1 deletion Sources/Variable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,16 @@ class FilterExpression: Resolvable {
func resolve(_ context: Context) throws -> Any? {
let result = try variable.resolve(context)

return try filters.reduce(result) { value, filter in
let filteredResult = try filters.reduce(result) { value, filter in
let arguments = try filter.1.map { try $0.resolve(context) }
return try filter.0.invoke(value: value, arguments: arguments, context: context)
}

if filteredResult == nil, context.environment.throwOnUnresolvedVariable {
Copy link
Collaborator

@ilyapuchka ilyapuchka Jan 12, 2020

Choose a reason for hiding this comment

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

I don't feel like this is a correct place for this check. nil can be a result of filter operation itself. Also it does not cover usage of undefined variables outside of filter.
What we need to do is to check for undefined variable in Variable.resolve method when we do if current == nil { return nil } and to throw instead of returning nil.

Copy link
Author

@acecilia acecilia Jan 12, 2020

Choose a reason for hiding this comment

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

Thanks for the feedback! :) I will break it down into some points:
a) "nil can be a result of filter operation itself": yes, that is correct. It is intentional, this is the reason why I implemented this feature: the main idea behind it is that getting a nil in a template indicates a bug (either because the variable is not defined or because of a filter failing to find a matching result), and instead of silently not printing anything, I would prefer to throw.
b) "Also it does not cover usage of undefined variables outside of filter": I do not fully understand what you mean, seems to be working correctly as per the test testUnresolvedVariable I added. I am not very familiar with the Stencil codebase, can you give me an example of template where this will not work?
c) "What we need to do is to check for undefined variable in Variable.resolve method": I also tried that, but then the default filter will be useless, as we will be throwing even when a default value is provided.

I added some more tests to show how I expect this feature to work.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe instead of throwOnUnresolvedVariable I should call the option throwOnUnresolvedExpression?

Copy link
Author

@acecilia acecilia Mar 22, 2020

Choose a reason for hiding this comment

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

@ilyapuchka could you have a look a my latest comments and give me some feedback?

throw TemplateSyntaxError("Variable could not be resolved")
}

return filteredResult
}
}

Expand Down
52 changes: 51 additions & 1 deletion Tests/StencilTests/EnvironmentSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,57 @@ final class EnvironmentTests: XCTestCase {
}
}

func testUnresolvedVariable() {
self.template = Template(templateString: "Hello {{ human.name }}")

it("can render a variable not defined in the context and throwing is disabled") {
self.environment = Environment()
let result = try self.environment.render(template: self.template, context: [:])
try expect(result) == "Hello "
}

it("throws when a variable is not defined in the context and throwing is enabled") {
self.environment = Environment(throwOnUnresolvedVariable: true)
try self.expectError(context: [:], reason: "Variable could not be resolved", token: "human.name")
}
}

func testUnresolvedVariableWithDefaultFilter() {
self.template = Template(templateString: """
Hello {{ human.name|default:"World" }}
""")
self.environment = Environment(throwOnUnresolvedVariable: true)

it("does not throw when a filter provides a default value, even though the variable is undefined") {
let result = try self.environment.render(template: self.template, context: [:])
try expect(result) == "Hello World"
}
}

func testUnresolvedVariableWithTags() {
self.template = Template(templateString: """
{% if human.name %}Hello {{ human.name }}{% else %}No hello for you{% endif %}
""")
self.environment = Environment(throwOnUnresolvedVariable: true)

it("does not throw when using tags with defined variables") {
let result = try self.environment.render(template: self.template, context: ["human": ["name": "John"]])
try expect(result) == "Hello John"
}

it("throws when using tags with undefined variables") {
try self.expectError(context: [:], reason: "Variable could not be resolved", token: "if human.name")
}

it("does not throw when using empty variables instead of undefined ones") {
let result = try self.environment.render(template: self.template, context: ["human": ["name": ""]])
try expect(result) == "No hello for you"
}
}


private func expectError(
context: [String: Any] = ["names": ["Bob", "Alice"], "name": "Bob"],
reason: String,
token: String,
file: String = #file,
Expand All @@ -192,7 +242,7 @@ final class EnvironmentTests: XCTestCase {
let expectedError = expectedSyntaxError(token: token, template: template, description: reason)

let error = try expect(
self.environment.render(template: self.template, context: ["names": ["Bob", "Alice"], "name": "Bob"]),
self.environment.render(template: self.template, context: context),
file: file,
line: line,
function: function
Expand Down