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

CommandBehavior.SequentialAccess causes error with SqlHydra readers #25

Closed
JordanMarr opened this issue Jul 27, 2021 · 5 comments
Closed

Comments

@JordanMarr
Copy link

I want to add a sample of using Donald with the new SqlHydra generated data readers because the two seem like a great fit.

However, I ran into a problem that is related to the fact that Donald uses CommandBehavior.SequentialAccess by default. I think the issue is that the generated readers may, in some scenarios, try to read twice from the same reader ordinal, which SequentialAccess does not allow. It would be great if this wasn't the default, or if it could be changed.

Another issue (minor) is that Donald always returns a vanilla IDataReader, whereas SqlHydra can generate using a more specific reader (which can be useful in scenarios like where SQL Server has a custom reader for DateTimeOffset). This requires that the IDataReader passed by Db.query has to be manually casted to the provider specific reader generated by SqlHydra.

Changing this so that Donald can return a more specific data reader (based on the connection) could be beneficial for Donald in general, as well as making interop easier with SqlHydra. I think you may be able to do this by making Db.query take a generic type type arg for the data reader that implements a generic constraint that forces it to implement IDataReader.

For example, a RequiredColumn in SqlHydra can take an IDataReader or anything that implements it, like Microsoft.Data.SqlClient.SqlDataReader.

type RequiredColumn<'T, 'TReader when 'TReader :> System.Data.IDataReader>(reader: 'TReader, getter: int -> 'T, column) =
        inherit Column(reader, column)
        member __.Read(?alias) = alias |> Option.defaultValue __.Name |> reader.GetOrdinal |> getter
@pimbrouwers
Copy link
Owner

pimbrouwers commented Jul 27, 2021

Hey Jordan! Thanks for doing this.

I like the notion of sequential access being the default, since the performance gains are just to great. But I just realized that even though the underyling extension method allows for this to be configured at runtime. I never expose this in the outer API.

So here's my thought, we add two sister funcs for query and querySingle. The question is, do we fully expose the underlying Enum? Or do we assume most people want the "CommandBehaviour.Default"?

let queryConfigured (cmdBehaviour : CommandBehaviour) (map : IDataReader -> 'a) (cmd : IDbCommand) : DbResult<'a list> =
    tryDo (fun cmd ->     
        use rd = cmd.ExecReader(cmdBehaviour)
        let results = [ while rd.Read() do yield map rd ]
        rd.Close() |> ignore
        results) 
        cmd

// vs

let queryDefault (map : IDataReader -> 'a) (cmd : IDbCommand) : DbResult<'a list> = 
    tryDo (fun cmd ->     
        use rd = cmd.ExecReader(CommandBehaviour.Default)
        let results = [ while rd.Read() do yield map rd ]
        rd.Close() |> ignore
        results) 
        cmd

@JordanMarr
Copy link
Author

JordanMarr commented Jul 27, 2021

That's a tough one (but aren't they all?)! 🤔

Another option could be to leave all that as-is and have a executeReader() that returns a 'TReader`:

use reader = 
   conn
   |> Db.newCommand "SELECT ..."
   |> Db.executeReader

let p = SalesLT.ProductReader(reader)
let c = SalesLT.ProductCategoryReader(reader)

return [
    while reader.Read() do
        p.Read(), 
        c.Read()
    ]

or Async:

```F#
use! reader = 
   conn
   |> Db.newCommand "SELECT ..."
   |> Db.Async.executeReader

let p = SalesLT.ProductReader(reader)
let c = SalesLT.ProductCategoryReader(reader)

return [
    while reader.Read() do
        p.Read(), 
        c.Read()
    ]

You may not like that one as much since it makes the user responsible for iterating and disposing the reader as opposed to it being handled within Donald, which is a con. One pro is that the generated readers can be initialized before the looping, as opposed to the c'tor being called each time -- which actually may not be that big of a deal either way.. IDK.

Decisions, decisions! I'll be fine for whatever you think is best for Donald. 😁

@pimbrouwers
Copy link
Owner

pimbrouwers commented Jul 27, 2021

Indeed! It's funny you suggest that, there was originally a read func which I had thought would be useful in multi-result set cases. Perhaps I add it again? Something like:

/// Execute paramterized query and return IDataReader
let read (cmd : IDbCommand) : IDataReader =
    cmd.ExecReader(CommandBehavior.Default)

/// Execute paramterized query and return IDataReader
let read (cmd : IDbCommand) : Task<IDataReader> =
    let dbCmd = cmd :?> DbCommand
    dbCmd.ExecReaderAsync(CommandBehavior.Default)
    |> continueWith (fun rd -> rd.Result :> IDataReader)

@JordanMarr
Copy link
Author

Oh yeah, that would provide the ideal extension point for this workflow!

@pimbrouwers
Copy link
Owner

Fantastic!

This is done and deployed to nuget as part of 6.2.2.

Thanks for your help on this!

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

2 participants