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

Add a basic upsert method #16

Merged
merged 12 commits into from
Nov 14, 2023
Merged

Conversation

binarygary
Copy link
Contributor

This is a simplified implementation of #8. There wasn't any discussion in that issue about whether this is a desired feature or not, but I thought I'd give it a try as an excuse play with slic and this library.

In #8 parameter 1 suggests supporting multiple inserts in a nested array. This does not appear to be available in the CRUD trait or in this library.
The second is the columns to match against.
The third param is the columns to update on match. I've opted to omit this and update all columns when a match is found.

Usage:

// Would result in a new row - Oakland to San Diego for 100.
DB::table('table_name')
    ->upsert(
        ['departure' => 'Oakland', 'destination' => 'San Diego', 'price' => '100'] , 
        ['departure','destination']
    );


// Would update existing row - Oakland to San Diego for 99.
DB::table('table_name')
    ->upsert(
        ['departure' => 'Oakland', 'destination' => 'San Diego', 'price' => '99'] , 
        ['departure','destination'] 
    );

Test coverage has been added for:

  • Adding a new row to the DB using upsert
  • Upserting an existing row with 1 matching column.
  • Upserting an existing row with 2 matching columns.

@nikolaystrikhar
Copy link
Contributor

Elegantly implemented! I left a couple of comments about some minor issues :-)

I wonder what @JasonTheAdams and @borkweb think about this approach.

@binarygary
Copy link
Contributor Author

binarygary commented Nov 13, 2023

@nikolaystrikhar Thanks for the quick review!
I've made the requested updates. I did guess on the since tag.

@nikolaystrikhar
Copy link
Contributor

nikolaystrikhar commented Nov 14, 2023

@binarygary Since I'm the only one who checked it, I decided to add more comments about small stuff, but so useful when it comes to many brands :-) And one format fix. Didn't want to bother you :D

And I'm not sure if I can approve tbh, I will ping Matt B about it, so he checks.

Regarding the since tag, I wonder how we are going to do it in libraries, @borkweb maybe we want TBD like we are doing everywhere and then you can update it when you release a new version?

Co-authored-by: Nikolay Strikhar <nikolaystrikhar@gmail.com>
Copy link
Member

@borkweb borkweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@binarygary - thank you for the PR! This is a good addition! 👯

src/DB/QueryBuilder/Concerns/CRUD.php Outdated Show resolved Hide resolved
src/DB/QueryBuilder/Concerns/CRUD.php Outdated Show resolved Hide resolved
src/DB/QueryBuilder/Concerns/CRUD.php Outdated Show resolved Hide resolved
@borkweb borkweb merged commit 9c76724 into stellarwp:main Nov 14, 2023
@JasonTheAdams
Copy link

@binarygary @borkweb In the future we might consider optimizing this a bit more. Namely, when checking to see if the dataset already exists, we can create a modified query that is faster than the final update/insert query. Three things come to mind:

  1. Clone the current query so we don't modify the main query, then on the clone...
  2. Modify the select to be SELECT 1, since we don't actually need to grab the data to see if the row exists
  3. Set a LIMIT 1 since we only need to know if there's at least 1 row

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.

4 participants