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

[11.x] Add upsertUsing functionality #52405

Closed
wants to merge 7 commits into from

Conversation

isaacdew2
Copy link

Overview

This branch is based on an idea in the discussions - #46589

It adds an upsertUsing method which is similar to the insertUsing method and allows developers to upsert data using a subquery.

I've implemented it only for MySql for now.

Benefits

Really it's an extension of insertUsing bringing the power of upserts. The alternative currently would be to either write a raw statement or to load records into memory and use the upsert method to enter them into the new table. Obviously, keeping it in MySQL is much faster and this method makes that easier to do for devs.

My use case is syncing data from a staging table to the live table.

Breaking Changes

None! It's a new method and doesn't change any existing functionality!

Notes

One thing to note - I don't use the laravel_upsert_alias because it's difficult to implement with a subquery. I could nest the subquery like so:

insert into `table1` (`foo`) select * from (select `bar` from `table2` where `foreign_id` = ?) as laravel_upsert_alias on duplicate key update `foo` = `laravel_upsert_alias`.`foo`

However, this would result in an error since the column foo doesn't exist in the subquery. If anyone has a good solution for this let me know! Although I'm not sure the alias would provide any benefit in this scenario anyway.

@driesvints driesvints changed the title Add upsertUsing functionality for MySQL [11.x] Add upsertUsing functionality for MySQL Aug 9, 2024
@taylorotwell
Copy link
Member

I guess I'm not fundmentally opposed to this but would need to work for other databases as well I think.

@taylorotwell taylorotwell marked this pull request as draft August 19, 2024 03:06
@isaacdew2
Copy link
Author

That makes sense and I thought that might be the case! I'll work on adding it for the other databases that support upserts.

@isaacdew2 isaacdew2 changed the title [11.x] Add upsertUsing functionality for MySQL [11.x] Add upsertUsing functionality Aug 19, 2024
@isaacdew2
Copy link
Author

isaacdew2 commented Sep 5, 2024

@taylorotwell So SQLite warns about parsing ambiguity of on when doing an upsert using a subquery (the docs) but in my testing this is only an issue when nothing comes after a join in the subquery or that join has no on clause.

So this produces an error:

User::upsertUsing([
    'name',
    'email',
    'password'
], function ($query) {
    $query->selectRaw("'Test McTesterson' as name, email, password")
        ->from('users')
        // notice no join conditions
        ->join('posts', fn ($join) => $join);
}, ['email'], [
    'name'
]);

But this does not:

User::upsertUsing([
    'name',
    'email',
    'password'
], function ($query) {
    $query->selectRaw("'Test McTesterson' as name, email, password")
        ->from('users')
         // notice no join conditions
        ->join('posts', fn ($join) => $join)
        // but something comes between the join and the "on conflict" clause 
        ->orderBy('users.id', 'desc');
}, ['email'], [
    'name'
]);

This also does not produce errors:

User::upsertUsing([
    'name',
    'email',
    'password'
], function ($query) {
    $query->selectRaw("'Test McTesterson' as name, email, password")
        ->from('users')
         // notice there are join conditions
        ->join('posts', 'users.id', 'posts.user_id');
}, ['email'], [
    'name'
]);

So it seems that running into this issue would be quite rare and I'm not sure it should be the responsibility of the ORM to protect the developer from making this mistake. Not to mention that trying to automatically append a where clause to the subquery if there isn't one would be added complexity especially if just for SQLite. Thoughts? Do you think I should try to account for rare cases of parsing ambiguity or keep the straightforward approach?

@isaacdew2 isaacdew2 marked this pull request as ready for review September 20, 2024 23:11
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

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

Successfully merging this pull request may close these issues.

2 participants