-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Created a hash decoder for hashed ids within Rails params. #52
base: master
Are you sure you want to change the base?
Conversation
@dbastin just seeing if I understand this right (and that your memory is sharp!), so would this help in the case that I am passing in a form with a hashid reference to another record? i.e. Then would I expect for the following to work? # person params decodes the hashid for house_id
p = Person.new(person_params)
# house now correctly relates to the decoded house_id
p.house |
@krsyoung — Exactly right. This pull-request enables hashed ids coming in via request parameters from a form for all sorts of associations, including the one mentioned in your example. You'll need to explicitly decode the params like this though (seems from your code comment that you know this already)...
|
Amazing @dbastin! Will try it out and see if I can give it another vote / help for a merge! Very helpful (and nice looking code!) |
@dbastin I find this Pull request not compatible with idea of current hashid state. It bases on model layer and you make use of controller layer. I already tried decoding like that on model layer in work, it works fine. Soon I will send here example of how we can do it |
@Tengoot — Controller layer? I don't follow. The pull request is decoding a Hash that describes a Model. Nothing to do with Controllers. In my mind, this process is just one part of the larger ORM process that is triggered on each request. Furthermore, ORM conceptually sits within the Model layer. I could be looking at it wrong. Anyway, I'd love to see an example of how you perform decoding for nested hashed id params without recursively parsing the params hash like I have done. It would be great to see a better way to do this! |
Repaired conflict. Merged. @Tengoot - Still waiting to see the tl;dr - hashid-rails is great at handling hashed ids in URLs but does not handle hashed ids within form params. This pull request adds that functionality. |
@dbastin tested it on local and work in all cases I have, except with polymorphic associations that I got an error like this I just took your code and used it on a service, added a change to make those on associations that are not polymorphic at the moment, as a side comment, if the value is empty, why not returning directly the value and not try to calculate it? I can be trying to create a new nested resource and it goes on all the process even if attr_id is empty |
Hi Folks
I've created a class that decodes ids for inbound Rails param hashes. You use it like this...
So, in addition to having urls like
https://www.example.com/posts/e45Frd
behaving likehttps://www.example.com/posts/12345
you now also get the inbound hashids that are contained within form params converted to their real ids as well.I'm a little surprised that something like this does not already exist within any id obfuscating gem I've tried, so a part of me thinks I'm perhaps missing something and there really is no need for any of this code? Or perhaps no-one is obfuscating ids within form params? No idea.
At any rate, this code is test driven, 100% covered, and complies with your Rubocop rules (except for one method Hashid::Rails::Decoder#decode that I tried splitting up but decided to leave for clarity).
I also fixed up a few existing minor Rubocop failures.
You'll see I've had to make the schema a little more complicated owing to the fact I needed to demonstrate different object association types. I think it remains pretty clear though.
P.S. I've made an earlier pull request but that failed a Travis build using Ruby 2.3.0, so I fixed it and force pushed a squashed single commit. This prevents the old pull request from being reopened. Apologies if that is an issue.