mysql-d / mysql-native

Native D client driver for MySQL/MariaDB, works with or without Vibe.d
https://github.com/mysql-d/mysql-native
Boost Software License 1.0
80 stars 28 forks source link

queryValue requires awkward get.get!T #195

Open schveiguy opened 5 years ago

schveiguy commented 5 years ago

queryValue returns a Nullable!Variant. Let's say it returns a long (In my case, I was querying a COUNT(*) to get the total rows)

My code I put was:

auto total = conn.queryValue("select count(*) from ...").get!long

But this doesn't work, because Nullable intercepts the get call! So instead I have to do:

auto total = conn.queryValue("select count(*) from ...").get.get!long

Which IMO is ugly and annoying. Really this is an issue with Nullable and Variant reusing the same member name, but I doubt that will change.

Is it possible to instead specify the type to queryValue itself, and have it return a Nullable!T? something like:

// no need for get here, I know I'm going to get a value
long total = conn.queryValue!long("select count(*) from ...");
schveiguy commented 5 years ago

Or alternatively, you can use a different mechanism from Nullable to indicate an empty set.

schveiguy commented 5 years ago

I suppose this is moot since Phobos' Nullable will eventually require a get call.

However, I'll leave this open, because an updated API surely could do better than requiring get.get.

schveiguy commented 5 years ago

This would also be alleviated somewhat with the move to TaggedUnion as discussed in #175

Abscissa commented 5 years ago

Is it possible to instead specify the type to queryValue itself, and have it return a Nullable!T? something like:

Sounds like an excellent idea.

Only potential problem is that some of the queryValue overloads already take templated varargs (for prepared statements):

Nullable!Variant queryValue(T...)(conn, sql, T args) {...}
int myInt=...;
myConnection.queryRow("SELECT * FROM `myTable` WHERE `a` = ?", myInt)

It shouldn’t be a problem to change it to:

Nullable!T1 queryValue(T1, T2...)(conn, sql, T2 args) {...}

Buuuut….we’d probably want Variant (or rather, TaggedUnion, whatever...) as a default, and I’m wondering if that would be inherently ambiguous:

Nullable!T1 queryValue(T1=Variant, T2...)(conn, sql, T2 args) {...}

Assuming that works, we’d also need to decide how to handle the case when the type requested doesn’t match the type received. Attempt a silent conversion? Throw? Support both possibilities using different function names: queryValueA vs queryValueB (but with better names ;) )???

This would also be alleviated somewhat with the move to TaggedUnion as discussed in #175

Yea, TaggedUnion definitely sounds like a good idea. One of my other projects, SDLang-D I think, switched from Variant to TaggedAlgebraic some time ago (before TaggedUnion existed) and it definitely proved to be a good move.

schveiguy commented 5 years ago

Only potential problem is that some of the queryValue overloads already take templated varargs

That is solved with double-layer templates:

template queryValue(Ret = Variant)
{
    Nullable!Ret queryValue(Args...)(Connection conn, string sql, Args args) { ...}
}

Yes, still does full IFTI. At that point, you cannot specify the Args part of the template, but I'm assuming that is not a problem.

Assuming that works, we’d also need to decide how to handle the case when the type requested doesn’t match the type received

Existing behavior is to throw, so just throw.

schveiguy commented 5 years ago

In regards to the switch to TaggedUnion, would probably be something like:

auto total = conn.queryValue("select count(*) from ...").get.longValue;

So really maybe this request will resolve itself once we get there.

Abscissa commented 5 years ago

That is solved with double-layer templates: [...] Existing behavior is to throw, so just throw.

Awesome, sounds great.

In regards to the switch to TaggedUnion, would probably be something like:

auto total = conn.queryValue("select count(*) from ...").get.longValue;

So really maybe this request will resolve itself once we get there.

Ahh, true. But maybe there's still value (perhaps for generic code?) in being able to specify the requested type as the actual type rather than via TaggedUnion's identifiers? In any case, the template idea is probably just a cleaner interface anyway: It's a little more idiomatic and doesn't require anyone to know anything whatsoever about TaggedUnion.

Oh, also, another thought on the whole migration thing: Maybe we could offer a simple util function to just convert the TaggedUnion to a Variant:

alias MySQLValue = TaggedUnion!blahblahwhatever...;

Variant toVariant(MySQLValue v) {...}
Nullable!Variant toVariant(Nullable!MySQLValue v) {...}
...
var = conn.queryValue(sql).toVariant;

Maybe that could even obviate the need for all those migration path gymnastics I outlined earlier (ugh, might've been in a different ticket...)

schveiguy commented 3 years ago

I suppose this is moot since Phobos' Nullable will eventually require a get call.

Don't know why I said this, it will still require get.get.