Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6814 (closed enhancement: wontfix)

Opened 4 years ago

Last modified 2 years ago

[PATCH] Add ability to change the name of the :id parameter in Routing resources

Reported by: tsaleh Assigned to: core
Priority: normal Milestone: 2.x
Component: ActionPack Version: edge
Severity: normal Keywords: resource routing id
Cc: jyurek@thoughtbot.com, rails@carlivar.com, dkubb, demisone

Description

This patch adds the ability to specify the name of the :id parameter in the RESTFull routes. We needed this in our current project for two reasons: 1) we needed the :id param to be consistent in nested routes (currently it's either :id of :client_id, depending on the level), and 2) we wanted the ability to change the primary key to one of our choosing, while keeping the controller code readable (for example, ISBN numbers accessed as params[:isbn]).


Adds a parameter named :key to the options to map.resources. If set, the value of this parameter is used instead of :id as the parameter name in all member urls. Also, if the value starts with the singular name of the controller (such as :user_name), then 'user_' will not be added to the prefix_path. Here's an example:

map.resources :clients, :key => :client_name do |client|
  client.resources :sites, :key => :name do |site|
    site.resources :article, :key => :title
  end
end

creates the following urls:

/clients/:client_name
/clients/:client_name/sites/:name
/clients/:client_name/sites/:site_name/articles/:title

Notice that because 'client_name' starts with 'client' (singular version of the controller), the nested path parameter remains 'client_name', and is not automatically set to 'client_client_name'. This example shows the full range of the :key option, but common usage would be more in line with this:

map.resources :users, :key => :user_id do |user|
  user.resources :books, :key => :isbn do |book|
    book.resources :authors
  end
end
/users/2 => /users/:user_id
/users/2/books/0-12-345678-9 => /users/:user_id/books/:isbn
/users/2/books/0-12-345678-9/authors/3 => /users/:user_id/books/:isbn/authors/:id

The attached patch includes relevant tests.

Attachments

add_key_option_to_route_resources.diff (6.2 kB) - added by tsaleh on 12/10/06 22:27:44.
Patch file
1_2_stable_add_key_option_to_route_resources.diff (11.5 kB) - added by Conrad Poohs on 03/01/07 00:14:19.
Patch updated for 1.2 stable branch (Rev.6270)
updated_add_key_option_to_route_resources.diff (12.0 kB) - added by Conrad Poohs on 03/01/07 04:03:20.
Patch updated for current Edge Rails trunk (Rev. 6270)
resource_keys.rb (0.9 kB) - added by bpartridge on 08/12/08 03:44:34.
Untested but working plugin module (not full plugin) for this functionality

Change History

12/10/06 22:27:44 changed by tsaleh

  • attachment add_key_option_to_route_resources.diff added.

Patch file

12/10/06 23:41:59 changed by Chris Wanstrath

Sorry, maybe I'm missing something, but why not just override to_param on your models? I do exactly what you're talking about by overriding to_param in my RESTful app.

12/11/06 01:01:00 changed by tsaleh

That's a good question, and one that I should have addressed in the original post. This patch works in conjunction with AR::Base.to_params. In the ISBN example, you would override to_params to return the ISBN, and set the :key parameter to :isbn. Otherwise, the readability of your controller code is deminished:

def show
  @book = Book.find_by_isbn(params[:id])
end

This confuses the names :id and :isbn, which is somewhat poor. With the patch, the show function is clearer:

def show
  @book = Book.find_by_isbn(params[:isbn])
end

In addition, we found that our code could be greatly simplified if the keys in the params hash were always referring to the same models. With the normal code :id can refer to any of your model classes, depending on which controller you're in. If you specify a :key value of :XXX_id for each resource, however, then you can move a good deal of your code to helper functions in application.rb:

def get_book
  Book.find(params[:book_id])
end

We found we could greatly improve the readability of our code by using this technique consistently.

12/11/06 05:43:26 changed by ulysses

  • keywords deleted.

This is not a regression; please do not abuse the keywords field. And please provide a subject.

12/11/06 14:11:00 changed by tsaleh

  • summary changed from [PATCH] to [PATCH] Add ability to change the name of the :id parameter in Routing resources.

Sorry about that. I honestly wasn't sure exactly what the 1.2regression keyword meant, so I thought I should play it safe by including it. Is that only for bugs (as opposed to enhancements)?

Have also included the forgotten subject.

(follow-up: ↓ 7 ) 12/11/06 22:33:10 changed by toolmantim

+1 for this patch

12/12/06 00:29:22 changed by ulysses

Thanks for adding a subject. The 1.2regression keyword is for bugs that did not exist in <1.2 and do in the RC. :-)

