From c07d53044727db7edf8550c2e8ccfe1fa40177d2 Mon Sep 17 00:00:00 2001 From: Gwynne Raskind Date: Sat, 12 Nov 2022 06:06:07 -0600 Subject: [PATCH] Modernize SQLite feature support (#100) * Drop support for Swift <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 --- .github/workflows/test.yml | 127 ++++++++++++------ Package.swift | 8 +- .../SQLiteKit/SQLiteConnection+SQLKit.swift | 82 ++++++++++- Sources/SQLiteKit/SQLiteDialect.swift | 84 +++++------- Tests/SQLiteKitTests/SQLiteKitTests.swift | 10 +- 5 files changed, 211 insertions(+), 100 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cedc871..b0b24b0 100644 --- 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 index 4c83394..7242b84 100644 --- 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 index 20fda3c..501cfd7 100644 --- 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 index 2943319..411d3b4 100644 --- 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 index 7d4affb..65b7c53 100644 --- 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() {