fixes #8597 - friendly ID should escape slashes for URL parameters

[unorthodoxgeek]

just a short explanation - there was a similar PR on friendly id, to which they answered - hey, why not use sluggable? (which means another table in the DB, just for fetching the ID)
there's also an option to sanitise names, which means we don't let slashes in the name (feels intrusive to me, and doesn't solve current broken items)

I think that this monkey patch fixes it in the best possible way - we are able to both keep current (broken) data and have it working, we don't need to add a database table which might break current friendly ids, and this seems like the most performant option of the ones I see.

[ohadlevy]

@isratrade can you review please? thanks!

[isratrade]

@unorthodoxgeek, I would suggest do add this to ptable.rb and it works fine.

  def to_param
    Parameterizable.parameterize("#{id}-#{name}")
  end

Imho, not every model needs a friendly id like /environments/production.
With the tupane, the user doesn't even see the URL anyway, but they did, it would look like this.
http://localhost:3000/ptables/21-debian%20default%20-dev-sda/edit which seems fine. The error occurs if there is not the leading id (21)

[dLobatog]

@unorthodoxgeek ping 🔔

[unorthodoxgeek]

fixed. LDAP tests might fail because of lack of Gemfile.lock

[dLobatog]

@unorthodoxgeek I agree with @isratrade on a first pass, in Operatingsystem we keep FriendlyId too and just add the to_param method. I'm not entirely familiar with the for and against arguments for FriendlyId so if you want to review the use of FriendlyId in the codebase that'd be appreciated, otherwise just add to_param to keep stuff consistent.

[unorthodoxgeek]

@elobato - I remove friendly ID in rails 4 anyway...
What @isratrade suggested is basically copying the method from ByIdName to the class, which makes no sense, I just include it, and it works.

[isratrade]

@unorthodoxgeek, why are you removing friendly ID? Are you rewriting existing UI or API calls such as api/environments/production (rather than with ## id such as api/environments/1-production)

[unorthodoxgeek]

[test]

[unorthodoxgeek]

@isratrade - friendly id doesn't do anything once Parameterizable::ByIdName is included.

[isratrade]

I'm not sure this it's correct that friendly_id does nothing if Parameterizable::ByIdName, but if this is the case, why woudn't you want to refactor on the model that have both friendly_id and Parameterizable::ByIdName. Looking at the code, this mixin only adds the def to_param method. Where does it affect friendly_id?

[unorthodoxgeek]

friendly id defines the to_param and the find methods on the model. the parametrizable method defines the same methods. thus - it's pointless to have both on the same model.

[isratrade]

@unorthodoxgeek, if you remove friendly_id can you do this:

Environment.find('production')

[unorthodoxgeek]

no, nor can I do it with friendly id, because the find method is overridden by the parametrizable module.
and it doesn't matter, because we only care about the links, which use to_param, and have the item's id in them.

[isratrade]

but the API also uses names as identifiers in the url, so we can't forgot about this, or it could break people's scripts.

[unorthodoxgeek]

API should use IDs, if it supports names it's a huge design flaw on our part.

[isratrade]

Throughout the API documentation, using apipie, we assign identifier as the type which is defined as
Must be an identifier, string from 1 to 128 characters containing only alphanumeric characters, space, underscore(_), hypen(-) with no leading or trailing space.

[unorthodoxgeek]

that's wrong on multiple levels :( anyway, this means we either stop allowing this horribleness, or we declare this ticket as unsolvable. it's one or the other.

[isratrade]

The answer to the issue should be that forward slashes are not allowed as identifiers, so this
/ptables/AutoYaST%20/dev/sda/edit
is not acceptable. If the UI caused this, it needs to be fixed by adding the id- plus url encode the slashes which are just part of the name.

[domcleal]

I agree that names in the API is problematic, and we can't claim to make it work 100%. Still, it is what it is for now, and this needs fixing.

Merged as 053c032 with friendly_id left in, thanks @unorthodoxgeek.