Array parameters and extending the library

[absolutejam]

Morning!

I've been working towards moving all of my DB access in F# to using your libraries, especially in the case of replacing EF Core. Kudos on the great work; the API is really clean and the docs are great!

I don't know if I'm looking in the wrong place or if it's not available, but I'm trying to add a string array parameter which seems to be supported by the underlying Npgsql library, but I can't seem to find the DU representation in the docs or source.

First of all, If I've totally overlooked something, please let me know and I'll be happy to contribute docs or whatever is needed. But if this isn't available, is it worth extending the DU to allow this scenario, but also consider a way for extensibility outside of source changes?

For example, a DU case that is simply NpgsqlOther of NpgsqlParameter (or unit -> NpgsqlParameter) , or possibly a function that is called with the overall Npgsql connection (Or whatever other mutable object it uses for state tracking) prior to execution, to perform any additional transformations outside of the scope of the API?

[akhansari]

Hello James
Very glad to see that the library will also be used in another prod code :)

Do you mean Arrays type?
Actually all the core wrapper logic is in Vp.FSharp.Sql and Vp.FSharp.Sql.PostgreSql contains only the Command, Transaction and Types. So yes I think it will be easy to add this in Types.fs but I am not sure if it's possible to extend without forking.

Wdyt @natalie-perret-1986?
I'll try make PR this week.

[natalie-o-perret]

@absolutejam @akhansari Hi there 🙋‍♀️,

I don't know if I'm looking in the wrong place or if it's not available, but I'm trying to add a string array parameter which seems to be supported by the underlying Npgsql library, but I can't seem to find the DU representation in the docs or source.

You're absolutely right, it's not supported atm, I'm kinda busy right now, and in all honesty I can't remember exactly why, but I gave it some thought last year, and from what I can recollect, there is bit of work involved for supporting arrays.

One way to put it is that it's fairly easy to add the support, BUT, it's hard to get it right from an API standpoint (in terms of consistency), I need to re-think about it cause it's a wee fuzzy as I'm in the middle of a lot of things at work.

but also consider a way for extensibility outside of source changes?

Yea, there is something somewhere like that in my tiny brain, but again the so-called "right design" is hard to nail.

For example, a DU case that is simply NpgsqlOther of NpgsqlParameter (or unit -> NpgsqlParameter) , or possibly a function that is called with the overall Npgsql connection (Or whatever other mutable object it uses for state tracking) prior to execution, to perform any additional transformations outside of the scope of the API?

This is also something I thought about and yea in theory, this is the way I envisioned adding support for not-yet-supported types but again there was something that didn't really click at the time.

Is that okay if I'm getting back to you two this coming Sat. (probably night / evening)?

[absolutejam]

Thanks for the feedback 😄

I totally understand - As soon as you implement it, it gets relied upon and becomes a lot harder to change!

There's no rush, I can totally understand being swamped 😓

I have forked it locally for now and I've added the following temporary workarounds for my usage...

Extended PostgreSqlDbValue DU:

    | IntegerArray of int32 array
    | UuidArray of Guid array
    | NpgsqlOther of NpgsqlParameter

Extended DbValueToParameter function (I also had to make the local let parameter binding mutable for the NpgsqlOther to work):

    | IntegerArray value ->
        parameter.Value <- value
        parameter.NpgsqlDbType <- NpgsqlDbType.Array ||| NpgsqlDbType.Integer
    | UuidArray value ->
        parameter.Value <- value
        parameter.NpgsqlDbType <- NpgsqlDbType.Array ||| NpgsqlDbType.Uuid
    | NpgsqlOther p ->
        parameter <- p

Obviously I've only tackled a couple of array types that I needed as well as adding a back-door. I'm not sure what greater ramifications this could have, but this seems to be working just fine for me at the moment.

Happy to chat about this here or on Slack, and I'm happy to contribute changes & tests where required.

[natalie-o-perret]

WIP - sticky notes

Passively tinkering: https://www.npgsql.org/efcore/mapping/array.html

For example, a DU case that is simply NpgsqlOther of NpgsqlParameter (or unit -> NpgsqlParameter)

@absolutejam
So I gave it some thought, and this does look tempting.
We might need to agree upon a name but other than that, should be fine.

