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

Connection::run(query: Insert) failed to unwrap lastInsertRowid with OnConflict.Ignore #235

Closed
asfdfdfd opened this issue Sep 28, 2015 · 9 comments

Comments

@asfdfdfd
Copy link

branch: swift-2
commit: 8bd27bc

Model

class News {

    let id: Int
    let header: String

    init(row: Row) {
        id = row[NewsColumn.Id]
        header = row[NewsColumn.Header]
    }
}

Insert code

try db.run(tableNews.insert(or: OnConflict.Ignore,
    NewsColumn.Id <- (rawNews["id"] as! Int),
    NewsColumn.Header <- (rawNews["name"] as! String),
))

This insert code will fail here if model with the same Id already exists.

@asfdfdfd
Copy link
Author

https://www.sqlite.org/c3ref/last_insert_rowid.html

The sqlite3_last_insert_rowid(D) interface returns the rowid of the most recent successful INSERT into a rowid table [...]

An INSERT that fails due to a constraint violation is not a successful INSERT and does not change the value returned by this routine. Thus [...], INSERT OR IGNORE, [...] make no changes to the return value of this routine when their insertion fails.

@ghost
Copy link

ghost commented Dec 7, 2015

This method is the culprit, self.lastInsertRowid is nullable, but the return type is Int64.

public func run(query: Insert) throws -> Int64 {
  let expression = query.expression
  return try sync {
    try self.run(expression.template, expression.bindings)
    return self.lastInsertRowid!
  }
}

Temporary fix:

public func run(query: Insert) throws -> Int64 {
  let expression = query.expression
  return try sync {
    try self.run(expression.template, expression.bindings)
    return (self.lastInsertRowid ?? -1)!
  }
}

@asfdfdfd
Copy link
Author

asfdfdfd commented Dec 7, 2015

Also here is alternative fix, but it was rejected.

@ghost
Copy link

ghost commented Dec 7, 2015

I tried that fix, but it does not build for me.

@asfdfdfd
Copy link
Author

asfdfdfd commented Dec 7, 2015

With my fix you have to check return value on nil. With 'if let' pattern, for example.

@ghost
Copy link

ghost commented Dec 7, 2015

True, also I'm not sure how Stephen feels about returning -1 for failed Insert statements. We shall see.

@iNoles
Copy link

iNoles commented Dec 10, 2015

Since a row id should be unique number with Ignore, any column with -1 wouldn't fix a problem. I think insert methods should just do db.run without lastInsertRowid.

@iNoles
Copy link

iNoles commented Feb 3, 2016

@stephencelis How about

public func run(query: Insert) throws -> Connection {
        let expression = query.expression
        return try sync {
            try self.run(expression.template, expression.bindings)
            return self
        }
    }

@jberkel
Copy link
Collaborator

jberkel commented Dec 7, 2016

Fixed in 75394da

@jberkel jberkel closed this as completed Dec 7, 2016
This was referenced Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants