Dzoukr / Dapper.FSharp

Lightweight F# extension for StackOverflow Dapper with support for MSSQL, MySQL, PostgreSQL, and SQLite
MIT License
365 stars 35 forks source link

Is there a convenient way to retrive the value of a 'auto_increment' column after an insert? #10

Closed travis-leith closed 2 years ago

travis-leith commented 3 years ago

doesn't seem to be. would it be difficult to add?

travis-leith commented 3 years ago

I managed to get this working by making the following changes

type InsertQuery<'a> = {
    Table : string
    Values : 'a list
    ReturnAutoNumber : bool
}

let evalInsertQuery fields (q:InsertQuery<_>) =
        let fieldNames = fields |> List.map inQuotes |> String.concat ", " |> sprintf "(%s)"
        let values =
            q.Values
            |> List.mapi (fun i _ -> fields |> List.map (fun field -> sprintf "@%s%i" field i ) |> String.concat ", " |> sprintf "(%s)")
            |> String.concat ", "
        sprintf "INSERT INTO %s %s VALUES %s;%s" q.Table fieldNames values (if q.ReturnAutoNumber then "select LAST_INSERT_ID();" else "")

member this.InsertAsync<'a> (q:InsertQuery<'a>, ?trans:IDbTransaction, ?timeout:int, ?logFunction) =
        let query, values = q |> Deconstructor.insert<'a>
        if logFunction.IsSome then (query, values) |> logFunction.Value
        //this.ExecuteAsync(query, values, transaction = Option.toObj trans, commandTimeout = Option.toNullable timeout)
        this.QueryAsync<uint32>(query, values, transaction = Option.toObj trans, commandTimeout = Option.toNullable timeout)

[<CustomOperation "returnAutoNumber">]
    member __.ReturnAutoNumber (state:InsertQuery<'a>, value:bool) = { state with ReturnAutoNumber = value }

This suits my purposes right now, but is not yet good enough for inclusion in the library; InsertAsync is not generic enough as it always returns seq<uint32. Should be fixable by adding another type parameter.

@Dzoukr if you are interested in including this, I can submit a PR when I am done with my current project.

Dzoukr commented 3 years ago

Hi @travis-leith,

I think PR is not necessary since there is already InsertOutputAsync method. You can do it like this:

let! ins =
    insert {
        table "Articles"
    value ({| Title = "MyTitle" |})
    } |> conn.InsertOutputAsync<{| Title : string |}, {| Id : int |}>
let lastInserted = ins |> Seq.head |> (fun (x:{| Id : int |}) -> x.Id)

See this test I just pushed https://github.com/Dzoukr/Dapper.FSharp/blob/master/tests/Dapper.FSharp.Tests/MSSQL/IssuesTests.fs#L90-L99

travis-leith commented 3 years ago

My use case is for MySql. Since there is no OUTPUT statement in MySql perhaps this could be implemented as something along the lines of

let evalOutputInsertQuery fields outputFields (q:InsertQuery<_>) =
        let fieldNames = fields |> List.map inBrackets |> String.concat ", " |> sprintf "(%s)"
        let values =
            q.Values
            |> List.mapi (fun i _ -> fields |> List.map (fun field -> sprintf "@%s%i" field i ) |> String.concat ", " |> sprintf "(%s)")
            |> String.concat ", "
        match outputFields with
        | [] ->
            sprintf "INSERT INTO %s %s VALUES %s" q.Table fieldNames values
        | outputFields ->
            let outputFieldNames = outputFields |> List.map (sprintf "%s") |> String.concat ", "
            sprintf "INSERT INTO %s %s; SELECT FROM %s %s VALUES %s" q.Table fieldNames q.Table outputFieldNames values

what do you think?

Dzoukr commented 3 years ago

🤔 Are you sure it's correct? If I insert 10 values into table having 90 rows already, will I get 10 inserted values back? I suspect this would return all 100.

travis-leith commented 3 years ago

Oops, yes you are correct. In that case, I still think my original suggestion might be a useful abstraction that would be consistent across db types. For MSSQL it would return SCOPE_IDENTITY. Presumably Postgres has something similar. The behavior is different for MSSQL vs MySql in the case of multiple inserts. MSSQL returns first value, MySql returns last.

So library options would be either leave it up to the user to be aware of the specific behavior, or have it only apply in the case of a single value.

Dzoukr commented 3 years ago

Yeah, but that would change the default behavior of underlaying Dapper ExecuteAsync returning number of affected rows. Need to think about it. Maybe taking IDbConnection with transaction and making custom select LAST_INSERT_ID() over Dapper could do the thing. 🤔

Dzoukr commented 3 years ago

Don't get me wrong. I appreciate all the help and ideas, but trying to keep library as simple as possible. That's the reason there is no support for MIN, MAX, COUNT and other aggregate functions for example. I always consider whether changing API keeps library stupidly simple. 😄

travis-leith commented 3 years ago

Completely understand the desire to keep things simple. Although in defense of the idea: min, max count etc are easily achievable outside of sql using regular f#, so no great loss for the library there. However, the pattern of creating a child record, getting the automatically generated id for that child, and then using it to create a parent record is a common pattern which isn't readily achievable using the current form of this library. As for the change to the API, it is only a single extra custom operation on the insert builder.

travis-leith commented 3 years ago

Not trying to be pushy btw, just exploring the pros and cons.

Dzoukr commented 3 years ago

As for the change to the API, it is only a single extra custom operation on the insert builder.

That's exactly the point where I am not convinced. It would change the return value meaning (from number of affected rows to the last generated ID) + AFAIK the identity column does not have to be INT only, so it would require another generic parameter to cover all the possible identity types.

However your issue let me to find out PostgreSQL has output clause as well, so I added support for it (https://github.com/Dzoukr/Dapper.FSharp/commit/55fa6266b99ea1257326b3f398fa94f11661201a)

Any idea about MySQL plans for output clause which would fix it in the cleanest way?

travis-leith commented 3 years ago

Any idea about MySQL plans for output clause which would fix it in the cleanest way?

I cannot find any indication of plans to add this.

travis-leith commented 3 years ago

I am happy to report that as of MariaDb 10.5, the RETURNING statement allows the following

create or replace table xxx(id int auto_increment primary key, y int);
insert into xxx(y) values (23), (43), (12) returning id;

It seems that this is only supported by MariaDb, nothing equivalent in MySql, as far as I am aware.

travis-leith commented 3 years ago

Personally, I think this is worth including in this library since many people (such as me) will be using the MySqlConnector for MariaDb.

dredgy commented 2 years ago

I think this is a necessary feature. I'm still using V2 so forgive me if this has been introduced in V3 (if it has, let me know, I'll update ASAP).

Is there no way of implementing InsertOutputAsync in MySQL by using last_insert_id()?. I also feel that the Maria solution posted by @travis-leith is perfectly fine to implement.

I'm unfortunately not in a position to use MSSQL, though Postgres is possible.

And it's not like it's a huge deal, but I think it would make the library from the outset even simpler. For my personal case, I would rather see it return the whole row of type 'a rather than just the ID, but I'm not hugely fussed.