search

composite-primary-keys / composite_primary_keys

Composite Primary Keys support for Active Record
1.03k stars 350 forks source link

Convert attribute names to strings before checking for them in has_attribute? #501

Closed DanielDe closed 5 years ago

DanielDe commented 5 years ago

Hi there, we're running into a bug with Simple Form in which column names specified as symbols aren't being recognized as valid attributes on our models. I believe I've traced the problem down to the override of has_attribute? in this gem.

Rails's version of has_attribute? first converts the specified attribute name to a string with to_s, which is what was missing from this override.

Working from my fork with this patch has fixed the issue for us.

I wasn't able to run the tests (rake postgresql:build_database kept failing for me), but it looks like you folks have integrated CI so I'm hoping they'll run automatically with this PR.

And thanks for your work on this library!

codeodor commented 5 years ago

There was some discussion here about this: https://github.com/composite-primary-keys/composite_primary_keys/commit/846b5e86089f866556769658de53ff9d9f82ff52#r35312784

It notes that there were a lot of .to_s calls removed. Should all of them be reviewed?

Maybe Rails 6 beta had them missing when that commit was made and then added them before release?

DanielDe commented 5 years ago

Maybe Rails 6 beta had them missing when that commit was made and then added them before release?

I just poked through the Rails history and this doesn't seem to be the case. So I'm not sure why @cfis removed those calls to to_s.

It notes that there were a lot of .to_s calls removed.

I'm not seeing the PR that Javier was referring to. If you can point me in the right direction I'm happy to take a look.

codeodor commented 5 years ago

I don't think I saw @javierjulio follow up on that one. Tagging him in case I missed something, or if they decided to patch their app locally instead.

javierjulio commented 5 years ago

@codeodor I had a coworker express an interest in submitting a PR but they didn't follow up, sorry. This looks good @DanielDe, thanks! From the issue we ran into this is what we noticed. Once discovered it was pointed out that other to_s calls were removed but we are not sure if they are causing regressions. Just a heads up. Thanks again!

cfis commented 5 years ago

Sorry missed this conversation earlier. Can you update the pull request to show the correct overridden code also (its in the comment under # CPK).

DanielDe commented 5 years ago

Yep, updated @cfis!

cfis commented 5 years ago

Great - thanks for fixing.

javierjulio commented 5 years ago

Thanks everyone! ❤️

DanielDe commented 5 years ago

Hey @cfis, thanks for merging! Is there a process for cutting a new point release with this change?