From 175f489acbaed77ad52a18d805e4b6eeae1abfdb Mon Sep 17 00:00:00 2001 From: Kevin Gilpin Date: Thu, 13 May 2021 14:22:32 -0400 Subject: [PATCH] fix: Add the proper template function hooks for Rails 6.0.7 Ignore methods named 'call' --- lib/appmap/config.rb | 131 +++++++++++++++++++-------- lib/appmap/handler/rails/template.rb | 14 ++- lib/appmap/hook.rb | 4 +- spec/hook_spec.rb | 55 +---------- 4 files changed, 108 insertions(+), 96 deletions(-) diff --git a/lib/appmap/config.rb b/lib/appmap/config.rb index 099d1dda..2e647dff 100644 --- a/lib/appmap/config.rb +++ b/lib/appmap/config.rb @@ -85,18 +85,7 @@ def to_h end end - Function = Struct.new(:package, :cls, :labels, :function_names) do # :nodoc: - def to_h - { - package: package, - class: cls, - labels: labels, - functions: function_names.map(&:to_sym) - }.compact - end - end - private_constant :Function - + # Identifies specific methods within a package which should be hooked. class TargetMethods # :nodoc: attr_reader :method_names, :package @@ -118,28 +107,91 @@ def to_h end private_constant :TargetMethods - OPENSSL_PACKAGES = ->(labels) { Package.build_from_path('openssl', package_name: 'openssl', labels: labels) } + # Function represents a specific function configured for hooking by the +functions+ + # entry in appmap.yml. When the Config is initialized, each Function is converted into + # a Package and TargetMethods. It's called a Function rather than a Method, because Function + # is the AppMap terminology. + Function = Struct.new(:package, :cls, :labels, :function_names) do # :nodoc: + def to_h + { + package: package, + class: cls, + labels: labels, + functions: function_names.map(&:to_sym) + }.compact + end + end + private_constant :Function - # Methods that should always be hooked, with their containing - # package and labels that should be applied to them. - HOOKED_METHODS = { - 'ActionView::Renderer' => TargetMethods.new(:render, Package.build_from_gem('actionview', shallow: false, package_name: 'action_view', labels: %w[mvc.view], optional: true).tap do |package| - package.handler_class = AppMap::Handler::Rails::Template::RenderHandler if package - end), - 'ActionView::Resolver' => TargetMethods.new(%i[find_all find_all_anywhere], Package.build_from_gem('actionview', shallow: false, package_name: 'action_view', labels: %w[mvc.template.resolver], optional: true).tap do |package| - package.handler_class = AppMap::Handler::Rails::Template::ResolverHandler if package - end), - 'ActionDispatch::Request::Session' => TargetMethods.new(%i[destroy [] dig values []= clear update delete fetch merge], Package.build_from_gem('actionpack', shallow: false, package_name: 'action_dispatch', labels: %w[http.session], optional: true)), - 'ActionDispatch::Cookies::CookieJar' => TargetMethods.new(%i[[]= clear update delete recycle], Package.build_from_gem('actionpack', shallow: false, package_name: 'action_dispatch', labels: %w[http.cookie], optional: true)), - 'ActionDispatch::Cookies::EncryptedCookieJar' => TargetMethods.new(%i[[]=], Package.build_from_gem('actionpack', shallow: false, package_name: 'action_dispatch', labels: %w[http.cookie crypto.encrypt], optional: true)), - 'CanCan::ControllerAdditions' => TargetMethods.new(%i[authorize! can? cannot?], Package.build_from_gem('cancancan', shallow: false, labels: %w[security.authorization], optional: true)), - 'CanCan::Ability' => TargetMethods.new(%i[authorize!], Package.build_from_gem('cancancan', shallow: false, labels: %w[security.authorization], optional: true)), - 'ActionController::Instrumentation' => [ - TargetMethods.new(%i[process_action send_file send_data redirect_to], Package.build_from_gem('actionpack', shallow: false, package_name: 'action_controller', labels: %w[mvc.controller], optional: true)) - ] - }.freeze + ClassTargetMethods = Struct.new(:cls, :target_methods) # :nodoc: + private_constant :ClassTargetMethods + + MethodHook = Struct.new(:cls, :method_names, :labels) # :nodoc: + private_constant :MethodHook + + class << self + def package_hooks(gem_name, methods, handler_class: nil, package_name: nil) + Array(methods).map do |method| + package = Package.build_from_gem(gem_name, package_name: package_name, labels: method.labels, shallow: false, optional: true) + next unless package + + package.handler_class = handler_class if handler_class + ClassTargetMethods.new(method.cls, TargetMethods.new(Array(method.method_names), package)) + end.compact + end + + def method_hook(cls, method_names, labels) + MethodHook.new(cls, method_names, labels) + end + end + + # Hook well-known functions. When a function configured here is available in the bundle, it will be hooked with the + # predefined labels specified here. If any of these hooks are not desired, they can be disabled in the +exclude+ section + # of appmap.yml. + METHOD_HOOKS = [ + package_hooks('actionview', + [ + method_hook('ActionView::Renderer', :render, %w[mvc.view]), + method_hook('ActionView::TemplateRenderer', :render, %w[mvc.view]), + method_hook('ActionView::PartialRenderer', :render, %w[mvc.view]) + ], + handler_class: AppMap::Handler::Rails::Template::RenderHandler, + package_name: 'action_view' + ), + package_hooks('actionview', + [ + method_hook('ActionView::Resolver', %i[find_all find_all_anywhere], %w[mvc.template.resolver]) + ], + handler_class: AppMap::Handler::Rails::Template::ResolverHandler, + package_name: 'action_view' + ), + package_hooks('actionpack', + [ + method_hook('ActionDispatch::Request::Session', %i[destroy [] dig values []= clear update delete fetch merge], %w[http.session]), + method_hook('ActionDispatch::Cookies::CookieJar', %i[[]= clear update delete recycle], %w[http.session]), + method_hook('ActionDispatch::Cookies::EncryptedCookieJar', %i[[]= clear update delete recycle], %w[http.cookie crypto.encrypt]) + ], + package_name: 'action_dispatch' + ), + package_hooks('cancancan', + [ + method_hook('CanCan::ControllerAdditions', %i[authorize! can? cannot?], %w[security.authorization]), + method_hook('CanCan::Ability', %i[authorize?], %w[security.authorization]) + ] + ), + package_hooks('actionpack', + [ + method_hook('ActionController::Instrumentation', %i[process_action send_file send_data redirect_to], %w[mvc.controller]) + ], + package_name: 'action_controller' + ) + ].flatten.freeze - BUILTIN_METHODS = { + OPENSSL_PACKAGES = ->(labels) { Package.build_from_path('openssl', package_name: 'openssl', labels: labels) } + + # Hook functions which are builtin to Ruby. Because they are builtins, they may be loaded before appmap. + # Therefore, we can't rely on TracePoint to report the loading of this code. + BUILTIN_HOOKS = { 'OpenSSL::PKey::PKey' => TargetMethods.new(:sign, OPENSSL_PACKAGES.(%w[crypto.pkey])), 'OpenSSL::X509::Request' => TargetMethods.new(%i[sign verify], OPENSSL_PACKAGES.(%w[crypto.x509])), 'OpenSSL::PKCS5' => TargetMethods.new(%i[pbkdf2_hmac_sha1 pbkdf2_hmac], OPENSSL_PACKAGES.(%w[crypto.pkcs5])), @@ -166,26 +218,29 @@ def to_h 'JSON::Ext::Generator::State' => TargetMethods.new(:generate, Package.build_from_path('json', package_name: 'json', labels: %w[format.json])), }.freeze - attr_reader :name, :packages, :exclude, :hooked_methods, :builtin_methods + attr_reader :name, :packages, :exclude, :hooked_methods, :builtin_hooks def initialize(name, packages, exclude: [], functions: []) @name = name @packages = packages - @hook_paths = packages.map(&:path) + @hook_paths = Set.new(packages.map(&:path)) @exclude = exclude - @builtin_methods = BUILTIN_METHODS + @builtin_hooks = BUILTIN_HOOKS @functions = functions - @hooked_methods = HOOKED_METHODS.dup + + @hooked_methods = METHOD_HOOKS.each_with_object(Hash.new { |h,k| h[k] = [] }) do |cls_target_methods, hooked_methods| + hooked_methods[cls_target_methods.cls] << cls_target_methods.target_methods + end + functions.each do |func| package_options = {} package_options[:labels] = func.labels if func.labels - @hooked_methods[func.cls] ||= [] @hooked_methods[func.cls] << TargetMethods.new(func.function_names, Package.build_from_path(func.package, package_options)) end @hooked_methods.each_value do |hooks| Array(hooks).each do |hook| - @hook_paths << hook.package.path if hook.package + @hook_paths << hook.package.path end end end diff --git a/lib/appmap/handler/rails/template.rb b/lib/appmap/handler/rails/template.rb index b918bce6..350e6958 100644 --- a/lib/appmap/handler/rails/template.rb +++ b/lib/appmap/handler/rails/template.rb @@ -105,12 +105,18 @@ def handle_call(defined_class, hook_method, receiver, args) # If so, populate the template path. In all cases, add a TemplateMethod so that the # template will be recorded in the classMap. def handle_return(call_event_id, elapsed, return_value, exception) - warn "Resolver return: #{return_value.inspect}" if LOG - renderer = Array(Thread.current[TEMPLATE_RENDERER]).last - path = Array(return_value).first&.inspect + path_obj = Array(return_value).first + + warn "Resolver return: #{path_obj}" if LOG - if path + if path_obj + path = if path_obj.respond_to?(:identifier) && path_obj.inspect.index('#<') + path_obj.identifier + else + path_obj.inspect + end + path = path[Dir.pwd.length + 1..-1] if path.index(Dir.pwd) == 0 AppMap.tracing.record_method(TemplateMethod.new(path)) renderer.path ||= path if renderer end diff --git a/lib/appmap/hook.rb b/lib/appmap/hook.rb index b4ec9fd2..961b5759 100644 --- a/lib/appmap/hook.rb +++ b/lib/appmap/hook.rb @@ -64,7 +64,7 @@ def hook_builtins end end - config.builtin_methods.each do |class_name, hooks| + config.builtin_hooks.each do |class_name, hooks| Array(hooks).each do |hook| require hook.package.package_name if hook.package.package_name Array(hook.method_names).each do |method_name| @@ -139,6 +139,8 @@ def trace_end(trace_point) # a stack overflow in the defined hook method. next if %w[Marshal AppMap ActiveSupport].member?((hook_cls&.name || '').split('::')[0]) + next if method_id == :call + method = begin hook_cls.public_instance_method(method_id) rescue NameError diff --git a/spec/hook_spec.rb b/spec/hook_spec.rb index 67023087..d22a9e6b 100644 --- a/spec/hook_spec.rb +++ b/spec/hook_spec.rb @@ -64,65 +64,14 @@ def test_hook_behavior(file, events_yaml, setup: nil, &block) expect(config.never_hook?(ExcludeTest, ExcludeTest.method(:cls_method))).to be_truthy end - it "handles an instance method named 'call' without issues" do + it "an instance method named 'call' will be ignored" do events_yaml = <<~YAML - --- - - :id: 1 - :event: :call - :defined_class: MethodNamedCall - :method_id: call - :path: spec/fixtures/hook/method_named_call.rb - :lineno: 8 - :static: false - :parameters: - - :name: :a - :class: Integer - :value: '1' - :kind: :req - - :name: :b - :class: Integer - :value: '2' - :kind: :req - - :name: :c - :class: Integer - :value: '3' - :kind: :req - - :name: :d - :class: Integer - :value: '4' - :kind: :req - - :name: :e - :class: Integer - :value: '5' - :kind: :req - :receiver: - :class: MethodNamedCall - :value: MethodNamedCall - - :id: 2 - :event: :return - :parent_id: 1 - :return_value: - :class: String - :value: 1 2 3 4 5 + --- [] YAML _, tracer = test_hook_behavior 'spec/fixtures/hook/method_named_call.rb', events_yaml do expect(MethodNamedCall.new.call(1, 2, 3, 4, 5)).to eq('1 2 3 4 5') end - class_map = AppMap.class_map(tracer.event_methods) - expect(Diffy::Diff.new(<<~CLASSMAP, YAML.dump(class_map)).to_s).to eq('') - --- - - :name: spec/fixtures/hook/method_named_call.rb - :type: package - :children: - - :name: MethodNamedCall - :type: class - :children: - - :name: call - :type: function - :location: spec/fixtures/hook/method_named_call.rb:8 - :static: false - CLASSMAP end it 'can custom hook and label a function' do