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

Allow to specify custom attributes to update on conflict for upsert_all method #35631

Closed
wants to merge 2 commits into from

Conversation

@simi
Copy link
Contributor

@simi simi commented Mar 15, 2019

This allows to specify attributes to update on conflict. It opens door to handling timestamps updates manually in reasonable transparent way.

now = Time.now
row = { slug: 'new-title', post: 'New post', created_at: now, updated_at: now }
Post.upsert(row1, unique_by: { columns: [:slug]}, update: [:post, :updated_at])

In combination with defaults (I'll try to add this in different PR) mentioned at #35493 (comment) it can provide easy and transparent way to handle timestamps. Anyway I think it should not be default behaviour since methods like update_all doesn't handle this on its own as well.

WDYT @boblail @dhh @kaspth

references #35493 #35494

I'll update documentation once approved.

@simi simi force-pushed the upsert_all_updatable_columns branch from 5ddb666 to ef250ed Mar 15, 2019
@simi simi force-pushed the upsert_all_updatable_columns branch 2 times, most recently from 5edfee7 to c529390 Mar 15, 2019
@simi
Copy link
Contributor Author

@simi simi commented Mar 16, 2019

@boblail ping

activerecord/test/cases/insert_all_test.rb Outdated Show resolved Hide resolved
activerecord/test/cases/insert_all_test.rb Show resolved Hide resolved
activerecord/test/cases/insert_all_test.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/insert_all.rb Show resolved Hide resolved
activerecord/lib/active_record/persistence.rb Outdated Show resolved Hide resolved
@simi
Copy link
Contributor Author

@simi simi commented Mar 16, 2019

@boblail 👀 requested changes addressed.

@boblail
Copy link
Contributor

@boblail boblail commented Mar 16, 2019

LGTM!

I'm curious what Rails Members think of the feature and of unifying with #35636!

@simi
Copy link
Contributor Author

@simi simi commented Mar 16, 2019

I think update option can work with array of columns or with SQL string (similar to joins for example).

@simi simi force-pushed the upsert_all_updatable_columns branch from 52ec6f3 to 2c131e9 Mar 16, 2019
@kaspth
Copy link
Member

@kaspth kaspth commented Mar 31, 2019

I'm not liking the extra API exposed here, doesn't match Rails' aesthetic. Let's see where #35636 goes.

@kaspth kaspth closed this Mar 31, 2019
@simi
Copy link
Contributor Author

@simi simi commented Apr 1, 2019

@kaspth is it only about the API or doesn't you like option to specify which columns to use on update without need of using SQL fragment? In my eyes that is actually really handy and powerful. I can wait until #35636 is resolved and I'll try to rethink this agian. Anyway thanks for your time you spend on this already.

@kaspth
Copy link
Member

@kaspth kaspth commented Apr 1, 2019

I want to do something closer to only/with that I alluded to in #35686.

@simi
Copy link
Contributor Author

@simi simi commented Apr 1, 2019

It is hard to follow for me @kaspth since #35686 was closed.

@kaspth
Copy link
Member

@kaspth kaspth commented Apr 1, 2019

I meant this part from the description:

Book.upsert_all rows, only: %i( id name author_id )
Book.upsert_all rows, with: "status = GREATEST(books.status, excluded.status)"

Though I'm thinking of putting a halt on more changes for insert_all for Rails 6. I want the relation support, but that's probably it for now for me.

@simi
Copy link
Contributor Author

@simi simi commented Apr 1, 2019

👍 Ahh, ok. I'll focus on relation support for now. We can reopen and rethink specifying custom attributes to update on conflict once 6 is released and get more time unless next release.

@shaicoleman
Copy link

@shaicoleman shaicoleman commented Sep 25, 2020

@simi , @kaspth , can you look into reopening this?

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

Successfully merging this pull request may close these issues.

None yet

4 participants