Expose column names in row struct

[jpf91]

mysql-native's toStruct is quite limited currently (e.g. when compared to mysql-lited or vibe.data serialization). A better implementation is blocked mainly by the fact, that the Row does currently not know the column names. Only indices are available (but these depend on the sql query string, so it's also not easily possible to recover field names from these ids).

This PR simply exposes the names of the columns. A basic toStruct mapper could then look like this:

T parse(T)(const Row row)
{
    T result;
    row.parse(result);
    return result;
}

void parse(T)(const Row row, ref T result)
{
    foreach (size_t i, name; row.names)
    {
        result.setField(name, row[i]);
    }
}

void setField(T)(ref T result, string name, Variant value)
{
    import std.traits;

    foreach (idx, member; result.tupleof)
    {
        enum mname = T.tupleof[idx].stringof;
        if (name == mname)
        {
            alias FieldType = typeof(__traits(getMember, result, mname));
            convertField!FieldType(__traits(getMember, result, mname), value);
        }
    }
}

void convertField(T)(ref T field, Variant value)
{
    import std.traits, std.datetime;
    alias UT = Unqual!T;

    // Convert between varian of possily typeof(null) and Nullable!T
    static if (is(typeof(UT.nullify)))
    {
        if (value.type == typeid(typeof(null)))
            field.nullify();
        else
            field = value.coerce!(Unqual!(ReturnType!(UT.get)));
    }
    else static if (is(UT == DateTime))
    {
        field = value.get!UT;
    }
    // All other types
    else
    {
        field = value.coerce!UT;
    }
}

[jpf91]

#168 is related. An interface providing only opIndex(string) would required calling that function for every member name though, so wouldn't be very efficient for the conversion usecase. OTOH opIndex(string) is easy to implement with these changes here.

[Abscissa]

Thanks. This is actually an area I consider to be fairly high-priority for mysqln (as you've already noted, it's related to my own #168). It's something I've been itching to address for awhile, so I definitely appreciate this PR.

At this exact moment today, I'm in a little bit too much of an, ummm, festive late-October party mode, shall we say, to review this competently. But nonetheless, I wanted to reply ASAP to let you know I've received this PR and consider it a priority. I haven't overlooked it.

At an initial glance, it looks like the CI build failures are entirely unrelated to this PR (I'll have to address that.) So that's good news. I'll take a closer look at this as soon as I get a chance. Thanks in advance for your patience.

[bausshf]

Is it just me or is "name" never initialized? It's just set as an empty array on the result.

It has values assigned to it, but it's never actually initialized.

It's an out parameter too which means it must be initialized before exiting the function's scope.

Maybe Github isn't showing enough information? (I didn't look at the full file, just what Github showed as changes.)

[jpf91]

Assigning to .lengthinitializes the array: https://github.com/mysql-d/mysql-native/pull/188/files#diff-f8e22a4543286a3a4c329f78076371e0R649

[bausshf]

Assigning to .lengthinitializes the array: https://github.com/mysql-d/mysql-native/pull/188/files#diff-f8e22a4543286a3a4c329f78076371e0R649

Yeah, I knew there was something I missed. (I think I was primarily looking at the left-hand-side.

[jpf91]

@Abscissa Any update here? I rely on this (or some similar mechanism) for one of my projects and it'd be nice if I could just use the upstream repo in the dub configuration ;-)

[Abscissa]

@jpf91 Sorry bout that, I'm been pretty busy lately. I'll make sure to take a look at this as soon as I can.

[Abscissa]

This mostly looks good, but Row.names shouldn't be publicly writable. Let's make that a private _names (or package access if need be), and toss in a public string getName(size_t index) for read-only access. With that change made, I'll merge this PR.

If we want, we could add an improved API for reading the names separately from this PR. But I want to avoid starting names out as public writable, because tightening that up to read-only later on would be a breaking API change.

[jpf91]

@Abscissa good point. I've pushed the changes as you suggested.

[bausshf]

I think a way to iterate through the names would be nice saving you from multiple calls to retrieve all the names.

[Abscissa]

First of all, thanks for your patience, and I apologize for all the delays this has taken. I'm going to merge this PR now (it looks good, I like it) and tag a new release.

But regardless of that, I wanted to ask (probably a stupid question) about this particular line. I have to admit I'm not 100% up-to-date with the details of D's inout. IIUC, I think this function signature means that if the user has a Row, const(Row) or immutable(Row), then the return value of getName will be either string, const(string) or immutable(string) respectively. Is that correct?

[jpf91]

Thanks for merging this and sorry for the late answer: I don't know too much about inout either, I just copied it from inout(Variant) opIndex(size_t i) inout. However, as far as I know your understanding is correct. Thinking about it now, the inout here is probably useless.

[Abscissa]

I think a way to iterate through the names would be nice saving you from multiple calls to retrieve all the names.

Not necessarily a bad idea, but I don't think it's worth holding up this PR any further. Please file this as a separate issue so we don't loose track of it when I merge this PR.

[Abscissa]

Tagged v2.3.0

[Abscissa]

I just remembered this issue started out with a suggestion for toStruct improvements. I'll open a new ticket for that.