Better handle SyntaxError in Action View #48957

cmaruz · 2023-08-16T20:32:54Z

Motivation / Background

This Pull Request has been created to fix an issue where syntax errors generated inside an eval method cause an Internal Server Error due to a TypeError.

Detail

In order to display the extraced source of a syntax error, we try to locate the node id for the backtrace location. The call to RubyVM::AbstractSyntaxTree.node_id_for_backtrace_location is expecting a Thread::Backtrace::Location object, but we are passing a SourceMapLocation instead.

Once this is fixed, another issue then appears. The first location of the backtrace for an eval error does not include the filename of where eval is called, so we can't extract the source. This means that the development view fails to show the extracted source because it is hard-coded to show the source extracted for the first location.

This commit does two things:

it addresses the issue by making sure that we are always passing a Thread::Backtrace::Location instead
it allows the development view to show the extracted source fragment by ensuring the first location of the backtrace for a syntax error coming from eval includes the name of the file where eval is called

Additional information

I have tried to keep the changes consistent with the approach introduced in #46171.

cmaruz · 2023-08-23T05:38:39Z

cc: @tenderlove, @byroot

byroot · 2023-08-23T10:45:41Z

activesupport/lib/active_support/syntax_error_proxy.rb

+      end
+
+      def error_originated_in_eval_block?
+        __getobj__.to_s.match?(/^\(eval\)/)


Suggested change

__getobj__.to_s.match?(/^\(eval\)/)

__getobj__.to_s.start_with?("(eval")

This won't match anymore in 3.3: https://bugs.ruby-lang.org/issues/19755

byroot · 2023-08-23T10:46:39Z

yarn.lock

-"@rails/activestorage@>= 7.0.0-alpha1":
-  version "7.0.4"
-  resolved "https://registry.yarnpkg.com/@rails/activestorage/-/activestorage-7.0.4.tgz#56bc3052f60e8867b40a87ed5ff3c0afe536643e"
-  integrity sha512-K2fV9C0z88fapu58nt9uHzMylsmJWUFuaIFvuZQEf1WTiPMsgTsj1bf8+cD81zXfTXYMZDQNRq4164JmuBKD3A==
-  dependencies:
-    spark-md5 "^3.0.1"
-


This is unrelated to your PR.

byroot · 2023-08-23T10:48:38Z

activesupport/lib/active_support/core_ext/thread/backtrace/location.rb

@@ -9,4 +9,7 @@ def spot(ex)
    def spot(ex)
    end
  end
+  def thread_backtrace_location


Suggested change

def thread_backtrace_location

def thread_backtrace_location

I don't like the idea of adding a core_ext for this.

I'd rather use is_a? in the caller. Polymorphism is nicer indeed, but not at the cost of reopening a core class.

Thanks @byroot!

I can change the logic to use is_a?, but I wanted to keep in line with the spirit of #46171 where Thread::Backtrace::Location has been monkey patched.

My fear is that if I change this code to use is_a?, then this is not consistent with the rest of the code, which is instead relying on Thread::Backtrace::Location being monkey patched.

I am happy to use is_a? but I wanted to hear your thoughts.

Yeah, I disagree with how @tenderlove implemented it, I think it would have been possible to not add that Location#spot method.

Yeah, I disagree with how @tenderlove implemented it, I think it would have been possible to not add that Location#spot method.

If you want to refactor, I'm fine with it! 😂

One alternative would be to implement itself on BacktraceLocationProxy. Then we wouldn't need to add another method to Thread::Backtrace::Location

TBH, I think this def spot(ex) monkey patch should actually be part of ErrorHighlight. ErrorHighlight only works with BacktraceLocation objects, so the double dispatch / factory pattern makes sense to me.

If you want to refactor, I'm fine with it! 😂

I know :)

I'll see if get some time to avoid that patch, in the meantime I think we should mark many of this stuff as private API: https://edgeapi.rubyonrails.org/classes/Thread/Backtrace/Location.html#method-i-spot

Thanks guys!

I have made the following changes to exception_wrapper.rb:

Before calling spot on the template object, check if we have a real Thread::Backtrace::Location object to prevent calling MRI with the wrong object;

When building the SourceMapLocation object, ensure that we are passing a Thread::Backtrace::Location object and not a BacktraceLocationProxy object

byroot · 2023-08-25T08:28:50Z

activesupport/lib/active_support/syntax_error_proxy.rb

+      end
+
+      def error_originated_in_eval_block?
+        __getobj__.to_s.start_with?("(eval")


Suggested change

__getobj__.to_s.start_with?("(eval")

__getobj__.path.start_with?("(eval")

Using path is simpler than rendering Location#to_s. Also I'm not convinced this method extraction is really worth it.

zzak · 2023-09-27T02:51:31Z

@cmaruz Build is failing, could you take a look? 🙇

https://buildkite.com/rails/rails/builds/99163#018a3c4e-42cc-4132-baf4-58d424793ac3/1066-1255

bin/rails test /rails/actionpack/test/dispatch/debug_exceptions_test.rb:645