It would be a great addition in terms of extensibility for the library, when it comes to using parameters that aren't yet fully baked-in. This also could be done for other Vp.FSharp.Sql DB providers

I like NpgsqlParameter over NpgsqlOther, maybe Raw could just do as well.

I'm still giving it a few more days to see if there is any sort of caveats that we may have missed out.

Thanks again for opening this issue, really appreciated!

[natalie-o-perret]

and there she is, back 👩‍🚀

It's nice to take some time to think about a design, and I might have found something that in terms of consistency isn't all grand-like.

Assuming new DU cases like below, in the case of a broad generic system to support whatever NpgsqlParameter:

    | IntegerArray of int32 array
    | UuidArray of Guid array
    | NpgsqlOther of NpgsqlParameter

My issue is that if you pass a NpgsqlParameter, this class actually carries a parameter name of its own which may or may not take precedence over the "other" parameter name already carried by the tuple:

Vp.FSharp.Sql.PostgreSql/Vp.FSharp.Sql.PostgreSql/Types.fs

Lines 104 to 109 in e6f7957

	[<AbstractClass; Sealed>]
	type internal Constants private () =
	static member DbValueToParameter name value =
	let parameter = NpgsqlParameter()
	parameter.ParameterName <- name

Which makes things not only inconsistent but also counter-intuitive and to some extent might even mislead the developers using the library.

So in order to make things slightly more consistent, I'd suggest , instead, to have the DU case as follows:

//... whatever goes here
    | NpgsqlOther of (NpgsqlDbType * obj)

//... some other things go here
    | NpgsqlOther (dbType, value) ->
        // plain and simple
        parameter.Value <- value
        parameter.NpgsqlDbType <- dbType

@absolutejam @akhansari wdyt?

Btw, I'm still not too convinced about the name, i.e. NpgsqlOther (despite the fact that I cannot come up with anything substantially better).

Note:
Only annoying bit is that peeps might need to box explictely to fit the snd obj requirement of the tuple, i.e. the one feeding NpgsqlParameter.Value

[absolutejam]

Makes sense to me!

I left it fully open initially in case there was some super-advanced stuff needed to be done with the raw NpgsqlParameter (I have no idea if there is; I never delve this deep 😂). But your proposal makes it nice and apparent and more searchable too.

And from my super in-depth testing, I think the boxing is automatic in this case:

type Foo =
  | CaseA of string
  | CaseB of string * obj

> let x [| CaseB ("x", 10); CaseB ("y", "apple") |];;
val x : Foo [] = [|B ("x", 10); B ("y", "apple")|]

> match x.[0] with
- | CaseB (_, second) -> second
- | _ -> failwith "lol";;
val it : obj = 10

I'll leave the naming to you... that's the hardest part 😈

Did you want to bother with explicit array parameters, or just leave these to the open-ended case?

[natalie-o-perret]

But your proposal makes it nice and apparent and more searchable too.

I just wanted something consistent with the rest of the pre-existing design, if I were to pass NpgsqlParameter I would probably add a whole other way to pass parameters. But it believe this would would defeat the whole purpose of conciseness brought by the library.

Did you want to bother with explicit array parameters, or just leave these to the open-ended case?

I think I'm gonna start with that raw / open / whatever (NpgsqlWhatev sounds like a sweet lullaby to my ears) and then we can add the support for pg arrays down the line (among some others: https://www.postgresql.org/docs/current/datatype.html)

In all seriousness, these below are what I'm struggling with:

• Object
• Raw
• Other
• Custom
• NpgsqlType

Pseudo-rationale

• I would probably settle for Raw, Custom, Other cause NpgsqlType sounds really off.
• Looking at the sql server provider, we have: https://github.com/veepee-oss/Vp.FSharp.Sql.SqlServer/blob/main/Vp.FSharp.Sql.SqlServer/Types.fs#L56
• And since we likely want this Other thingy in every provider I would rule out Object, i.e. could be confused for a sql variant type.
• Raw reminds me of "raw bytes", so probably a nay.
• Custom is a teeny tiny more explicit, Other a bit less.
• Putting everything into perspective, the DU is supposed to represent and list the native DB types with the relevant .NET counterparts.
• In that regard, I'd say Custom might also be misleading, something custom like a custom type on the DB side akin to a variant. Therefore, I'm leaning towards Other which despite being vague serves that purpose of something not defined purely as a DB type.
• BUT I tend to use custom in other libs at work in the same way I would impl. it here

😕

I'll leave the naming to you... that's the hardest part 😈

Note: don't edit GitHub comments using a browser on a smartphone

[natalie-o-perret]

Custom or Other.

What a terrible dilemma 😐

[akhansari]

Why not CustomType

[natalie-o-perret]

Why not CustomType

For the same reasons Custom might or might not do (and conversely when it comes to Other / OtherType)

[natalie-o-perret]

And so I hereby decide, Custom it is, and Custom it shall be.

Pseudo-rationale, part 2: for the grand sake of the mighty consistency and argumentum ad verecundiam.

It is usually expected in F# libraries that when you have a bunch of DU cases already defined for fixed types but eventually boiling down to the same type bts in your implementation, that Custom is a DU case not only enabling you to recreate the execution path leading to other existing DU cases without requiring you to add other functions but also to add whatever options (i.e. types) that aren't already baked in without any substantial code modifications.

Yes, this is pretty much an empirical slash appeal from authority thingy, but at least the case is now closed.

Note 1: 3-4am edits are bad.
Note 2: should add a template for filing new issues

[natalie-o-perret]

Should anyone present know of any reason that this design decision should not be joined in holy matrimony, speak now or forever hold your peace.

[natalie-o-perret]

Keep this issue open because we can add array support but a more generic approach is preferred so that users aren't forced to fork the repo to do so.

[natalie-o-perret]

In retrospect, again, some more food for thought:

• https://dba.stackexchange.com/a/192873/181339
• https://github.com/veepee-oss/Vp.FSharp.Sql/blob/main/DESIGN.md?plain=1#L58

[natalie-o-perret]

@absolutejam 🙋‍♀️ long time no see, you might be interested in https://github.com/veepee-oss/Vp.FSharp.Sql.PostgreSql/pull/16/files
(I know, it's probably wayyyy long overdue)

With the impl available in the PR, this is what it would look like:

// Valid stuff in regard to PG / .NET type consistency, but no safeguard when it comes to the initial intent
PostgreSqlDbValue.Array         (NpgsqlDbType.Integer, [| 123;   456   |]) |> ignore // Valid
PostgreSqlDbValue.Array         (NpgsqlDbType.Text,    [| "abc"; "def" |]) |> ignore // Valid
​
// Valid stuff in regard to PG / .NET type consistency, safeguard when it comes to the initial intent via generics
PostgreSqlDbValue.Array<int32>  (NpgsqlDbType.Integer, [| 123;   456   |]) |> ignore // Valid
PostgreSqlDbValue.Array<string> (NpgsqlDbType.Text,    [| "abc"; "def" |]) |> ignore // Valid
​
// Invalid stuff in regard to PG / .NET type consistency
PostgreSqlDbValue.Array         (NpgsqlDbType.Text,    [| 123;   456   |]) |> ignore // Invalid (runtime)
PostgreSqlDbValue.Array         (NpgsqlDbType.Integer, [| "abc"; "def" |]) |> ignore // Invalid (runtime)
PostgreSqlDbValue.Array<int32>  (NpgsqlDbType.Text,    [| 123;   456   |]) |> ignore // Invalid (runtime)
PostgreSqlDbValue.Array<string> (NpgsqlDbType.Integer, [| "abc"; "def" |]) |> ignore // Invalid (runtime)
​
// Invalid stuff in regard to PG / .NET type consistency, safeguard when it comes to the initial intent via generics
PostgreSqlDbValue.Array<string> (NpgsqlDbType.Text,    [| 123;   456   |]) |> ignore // Invalid (compilation)
PostgreSqlDbValue.Array<int32>  (NpgsqlDbType.Integer, [| "abc"; "def" |]) |> ignore // Invalid (compilation)

[natalie-o-perret]

Solved in https://github.com/veepee-oss/Vp.FSharp.Sql.PostgreSql/releases/tag/v1.0.31