Skip to content
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

Gemfile order with composite_primary_keys gem #396

Closed
davetapley opened this issue Apr 3, 2017 · 8 comments
Closed

Gemfile order with composite_primary_keys gem #396

davetapley opened this issue Apr 3, 2017 · 8 comments
Labels

Comments

@davetapley
Copy link

This Gemfile snippet:

gem 'activerecord-import', '~> 0.17'
gem 'composite_primary_keys', '~> 8.x'

Causes:

undefined method `import' for #<Class:0x0055aba6b9b110>

However, if I flip the gem lines around:

gem 'composite_primary_keys', '~> 8.x'
gem 'activerecord-import', '~> 0.17'

Then import works correctly.

I don't know if this is expected behavior, but it'd be nice to put in the readme if it is.

This appears unrelated to #111.

@jkowens
Copy link
Collaborator

jkowens commented Apr 3, 2017

This wasn't something I expected, but looking through the composite_primary_keys source code it looks like it monkey patches the ActiveRecord::Base.establish_connection method as well (at least version 8.x does). My guess is that when it's loaded after activerecord-import it loses our method definition.

It looks like starting with composite_primary_keys 9.0.5 they changed the way they handle database connections, so it might not be an issue for projects using ActiveRecord 5. It would be nice if this is something that could be fixed in the composite_primary_keys project, but I would happily accept a PR to document this behavior. Thanks!

@jkowens
Copy link
Collaborator

jkowens commented Apr 3, 2017

Hey @codeodor, I see you are a composite_primary_keys contributor. Could you confirm (or deny) my suspicions here? 😄

@codeodor
Copy link
Contributor

codeodor commented Apr 4, 2017

I don't recall the specifics on the establish connection, but yes there is definitely a lot of monkeypatching going on, and it would not surprise me if that is the issue here.

I used AR-import in the same project as CPK too, and I recall either having to drop it or rearrange the gemfile at some point.

I think a better way would be to alias the methods to keep the chain, and we try to do that in places, but in some places it's not feasible because the change CPK needs to make is in the middle of the method, as opposed to around it. I'm not sure if that's the case in version 8, but if we moved it in 9, that's good news!

I do try to review when we upgrade for AR to see if there's any we can remove. I haven't looked in to 5.1 yet, so I couldn't say whether what you noticed in 9 will remain the case for whatever version comes next to support AR 5.1.

@jkowens
Copy link
Collaborator

jkowens commented Apr 4, 2017

Thanks @codeodor! Ok sounds like some documentation for using the two gems together would be a good idea in this project's readme.

@dillonwelch
Copy link
Contributor

@dukedave I realize this is pretty old and I'm not sure if this is still a problem for you anymore. If it is, would you be able to provide a sample project that demonstrates the error?

@davetapley
Copy link
Author

@oniofchaos as far as I can remember it wasn't a problem so long as we kept the order flipped around in the Gemfile.

I'm afraid I no longer work on the project which used the gem.

@dillonwelch
Copy link
Contributor

That's fair. Do you think it makes sense to close the issue to keep the issue list clean?

@davetapley
Copy link
Author

Deal 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants