when there is a puppet class that its name is an integer, we can neither
edit nor delete it.
There are at least two approaches to handle this problem but each one has
its pros and cons, thus that's why I am opening this thread.
Some background (taken from the issue tracker):
From the log, I have noticed a strange behavior.
If the name contains only numerical characters, ActiveRecord runs a query
that looks for an id that matches.
Otherwise, it goes over the records' name attribute and compares.
The log:
Started GET "/puppetclasses/123bla/edit" for 172.17.42.1 at 2014-08-25
19:37:46 +0000
Processing by PuppetclassesController#edit as HTML
Parameters: {"id"=>"123bla"}
Puppetclass Load (0.3ms) SELECT "puppetclasses".* FROM "puppetclasses"
WHERE "puppetclasses"."name" = '123bla' ORDER BY puppetclasses.name LIMIT 1
Started GET "/puppetclasses/123/edit" for 172.17.42.1 at 2014-08-25
19:37:53 +0000
Processing by PuppetclassesController#edit as HTML
Parameters: {"id"=>"123"}
Puppetclass Load (0.1ms) SELECT "puppetclasses".* FROM "puppetclasses"
WHERE "puppetclasses"."id" = ? ORDER BY puppetclasses.name LIMIT 1 "id",
"123"
<http://projects.theforeman.org/projects/foreman/wiki/%22id%22_%22123%22>
not found: Couldn't find Puppetclass with id=123 [...]
The reason for that behavior is this line:
@puppetclass = (params[:id] =~ /\A\d+\Z/) ? pc.find(params[:id]) :
pc.find_by_name(params[:id])
One approach is to keep the old behavior but allowing deleting and editing:
if the params[:id] is an integer and there exists a record with that id,
then use find method. Otherwise, use the find_by_name method.
I have already written a pull request
<https://github.com/theforeman/foreman/pull/1723> that does this and it
passes.
The second one is to do what friendly_id does: if there is an integer to
find friendly_id will still look for a name first and if it's not found
look for a matching id. By that, it won't be a big "surprise", when we move
to friendly_id. Nevertheless, we need to change the tests (in order to
pass) and regardless to that, we may need to add more tests to check the
new functionality.
Another way, which has been suggested by Dominic, is returning #{id}-#{name} in to_param method.
BTW - no matter what way we will take, we have to handle cases like name-id
collisions but that is a different PR.
For the Web UI, I would recommend to go with Dominic's suggestion of
def to_param
"#{id}-#{name.parameterize}"
end
In this scenario, the generated URL's show the ID so id field "wins" if there is a conflict with name. Also, in /app/controllers/application_controller.rb line #130-136, our current 'find_by_name' logic checks first for 'id' if params[:id] starts with an integer, so id "wins"
However, in the API find logic, the NAME field "wins"
/app/controllers/api/base_controller.rb lines #142-144
def resource_identifying_attributes
%w(name id)
end
The goal of PR #1246 to implement the friendly_id gem was to clean up the find by logic between to the API and UI and make it more consistent.
I can see justification for saying id "wins" or name "wins" if there is an edge case where the name of a puppetclass is 33 and another puppetclass has an id of 33. I think it's an unlikely edge case though.
Regards,
Joseph
ยทยทยท
----- Original Message -----
> From: "boaz s"
> To: foreman-dev@googlegroups.com
> Sent: Tuesday, September 2, 2014 10:43:05 AM
> Subject: [foreman-dev] Puppet class that has only numerical characters in its name issue
>
> Hi all,
>
> I am trying to fix issue 6914
> - when there is a puppet class that its name is an integer, we can neither
> edit nor delete it.
>
> There are at least two approaches to handle this problem but each one has
> its pros and cons, thus that's why I am opening this thread.
> Some background (taken from the issue tracker):
>
> ```
>
> From the log, I have noticed a strange behavior.
> If the name contains only numerical characters, ActiveRecord runs a query
> that looks for an id that matches.
> Otherwise, it goes over the records' name attribute and compares.
>
> The log:
> Started GET "/puppetclasses/123bla/edit" for 172.17.42.1 at 2014-08-25
> 19:37:46 +0000
> Processing by PuppetclassesController#edit as HTML
> Parameters: {"id"=>"123bla"}
> Puppetclass Load (0.3ms) SELECT "puppetclasses".* FROM "puppetclasses"
> WHERE "puppetclasses"."name" = '123bla' ORDER BY puppetclasses.name LIMIT 1
>
> Started GET "/puppetclasses/123/edit" for 172.17.42.1 at 2014-08-25
> 19:37:53 +0000
> Processing by PuppetclassesController#edit as HTML
> Parameters: {"id"=>"123"}
> Puppetclass Load (0.1ms) SELECT "puppetclasses".* FROM "puppetclasses"
> WHERE "puppetclasses"."id" = ? ORDER BY puppetclasses.name LIMIT 1 "id",
> "123"
>
> not found: Couldn't find Puppetclass with id=123 [...]
> The reason for that behavior is this line:
> @puppetclass = (params[:id] =~ /\A\d+\Z/) ? pc.find(params[:id]) :
> pc.find_by_name(params[:id])
> ```
>
> One approach is to keep the old behavior but allowing deleting and editing:
> if the `params[:id]` is an integer and there exists a record with that id,
> then use `find` method. Otherwise, use the `find_by_name` method.
> I have already written a pull request
> that does this and it
> passes.
>
> The second one is to do what `friendly_id` does: if there is an integer to
> find `friendly_id` will still look for a name first and if it's not found
> look for a matching id. By that, it won't be a big "surprise", when we move
> to `friendly_id`. Nevertheless, we need to change the tests (in order to
> pass) and regardless to that, we may need to add more tests to check the
> new functionality.
>
> Another way, which has been suggested by Dominic, is returning
> `#{id}-#{name}` in `to_param` method.
>
> BTW - no matter what way we will take, we have to handle cases like name-id
> collisions but that is a different PR.
>
> Best Regards.
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
