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

Exception in ERB template does not render exception view properly #48173

Closed
jasonkim opened this issue May 9, 2023 · 2 comments · Fixed by #48184
Closed

Exception in ERB template does not render exception view properly #48173

jasonkim opened this issue May 9, 2023 · 2 comments · Fixed by #48184

Comments

@jasonkim
Copy link

jasonkim commented May 9, 2023

We've noticed that any error within the ERB template does not handle the error the same way as the error coming from controller. After some debugging, I noticed that there's an error thrown within create_template call. From there, I found this PR that touched the relevant section. And validated the behavior with modified test below.

Steps to reproduce

I've changed the test scenario here to wrap the error case with noop if-statement, which should not change the behavior.
It changes the file actionview/test/fixtures/test/runtime_error.html.erb to

<h1>Oh no!</h1>
This template has a runtime error
<% if true %>
  <%= method_that_does_not_exist %>
<% end %>
Yikes!

This is used by the test actionview/test/template/render_test.rb

Your reproduction script goes here

After making the above change (or checkout this fork), run

cd actionview
bin/test test/template/render_test.rb

Expected behavior

The test should pass

Actual behavior

Test fails with error

Error:
LazyViewRenderTest#test_render_runtime_error:
RuntimeError: 
    /workspaces/rails/actionview/lib/action_view/template/handlers/erb.rb:113:in `find_offset'
    /workspaces/rails/actionview/lib/action_view/template/handlers/erb.rb:44:in `translate_location'
    /workspaces/rails/actionview/lib/action_view/template.rb:182:in `translate_location'
    /workspaces/rails/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb:238:in `spot'
    /workspaces/rails/actionview/test/template/render_test.rb:220:in `test_render_runtime_error'


bin/test test/template/render_test.rb:212

System configuration

Rails version:
main

Ruby version:
3.2

@casperisfine
Copy link
Contributor

cc @tenderlove.

I'm not very familiar with this code, but the parser very clearly give up by raising RuntimeError when it fails to parse the template somehow.

Such raw raise suggest that it was expected this case wouldn't be hit, but clearly it is.

I suspect there are two fixes needed here:

  • This code is clearly valid, so we should be able to parse it fine, something in the parser need to be improved.
  • Even if we fixed that particular error, we should raise a proper error when we fail to parse, so that we can fallback to not mapping the source code.

I'll have a quick look at the second part of the fix.

casperisfine pushed a commit to Shopify/rails that referenced this issue May 10, 2023
Fix: rails#48173

If for some reason the column identification fails, we should
fallback to the old error rendering with only line information.
@obie
Copy link

obie commented May 11, 2023

Brief comment to say that I'm seeing this constantly on my 7.1 apps and it's a huge PITA.

tenderlove added a commit to tenderlove/rails that referenced this issue May 24, 2023
Text tokens may be conditionally appended to the output buffer.  For
example if we have a template like this:

```erb
<h1>Oh no!</h1>
This template has a runtime error
<b>
  <% if true %>
    <%= method_that_does_not_exist %>
  <% end %>
</b>
Yikes!
```

In the above case, a string literal (`"    "`) will be conditionally
appended to the output buffer with some code like below:

```ruby
@output_buffer.safe_append='    '.freeze
```

This commit teaches text tokens (string literals) to scan forward in the
compiled template until it finds the literal, thereby skipping the code
generated for appending to the output buffer.

Related to:

* Bug: rails#48173
* PR: rails#48184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants