Major overhaul of the entire SQLKit package #172
Conversation
gwynne
added
bug
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
===========================================
+ Coverage 0 100.00% +100.00%
===========================================
Files 0 97 +97
Lines 0 2603 +2603
===========================================
+ Hits 0 2603 +2603
|
There was a problem hiding this comment.
Overall I think this LGTM. I have a few comments about API breakages, but I'm not sure if they're true or not.
Great to see lots of extra tests and documentation coverage. I will say this PR is very hard to review due to the sheer size and changes involved, it probably would have been better to break it up into separate PRs as there's definitely places where things could be missed
gwynne
force-pushed
the
overhaul
branch
2 times, most recently
from
e8766e5
to
a02ebb0
Compare
…" version, numerous other SQLQueryString cleanups
gwynne
force-pushed
the
overhaul
branch
2 times, most recently
from
ab6d9e0
to
7c76908
Compare
…ions, add SQLConflictResolutionStrategy API, fix SQLCreateIndex and SQLCreateTrigger serialization
…m code coverage. Reach 100% test coverage
…, for example, safely implementing transactions. Make SQLReportedDatabaseVersion slightly less awkward. Add test coverage.
…d boost in unusual scenarios.
… the primary key column constraint algorithm and add additional coverage to tests for non-default scenarios.
… codable SQLExpressions, fix un-dropped enum type when dialect uses type name enums, maintain test coverage.
…them. These are not intended for users. Rename FakeSendable->FakeSendableCodable, make it slightly more capable, include it in the SPI
| self.sql = sql | ||
| self.binds = binds | ||
| } | ||
|
|
||
| @inlinable | ||
| public func serialize(to serializer: inout SQLSerializer) { | ||
| serializer.write(self.sql) | ||
| serializer.binds += self.binds |
There was a problem hiding this comment.
Just as a note, this removal makes this a (pretty big) breaking change. I'm not sure there's anything to be done about it now, since the release was, I believe, quite a while ago. But just a note for the future
There was a problem hiding this comment.
It was a very much deliberate breakage. This particular "functionality" of SQLRaw was a misfeature accidentally left in the code during Tanner's work on Vapor 4; it should never have been present to begin with. I couldn't remove the binds property itself without causing source breakage, but I could at least discourage its usage.
There was a problem hiding this comment.
That being said...
If you are a user whose code was broken by this change, I'm not dead-set against undoing it, even this long afterwards; putting it back would be a purely additive change. The binds property will be gone anyway in Fluent 5, so it doesn't matter as much for Fluent 4 at this point.
There was a problem hiding this comment.
I don't personally need it (I only used it in two places, and I figured out the \(bind: bindVal) thing). I just think it's unexpected for a minor version bump to remove significant (though apparently unintentional) runtime functionality with only a compile-time warning as an indication
Idk if this project uses Semver. If not, then the expectation that that wouldn't happen is my fault, and you should just ignore me :)
There was a problem hiding this comment.
It does, but I made an executive decision to sidestep the rules, since the functionality was never intended to exist in the first place. You're the first to notice (or at least, the first to say something about it), if truth be told 😅.
There was a problem hiding this comment.
Just to nitpick 😅 - SemVer definitely allows breaking behaviour changes, otherwise you'd never be able to fix bugs as that's a change in behaviour. SemVer is purely about API changes, and in Swift's case, whether something still compiles, not if it behaves the same way (not saying that's any comfort for having something major changed and we tend to try and avoid that in most places)
This will be the last release of SQLKit version 3.
A hopefully complete (but probably not) list of significant changes:
ExistentialAny-compliantSendable-complaint (zero concurrency warnings).SQLDatabaseReportedVersionis nowEquatableandComparable, as it ought to have been from the start.asyncversions of various APIs.SQLQueryEncoderandSQLRowDecoderhave been massively overhauled; both are now considerably more flexible and less restrictive.SQLBenchmarkis nowasync.assert()s andprint()s are now consistently routed through the database's logger instead, and less noisy logging is done.SQLCreateTriggerare now correctly implemented.SQLIdentifierandSQLLiteral.stringnow automatically escape the appropriate quote characters when serializing.SQLBetween(x BETWEEN y AND z),SQLQualifiedTable(schema.table),SQLSubqueryandSQLSubqueryBuilder,SQLUnqualifiedColumnListBuilder, andSQLAliasedColumnListBuilder.SQLPredicateBuilderandSQLSecondaryPredicateBuildernow provide 1-to-1 corresponding APIs for all four variants ("and where", "and having", "or where", "or having").SQLInsertandSQLInsertBuildernow support theINSERT ... SELECTsyntax.SQLQueryFetchergained a number of convenience APIs for decoding models and single columns. Several builders also gained convenience methods for encoding or decoding of models. (Note that "models" in this usage does NOT refer to Fluent'sModelprotocol, but rather to anyCodablestructure.)SQLDropBehavioris now used by all builders that support the modifier and respects the current dialect properly.SQLCreateIndexnow supports index predicates.SQLDistinctnow serializes correctly.SQLDatabasegained awithSession(_:)API which, when implemented correctly by drivers, allows implementing transactions safely using pure SQLKit (no need for Fluent-level or driver-level intervention).SQLColumnConstraintAlgorithm.primaryKeyno longer emits incorrect SQL if the active dialect usesNULLas itsDEFAULTliteral.