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

Not Implemented Exception While Using SRTP #90

Closed 1eyewonder closed 11 months ago

1eyewonder commented 11 months ago

Issue Calling code similar to below will result in a Not Implemented Exception. Area of code is found here for the 'isIn' function https://github.com/Dzoukr/Dapper.FSharp/blob/master/src/Dapper.FSharp/MSSQL/LinqExpressionVisitors.fs#L218

Use Case I have a quite a few similar queries where I need to check for existence in various tables and other minute validations. If I am thinking about this correctly, I think adding support for SRTP would provide benefit in being able to create re-usable queries so I don't have to create a new query for each table.

#r "nuget: Dapper.FSharp"
open Dapper.FSharp
open Dapper.FSharp.MSSQL
open System.Data

[<CLIMutable>]
type Person = 
    { 
        Id : int 
    }

[<CLIMutable>]
type Dog = 
    { 
        Id : int 
    }

let people = table'<Person> "People"
let dogs = table'<Dog> "Dogs"

let inline getIds<'T 
    when 'T: (member Id : int)> 
    (ids : int list) 
    (querySource : QuerySource<'T>) 
    (conn : IDbConnection) 
    = async {
    // This is required in order to use SRTP w/ the query builder
    let getId (item : 'T) = item.Id

    let query = select {
        for row in querySource do
            where (isIn (getId row) ids)
    }

    return! conn.SelectAsync<'T>(query) |> Async.AwaitTask
}
JordanMarr commented 11 months ago

I'm pretty sure that only the SelectAsync<'T> method reflects the columns to read. So rather than using an SRTP, you could probably just create a single partial object that has only the Id field for querying, and then pass in a generic argument of the full table class that contains all the properties you actually want to read.

[<CLIMutable>]
type IdColumn = { Id: int  }

let queryByIds<'T> (ids: int list) tableName (conn: IDbConnection) = 
  async {
    let query = 
      select {
          for row in table'<IdColumn> tableName do
          where (isIn row.Id ids)
      }

    return! conn.SelectAsync<'T>(query) |> Async.AwaitTask
  }
let! dogs = queryByIds<Dog> [1;2;3] "Dogs" conn
1eyewonder commented 11 months ago

Wow, I feel stupid for not thinking of this 🤦‍♂️. Thank you, this makes sense!

JordanMarr commented 11 months ago

Not stupid at all! I’m just very familiar with the internals of the codebase.

1eyewonder commented 11 months ago

So more of a philosophical question now. Is the SRTP support still a valid request/issue for this repo? While the 'ID' use case is very straight forward, does it make sense to suggest this route for other alternatives? Silly examples below.

[<CLIMutable>]
type IdColumn = { Id: int }

[<CLIMutable>]
type IdAndFoo1Column = { Id: int; Foo1: int; }

[<CLIMutable>]
type IdFoo1Foo2Column = { Id: int; Foo1: int; Foo2: int }

So while code quantity wouldn't really differ between the DTO and the SRTP routes, the pain of naming is placed on users(devs). The STRP would just allow users to focus on what is needed for the generic queries rather than come up with a DTO for each use case. (Granted I'm not sure how often users come across this since I haven't really dealt with it until the other day). I'm sure it may just end up being diminishing returns trying to pursue this route, but I figured I'd ask the question while the issue was still open. If you think it's not worth pursuing, I can go ahead and close the issue.

JordanMarr commented 11 months ago

Personally, I will always opt for an extra type or function over SRTP for simplicity. I think using an anonymous record would be the better play in this case:

let queryByIds<'T> (ids: int list) tableName (conn: IDbConnection) = 
  async {
    let query = 
      select {
          for row in table'<{| Id: int |}> tableName do
          where (isIn row.Id ids)
      }

    return! conn.SelectAsync<'T>(query) |> Async.AwaitTask
  }
let! dogs = queryByIds<Dog> [1;2;3] "Dogs" conn
1eyewonder commented 11 months ago

Yep, that makes sense. Thank you so much for your help!