vapor / fluent-kit

Swift ORM (queries, models, and relations) for NoSQL and SQL databases
MIT License
218 stars 116 forks source link

Upsert Support #488

Closed grahamburgsma closed 1 year ago

grahamburgsma commented 2 years ago

Adds basic support for upsert/conflict strategy. I'm looking for feedback on the API.

Addresses #50.

Query

try Thing.query(on: db)
    .set(\.$name, to: "First")
    .create(
        onConflict: \.$name,
        strategy: .update {
            $0.set(\.$name, to: "Last")
        }
    )

Model

try Thing(name: "First")
  .create(
    onConflict: \.$name,
    .update {
        $0.set(\.$name, to: "Last")
    },
    on: db
)

When used on a Model, the conflict fields are returned by the query and saved back to the model. So if there is a conflict, the model will have all the correct values.

codecov-commenter commented 2 years ago

Codecov Report

Merging #488 (c41e94f) into main (9c7fbdc) will increase coverage by 2.76%. The diff coverage is 78.04%.

:exclamation: Current head c41e94f differs from pull request most recent head 0b223ff. Consider uploading reports for the commit 0b223ff to get more accurate results

@@            Coverage Diff             @@
##             main     #488      +/-   ##
==========================================
+ Coverage   37.80%   40.57%   +2.76%     
==========================================
  Files         104       97       -7     
  Lines        5579     5245     -334     
==========================================
+ Hits         2109     2128      +19     
+ Misses       3470     3117     -353     
Flag Coverage Ξ”
unittests 40.57% <78.04%> (+2.76%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
...urces/FluentKit/Query/Database/DatabaseQuery.swift 29.26% <0.00%> (-2.32%) :arrow_down:
...luentKit/Query/Database/DatabaseQuery+Upsert.swift 54.54% <54.54%> (ΓΈ)
Sources/FluentKit/Model/Model+CRUD.swift 54.92% <100.00%> (+2.22%) :arrow_up:
.../FluentKit/Query/Builder/QueryBuilder+Upsert.swift 100.00% <100.00%> (ΓΈ)
Sources/FluentSQL/SQLQueryConverter.swift 66.74% <100.00%> (+1.18%) :arrow_up:
Sources/FluentBenchmark/SolarSystem/Governor.swift
...ources/FluentBenchmark/SolarSystem/PlanetTag.swift
Sources/FluentBenchmark/FluentBenchmarker.swift
Sources/FluentBenchmark/SolarSystem/Star.swift
Sources/FluentBenchmark/SolarSystem/Tag.swift
... and 5 more
wibed commented 2 years ago

is this only for the mongo driver?

grahamburgsma commented 2 years ago

is this only for the mongo driver?

@wibed I've been testing with Postgres so far. It should work for all that Fluent/SQLKit supports. Each driver needs changes to enable it though, that's why it looks like all the tests are failing.

ghost commented 1 year ago

Any updates on this? Or #50?

grahamburgsma commented 1 year ago

Any updates on this?

@josephxanderson I never got any feedback on it or much interest in it, so I've kinda abandoned it. I imagine Fluent 5 will address this feature.

0xTim commented 1 year ago

cc @gwynne

gwynne commented 1 year ago

@grahamburgsma I must apologize; I quite honestly forgot to return to this 😰. While the PR is obviously a bit out of date at this point, it was very well written, and I have no significant complaints about the proffered API. If you're willing to bring the PR up to date with main, I'd gladly accept it now.

The only minor concern I have with the API as presented is there doesn't appear to be any way to get the effect of SQLKit's SQLConflictUpdateBuilder.set(excludedValueOf:); I would say it could be left to add later, but Fluent's design has a way of making it difficult to handle features in such an additive fashion without bending or breaking semver rules πŸ˜•. I definitely don't consider it a requirement for merging this, but if you have any thoughts on how to provide the additional functionality at the Fluent layer (I admittedly don't, at least not offhand πŸ˜…), I'd say go for it!

(Incidentally, you're correct that Fluent 5 will have support for this built in, but just the same, Fluent 4 can still very much benefit from having this available.)

grahamburgsma commented 1 year ago

Sorry for the late reply, thanks for looking it over @gwynne!

Here's where I'm at: I did update it with main and made some changes to get it back into a working state and have it working with Postgres. However when I tested against SQLite my changes had broken other things. I tried getting both to work but fixing one would break the other and still haven't tested the other DBs. Seems there are just too many differences between the inner workings of the different DB drivers.

If anyone is interested in taking over this work feel free to, I'll push my latest changes. For now, I don't have time to invest into this so I will close this PR. We'll have to put all our chips on Gwynne and Fluent 5 for this one πŸ˜†