Use resourceful routes for Blacklight routes

[MrDys]

CODEBASE-332: CODEBASE-329 is a ticket to make the routes Blacklight provides overridable in the local application, as named routes are not overridable (Rails 3 doesn't even try to keep track of overriding routes, resourceful routes end up working correctly because of the way they are actually implemented on top of the routes).

Below are three (of several) options for how to make Blacklight routes overridable. I have a moderate to strong preference for option 3, but would back down in the face of opposition (because it isn't really a great solution, and is to a significant extent getting around some limitations of Rails). In writing all this up, I've become less convinced that I shouldn't just do option 1 (which isn't great, I'd still prefer resourceful routing, but does maintains status quo)

1) One approach would be to encapsulate the Blacklight routes in a method provided by the gem as is. Relatively straightforward, but may deprive applications from some of the Rails routing features. This would give us the existing routes:

   opensearch_catalog        /catalog/opensearch(.:format)         
     citation_catalog        /catalog/citation(.:format)           
        email_catalog        /catalog/email(.:format)              
          sms_catalog        /catalog/sms(.:format)                
      endnote_catalog        /catalog/endnote(.:format)            

send_email_record_catalog /catalog/send_email_record(.:format)
catalog_facet /catalog/facet/:id(.:format)
unapi /catalog/unapi(.:format)
catalog_index GET /catalog(.:format)
librarian_view_catalog /catalog/:id/librarian_view(.:format) catalog GET /catalog/:id(.:format)
PUT /catalog/:id(.:format) # this is used to "allow the show view to paginate"

These routes are unused by Blacklight, with only stub implementations. I intend to drop them from Blacklight core. map_catalog /catalog/map(.:format)
/catalog/:id/image(.:format)
/catalog/:id/status(.:format)
/catalog/:id/availability(.:format)

I've always disliked the catalog/catalog_index routes as being somewhat confusing/unclear, although we could completely abandon the current resource routing and call them whatever we want.

2) Another approach would be to convert the Blacklight routes to be resourcefully routed, which is the more-or-less standard Rails way of making CRUD routes. The challenge with this approach for us is our records are not from an ORM like ActiveRecord, and we haven't exactly constructed our models and controllers to be easily converted.

A quick-and-dirty conversion will give us routes like [2]:

  opensearch_catalog_index GET    /catalog/opensearch(.:format)         
     citation_catalog_index GET    /catalog/citation(.:format)           
        email_catalog_index GET    /catalog/email(.:format)              
          sms_catalog_index GET    /catalog/sms(.:format)                
      endnote_catalog_index GET    /catalog/endnote(.:format)            

send_email_record_catalog_index GET /catalog/send_email_record(.:format)
facet_catalog_index GET /catalog/facet/:id(.:format)
unapi_catalog_index GET /catalog/unapi(.:format)
librarian_view_catalog GET /catalog/:id/librarian_view(.:format) catalog_index GET /catalog(.:format)
catalog GET /catalog/:id(.:format)
PUT /catalog/:id(.:format)

I don't particularly care for these routes, and I'm pretty sure they are less compelling than the first approach.

3) The first approach is very much on the table, but I came up with what may be the best attempt at using resourceful routing within Blacklight without being too distructive. Feel free to check out the full patch [1], and I'll try to highlight relevant bits inline.

By adding one really scary/clever piece of Rails magic (but one that is actually encouraged in the Rails documentation [3]) to option 2, we can get some decent resourceful routes:

ActiveSupport::Inflector.inflections do |inflect| inflect.irregular 'solr_document', 'catalog' end

This basically says "the singular form for 'catalog' is 'solr_document'", which isn't too far from what we're actually doing in practice, and gives us these routes:

  opensearch_catalog GET    /catalog/opensearch(.:format)         
        citation_catalog GET    /catalog/citation(.:format)           
           email_catalog GET    /catalog/email(.:format)              
             sms_catalog GET    /catalog/sms(.:format)                
         endnote_catalog GET    /catalog/endnote(.:format)            

send_email_record_catalog GET /catalog/send_email_record(.:format)
facet_catalog GET /catalog/facet/:id(.:format)
unapi_catalog GET /catalog/unapi(.:format)
librarian_view_solr_document GET /catalog/:id/librarian_view(.:format) catalog GET /catalog(.:format)
solr_document GET /catalog/:id(.:format)
PUT /catalog/:id(.:format)

This changes the named route "catalog_index" to "catalog", and the named route "catalog" to "solr_document", e.g.:

catalog_path(:q => 'asdf') solr_document_path(:id => 'u1')

We could write a set of deprecated helper functions to help people migrating local applications (although the actual conversion is a really simple find + replace)

The big downside to this approach is the fairly confusing Inflector magic, and to a lesser extent the aforementioned route name changes and the disagreement on the merits of resourceful routing.

By being closer to the Rails way of doing things, we can take advantage of some routing features, like translated paths (slightly contrived here): scope(:path_names => { :librarian_view => 'marc' }) do [ resources stuff here..] end

yields

librarian_view_solr_document GET /catalog/:id/marc(.:format)

This approach also has at least once nice side effect, as our defined routes will match up better with our data model. With a couple of tweaks to SolrDocument, we could even write: solr_document_path(@document) url_for(@document) solr_document_path(SolrDocument.new(:id => 'u1'))

(I've actually done some changes in my local app where being able to use the shorthand form with a SolrDocument object would be really nice, but I'm not sure if any one else has)

[1] https://github.com/cbeer/blacklight/commit/5f00b885f875cada7db588d055cdf7b42296b862 [2] https://gist.github.com/2e16b48f5413b42c20c1 [3] http://guides.rubyonrails.org/routing.html#overriding-the-singular-form

[MrDys]

Original reporter: cbeer

[MrDys]

jrochkind: If the main purpose of this redesign is to make the routes "over-rideable", I have some concerns about the fact that resourceful routes seem to be over-rideable just by happenstance, and not by design, and thus could change in the future. It seems like a mess to me, not worth making our code more confusing (which IMO it would be) to try to take advantage of an 'accident' in Rails code.

I think maybe the preferable way to handle this is what we've been moving to instead, having options you pass to the Blacklight #add_routes method which let you turn off the generation of a particular route that you want to customize. I'd rather make those options more fine-grained, if needed -- I think that approach uses less magic, is easier to understand, is less likely to break in future Rails versions. And in fact, using resourceful routes would make the "options to prevent generation of a specific route you want to customize" approach more difficult, I think, which is part of why I dont' like it. I'm imagining an option that says "omit the catalog#sms route" or whatever, finer grained options for omitting generation of BL routes (so you can write those routes yourself if you want), and think that's way preferable to trying to use more confusing undocumented Rails magic.