Allow another way to pass database context

[smoothdeveloper]

currently, the connection context can be any of those:

  type Connection =
      | Literal of string
      | NameInConfig of string
      | Transaction of SqlTransaction

In code base I work with (which is not in F#), it's more convenient to pass Func around which takes care of creating dbcommand with appropriate timeout, connection and transaction depending application context, this was introduced in the code base to replace a lot of manual connection / transaction management and keeping around the desired timeout value.

I'd like to allow this to work with F# SqlClient (so I can introduce it's usage in the codebase I work with) and wondering if there is interest in this?

After a quick look, the changes seems to be restricted to RuntimeSqlCommand implementation but there are probably several ways to deal with this.

The simplest way seems to

• add in the Connection discriminated union (SqlCommand.fs)

   | CreateCommandFunctor of (unit -> SqlCommand) 

• implement the necessary logic in the constructor
• expose additional constructors on provided type taking this unit -> SqlCommand (SqlCommandProvider.fs)
• write few tests to make sure it behaves properly

If this seems interesting and appropriate I can draft implementation, please provide feedback.

Thanks!

[dmitry-a-morozov]

In general you're absolutely right but normal coding rules unfortunately cannot be applied to type providers implementation. An issue with your proposal that it will require to construct CreateCommandFunctor lambda function inside quotations. Most likely here https://github.com/fsprojects/FSharp.Data.SqlClient/blob/master/src/SqlClient/SqlCommandProvider.fs#L164. Which will make it impossible to debug. Considering that RuntimeSqlCommand constructor is not something that user deals with directly this trade-off probably doesn't make sense. I know that without actual type providers development experience you can be skeptical about what I said. The best way to validate is to code this idea. So you're welcome to try but I warned you.

[smoothdeveloper]

@dmitry-a-morozov thanks for your feedback, I vaguely know TypeProvider authoring is a bit of a headache and I'm not acquainted with the quotation stuff.

I'm not sure I grasp what you mean with the debug consideration, I gave a try at implementing the changes in my fork and it seems to work properly, I also added an extension point to alter the command which is prepared which is also something I need, this is exposed only to the new constructor.

One thing I changed is that AsSqlCommand also carries the timeout setting in the cloned command, I think this was an overlook.

I didn't see any regression in the tests and I added tests covering the changes and expected behaviour, please have a look at the pull request: https://github.com/fsprojects/FSharp.Data.SqlClient/pull/108

Thanks!

[dmitry-a-morozov]

By debuggability I meant a code in between <@@ ... @@> or explicit constructed quotations cannot be debugged. (try to put break-point or step-into this code). The only way to diagnose problems in this code is to use old-fashion printf

[smoothdeveloper]

@dmitry-a-morozov I really need some discussion or that you show us how you can get a provided command instance ready provided a unit - SqlCommand

• I tried explained at https://github.com/fsprojects/FSharp.Data.SqlClient/pull/118#issuecomment-76329155 why having 3 things to pass as constructor instead of 1 is painful to manage in codebase and gives more room for mismatches
• I tried to show at https://github.com/fsprojects/FSharp.Data.SqlClient/commit/072071457dc3394afa8cf97fff4b3b2cc9e8ffc3#commitcomment-9950405 what I'm doing right now to get any of the provided commands ready to execute in my context without relying on unsupported ISqlCommand.

Given that in recent releases, API evolves toward giving less access to the base type (RuntimeSqlCommand or ISqlCommand) it makes it unclear if having a generic way to setup the commands is going to be supported, right now I have this.

let setupCommandWithContext<'TCommand when 'TCommand :> FSharp.Data.ISqlCommand> (createCommandFunctor: unit -> SqlCommand) (cmd: 'TCommand) =
  let dummyCommand = createCommandFunctor()
  let cmdAsIsqlCommand = cmd :> FSharp.Data.ISqlCommand
  cmdAsIsqlCommand.Raw.CommandTimeout <- dummyCommand.CommandTimeout
  cmdAsIsqlCommand.Raw.Transaction <- dummyCommand.Transaction
  cmdAsIsqlCommand.Raw.Connection <- dummyCommand.Connection
  ()

let createCommandWithContext<'TCommand when 'TCommand :> FSharp.Data.ISqlCommand and 'TCommand : (new : unit -> 'TCommand)> (createCommandFunctor: unit -> SqlCommand) =
  let cmd = (new 'TCommand())
  setupCommandWithContext createCommandFunctor cmd
  cmd

Issues are:

• I can't use the second: A generic construct requires that the type 'provided command type' have a public default constructor
• it means whenever I could have a single line expression, I now instead have to turn in into a three liner: one to call default constructor, second to call my setupCommandWithContext, third to execute the command, and in general a two step initialization is a bad idea, if me or my colleagues rely on default constructor (which is the only thing to do, other options leak a ton of state I don't want to see at call site) but forget to call setupCommandWithContext, it will silently work in dev environment (the connection string given to provided type works) but fail at run time
• it needs to access FSharp.Data.ISqlCommand which is getting increasingly unsupported for advanced users of the library trying to work around the fact that it can't be used in an extensible manner.

If you don't accept the PR (I can implement it on top of current master) which IMHO doesn't harm anything, and would open opportunities to stop adding more constructor overloads, can you clarify what I'm supposed to do?

My ideal is that:

• there shouldn't be default constructor (it's unusable in generic code as I've shown)
  • if it is needed for testing purpose, it can be replaced with a static method
  • if it's needed for sample purpose, the samples should convey how to use the library in real code
• with the constructor I propose, I can practially remove the need for every other one and expose static methods for CreateFromConnectionString, CreateFromConnectionStringName and CreateFromTransaction (don't force the user to also give connection in this case)
• have the library help reduces the need to leak or keep around those things:
  • reference to a connection
  • reference to a transaction
  • reference to a timeout setting
  • reference to a connection string
  • reference to a connection string name

[smoothdeveloper]

@dmitry-a-morozov I think what I need is addressed nicely with your comment https://github.com/fsprojects/FSharp.Data.SqlClient/commit/072071457dc3394afa8cf97fff4b3b2cc9e8ffc3#commitcomment-9990389

Thanks for keeping up and helping me figure this out.