(in reply to: ↑ 5 ; follow-up: ↓ 8 ) 01/16/07 21:56:48 changed by carlivar

  • cc changed from jyurek@thoughtbot.com to jyurek@thoughtbot.com, rails@carlivar.com.

Replying to toolmantim:

+1 here too. If I'm not mistaken, this will allow RESTful "pretty URLs" as well. (i.e. /authors/AynRand instead of /authors/265).

(in reply to: ↑ 7 ) 02/22/07 03:02:13 changed by tsaleh

I posted an article about this patch here.

02/27/07 19:28:35 changed by Conrad Poohs

+1 This is a feature most Rails developers will be looking for after converting to RESTful routes.

I had a bit of trouble getting generated paths (i.e. user_path(:name=>params[:name])) to accept argument symbols other than :id. So I modified the action_options_for method of resources.rb to require the :key symbol instead of :id (on line 392 in Rails 1.2.2):

        require_id = resource.kind_of?(SingletonResource) ? {} : { :requirements => { resource.key.to_sym => Regexp.new("[^#{Routing::SEPARATORS.join}]+") } }

This also requires an attr_reader for :key in the Resource class. For clarity I would also recommend that require_id be renamed to require_key.

02/27/07 23:26:40 changed by dkubb

  • cc changed from jyurek@thoughtbot.com, rails@carlivar.com to jyurek@thoughtbot.com, rails@carlivar.com, dkubb.

+1 for this patch.

Conrad, any chance you could submit an updated patch that includes the changes you made?

03/01/07 00:14:19 changed by Conrad Poohs

  • attachment 1_2_stable_add_key_option_to_route_resources.diff added.

Patch updated for 1.2 stable branch (Rev.6270)

03/01/07 04:03:20 changed by Conrad Poohs

  • attachment updated_add_key_option_to_route_resources.diff added.

Patch updated for current Edge Rails trunk (Rev. 6270)

03/01/07 04:15:16 changed by Conrad Poohs

Okay, the two patches I added today should cover recent changes to the stable-1.2 branch and Edge Rails trunk. I added a new test to the Edge patch, which tests combining this patch with custom id requirements to allow for custom key requirements. Should give resource routing even more flexibility.

I'm a Rails Trac newbie, so if I should have opened a new ticket for the Edge version, or changed this ticket's version to edge, please let me know. Also, how do we go about getting this committed to Edge Rails? I think Edge users would love it.

03/17/07 09:06:23 changed by dru

+ 1 for this patch, will it be accepted in 1.2.4?

04/04/07 22:47:23 changed by wsobel

+ 1 for this patch...

Would it be possible to add :requirements as well so we can add multiple resource mappings to allow for both id and and key based access?

This would complete my requirements since from a URI standpoint, it would be nice to allow GET to reference the object by name whereas for POST I would prefer using ids.

04/11/07 10:39:10 changed by ry

+1. This fixes a serious deficiency of restful routing: the fixed id parameter is confusing and gives the impression that map.resources is quite limited.

05/10/07 21:04:39 changed by demisone

+ 1 from me too :) It would be nice to be included in 1.2.4

05/10/07 21:15:28 changed by demisone

  • cc changed from jyurek@thoughtbot.com, rails@carlivar.com, dkubb to jyurek@thoughtbot.com, rails@carlivar.com, dkubb, demisone.

(in reply to: ↑ description ) 05/10/07 22:29:26 changed by carlivar