Error:
--
  | DebugExceptionsTest#test_display_backtrace_when_error_type_is_SyntaxError_wrapped_by_ActionView::Template::Error:
  | NoMethodError: undefined method `path' for #<SyntaxError: (eval):1: syntax error, unexpected end-of-input>
  | /rails/activesupport/lib/active_support/syntax_error_proxy.rb:46:in `parse_message_for_trace'
  | /rails/activesupport/lib/active_support/syntax_error_proxy.rb:35:in `backtrace_locations'
  | /rails/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb:274:in `build_backtrace'

cmaruz · 2023-11-29T13:35:26Z

Hi @zzak,

I fixed the failing test - it was due to the fact that in Ruby versions prior to 3.3, SyntaxError doesn't have a path method. Although I fixed it, I was wondering: is there a convention in the Rails codebase to isolate code which is dependant on the version of Ruby?

byroot · 2023-11-29T18:20:42Z

is there a convention in the Rails codebase to isolate code which is dependant on the version of Ruby?

It depends, but generally speaking we try to check for it at definition time rather than rutime.

So in your case it could be:

if SyntaxError.method_defined?(:path) # Ruby 3.3+
  def parse_message_for_trace
    # 3.3+ version of the method
  end
else
  def parse_message_for_trace
    # 3.2 and older version of the method
  end
end

cmaruz · 2023-12-01T06:06:16Z

Thanks @byroot, I have changed the check to be at definition time. Please let me know what you think!

method cause an Internal Server Error due to a TypeError. In order to display the extraced source of a syntax error, we try to locate the node id for the backtrace location. The call to RubyVM::AbstractSyntaxTree.node_id_for_backtrace_location is expecting a Thread::Backtrace::Location object, but we are passing a SourceMapLocation instead. This commit does two things: 1) it addresses the issue by making sure that we are always passing a Thread::Backtrace::Location instead 2) it allows the development view to show the extracted source fragment

cmaruz · 2023-12-05T07:16:57Z

I squashed the commits into a single commit and I think this PR is ready to go - the build failure seems to be unrelated and tests pass locally.

Please let me know if you have more feedback!

Better handle SyntaxError in Action View

eugeneius · 2024-01-16T15:21:20Z

Backported to 7-1-stable in b02f6c9.

rails-bot bot added actionpack activesupport labels Aug 16, 2023

cmaruz mentioned this pull request Aug 17, 2023

SyntaxError in an autoloading class referenced from a view causes "TypeError: Thread::Backtrace::Location object expected" #48326

Closed

byroot changed the title ~~Fix #48326~~ Better handle SyntaxError in Action View Aug 23, 2023

byroot reviewed Aug 23, 2023

View reviewed changes

byroot mentioned this pull request Aug 24, 2023

Add some :nodoc: to SyntaxErrorProxy and friends #49024

Merged

cmaruz force-pushed the 48326 branch 3 times, most recently from bde1b0e to 4598f41 Compare August 25, 2023 06:35

byroot reviewed Aug 25, 2023

View reviewed changes

cmaruz force-pushed the 48326 branch from 4598f41 to 2960b5d Compare August 28, 2023 13:21

cmaruz force-pushed the 48326 branch 2 times, most recently from a92ce81 to e6c5f41 Compare November 29, 2023 11:24

cmaruz force-pushed the 48326 branch 2 times, most recently from dd62149 to 016a256 Compare November 29, 2023 14:47

cmaruz force-pushed the 48326 branch 2 times, most recently from de733f3 to ac45315 Compare December 1, 2023 06:05

cmaruz force-pushed the 48326 branch from ac45315 to 3a90e2e Compare December 5, 2023 06:37

cmaruz force-pushed the 48326 branch from 3a90e2e to df6d2fb Compare December 5, 2023 06:40

byroot merged commit b979afe into rails:main Dec 5, 2023
3 of 4 checks passed

eugeneius pushed a commit that referenced this pull request Jan 16, 2024

Merge pull request #48957 from cmaruz/48326

b02f6c9

Better handle SyntaxError in Action View

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handle SyntaxError in Action View #48957

Better handle SyntaxError in Action View #48957

cmaruz commented Aug 16, 2023

cmaruz commented Aug 23, 2023

byroot Aug 23, 2023

byroot Aug 23, 2023

byroot Aug 23, 2023

cmaruz Aug 23, 2023

byroot Aug 23, 2023

tenderlove Aug 23, 2023

byroot Aug 24, 2023

cmaruz Aug 25, 2023

byroot Aug 25, 2023

zzak commented Sep 27, 2023

cmaruz commented Nov 29, 2023

byroot commented Nov 29, 2023

cmaruz commented Dec 1, 2023

cmaruz commented Dec 5, 2023

eugeneius commented Jan 16, 2024

	__getobj__.to_s.match?(/^\(eval\)/)
	__getobj__.to_s.start_with?("(eval")

	__getobj__.to_s.start_with?("(eval")
	__getobj__.path.start_with?("(eval")

Better handle SyntaxError in Action View #48957

Better handle SyntaxError in Action View #48957

Conversation

cmaruz commented Aug 16, 2023

Motivation / Background

Detail

Additional information

cmaruz commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzak commented Sep 27, 2023

cmaruz commented Nov 29, 2023

byroot commented Nov 29, 2023

cmaruz commented Dec 1, 2023

cmaruz commented Dec 5, 2023

eugeneius commented Jan 16, 2024