Redo operation upserts to use native ON CONFLICT DO UPDATE

[akadusei]

SQL-native upsert, using ON CONFLICT DO UPDATE

Closes #963
Fixes #917
Related #790
Related #874

A few caveats:

• Upserts now require actual unique columns. the previous implementation worked without them
• In PostgreSQL, NULL is not equal to NULL, so a nullable composite index may create the same row many times if at least one of the index columns value is null
• #updated? && #created? return false for upserts. (there's currently no way to tell them apart)
• I changed #new_record? to mean create? or upsert?. I'm not sure if that's what's expected

There may be edge cases I did not think about, so scrutinize this very closely.

[akadusei]

check_fomat job failure is from commit 3813ee3. Not sure why it's failing now

[jwoertink]

Look at that! 😄 I think this is probably the right way to go. I looked over it a little, but I'll need to really dig into how my apps use the upserts to see what all this will change.

I think the main thing I'm a little nervous about is having a SaveOperation that changes semantics internally based on how it's called externally.

Correct me if I'm wrong here, but it seems like now you may run into this?

class SaveThing < Thing::SaveOperation
  upsert_lookup_columns :key1, :key2
  permit_columns :key1, :key2

  after_commit(if: :new_record?) do |thing|
    SendNotificationEmail.new(thing).deliver_later
  end
end

# sends the email
SaveThing.create!(params)
# doesn't send the email
SaveThing.update!(thing, params)

# should send the email, but doesn't, and gives no
# warning to let you know until 9 hours of debugging production
SaveThing.upsert(key1: "doesn't exist", key2: "doesn't exist")

Or in other words, someone updates Avram, and expects the callbacks to work as they did, but instead of development-time catches indicating a refactor is required, it just goes out to production and quietly misbehaves.

Now, if that's not the case here, the this is extra great! But we will need to play around with it a bit more to make sure those cases are considered. Thanks for putting this together 🙌

[akadusei]

Your specific example should work OK. #new_record? is always true for upsert. The upsert implementation on main branch can distinguish between when a record is created vs updated, I believe. This one doesn't.

Flipping your example around, if key1 and key2 exists, upsert will find and update the record, but there is currently no way of knowing that it was an update rather than a create. So #new_record? would still return true and the email would be delivered.

[akadusei]

I am running into a bug where setting the operation outside .new call causes updates to nil to fail:

user = UserFactory.create &.name("test").nickname("sample").total_score(100)

operation = UpsertUserOperation.new(
  name: user.name,
  nickname: user.nickname,
  age: user.age,
  joined_at: user.joined_at
)

operation.total_score.value = nil # <= Set total_score to `nil` here

operation.upsert.should be_true
operation.upserted?.should be_true
operation.saved?.should be_true
operation.record.should_not be_nil

operation.record.try do |saved_user|
  saved_user.name.should eq("test")
  saved_user.total_score.should be_nil # <= This fails
end

This applies only when updating to nil. Any valid non-nil value passes.

From calling #compact! here >>

avram/src/avram/upsert.cr

Line 98 in 8a48211

upsert_values = attributes_to_hash(column_attributes).compact!

Attributes do not have a way of differentiating a nil value was actually received vs it is nil because nothing was received.