Fix invalid statement template compile error #42244

hahmed · 2021-05-17T23:01:30Z

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.

zzak · 2021-05-18T00:10:52Z

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

kamipo · 2021-05-18T04:28:13Z

@zzak c18166a#r50354351

hahmed · 2021-05-18T22:32:01Z

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)

kamipo · 2021-05-19T04:00:26Z

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

rails/railties/test/application/middleware/exceptions_test.rb

Lines 36 to 47 in f95c0b7

    
               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 · 2021-05-22T00:37:16Z

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

hahmed · 2021-05-22T19:19:53Z

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

Remove <% end %> from rails/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb
cd in railties
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 · 2021-06-09T21:23:15Z

railties/test/application/middleware/exceptions_test.rb

+      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" }


Weird, but I was expecting this to work:

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

…rror Fix invalid statement template compile error

rails-bot bot added the actionpack label May 17, 2021

hahmed marked this pull request as ready for review May 18, 2021 22:32

zzak approved these changes May 18, 2021

View reviewed changes

zzak self-assigned this May 19, 2021

rails-bot bot added the railties label May 21, 2021

hahmed marked this pull request as draft May 22, 2021 19:20

hahmed force-pushed the fix-invalid-statement-compile-error branch 2 times, most recently from da8e49f to 0fc1396 Compare June 9, 2021 21:22

hahmed commented Jun 9, 2021

View reviewed changes

Fix invalid statement template compile error

1be3277

hahmed force-pushed the fix-invalid-statement-compile-error branch from 0fc1396 to 1be3277 Compare June 9, 2021 21:32

hahmed marked this pull request as ready for review June 9, 2021 22:17

rafaelfranca merged commit b0d8d82 into rails:main Jun 10, 2021

rafaelfranca added a commit that referenced this pull request Jun 10, 2021

Merge pull request #42244 from hahmed/fix-invalid-statement-compile-e…

3bbf3ac

…rror Fix invalid statement template compile error

rafaelfranca added a commit that referenced this pull request Jun 10, 2021

Merge pull request #42244 from hahmed/fix-invalid-statement-compile-e…

b869a4e

…rror Fix invalid statement template compile error

hahmed deleted the fix-invalid-statement-compile-error branch June 10, 2021 05:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid statement template compile error #42244

Fix invalid statement template compile error #42244

hahmed commented May 17, 2021

zzak commented May 18, 2021

kamipo commented May 18, 2021

hahmed commented May 18, 2021

kamipo commented May 19, 2021

zzak commented May 22, 2021

hahmed commented May 22, 2021 •

edited

hahmed Jun 9, 2021 •

edited

Fix invalid statement template compile error #42244

Fix invalid statement template compile error #42244

Conversation

hahmed commented May 17, 2021

Summary

zzak commented May 18, 2021

kamipo commented May 18, 2021

hahmed commented May 18, 2021

kamipo commented May 19, 2021

zzak commented May 22, 2021

hahmed commented May 22, 2021 • edited

hahmed Jun 9, 2021 • edited

Choose a reason for hiding this comment

hahmed commented May 22, 2021 •

edited

hahmed Jun 9, 2021 •

edited