Add locking clause to read in DatabaseQuery.Action

[gregcotten]

Hi! This adds SQL-backed-only support for using a read action with "FOR UPDATE" or "FOR SHARE" which will allow the user to query+mutate the returned model(s)+save back on the database without worrying about race conditions.

[gregcotten]

I'm sure this would break anything that assumes DatabaseQuery.Action.read doesn't have an associated value - not really sure what to do there.

[gregcotten]

@gwynne just checking in to see if this at all makes sense!

[anonymouz4]

@gregcotten, any update on this?

Would be ideal, if it had more options like "FOR NO KEY UPDATE;" to avoid deadlocking in some cases

[gregcotten]

@gregcotten, any update on this?

Would be ideal, if it had more options like "FOR NO KEY UPDATE;" to avoid deadlocking in some cases

As far as I know "FOR NO KEY UPDATE" and "FOR KEY SHARE" are specific to Postgres flavor of SQL only, so we'd have to dive a bit deeper for those.

[codecov-commenter]

Codecov Report

Merging #483 (6c55a23) into main (3c06388) will increase coverage by 2.20%. The diff coverage is 41.86%.

@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
+ Coverage   37.77%   39.97%   +2.20%     
==========================================
  Files         104       95       -9     
  Lines        5583     5193     -390     
==========================================
- Hits         2109     2076      -33     
+ Misses       3474     3117     -357     

Flag	Coverage Δ
unittests	39.97% <41.86%> (+2.20%)	:arrow_up:

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

Impacted Files	Coverage Δ
...rces/FluentKit/Concurrency/Model+Concurrency.swift	25.00% <0.00%> (ø)
Sources/FluentKit/Database/Database.swift	76.00% <0.00%> (ø)
Sources/FluentKit/Properties/Children.swift	13.04% <0.00%> (ø)
Sources/FluentKit/Properties/OptionalChild.swift	14.40% <0.00%> (ø)
Sources/FluentKit/Properties/OptionalParent.swift	17.44% <0.00%> (ø)
Sources/FluentKit/Properties/Parent.swift	17.44% <0.00%> (ø)
Sources/FluentKit/Properties/Siblings.swift	10.44% <0.00%> (ø)
...luentKit/Query/Database/DatabaseQuery+Action.swift	0.00% <0.00%> (ø)
Sources/FluentKit/Model/Model.swift	30.00% <66.66%> (ø)
Sources/FluentKit/Query/Builder/QueryBuilder.swift	70.16% <100.00%> (ø)
... and 11 more

[gregcotten]

A thought: any relations that are eager loaded in the query won't inherit the same locking clause as the QueryBuilder - should we consider how to make that work?

[gwynne]

I've been hesitant to lift support for locking clauses from SQLKit to FluentKit, given the lack of direct control over various behaviors and additional queries Fluent performs on its own (such as adding soft-deletion conditionals to queries and running eager load lookups for relations), which would tend to interact unexpectedly with locking clauses. I'm also not really happy with the number of changes to existing API that are involved, even if they are purely additive 😕.

[gregcotten]

@gwynne

Yea, not really sure what to do here. I've moved on from the project since this PR.

I do think database locking needs to be addressed by this community. Any examples of modifying data "non-atomically" using FluentKit/Vapor should come with a big "here be dragons" disclaimer.

I can close this PR and open an issue, if that makes more sense.

[gregcotten]

@gwynne would be interesting to see what you're hoping to do here: https://github.com/vapor/fluent-kit/issues/167#issuecomment-977920472

[gwynne]

@gregcotten It is already possible to use the locking clauses by dropping down to SQLKit and issuing queries from there. (I'm planning when I get a chance to upstream a number of utility and helper methods which make SQLKit queries much easier to build and more compatible with Fluent models.)

As far as #167 goes, there's nothing in particular blocking that support from being added, I must admit I just haven't gotten around to it yet; I wanted to do it properly, with working support for all the database drivers and correct integration at the SQLKit layer, which is slightly more complicated than the trivial way of doing it (which basically goes something like database.withConnection { conn in (conn as! MySQLDatabase).simpleQuery("SET TRANSACTION ISOLATION LEVEL READ COMMITTED").flatMap { conn.transaction { /* your transactional logic here */ } } } (and similar for Postgres etc.) - which works, but is obviously not what one would call robust 😅.)

[gregcotten]

@gregcotten It is already possible to use the locking clauses by dropping down to SQLKit and issuing queries from there. (I'm planning when I get a chance to upstream a number of utility and helper methods which make SQLKit queries much easier to build and more compatible with Fluent models.)

@gwynne Of course! That's what we were doing, but I much prefer dealing with Fluent and it would be nice to have a "it just works" high-level locking implementation. I don't really know (and am scared to learn) what people are doing without it...