I sure hope the Milestone: 1.2.4 is accurate...

05/30/07 18:59:24 changed by bitsweat

  • keywords set to resource routing id.

I think the :id default is fine. Id is short for identifier after all, and this is the resource's identifier within its collection. Whether you treat it as a number or ISBN or surname is up to you.

05/30/07 19:18:47 changed by tsaleh

bitsweat: I'd normally agree with you, but the fact that ActiveRecord has an explicit #id method means that it will still be misleading in the controller code.

05/30/07 19:22:38 changed by besquared

I think that this would be a great addition because it's an overriding option which doesn't affect performance. The problem with assuming _id suffixes is that routes is assuming you're representing a tree structure based on joining tables with foreign keys in other tables. It's semantically tying your resources to your schema which kinda defeats the purpose of decoupling the URL from your Models in the first place.

The other usecase which it doesn't take into account is where a tree is stored as a set of columns in a *single* table. For ease throughout your application you want your column names to match your param names. The solution this so far is to add a before_filter to application.rb and set my_column = params[:my_column_id] for each variable you expect to be named with the _id suffix.

That's gross, please apply this patch.

05/30/07 19:52:16 changed by bitsweat

I think that argument's a stretch, not to mention contradictory re. its tie to the database schema.

This is a convention for URL interpolation placeholders, not a semantic lead blanket.

I understand the urge for purity but I don't see the pragmatic benefit in introducing more configuration-ese.

05/30/07 19:56:25 changed by david

  • status changed from new to closed.
  • resolution set to wontfix.

Me neither, I'm -1 on this patch. params[:id] is just about the record identifier from the URL. It doesn't have to relate to ActiveRecord::Base.id. I often use it in ways that it doesn't.

05/30/07 19:57:30 changed by david

BTW, despite the fact that this won't be making it into Rails core, you're of course always free to create a plugin that adds this functionality.

05/30/07 20:12:14 changed by ry

Me neither, I'm -1 on this patch. params[:id] is just about the record identifier from the URL. It doesn't have to relate to ActiveRecord::Base.id.

The main problem is in the url helpers. They call the id method on models passed to them. So if I use params[:id] to refer to something else, say, a universal product code. I end up having to do this all over my code:

product_url(:id => @product.upc)

05/30/07 20:17:55 changed by david

They call to_param, which by default is implemented as id, but there for you to overwrite:

  class Product
    def to_param() upc end
  end

  product_url(@product)

07/11/07 12:16:43 changed by anthonygreen

It would be nice not to have to add the equivalent(s) of

raise ::ActionController::RoutingError, “Recognition failed for #{request.path}” if @product.nil?

to all your controllers though

is find_by_upc not meant to have the same behaviour as find_by_id in these circumstances where you want human readable urls ?

08/12/08 03:44:34 changed by bpartridge

  • attachment resource_keys.rb added.

Untested but working plugin module (not full plugin) for this functionality

08/12/08 12:28:15 changed by bpartridge

  • version changed from 1.2.0rc1 to edge.
  • milestone changed from 1.2.4 to 2.x.

+1, FWIW. I think the original intent of the patch was lost among a number of tangential comments that led to its unfortunate demise. Although, as David says, overriding to_param can fix the product_url issue, one should note that the original patch never mentioned ActiveRecord - it was to improve the readability of controllers and routes.rb.

For instance, in my application, I have a crosscutting concern that most of my controllers need to know the document id. By default, doing map.resources :documents do |document| ... , DocumentsController refers to the id as :id while controllers for the nested routes use :document_id . Wouldn't it be nice if the ApplicationController could expose an attribute current_doc_id that always refers to params[:document_id] , even in DocumentsController? That's elegant and DRY to me. Furthermore, when doing many nested resources, every id can be explicitly labeled, so there will be no confusion for readers of the code.

But, as the gods have closed this patch, we mortals make plugins, as I have done above. Apologies for gravedigging, but this is a feature that makes Rails much more flexible, and I believe it is worthy of consideration once again.