Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PersistenceError.recordNotFound #486

Merged
merged 5 commits into from
Feb 26, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -52,6 +52,7 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one expection:

- [#478](https://github.com/groue/GRDB.swift/pull/478): Swift 5: SQL interpolation
- [#484](https://github.com/groue/GRDB.swift/pull/484): SE-0193 Cross-module inlining and specialization
- [#486](https://github.com/groue/GRDB.swift/pull/486): Refactor PersistenceError.recordNotFound

### Breaking Changes

92 changes: 44 additions & 48 deletions GRDB/Record/PersistableRecord.swift
Original file line number Diff line number Diff line change
@@ -20,16 +20,20 @@ public enum PersistenceError: Error, CustomStringConvertible {

/// Thrown by MutablePersistableRecord.update() when no matching row could be
/// found in the database.
case recordNotFound(MutablePersistableRecord)
///
/// - databaseTableName: the table of the unfound record
/// - key: the key of the unfound record (column and values)
case recordNotFound(databaseTableName: String, key: [String: DatabaseValue])
}

// CustomStringConvertible
extension PersistenceError {
/// :nodoc:
public var description: String {
switch self {
case .recordNotFound(let record):
return "Not found: \(record)"
case let .recordNotFound(databaseTableName: databaseTableName, key: key):
let row = Row(key) // For nice output
return "Key not found in table \(databaseTableName): \(row.description)"
}
}
}
@@ -662,18 +666,19 @@ extension MutablePersistableRecord {

// MARK: - CRUD Internals

/// Return true if record has a non-null primary key
/// Return a non-nil dictionary if record has a non-null primary key
@usableFromInline
func canUpdate(_ db: Database) throws -> Bool {
func primaryKey(_ db: Database) throws -> [String: DatabaseValue]? {
let databaseTableName = type(of: self).databaseTableName
let primaryKey = try db.primaryKey(databaseTableName)
let primaryKeyInfo = try db.primaryKey(databaseTableName)
let container = try PersistenceContainer(db, self)
for column in primaryKey.columns {
if let value = container[caseInsensitive: column], !value.databaseValue.isNull {
return true
}
let primaryKey = Dictionary(uniqueKeysWithValues: primaryKeyInfo.columns.map {
($0, container[caseInsensitive: $0]?.databaseValue ?? .null)
})
if primaryKey.allSatisfy({ $0.value.isNull }) {
return nil
}
return false
return primaryKey
}

/// Don't invoke this method directly: it is an internal method for types
@@ -709,13 +714,14 @@ extension MutablePersistableRecord {
/// match any row in the database.
@inlinable
public func performUpdate(_ db: Database, columns: Set<String>) throws {
guard let statement = try DAO(db, self).updateStatement(columns: columns, onConflict: type(of: self).persistenceConflictPolicy.conflictResolutionForUpdate) else {
let dao = try DAO(db, self)
guard let statement = try dao.updateStatement(columns: columns, onConflict: type(of: self).persistenceConflictPolicy.conflictResolutionForUpdate) else {
// Nil primary key
throw PersistenceError.recordNotFound(self)
throw dao.makeRecordNotFoundError()
}
try statement.execute()
if db.changesCount == 0 {
throw PersistenceError.recordNotFound(self)
throw dao.makeRecordNotFoundError()
}
}

@@ -730,18 +736,12 @@ extension MutablePersistableRecord {
/// This default implementation forwards the job to `update` or `insert`.
@inlinable
public mutating func performSave(_ db: Database) throws {
// Make sure we call self.insert and self.update so that classes
// that override insert or save have opportunity to perform their
// custom job.

if try canUpdate(db) {
// Call self.insert and self.update so that we support classes that
// override those methods.
if let key = try primaryKey(db) {
do {
try update(db)
} catch PersistenceError.recordNotFound {
// TODO: check that the not persisted objet is self
//
// Why? Adopting types could override update() and update
// another object which may be the one throwing this error.
} catch PersistenceError.recordNotFound(databaseTableName: type(of: self).databaseTableName, key: key) {
try insert(db)
}
} else {
@@ -1055,17 +1055,12 @@ extension PersistableRecord {
/// This default implementation forwards the job to `update` or `insert`.
@inlinable
public func performSave(_ db: Database) throws {
// Make sure we call self.insert and self.update so that classes that
// override insert or save have opportunity to perform their custom job.

if try canUpdate(db) {
// Call self.insert and self.update so that we support classes that
// override those methods.
if let key = try primaryKey(db) {
do {
try update(db)
} catch PersistenceError.recordNotFound {
// TODO: check that the not persisted objet is self
//
// Why? Adopting types could override update() and update another
// object which may be the one throwing this error.
} catch PersistenceError.recordNotFound(databaseTableName: type(of: self).databaseTableName, key: key) {
try insert(db)
}
} else {
@@ -1150,13 +1145,9 @@ public enum DatabaseUUIDEncodingStrategy {
/// DAO takes care of PersistableRecord CRUD
@usableFromInline
final class DAO<Record: MutablePersistableRecord> {

/// The database
let db: Database

/// The record
let record: Record

/// DAO keeps a copy the record's persistenceContainer, so that this
/// dictionary is built once whatever the database operation. It is
/// guaranteed to have at least one (key, value) pair.
@@ -1165,22 +1156,16 @@ final class DAO<Record: MutablePersistableRecord> {
/// The table name
let databaseTableName: String

/// The table primary key
/// The table primary key info
@usableFromInline let primaryKey: PrimaryKeyInfo

@usableFromInline
init(_ db: Database, _ record: Record) throws {
let databaseTableName = type(of: record).databaseTableName
let primaryKey = try db.primaryKey(databaseTableName)
let persistenceContainer = try PersistenceContainer(db, record)

GRDBPrecondition(!persistenceContainer.isEmpty, "\(type(of: record)): invalid empty persistence container")

self.db = db
self.record = record
self.persistenceContainer = persistenceContainer
self.databaseTableName = databaseTableName
self.primaryKey = primaryKey
databaseTableName = type(of: record).databaseTableName
primaryKey = try db.primaryKey(databaseTableName)
persistenceContainer = try PersistenceContainer(db, record)
GRDBPrecondition(!persistenceContainer.isEmpty, "\(type(of: record)): invalid empty persistence container")
}

@usableFromInline
@@ -1283,6 +1268,17 @@ final class DAO<Record: MutablePersistableRecord> {
statement.unsafeSetArguments(StatementArguments(primaryKeyValues))
return statement
}

/// Throws a PersistenceError.recordNotFound error
@usableFromInline
func makeRecordNotFoundError() -> Error {
let key = Dictionary(uniqueKeysWithValues: primaryKey.columns.map {
($0, persistenceContainer[caseInsensitive: $0]?.databaseValue ?? .null)
})
return PersistenceError.recordNotFound(
databaseTableName: databaseTableName,
key: key)
}
}


4 changes: 2 additions & 2 deletions GRDB/Record/Record.swift
Original file line number Diff line number Diff line change
@@ -276,11 +276,11 @@ open class Record : FetchableRecord, TableRecord, PersistableRecord {
let dao = try DAO(db, self)
guard let statement = try dao.updateStatement(columns: columns, onConflict: type(of: self).persistenceConflictPolicy.conflictResolutionForUpdate) else {
// Nil primary key
throw PersistenceError.recordNotFound(self)
throw dao.makeRecordNotFoundError()
}
try statement.execute()
if db.changesCount == 0 {
throw PersistenceError.recordNotFound(self)
throw dao.makeRecordNotFoundError()
}

// Set hasDatabaseChanges to false
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
@@ -682,7 +682,7 @@ let playerId = db.lastInsertedRowID
Don't miss [Records](#records), that provide classic **persistence methods**:

```swift
let player = Player(name: "Arthur", score: 1000)
var player = Player(name: "Arthur", score: 1000)
try player.insert(db)
let playerId = player.id
```
@@ -7510,8 +7510,8 @@ do {
```swift
do {
try player.update(db)
} catch PersistenceError.recordNotFound {
// There was nothing to update
} catch let PersistenceError.recordNotFound(databaseTableName: table, key: key) {
print("Key \(key) was not found in table \(table).")
}
```

1 change: 0 additions & 1 deletion TODO.md
Original file line number Diff line number Diff line change
@@ -24,7 +24,6 @@
- [ ] Allow joining methods on DerivableRequest
- [ ] DatabaseWriter.assertWriteAccess()
- [ ] Configuration.crashOnError = true
- [ ] Remove associated record from PersistenceError.recordNotFound. It should be possible, in userland, to write a method that throws this error without having a fully constructed record.
- [ ] Support for "INSERT INTO ... SELECT ...". For example:

```swift
19 changes: 19 additions & 0 deletions Tests/GRDBTests/MutablePersistableRecordTests.swift
Original file line number Diff line number Diff line change
@@ -696,4 +696,23 @@ class MutablePersistableRecordTests: GRDBTestCase {
}
}
}

func testPersistenceErrorRecordNotFoundDescription() {
do {
let error = PersistenceError.recordNotFound(
databaseTableName: "place",
key: ["id": .null])
XCTAssertEqual(
error.description,
"Key not found in table place: [id:NULL]")
}
do {
let error = PersistenceError.recordNotFound(
databaseTableName: "user",
key: ["uuid": "E621E1F8-C36C-495A-93FC-0C247A3E6E5F".databaseValue])
XCTAssertEqual(
error.description,
"Key not found in table user: [uuid:\"E621E1F8-C36C-495A-93FC-0C247A3E6E5F\"]")
}
}
}
8 changes: 6 additions & 2 deletions Tests/GRDBTests/RecordMinimalPrimaryKeyRowIDTests.swift
Original file line number Diff line number Diff line change
@@ -115,8 +115,10 @@ class RecordMinimalPrimaryKeyRowIDTests : GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "minimalRowIDs")
XCTAssertEqual(key, ["id": record.id.databaseValue])
}
}
}
@@ -142,8 +144,10 @@ class RecordMinimalPrimaryKeyRowIDTests : GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "minimalRowIDs")
XCTAssertEqual(key, ["id": record.id.databaseValue])
}
}
}
8 changes: 6 additions & 2 deletions Tests/GRDBTests/RecordMinimalPrimaryKeySingleTests.swift
Original file line number Diff line number Diff line change
@@ -114,8 +114,10 @@ class RecordMinimalPrimaryKeySingleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "minimalSingles")
XCTAssertEqual(key, ["UUID": "theUUID".databaseValue])
}
}
}
@@ -143,8 +145,10 @@ class RecordMinimalPrimaryKeySingleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "minimalSingles")
XCTAssertEqual(key, ["UUID": "theUUID".databaseValue])
}
}
}
12 changes: 9 additions & 3 deletions Tests/GRDBTests/RecordPrimaryKeyHiddenRowIDTests.swift
Original file line number Diff line number Diff line change
@@ -169,8 +169,10 @@ class RecordPrimaryKeyHiddenRowIDTests : GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["rowid": .null])
}
}
}
@@ -182,8 +184,10 @@ class RecordPrimaryKeyHiddenRowIDTests : GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["rowid": record.id.databaseValue])
}
}
}
@@ -210,8 +214,10 @@ class RecordPrimaryKeyHiddenRowIDTests : GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["rowid": record.id.databaseValue])
}
}
}
12 changes: 9 additions & 3 deletions Tests/GRDBTests/RecordPrimaryKeyMultipleTests.swift
Original file line number Diff line number Diff line change
@@ -124,8 +124,10 @@ class RecordPrimaryKeyMultipleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "citizenships")
XCTAssertEqual(key, ["countryName": .null, "personName": .null])
}
}
}
@@ -137,8 +139,10 @@ class RecordPrimaryKeyMultipleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "citizenships")
XCTAssertEqual(key, ["countryName": "France".databaseValue, "personName": "Arthur".databaseValue])
}
}
}
@@ -165,8 +169,10 @@ class RecordPrimaryKeyMultipleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "citizenships")
XCTAssertEqual(key, ["countryName": "France".databaseValue, "personName": "Arthur".databaseValue])
}
}
}
12 changes: 9 additions & 3 deletions Tests/GRDBTests/RecordPrimaryKeyRowIDTests.swift
Original file line number Diff line number Diff line change
@@ -166,8 +166,10 @@ class RecordPrimaryKeyRowIDTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["id": .null])
}
}
}
@@ -179,8 +181,10 @@ class RecordPrimaryKeyRowIDTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["id": record.id.databaseValue])
}
}
}
@@ -207,8 +211,10 @@ class RecordPrimaryKeyRowIDTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "persons")
XCTAssertEqual(key, ["id": record.id.databaseValue])
}
}
}
12 changes: 9 additions & 3 deletions Tests/GRDBTests/RecordPrimaryKeySingleTests.swift
Original file line number Diff line number Diff line change
@@ -117,8 +117,10 @@ class RecordPrimaryKeySingleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "pets")
XCTAssertEqual(key, ["UUID": .null])
}
}
}
@@ -130,8 +132,10 @@ class RecordPrimaryKeySingleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "pets")
XCTAssertEqual(key, ["UUID": "BobbyUUID".databaseValue])
}
}
}
@@ -158,8 +162,10 @@ class RecordPrimaryKeySingleTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "pets")
XCTAssertEqual(key, ["UUID": "BobbyUUID".databaseValue])
}
}
}
Original file line number Diff line number Diff line change
@@ -121,8 +121,10 @@ class RecordPrimaryKeySingleWithReplaceConflictResolutionTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "emails")
XCTAssertEqual(key, ["email": record.email.databaseValue])
}
}
}
@@ -150,8 +152,10 @@ class RecordPrimaryKeySingleWithReplaceConflictResolutionTests: GRDBTestCase {
do {
try record.update(db)
XCTFail("Expected PersistenceError.recordNotFound")
} catch PersistenceError.recordNotFound {
} catch let PersistenceError.recordNotFound(databaseTableName: databaseTableName, key: key) {
// Expected PersistenceError.recordNotFound
XCTAssertEqual(databaseTableName, "emails")
XCTAssertEqual(key, ["email": record.email.databaseValue])
}
}
}