search

pushtype / push_type

PushType is a modern, open source content management system for Ruby on Rails.
http://www.pushtype.org
Other
290 stars 32 forks source link

404 error handling #34

Open jonleighton opened 7 years ago

jonleighton commented 7 years ago

When PushType can't find a node, it raises ActiveRecord::RecordNotFound. I realised that I can add a rescue_from handler to my ApplicationController in order to handle this gracefully, but this isn't documented.

It would be helpful to add some doc for this, but I actually think it would be better to ensure that the *permalink route doesn't match unless the permalink exists in the database. Then, users can put other routes after the mount_push_type call and define their own handling for 404s (or just let Rails do its default thing, rendering public/404.html).

aaronrussell commented 7 years ago

I'm currently travelling so excuse the brevity. I may have missed the point here but does Rails not render a 404 by default with a ActiveRecord::RecordNotFound? Why would a rescue_from be needed?

Currently mount_push_type must be at the end of your routes file. I don't disagree - it would be better to only match the route if the permalink exists - but I'm unsure how best to do this. I would assume we'd create a middleware that queries the database, but I'd want to avoid the controller making the same query again.

jonleighton commented 7 years ago

I may have missed the point here but does Rails not render a 404 by default with a ActiveRecord::RecordNotFound?

Yes, it looks like it does. The reason I started looking into this, though, is because we started to get these exceptions reported to our exception notification system. It looks like Rails renders the 404 page but the exception still bubbles up (I see FATAL entries in the log file too). So I guess what I mean by "gracefully" is "without bubbling up".

I'm unsure how best to do this. I would assume we'd create a middleware that queries the database, but I'd want to avoid the controller making the same query again.

It could be done with a routing constraint. If the AR query cache is active at this point, then a double database query could be avoided. I think it would be active, but I haven't actually checked.

aaronrussell commented 7 years ago

I'll do a few experiments once I'm back. If the query gets cached in a routing constrain then no reason not to do this :)