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

Fix invalid statement template compile error #42244

Merged
merged 1 commit into from Jun 10, 2021

Conversation

@hahmed
Copy link
Contributor

@hahmed hahmed commented May 17, 2021

Summary

In the view actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.text.erb there is a missing end, causing a compile error with the template.

@rails-bot rails-bot bot added the actionpack label May 17, 2021
@zzak
Copy link
Member

@zzak zzak commented May 18, 2021

@hahmed Weird that this wasn't picked up before, how did you find it? 🤔

@kamipo
Copy link
Member

@kamipo kamipo commented May 18, 2021

@hahmed
Copy link
Contributor Author

@hahmed hahmed commented May 18, 2021

That's where I found it from, thanks @kamipo, was trying to find a way to write a test, before I moved it to ready for review.

Currently there are no tests around the invalid_statement view and a few others. In this case, that could be because where the class originates from i.e. ActiveRecord::StatementInvalid which made it tricky to test here https://github.com/rails/rails/blob/main/actionpack/test/dispatch/show_exceptions_test.rb

For example I tried to test but could not throw a StatementInvalid error, action pack does not have ActiveRecord dependency, test example:

test "statement invalid error page is returned" do
    @app = ProductionApp
    get "/statement_invalid.json", env: { "action_dispatch.show_exceptions" => true }
    assert_response 500
    assert_match "ActiveRecord::StatementInvalid", body
  end

If you do have ideas on how to test, let me know 👍 (will make the PR ready to review)

@hahmed hahmed marked this pull request as ready for review May 18, 2021
zzak
zzak approved these changes May 18, 2021
@kamipo
Copy link
Member

@kamipo kamipo commented May 19, 2021

One possible way is to test it in MiddlewareExceptionsTest like here:

test "renders active record exceptions as 404" do
controller :foo, <<-RUBY
class FooController < ActionController::Base
def index
raise ActiveRecord::RecordNotFound
end
end
RUBY
get "/foo"
assert_equal 404, last_response.status
end

@zzak zzak self-assigned this May 19, 2021
@rails-bot rails-bot bot added the railties label May 21, 2021
@zzak
Copy link
Member

@zzak zzak commented May 22, 2021

@hahmed Since you're adding a regression test, could you show us the result before the patch?

@hahmed
Copy link
Contributor Author

@hahmed hahmed commented May 22, 2021

I added a regression to catch any issues with the .html.erb template, so for example if we remove an <% end %> from the file rails/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb -- which results in a compile error. Here is the error:

Failure:
ApplicationTests::MiddlewareExceptionsTest#test_displays_statement_invalid_template_correctly [test/application/middleware/exceptions_test.rb:188]:
Expected /<title>Action\ Controller:\ Exception\ caught<\/title>/ to match # encoding: ASCII-8BIT
#    valid: true
"<!DOCTYPE html>\n<html>\n<head>\n  <title>We're sorry, but something went wrong (500)</title>\n  <meta name=\"viewport\" content=\"width=device-width,initial-scale=1\">\n  <style>\n  .rails-default-error-page {\n    background-color: #EFEFEF;\n    color: #2E2F30;\n    text-align: center;\n    font-family: arial, sans-serif;\n    margin: 0;\n  }\n\n  .rails-default-error-page div.dialog {\n    width: 95%;\n    max-width: 33em;\n    margin: 4em auto 0;\n  }\n\n  .rails-default-error-page div.dialog > div {\n    border: 1px solid #CCC;\n    border-right-color: #999;\n    border-left-color: #999;\n    border-bottom-color: #BBB;\n    border-top: #B00100 solid 4px;\n    border-top-left-radius: 9px;\n    border-top-right-radius: 9px;\n    background-color: white;\n    padding: 7px 12% 0;\n    box-shadow: 0 3px 8px rgba(50, 50, 50, 0.17);\n  }\n\n  .rails-default-error-page h1 {\n    font-size: 100%;\n    color: #730E15;\n    line-height: 1.5em;\n  }\n\n  .rails-default-error-page div.dialog > p {\n    margin: 0 0 1em;\n    padding: 1em;\n    background-color: #F7F7F7;\n    border: 1px solid #CCC;\n    border-right-color: #999;\n    border-left-color: #999;\n    border-bottom-color: #999;\n    border-bottom-left-radius: 4px;\n    border-bottom-right-radius: 4px;\n    border-top-color: #DADADA;\n    color: #666;\n    box-shadow: 0 3px 8px rgba(50, 50, 50, 0.17);\n  }\n  </style>\n</head>\n\n<body class=\"rails-default-error-page\">\n  <!-- This file lives in public/500.html -->\n  <div class=\"dialog\">\n    <div>\n      <h1>We're sorry, but something went wrong.</h1>\n    </div>\n    <p>If you are the application owner check the logs for more information.</p>\n  </div>\n</body>\n</html>\n".

To repo

  1. Remove <% end %> from rails/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
  2. cd in railties
  3. bundle exec ruby -w -Itest test/application/middleware/exceptions_test.rb

I could not see an existing test for this, but I'm going to take a closer look. I'm also going to add a test to catch regressions in the rails/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.text.erb -- original issue.

(moving back to draft and so I can work on adding that other test)

@hahmed hahmed marked this pull request as draft May 22, 2021
@hahmed hahmed force-pushed the fix-invalid-statement-compile-error branch 2 times, most recently from da8e49f to 0fc1396 Jun 9, 2021
app.config.consider_all_requests_local = true
app.config.action_dispatch.ignore_accept_header = false

get "/foo", {}, { "HTTP_ACCEPT" => "text/plain", "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest" }
Copy link
Contributor Author

@hahmed hahmed Jun 9, 2021

Weird, but I was expecting this to work:

get "/foo", headers: { "HTTP_ACCEPT" => "text/plain", "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest" }

@hahmed hahmed force-pushed the fix-invalid-statement-compile-error branch from 0fc1396 to 1be3277 Jun 9, 2021
@hahmed hahmed marked this pull request as ready for review Jun 9, 2021
@rafaelfranca rafaelfranca merged commit b0d8d82 into rails:main Jun 10, 2021
4 checks passed
rafaelfranca added a commit that referenced this issue Jun 10, 2021
…rror

Fix invalid statement template compile error
rafaelfranca added a commit that referenced this issue Jun 10, 2021
…rror

Fix invalid statement template compile error
@hahmed hahmed deleted the fix-invalid-statement-compile-error branch Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants