Feature request: string parameter overloads to help with lack of implicit conversion in F#

[chadunit]

This library uses implicit conversions for things like SQLiteObjectName, allowing a C# consumer to provide bare strings in places where the type is expected.

Implicit conversions are not supported in F# and thus require explicit use of the constructor e.g. SQLiteObjectName("table"), resulting in some mild extra verbosity. See example below:

open Tortuga.Chain
open Tortuga.Chain.SQLite

// model
type MyTable = { id : int64 }

let chain = SQLiteDataSource("Data Source=TestChain.db;Version=3;")
chain.Sql("DROP TABLE IF EXISTS MyTable;").Execute() |> ignore
chain.Sql("CREATE TABLE MyTable (id INTEGER PRIMARY KEY);").Execute() |> ignore

// cannot use "MyTable" bare string here in F#
// error FS0041: No overloads match for method 'Insert'.
// chain.Insert("MyTable", { id = 1L }).AsNonQuery().Execute() |> ignore

// must construct SQLiteObjectName()
chain.Insert(SQLiteObjectName("MyTable"), { id = 1L }).AsNonQuery().Execute() |> ignore

Although this can be somewhat mitigated on the consumer side by introducing extra variables or helper functions, it would be nice to consider providing string parameter overloads that could take bare string identifiers and convert them internally.

[Grauenwolf]

This will have to be done for each database separately. It will double the API surface area, so we need to be careful to keep it in sync when new methods are added.

It should not require changes to Chain.Core.

Risk is low if we just forward calls to the XxxObjectName variants. Do NOT duplicate implementation details, however minor. That will make our test coverage drop dramatically.

[Grauenwolf]

SQLite is up first since we need to fix the quoting bug anyways.

[chadunit]

Great, thanks for taking a look!

FYI no urgency needed on my account, file these under "nice-to-have", and I don't have an immediate need for keyword-conflicting table names either, just something I ran into playing around.

[Grauenwolf]

Almost done with the fix. Were there any other things you noticed that caused problems (or even just annoyances) for F#?

[chadunit]

Thanks, the overload works great. Haven't run into anything else actionable on your end but I'll keep an eye out. F#'s new anonymous records don't work for object materialization yet but I expect they will after https://github.com/Microsoft/visualfsharp/issues/6214 makes it into preview bits. They do work fine for query parameters.

[Grauenwolf]

That's really cool. I wish VB and C# had anonymous records. It would really cut down on the number of DTOs I need to write when I'm only pulling out a couple of fields.

[chadunit]

I may have spoken too soon. The Insert overload works, but From does not and it's a bit strange.

Using code like this:

chain.From("Table1", { id1 = 1L }).AsCount().Execute()

Produces run time exception:

Tortuga.Chain.MissingObjectException
  HResult=0x80131500
  Message=Could not find table or view Object
  Source=Tortuga.Chain.SQLite
  StackTrace:
   at Tortuga.Chain.SQLite.SQLiteMetadataCache.GetTableOrViewInternal(SQLiteObjectName tableName) in C:\Source\Chain\Tortuga.Chain\Tortuga.Chain.SQLite.source\shared\SQLite\SQLiteMetadataCache.cs:line 64
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Tortuga.Chain.SQLite.SQLiteMetadataCache.GetTableOrView(SQLiteObjectName tableName) in C:\Source\Chain\Tortuga.Chain\Tortuga.Chain.SQLite.source\shared\SQLite\SQLiteMetadataCache.cs:line 39
   at Tortuga.Chain.SQLite.SQLiteMetadataCache.GetTableOrViewFromClass[TObject]() in C:\Source\Chain\Tortuga.Chain\Tortuga.Chain.SQLite.source\shared\SQLite\SQLiteMetadataCache.cs:line 226
   at Tortuga.Chain.SQLite.SQLiteDataSourceBase.From[TObject](String whereClause, Object argumentValue) in C:\Source\Chain\Tortuga.Chain\Tortuga.Chain.SQLite.source\shared\SQLite\SQLiteDataSourceBase.CommandBuilders.cs:line 249
   at TestOrm3.tortugaChain() in C:\src\Server3\TestOrm3\TestOrm3.fs:line 256
   at TestOrm3.main(String[] argv) in C:\src\Server3\TestOrm3\TestOrm3.fs:line 335

It appears for some reason it is resolving to the generic overload From<TObject>(string,obj) with the type being a plain System.Object, even though I have not provided a type parameter.

I tried a small F#-only overload test with similar definitions, but contrary to above, the overload resolution does not exhibit the strange behavior, it "correctly" calls the non-generic version.

type MyClass() =
  member this.MyMethod(s:string, o:obj, [<Optional; DefaultParameterValue(null:obj)>] o2:obj) = 
    printfn "non-generic called"
  member this.MyMethod<'a when 'a : not struct>(s:string, o:obj, [<Optional; DefaultParameterValue(null:obj)>] o2:obj) = 
    printfn "generic called"

MyClass().MyMethod("", { id1 = 1L })  // prints "non-generic called"

I don't really know what's going on and without an isolated repo not sure I have enough for an F# issue report yet (if it's even an issue and not by design based on some unknown criteria). I'll probably try to look at it a bit more but cannot promise anything due to time/ignorance. Letting you know so you can decide how to proceed, in case you just want to revert or something for the time being.

[Grauenwolf]

When I try your sample, I get the error message "The record label 'id1' is not defined." so I can't test my theory.

Can you remove the optional parameter on the generic method?

In C# parlance I'm comparing

void From(string tableOrViewName, object filterValue, FilterOptions filterOptions = FilterOptions.None)
void From<TObject>(string whereClause, object argumentValue) where TObject : class

I'm think that it is preferring to bind to the generic version because the number of parameters.

[chadunit]

Ah, indeed you are correct, it picks the generic version in that case. Here is the updated and more complete example (full Program.fs for a .NET Core console app):

open System
open System.Runtime.InteropServices

type MyClass() =
  member this.MyMethod(s:string, o:obj, [<Optional; DefaultParameterValue(null:obj)>] o2:obj) =
    printfn "non-generic called"
  member this.MyMethod<'a when 'a : not struct>(s:string, o:obj) =
    printfn "generic called"

[<EntryPoint>]
let main argv =

  MyClass().MyMethod("", Object())

  printfn "done"
  Console.ReadLine() |> ignore
  0 // return an integer exit code

[Grauenwolf]

So basically we can't use optional parameters when there is an alternative that uses generics? That's no fun.

Can you talk with the F# people to see if this was intentional? It seems like a bad default because it is so different than the overload resolution we see in C# (and presumably VB).

[Grauenwolf]

In the mean time I'll open a ticket to offer a better error message for GetTableOrViewFromClass<TObject>.

https://github.com/docevaad/Chain/issues/263

[chadunit]

Yes I'll take this over to F# repo.

[Grauenwolf]

Thank you.

[chadunit]

Based on the other issue I think there's no way around it on F# side.

I think splitting the optional overload into 2 would technically work for cases of 1 optional argument, right? Non-optional args can still be named so it doesn't break callers using the named arg syntax. If there's only limited cases where this is needed (does anything other than From() have a conflicting string overload?) maybe it's viable without exploding the overload count too much.

[Grauenwolf]

If I recall correctly, there is never more than one optional parameter per method. So I think we can get away with that.

And since the string methods are new, making that last parameter non-optional should't break anyone's code.

When you have a change, test Tortuga.Chain.SQLite.2.1.6986.22197.nupkg please.

[chadunit]

It works!

[Grauenwolf]

Ok, we've got a plan.

Thank you for your help.

[Grauenwolf]

This is actually working out better than I expected. It forced me to think about how to better share code across the different databases. In the end we're going to end up with far less duplicated code than we had before adding all the new methods for F#.

[Grauenwolf]

All but SQL Server deployed.

We need to spilt out SqlServerDataSource and OleDbSqlSeverDataSource into separate packages in order to use the new design.

This is code complete and tested, but the new project headers and matching NuGet scripts need to be written.

We also need to give our sponsor a chance to weigh in. They commissioned the OleDb version of SQL Server a couple years ago. If they are still using it we can't pull the rug out from under then without warning.

[Grauenwolf]

And we're finally done!