tyler-sommer/stick

Better error handling

tyler-sommer opened this issue · 1 comments

tyler-sommer commented

There are several areas that could use some tweaking with regard to error handling in Stick. This issue will be used to track and explain the reasoning behind these changes.

Each section below describes a different category of improvement. Tasks to carry out these improvements are defined together at the end, grouped by category.

Missing default statements

Incomplete switch statements with no default handling may silently fail which could lead to frustration if a user encounters this behavior.

Places affected

  • Fixed in v1.0.6: func (s *state) evalExpr(node parse.Node) in exec.go in two places
    • when handling an unknown binary operator
    • when handling an unknown Expr
  • func (s *state) walkChild(node parse.Node) in exec.go

These cases are unlikely to encounter these silent failures in normal operation ("should never happen" barring the existence of a bug), but anyone hacking on stick itself may experience frustration when something isn't working and there is no hint as to what's going on.

Fixing these is technically a BC break as any templates relying on these silent failures will now fail to execute, returning an error instead. However, I don't think this is a real concern in practice and fixing these does not warrant a major version bump.

Impossible to return an error

User-defined additions to Stick do not currently support returning any error. This results in needing to return some "empty" value rather than trying to signal an error.

Places affected

  • user defined Funcs
  • user defined Tests
  • user defined Filters
    • twig_compat filters are littered with places that should have error handling
  • NodeVisitor Enter
  • NodeVisitor Leave

I'm a little bit on the fence on whether some of these should be fixed. I see two main considerations:

  • overly sensitive error handling (in cases where returning some "empty" value would be suitable instead)
  • and difficult-to-debug situations (where user-defined functionality fails silently and inexplicably).

Behavior in Twig itself is fairly loose and template execution generally tends to be forgiving of type mismatches and failed coercions. In Twig, user-defined functionality would need to throw an exception which would interrupt template execution. This can also be done in Go, if desired, with panic(). However, in many places it makes sense to just return some sensible "empty" value rather than halting template execution. Using panic() is also generally kind of smelly, though, so I hesitate to make that the go-to solution.

On the flip side, the current situation can lead to difficult to debug situations where user-defined functionality is not working as expected and there is no sign of what's going on because any error is being silently thrown away. Of course, logging could be used to provide some clue to the user.

One final factor to consider here is modifying the affected method signatures to return an error (arguably the best choice) would be a fairly major BC break, requiring all existing user-defined code to be updated. This would probably warrant a major version bump.

Tentative plan

  • For v1.0.x, in order to avoid breaking BC, prefer using panic() in user-defined code and convert it to an error using recover() during parsing (for NodeVisitors) and template execution.
    • "Silent" handling in many cases will still be preferred. As a workaround for silently ignoring errors, this could at least be logged so that feedback is available somewhere.
    • To enable logging from built-in user-defined functionality (ie. twig_compat), will probably add the ability to configure a Logger and somehow make that usable from existing filters and such. With that done, all silent error handling can at least be updated to log a message.
  • In some future (currently unplanned) v2.0.x, this would be fixed by modifying the affected types so they can return an error.
    • "Silent" handling may still be preferred in some places. In this case, the same log message workaround can be applied.

Inconsistent error messages

Error messages throughout Stick are inconsistent and could use some attention.

These are relatively minor, though any code expecting specific error messages may stop working when the messages change. I think this is unlikely and don't consider this reason enough to bump the major version, however.

Places affected

  • Prefixed log messages
    • All error messages in Stick should be prefixed with the method name where the error originated as well as (or at least) the package name.
    • Prefixes are great, but proper context added through specialized error types (and wrapping errors with those types) may be more suitable.
  • exec.go uses a mix of fmt.Sprintf and string concatenation
    • Stick should always use fmt.Sprintf or fmt.Errorf
  • many places are capitalized or awkwardly worded
    • Idiomatic Go suggests avoiding complete sentencies in error messages
    • Prefer "unsupported expression type" over "Unable to evaluate unsupported Expr type"
  • package parse has dedicated error types
    • This is the direction error handling should go for other places too.

Other improvements

  • Template execution errors currently provide no context about where in the template (or even which template) they occur.
    • These errors should be wrapped with an enriched type before being returned to the user, including filename, line, and offset.
  • Fixed in v1.0.6: Writing tests for errors during template execution is not possible
    • Modifying a parsed template Tree during test execution is necessary to trigger all possible error states
  • Tests should cover most (all) error cases

Tasks

Missing default case in switch statements

  • Add default case to walkExpr for unsupported expressions and binary operators (v1.0.6)
  • Add default case to walkChild in exec.go

User-defined functionality

  • Add recover() handlers to convert panics to errors during parsing and template execution.
  • Add ability to set a Logger on Env and pass that into state.
  • Update all existing built-in user-defined additions to panic on errors that warrant it, and log all others.
  • Update docs to explain panic() mechanism in user-defined functionality
  • Update docs with example on adding logging to a NodeVisitor

Consistent error messages

  • Update all string concatenation to prefer fmt.Sprintf or fmt.Errorf
  • Update all errors created by Stick itself during parsing and execution to follow idiomatic conventions, lowercase, no ending punctuation, and be prefixed with the method name.
  • Wrap all errors created by external code, adding the necessary context and the Stick method name where the error was first received.

Other improvements

  • Enable exec tests to make assertions about errors (v1.0.6)
  • Add ability for exec tests to manipulate parsed template Tree (v1.0.6)
  • Enrich template execution errors with template name, line, and offset of where the error occurred.
  • Add more tests that cover error states in template execution
tyler-sommer commented

Updated the original comment with comprehensive plans for improving error handling :D