jesjos / active_record_upsert

Upsert for Rails 5 / Active Record 5
MIT License
207 stars 51 forks source link

Primary key not returned when upserting specific attributes #79

Closed abinoda closed 6 years ago

abinoda commented 6 years ago

This issue was introduced when I upgraded from 0.9.1 to 0.9.3. See the example belowuser.id is nil in the final object. Again, this code works fine in v0.9.1 but not 0.9.3.

class User < ApplicationRecord
  upsert_keys [:my_id]
end

user = User.new(
  name:   name,
  email:  email,
  my_id:  my_id
)

# Overwrite email field but not name
user.upsert!(attributes: [:name])

user.id.nil? # => true
jesjos commented 6 years ago

Hi! Thanks for reporting this!

I'm trying to reproduce this using a unit test, but it's going nowhere. See this commit: https://github.com/jesjos/active_record_upsert/commit/3c2821354309c2e04310bc0b7d65f2f9a4064816

I expected it to (mis)behave the same way, but I can't reproduce the behavior.

Maybe there's something else about your User record or how the db table was setup that causes the problem? Or maybe there's some kind of version incompatilibility with Rails?

abinoda commented 6 years ago

@jesjos Just left a comment https://github.com/jesjos/active_record_upsert/commit/3c2821354309c2e04310bc0b7d65f2f9a4064816#r30519950

abinoda commented 6 years ago

@jesjos I'll pull down the code and check out that test. Will get back to you.

abinoda commented 6 years ago

@jesjos Ok, so I wasn't able to get that test to fail either, which got me curious. I tried upgrading my app to Rails 5.2 from 5.1.6 and the issue went away. So this issue is specific to Rails 5.1.

Now I'm trying to run the test suite for active_record_upsert with the Rails 5.1 Gemfile to try and isolate what the issue was but I'm getting tons of test failures.

What are your thoughts?

jesjos commented 6 years ago

I’m not in a position to try anything out right now, but I can try later today. It seems the travis builds aren’t building the correct rails versions. I need to take a look at that as well. I’ll get back to you.

jesjos commented 6 years ago

Can confirm our travis build matrix has not been working as intended, all tests have run on rails 5.2 :(

Seeing a lot of errors. Guess we'll have to start fixing them.

abinoda commented 6 years ago

@jesjos Gotcha. Yeah I ran manually by changing Gemfile contents to eval_gemfile "#{__dir__}/Gemfile.rails-5-1", running bundle, then running the tests. Glad we discovered this though!

jesjos commented 6 years ago

Yeah, the way I do it is I use an environment variable:

$ BUNDLE_GEMFILE=Gemfile.rails-5-1 bundle install
$ BUNDLE_GEMFILE=Gemfile.rails-5-1 bundle exec rspec
jesjos commented 6 years ago

Working on this here: https://github.com/jesjos/active_record_upsert/issues/79

I've got most tests working, some left.

jesjos commented 6 years ago

Closed in #82

olleolleolle commented 6 years ago

Yay!

@jesjos Thanks for listening and building!

@abinoda Thanks so much for reporting with such clarity and detail! (Happy that your test suite helped this project, too!)

:hearts: :yellow_heart: :green_heart: :blue_heart: :purple_heart:

abinoda commented 6 years ago

@jesjos Thanks for resolving this! And thank you @jesjos @olleolleolle for maintaining this library. It's been critical for my project and is a much needed piece in the Ruby/Rails ecosystem.