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

ActionController::RoutingError exception logs stacktrace #146

Closed
morgoth opened this issue Oct 23, 2015 · 12 comments
Closed

ActionController::RoutingError exception logs stacktrace #146

morgoth opened this issue Oct 23, 2015 · 12 comments

Comments

@morgoth
Copy link

morgoth commented Oct 23, 2015

It would be great to log this exception as 404 without bloating logs with exception stacktrace.
I can see that this is not an easy task in #27 and I also cannot find a good way to workaround this.
Do you have any clue how we can work on this issue?

@wgc-as
Copy link

wgc-as commented Nov 12, 2015

We added a new rule to the end of routes.rb:

  get '*unmatched_route', to: 'application#route_not_found'

And added the new controller action:

  def route_not_found
    render 'error_pages/404', status: :not_found
  end

With these Lograge will log this error as 404, and there should be no stack trace.

@pxlpnk
Copy link
Collaborator

pxlpnk commented Nov 12, 2015

@wgc-as I like the idea, could you craft a PR for the readme to add this?

@benlovell
Copy link
Collaborator

Agreed. This seems like a nice solution to a common problem. Thanks @wgc-as

@pxlpnk
Copy link
Collaborator

pxlpnk commented Nov 13, 2015

I had some minutes to fill so I prepared a PR, let me know what you think @benlovell @wgc-as @morgoth

@pxlpnk
Copy link
Collaborator

pxlpnk commented Nov 13, 2015

We added this to readme in the FAQ section.

@pxlpnk pxlpnk closed this as completed Nov 13, 2015
@swrobel
Copy link

swrobel commented Apr 1, 2016

Any thoughts on this cautionary note about catch-all routes & Rails engines that I found? https://github.com/vidibus/vidibus-routing_error#alternative-two

@ankane
Copy link

ankane commented Oct 21, 2016

Another approach (for Rails 4.2 at least) is to create an initializer with:

if Rails.application.config.lograge.enabled
  class ActionDispatch::DebugExceptions
    alias_method :old_log_error, :log_error

    def log_error(env, wrapper)
      exception = wrapper.exception
      if exception.is_a?(ActionController::RoutingError)
        data = {
          method: env["REQUEST_METHOD"],
          path: env["REQUEST_PATH"],
          status: wrapper.status_code,
          error: "#{exception.class.name}: #{exception.message}"
        }
        formatted_message = Lograge.formatter.call(data)
        logger(env).send(Lograge.log_level, formatted_message)
      else
        old_log_error env, wrapper
      end
    end
  end
end

Props to this StackOverflow question

@benlovell Are you open to a PR to add something like this to the gem?

@mtarnovan
Copy link

Use match instead of get to catch routing errors on non-GET routes. Additionally we can redirect from the route itself, without implementing a controller action:

match "*any", via: :all, to: redirect('/404')

@fgblomqvist
Copy link

fgblomqvist commented Feb 7, 2019

Updated version (for Rails 5.2) of the patch by @ankane:

config/initializers/routing_error_defatalizer.rb:

# frozen_string_literal: true

if Rails.application.config.lograge.enabled
  module ActionDispatch
    class DebugExceptions
      alias old_log_error log_error

      def log_error(request, wrapper)
        exception = wrapper.exception
        if exception.is_a?(ActionController::RoutingError)
          data = {
            method: request.env['REQUEST_METHOD'],
            path: request.env['REQUEST_PATH'],
            status: wrapper.status_code,
            error: "#{exception.class.name}: #{exception.message}"
          }
          formatted_message = Lograge.formatter.(data)
          logger(request).send(Lograge.log_level, formatted_message)
        else
          old_log_error request, wrapper
        end
      end
    end
  end
end

@mashedkeyboard
Copy link

@fgblomqvist's version of @ankane's patch works fantastically for non-resourceful routes - however, resourceful routes still seem to error with it. One solution could be adding ActiveRecord::RecordNotFound to the exception.is_a? statement, but that feels like it could have unintended consequences.

@fgblomqvist
Copy link

@mashedkeyboard I use it for all kinds of routes, resourceful and non-resourceful ones. I solved it by doing a pretty standard solution:

rescue_from ActiveRecord::RecordNotFound, with: :record_not_found if Rails.env.production?

def record_not_found
  not_found
end

def not_found(message = nil)
  flash[:page_error] = message.to_s if message.present?
  raise ActionController::RoutingError.new(message)
end

This is inside my ApplicationController.

@styrmis
Copy link

styrmis commented Jun 20, 2020

In case it helps anyone, I was finding that the match approach we were using to suppress the stacktraces from routing errors was causing requests bound for ActiveStorage to fail, as those routes appear to be added after the app routes are processed.

We are now using something of the form:

  # Return 404 for any unmatched paths except those bound for Rails-provided actions
  match '*path', to: 'application#not_found', via: :all, constraints: lambda { |req|
    req.path !~ %r{^/rails}
  }

to not match any request which appears to be destined for a Rails-provided action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants