Modernize SQLite feature support (#100)

gwynne · web-flow · commit c07d53044727 · 2022-11-12T06:06:07.000-06:00
* Drop support for Swift &lt;5.5

* Same heavy CI update SQLiteNIO got

* Do a mostly visual update to SQLiteDialect, removing quite a lot of confusing whitespace and simplifying most of the content. Adds a `customDataType(for:)` override to handling requests for bigint types and switches from plain `?` for placeholders to SQLite's numbered `?NNNN` representation per SQLite docs recommendation.

* Tiny bit of tests cleanup

* Provide a type implementing SQLDatabaseReportedVersion, vend it from SQLiteDatabase, and use it to enable version-dependent RETURNING and UPSERT support for SQLite.

* Require updated SQLiteNIO that has the new APIs
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -1,69 +1,118 @@
 name: test
 on:
-  pull_request:
+  pull_request: { branches: ['*'] }
   push: { branches: [ main ] }
-defaults:
-  run:
-    shell: bash
+
+env:
+  LOG_LEVEL: debug
+  SWIFT_DETERMINISTIC_HASHING: 1
+
 jobs:
-  dependents:
+
+  codecov:
     runs-on: ubuntu-latest
-    container: swift:5.5-focal
-    strategy:
-      fail-fast: false
-      matrix:
-        dependent:
-          - fluent-sqlite-driver
+    container: swift:5.7-jammy
+    steps:
+      # N.B.: When we switch to embedded SQLite, these first two steps should be removed,
+      # and the version saved to the environment should come from the checked-out package.
+      - name: Install libsqlite3 dependency
+        run: apt-get -q update && apt-get -q install -y libsqlite3-dev
+      - name: Save SQLite version to env
+        run: |
+          echo SQLITE_VERSION="$(pkg-config --modversion sqlite3)" >> $GITHUB_ENV
+      - name: Check out package
+        uses: actions/checkout@v3
+      - name: Run local tests with coverage
+        run: swift test --enable-code-coverage
+      - name: Submit coverage report to Codecov.io
+        uses: vapor/swift-codecov-action@v0.2
+        with:
+          cc_flags: 'unittests'
+          cc_env_vars: 'SWIFT_VERSION,SWIFT_PLATFORM,RUNNER_OS,RUNNER_ARCH,SQLITE_VERSION'
+          cc_fail_ci_if_error: true
+          cc_verbose: true
+          cc_dry_run: false
+
+  # Check for API breakage versus main
+  api-breakage:
+    if: github.event_name == 'pull_request'
+    runs-on: ubuntu-latest
+    container: swift:5.7-jammy
     steps:
-      - name: Install dependencies
+      - name: Install libsqlite3 dependency
         run: apt-get -q update && apt-get -q install -y libsqlite3-dev
       - name: Check out package
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
         with:
-          path: package
-      - name: Check out dependent
-        uses: actions/checkout@v2
+          fetch-depth: 0
+      # https://github.com/actions/checkout/issues/766
+      - name: Mark the workspace as safe
+        run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
+      - name: Check for API breaking changes
+        run: swift package diagnose-api-breaking-changes origin/main
+
+  # Make sure downstream dependents still work
+  dependents-check:
+    if: github.event_name == 'pull_request'
+    runs-on: ubuntu-latest
+    container: swift:5.7-jammy
+    steps:
+      - name: Install libsqlite3 dependency
+        run: apt-get -q update && apt-get -q install -y libsqlite3-dev
+      - name: Check out package
+        uses: actions/checkout@v3
         with:
-          repository: vapor/${{ matrix.dependent }}
-          path: dependent
-          ref: main
-      - name: Use local package
-        run: swift package edit sqlite-kit --path ../package
-        working-directory: dependent
-      - name: Run tests with Thread Sanitizer
-        run: swift test --enable-test-discovery --sanitize=thread
-        working-directory: dependent
-  linux:
+          path: sqlite-kit
+      - name: Check out FluentKit driver
+        uses: actions/checkout@v3
+        with:
+          repository: vapor/fluent-sqlite-driver
+          path: fluent-sqlite-driver
+      - name: Tell dependents to use local checkout
+        run: 'swift package --package-path fluent-sqlite-driver edit sqlite-kit --path sqlite-kit'
+      - name: Run FluentSQLiteDriver tests with Thread Sanitizer
+        run: swift test --package-path fluent-sqlite-driver --sanitize=thread
+
+  # Run unit tests (Linux)
+  linux-unit:
+    if: github.event_name == 'pull_request'
     strategy:
       fail-fast: false
       matrix:
         runner:
-          - swift:5.2-focal
-          - swift:5.5-focal
-          - swiftlang/swift:nightly-main-focal
+          - swift:5.5-bionic
+          - swift:5.6-focal
+          - swift:5.7-jammy
+          - swiftlang/swift:nightly-main-jammy
     container: ${{ matrix.runner }}
     runs-on: ubuntu-latest
     steps:
-      - name: Install dependencies
+      - name: Install libsqlite3 dependency
         run: apt-get -q update && apt-get -q install -y libsqlite3-dev
       - name: Check out code
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
       - name: Run tests with Thread Sanitizer
-        run: swift test --enable-test-discovery --sanitize=thread
-  macOS:
+        run: swift test --sanitize=thread
+
+
+  # Run unit tests (macOS).
+  macos-unit:
+    if: github.event_name == 'pull_request'
     strategy:
       fail-fast: false
       matrix:
-        version:
-          - latest
+        macos:
+          - macos-11
+          - macos-12
+        xcode:
           - latest-stable
-    runs-on: macos-11
+    runs-on: ${{ matrix.macos }}
     steps:
       - name: Select latest available Xcode
         uses: maxim-lobanov/setup-xcode@v1
         with: 
-          xcode-version: ${{ matrix.version }}
+          xcode-version: ${{ matrix.xcode }}
       - name: Check out code
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
       - name: Run tests with Thread Sanitizer
-        run: swift test --enable-test-discovery --sanitize=thread
+        run: swift test --sanitize=thread
diff --git a/Package.swift b/Package.swift
@@ -1,4 +1,4 @@
-// swift-tools-version:5.2
+// swift-tools-version:5.5
 import PackageDescription
 
 let package = Package(
@@ -11,9 +11,9 @@ let package = Package(
         .library(name: "SQLiteKit", targets: ["SQLiteKit"]),
     ],
     dependencies: [
-        .package(url: "https://github.com/vapor/sqlite-nio.git", from: "1.0.0"),
-        .package(url: "https://github.com/vapor/sql-kit.git", from: "3.16.0"),
-        .package(url: "https://github.com/vapor/async-kit.git", from: "1.0.0"),
+        .package(url: "https://github.com/vapor/sqlite-nio.git", from: "1.2.0"),
+        .package(url: "https://github.com/vapor/sql-kit.git", from: "3.19.0"),
+        .package(url: "https://github.com/vapor/async-kit.git", from: "1.14.0"),
     ],
     targets: [
         .target(name: "SQLiteKit", dependencies: [
diff --git a/Sources/SQLiteKit/SQLiteConnection+SQLKit.swift b/Sources/SQLiteKit/SQLiteConnection+SQLKit.swift
@@ -4,15 +4,93 @@ extension SQLiteDatabase {
     }
 }
 
+internal struct _SQLiteDatabaseVersion: SQLDatabaseReportedVersion {
+    /// The numeric value of the version. The format of the value is the one described in
+    /// https://sqlite.org/c3ref/c_source_id.html for the `SQLITE_VERSION_NUMBER` constant.
+    let intValue: Int
+    
+    /// The string representation of the version. The string is formatted according to the description in
+    /// https://sqlite.org/c3ref/c_source_id.html for the `SQLITE_VERSION` constant.
+    ///
+    /// This value is not used for equality or ordering comparisons; it is really only useful as a display value. We
+    /// maintain a stored property for it here rather than always generating it as-needed from the numeric value so
+    /// that we don't accidentally drop any additional information a particular library version might contain.
+    ///
+    /// - Note: The string value should always represent the same version as the numeric value. This requirement is
+    ///   asserted in debug builds, but not otherwise enforced.
+    let stringValue: String
+    
+    /// Separates a numeric value into individual components and returns them.
+    static func components(of intValue: Int) -> (major: Int, minor: Int, patch: Int) {
+        let major = intValue / 1_000_000,
+            minor = (intValue - major * 1_000_000) / 1_000,
+            patch = intValue - major * 1_000_000 - minor * 1_000
+        return (major: major, minor: minor, patch: patch)
+    }
+    
+    /// Get the version value representing the runtime version of the SQLite3 library in use.
+    static var runtimeVersion: _SQLiteDatabaseVersion {
+        self.init(intValue: Int(SQLiteConnection.libraryVersion()), stringValue: SQLiteConnection.libraryVersionString())
+    }
+    
+    /// Build a version value from individual components and synthesize the approiate string value.
+    init(major: Int, minor: Int, patch: Int) {
+        self.init(intValue: major * 1_000_000 + minor * 1_000 + patch)
+    }
+    
+    /// Designated initializer. Build a version value from the combined numeric value and a corresponding string value.
+    /// If the string value is omitted, it is synthesized
+    init(intValue: Int, stringValue: String? = nil) {
+        let components = Self.components(of: intValue)
+
+        self.intValue = intValue
+        if let stringValue = stringValue {
+            assert(stringValue.hasPrefix("\(components.major).\(components.minor).\(components.patch)"), "SQLite version string '\(stringValue)' must match numeric version '\(intValue)'")
+            self.stringValue = stringValue
+        } else {
+            self.stringValue = "\(components.major).\(components.major).\(components.patch)"
+        }
+    }
+
+    /// The major version number. This is likely to be 3 for a long time to come yet.
+    var majorVersion: Int { Self.components(of: self.intValue).major }
+    
+    /// The minor version number.
+    var minorVersion: Int { Self.components(of: self.intValue).minor }
+    
+    /// The patch version number.
+    var patchVersion: Int { Self.components(of: self.intValue).patch }
+
+    /// See ``SQLDatabaseReportedVersion/isEqual(to:)``.
+    func isEqual(to otherVersion: SQLDatabaseReportedVersion) -> Bool {
+        (otherVersion as? _SQLiteDatabaseVersion).map { $0.intValue == self.intValue } ?? false
+    }
+    
+    /// See ``SQLDatabaseReportedVersion/isOlder(than:)``.
+    func isOlder(than otherVersion: SQLDatabaseReportedVersion) -> Bool {
+        (otherVersion as? _SQLiteDatabaseVersion).map {
+            (self.majorVersion < $0.majorVersion ? true :
+            (self.majorVersion > $0.majorVersion ? false :
+            (self.minorVersion < $0.minorVersion ? true :
+            (self.minorVersion > $0.minorVersion ? false :
+            (self.patchVersion < $0.patchVersion ? true : false)))))
+        } ?? false
+    }
+}
+
 private struct _SQLiteSQLDatabase: SQLDatabase {
     let database: SQLiteDatabase
     
     var eventLoop: EventLoop {
-        return self.database.eventLoop
+        self.database.eventLoop
+    }
+    
+    var version: SQLDatabaseReportedVersion? {
+        _SQLiteDatabaseVersion.runtimeVersion
     }
     
     var logger: Logger {
-        return self.database.logger
+        self.database.logger
     }
     
     var dialect: SQLDialect {
diff --git a/Sources/SQLiteKit/SQLiteDialect.swift b/Sources/SQLiteKit/SQLiteDialect.swift
@@ -1,58 +1,42 @@
+import SQLKit
+
+/// The ``SQLDialect`` defintions for SQLite.
+///
+/// - Note: There is only ever one SQLite library in use by SQLiteNIO in any given process (even if there are
+///   other versions of the library being used by other things). As such, there is no need for the dialect to
+///   concern itself with what version the connection using it "connected" to - it can always just look up the
+///   global "runtime version".
 public struct SQLiteDialect: SQLDialect {
-    public var name: String {
-        "sqlite"
-    }
+    public var name: String { "sqlite" }
     
-    public var identifierQuote: SQLExpression {
-        return SQLRaw("\"")
-    }
-
-    public var literalStringQuote: SQLExpression {
-        return SQLRaw("'")
-    }
-
-    public var autoIncrementClause: SQLExpression {
-        return SQLRaw("AUTOINCREMENT")
-    }
-
-    public func bindPlaceholder(at position: Int) -> SQLExpression {
-        return SQLRaw("?")
-    }
-
-    public func literalBoolean(_ value: Bool) -> SQLExpression {
-        switch value {
-        case true: return SQLRaw("TRUE")
-        case false: return SQLRaw("FALSE")
+    public var identifierQuote: SQLExpression { SQLRaw("\"") }
+    public var literalStringQuote: SQLExpression { SQLRaw("'") }
+    public func bindPlaceholder(at position: Int) -> SQLExpression { SQLRaw("?\(position)") }
+    public func literalBoolean(_ value: Bool) -> SQLExpression { SQLRaw(value ? "TRUE" : "FALSE") }
+    public var literalDefault: SQLExpression { SQLLiteral.null }
+
+    public var supportsAutoIncrement: Bool { false }
+    public var autoIncrementClause: SQLExpression { SQLRaw("AUTOINCREMENT") }
+
+    public var enumSyntax: SQLEnumSyntax { .unsupported }
+    public var triggerSyntax: SQLTriggerSyntax { .init(create: [.supportsBody, .supportsCondition]) }
+    public var alterTableSyntax: SQLAlterTableSyntax { .init(allowsBatch: false) }
+    public var upsertSyntax: SQLUpsertSyntax { self.isAtLeastVersion(3, 24, 0) ? .standard : .unsupported } // `UPSERT` was added to SQLite in 3.24.0.
+    public var supportsReturning: Bool { self.isAtLeastVersion(3, 35, 0) } // `RETURNING` was added to SQLite in 3.35.0.
+    public var unionFeatures: SQLUnionFeatures { [.union, .unionAll, .intersect, .except] }
+    
+    public func customDataType(for dataType: SQLDataType) -> SQLExpression? {
+        if case .bigint = dataType {
+            // Translate requests for bigint to requests for SQLite's plain integer type. This yields the autoincrement
+            // primary key behavior when a 64-bit integer is requested from a higher layer.
+            return SQLDataType.int
         }
+        return nil
     }
 
-    public var literalDefault: SQLExpression {
-        return SQLLiteral.null
-    }
-
-    public var enumSyntax: SQLEnumSyntax {
-        .unsupported
-    }
-
-    public var supportsAutoIncrement: Bool {
-        false
-    }
-
-    public var alterTableSyntax: SQLAlterTableSyntax {
-        .init(
-            alterColumnDefinitionClause: nil,
-            alterColumnDefinitionTypeKeyword: nil,
-            allowsBatch: false
-        )
-    }
-
-    public var triggerSyntax: SQLTriggerSyntax {
-        return .init(create: [.supportsBody, .supportsCondition])
-    }
+    public init() { }
     
-    public var unionFeatures: SQLUnionFeatures {
-        [.union, .unionAll, .intersect, .except]
+    private func isAtLeastVersion(_ major: Int, _ minor: Int, _ patch: Int) -> Bool {
+        _SQLiteDatabaseVersion.runtimeVersion.isNotOlder(than: _SQLiteDatabaseVersion(major: major, minor: minor, patch: patch))
     }
-
-    public init() { }
 }
diff --git a/Tests/SQLiteKitTests/SQLiteKitTests.swift b/Tests/SQLiteKitTests/SQLiteKitTests.swift
@@ -117,13 +117,13 @@ class SQLiteKitTests: XCTestCase {
             threadPool: self.threadPool
         )
 
-        let a1 = try a.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.next()).wait()
+        let a1 = try a.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.any()).wait()
         defer { try! a1.close().wait() }
-        let a2 = try a.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.next()).wait()
+        let a2 = try a.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.any()).wait()
         defer { try! a2.close().wait() }
-        let b1 = try b.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.next()).wait()
+        let b1 = try b.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.any()).wait()
         defer { try! b1.close().wait() }
-        let b2 = try b.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.next()).wait()
+        let b2 = try b.makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.any()).wait()
         defer { try! b2.close().wait() }
 
         _ = try a1.query("CREATE TABLE foo (bar INTEGER)").wait()
@@ -169,7 +169,7 @@ class SQLiteKitTests: XCTestCase {
         self.connection = try! SQLiteConnectionSource(
             configuration: .init(storage: .memory, enableForeignKeys: true),
             threadPool: self.threadPool
-        ).makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.next()).wait()
+        ).makeConnection(logger: .init(label: "test"), on: self.eventLoopGroup.any()).wait()
     }
 
     override func tearDown() {
