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

Breaks first_or_create (conflict with Squeel) #176

Closed
CyborgMaster opened this issue Jan 4, 2014 · 15 comments
Closed

Breaks first_or_create (conflict with Squeel) #176

CyborgMaster opened this issue Jan 4, 2014 · 15 comments

Comments

@CyborgMaster
Copy link

I'll take time to give more detail tomorrow, but for now, I'm pretty sure composite-primary-keys is breaking ActiveRecord's first_or_create. I'm running ActiveRecord 4.0.2 and the latest composite-primary-keys (6.0.1)

I've got an empty class:

class Role < ActiveRecord::Base
end

and I run the command: Role.where(:name => 'practice').first_or_create

which generates the following sql:

  Role Load (0.3ms)  SELECT `roles`.* FROM `roles` WHERE `roles`.`name` = 'practice' ORDER BY `roles`.`id` ASC LIMIT 1
   (0.1ms)  BEGIN
  SQL (0.3ms)  INSERT INTO `roles` (`created_at`, `updated_at`) VALUES ('2014-01-04 06:54:02', '2014-01-04 06:54:02')
   (0.5ms)  COMMIT

So the find works, but not the create.

Side note: this works fine Role.new(:name => 'practice').save, generating the following sql:

INSERT INTO `roles` (`created_at`, `name`, `updated_at`) VALUES ('2014-01-04 06:54:16', 'practice', '2014-01-04 06:54:16')

Any ideas?

@CyborgMaster
Copy link
Author

Also, I should add that the only thing I have to do to make this work is remove composite_primary_keys from the Gemfile.

@CyborgMaster
Copy link
Author

I set up a new rails project so I could create a bare minimum example that reproduces the problem. Turns out it is a conflict between the composite_primary_keys gem and the squeel gem. This is my Gemfile that reproduces the problem

source 'https://rubygems.org'
ruby '1.9.3'
gem 'rails', ' 4.0.2'

gem 'mysql2'
gem 'squeel'
gem 'composite_primary_keys'

The only other thing I have in the project is a single empty Role model as follows:

class Role < ActiveRecord::Base
end

and this is the problem run in the rails console:

Role.where(name: 'test10').first_or_create
  Role Load (0.2ms)  SELECT `roles`.* FROM `roles` WHERE `roles`.`name` = 'test10' ORDER BY `roles`.`id` ASC LIMIT 1
   (0.1ms)  BEGIN
  SQL (0.2ms)  INSERT INTO `roles` (`created_at`, `updated_at`) VALUES ('2014-01-04 19:12:12', '2014-01-04 19:12:12')
   (0.3ms)  COMMIT

The problem goes away if I remove either the composite_primary_keys gem or squeel, so it's definitely a conflict between the two. I'll dig further.

@CyborgMaster
Copy link
Author

Update: two gems together somehow manage to clear scope_attributes that is used by first_or_create

@CyborgMaster
Copy link
Author

Update: it seems like it might be caused by both gems mucking with the where_values_hash

@CyborgMaster
Copy link
Author

I think I have found a workaround, but I'm out of time, so I'll post it later.

@CyborgMaster
Copy link
Author

Ok, so I've got a workaround for now. It turns out that composite_primary_keys does indeed conflict with squeel.

They both replace the ActiveRecord method where_values_hash (activerecord-4.0.1/lib/active_record/relation.rb:501), with a slightly modified version that does what they need. Since they both replace it (and don't use super), this is a hardcore incompatibility. These two gems cannot be used together.

However, it turns out that squeel does everything that composite_primary_keys needs plus a little bit more. composite_primary_keys needs to find equality nodes inside of 'and' nodes (composite_primary_keys-6.0.1/lib/composite_primary_keys/relation.rb:49). Squeel also looks inside 'and' nodes (squeel-1.1.1/lib/squeel/adapters/active_record/relation_extensions.rb:398), but then in addition looks inside 'grouping' nodes, along with translating the squeel DSL. This means if we were to ignore the composite_primary_keys version and only use the squeel method, it would work.

Unfortunately that is impossible without patching. Squeel patches ActiveRecord's Relation class to add it's custom method. Composite_primary_keys also patches the Relation class, but only conditionally adds it's where_values_hash method on object initialization (composite_primary_keys-6.0.1/lib/composite_primary_keys/relation.rb:64). If it were to "statically" patch the class at load time, I could influence which version was used by regulating the include order in my Gemfile (last one wins). However, because composite_primary_keys adds the method at object initialization, it always wins and I have no way to force the squeel implimentation to be used.

For now, I am using a forked version of composite_primary_keys (https://github.com/CyborgMaster/composite_primary_keys), that simply makes the function add_cpk_where_values_hash a no-op. But I would like a better solution.

To the maintainers/authors: how can we add compatibility with squeel to this gem. Is there a way I can modify the patching of where_values_hash to do it at library load time instead of initialize time? Then I could do the Gemfile order solution above. Or is there a way to make composite_primary_keys detect squeel and modify it's behavior? There might be a better solution, but that's what I thought of off the top of my head.

I'll leave this issue open for now because what I have is only a hack work around, I would like to find a real, more permanent solution.

@jackdesert
Copy link

Any progress on this front? I am working on a project that already uses composite_primary_keys, and would like to add the squeel gem as well, but it's breaking occurrences of first_or_create.

@CyborgMaster
Copy link
Author

I haven't heard anything. I wish I had, but for now you are welcome to use my fork.

@cfis
Copy link
Contributor

cfis commented Mar 28, 2014

Want to submit a patch?

@CyborgMaster
Copy link
Author

I would be happy to, the only problem, as I said in my last big comment is I don't know the best way to solve this problem. To quote myself:

For now, I am using a forked version of composite_primary_keys (https://github.com/CyborgMaster/composite_primary_keys), that simply makes the function add_cpk_where_values_hash a no-op. But I would like a better solution.

To the maintainers/authors: how can we add compatibility with squeel to this gem. Is there a way I can modify the patching of where_values_hash to do it at library load time instead of initialize time? Then I could do the Gemfile order solution above. Or is there a way to make composite_primary_keys detect squeel and modify it's behavior? There might be a better solution, but that's what I thought of off the top of my head.

I'll leave this issue open for now because what I have is only a hack work around, I would like to find a real, more permanent solution.

@cfis
Copy link
Contributor

cfis commented Mar 30, 2014

Yeah, hard to say since I've never used squeel. I do notice though thatsqueel is no longer maintained, so maybe just stick with your workaround?

@CyborgMaster
Copy link
Author

I'm sure someone will pick up squeel, it's too good to let die (if no one else does, my company will have to, it's pretty mission critical to us).

Is there a way I can modify the patching of where_values_hash to do it at library load time instead of initialize time? Then I could fix it by making sure Squeel was included last in the Gemfile. It seem non-performant to patch this method on every object.

@cfis
Copy link
Contributor

cfis commented Apr 12, 2014

I don't remember why it was done the current way, would be nicer to do it just once like said. I'll leave this to you to explore further if you want, any time I have on CPK I think needs to focus on AR 4.1

@cfis
Copy link
Contributor

cfis commented Aug 7, 2014

Ok, I'm going to go ahead and close this.

@cfis cfis closed this as completed Aug 7, 2014
@CyborgMaster
Copy link
Author

You guys fixed this! Thanks!!! The pull request from @nhocki (#252) made the patching of where_values_hash happen only at load time. Now I can make this work simply by making sure squeel is after composite_primary_keys in my gemfile.

Can we get a release to RubyGems?

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

No branches or pull requests

3 participants