Dapper: Add support for order-agnostic constructor parameter SqlMapper.ITypeMap implementations

[carusology]

Background

I love Dapper because it adds great support for hydrating immutable objects from a database. For example, it allows us to hydrate objects of the following type like so:

public class Example {

    public Guid Id { get; }
    public String Name { get; }
    public DateTime Created { get; }
    public Guid CreatedBy { get; }
    public DateTime Updated { get; }
    public Guid UpdatedBy { get; }

    public Example(
        Guid id,
        String name,
        DateTime created,
        Guid createdBy,
        DateTime updated,
        Guid updatedBy) {

        this.Id = id;
        this.Name = name;
        this.Created = created;
        this.CreatedBy = createdBy;
        this.Updated = updated;
        this.UpdatedBy = updatedBy;
    }

}

public IEnumerable<Example> GetExamples() {
    var columns = String.Join(
        ","
        typeof(Example)
            .GetTypeInfo()
            .GetProperties()
            .Select(property => $"`{property.Name}`")
    );

    return this.connection.Query<Example>($"SELECT {columns} FROM ExampleTable");
}

Problem

Unfortunately, the columns provided to the query parameters must be in the same order that the constructor parameters are in. So, given the above example, if I changed the Example to look like this...

public class Example {

    public Guid Id { get; }
    public String Name { get; }
    /* [ ... the rest removed for emphasis ... ] */

    public Example(
        String name,
        Guid id,
        /* [ ... the rest removed for emphasis ... ] */) {
    }

}

... it will fail. Here is where I'm not 100% because I have a hard time reading the IL, but my understanding is that first it will fail because the DefaultTypeMap won't find the constructor because the names are in a different order. You can get around this by making your own SqlMapper.ITypeMap implementation that does not care about the order of the SQL query values matching the order of the constructor. However, this just makes the code fail (via, at least in .NET core, with weird NullReferenceExceptions) later because the SqlMapper code enumerates on the constructor parameters with the assumption they are in the same order as the SQL query.

Suggestion

Update the SqlMapper logic here such that it does not enumerable over the ConstructorParameter objects assuming they are in the same order from which they were returned from the query.

Workaround

The workaround for this is to fetch the SqlMapper.ITypeMap implementation Dapper has plugged in for the type you're about to query via this method. From there you can fetch the ConstructorInfo being used, if one happens to be used. This has the following negative consequences:

1. You have to reproduce the ConstructorInfo processing code which involves Reflection, and you have to call it every time in case the SqlMapper.ITypeMap implementation for the objects you are hydrating changed (and even then there's a race condition).
2. You have to add branching to your code if you have a mix of immutable and mutable objects that you hydrate to account for objects which have property setters instead of constructor arguments.

[NickCraver]

I'm not sure I fully understand the use case. Matching the constructor precisely is very intentional. For example, what if the constructor was 4 strings? How would you make any logical assumptions about ordering?

Unless I'm misunderstanding you're saying "it will fail" above when you really mean "it won't run the constructor that doesn't match", is that correct? I want to make sure we're in the same page here.

[mgravell]

is the idea here that name-based matches should also be OK, regardless of
order?

On 25 Apr 2017 1:18 a.m., "Nick Craver" [e-mail] wrote:

I'm not sure I fully understand the use case. Matching the constructor
precisely is very intentional. For example, what if the constructor was
4 strings? How would you make any logical assumptions about ordering?

Unless I'm misunderstanding you're saying "it will fail" above when you
really mean "it won't run the constructor that doesn't match", is that
correct? I want to make sure we're in the same page here.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/StackExchange/Dapper/issues/752#issuecomment-296856436,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABDsNkN1Qa4U4owLPK2zHI6ei-M3m2hks5rzTvGgaJpZM4NGtc9
.

[carusology]

Thanks for the prompt response and giving me the benefit of the doubt on your questions.

@mgravell has the right of it, @NickCraver. The solution I'm proposing is to use the name of the ConstructorInfo parameters to identify where the column could potentially map to, rather than the order in which the parameter appears in the constructor. For sure, four strings should not match a bunch of integers regardless of their parameter names. You would still want each potential constructor parameter to go through the other normal type checking (i.e. number of columns equals number of constructor parameters, parameter type matches column type, etc.) to validate that the potential mapping is valid.

Using the name for column to constructor parameter mapping isn't new to Dapper, as the DefaultTypeMap implementation does precisely that right now, first to validate the column names via SqlMapper.ITypeMap.FindConstructor() here, and second to fetch the ParameterInfo object on the identified ConstructorInfo via SqlMapper.ITypeMap.GetConstructorParameter() here.

It is important to note that these two checks are case-insensitive, so if you have two constructor parameters with the same names but in different cases (foo and FOO) this would fail because the same constructor parameter would get fetched twice (whichever one was first) in DefaultTypeMap.GetConstructorParameter() here. This means unique constructor parameter names, regardless of case, are already required with the DefaultTypeMap implementation.

To add more clarity, I did a quick rewrite of the current DefaultTypeMap.FindConstructor() that shows how it would work here. This isn't enough because SqlMapper still expects the order to match here, hence me writing this issue instead of simply making my own SqlMapper.ITypeMap implementation and going on with my day.

[carusology]

I should point out that edge cases are already solved very elegantly with Dapper. If you have two constructors like this...

class MyExample {
    public String Foo { get; }
    public Guid Bar { get; }
    public MyExample(String foo, Guid bar) { /* */ }
    public MyExample(Guid bar, String foo) { /* */ }
}

... the SqlMapper.ITypeMap implementation can match either constructor and move on. If you want to use a specific one, slap on the [ExplicitConstructor] attribute.

[carusology]

Let me answer the questions directly:

For example, what if the constructor was 4 strings? How would you make any logical assumptions about ordering?

The names of the columns would be checked against the names of the constructor parameters.

Unless I'm misunderstanding you're saying "it will fail" above when you really mean "it won't run the constructor that doesn't match", is that correct?

Kind of. I would argue the constructor _does_ match in principle, because it is obvious that SELECT foo, bar FROM MyExample matches the constructor MyExample(String bar, Guid foo) assuming the data stored in those columns map to those C# types. I am raising this issue because the current Dapper implementation is inconsistent with my more liberal interpretation of a "matching constructor."

[NickCraver]

Followup: how should params be handled?

[carusology]

Interesting edge case.

I believe that goes beyond the scope of this issue. From what I have read of Dapper's code, I do not believe params is currently supported (_maybe_ it is if you slap on [ExplicitConstructor]?), but it isn't necessary to support the type of names-based mapping for which I am advocating. Note that this belief is couched on my assumption that it is not possible for a SQL query to return an array as the value for a column.

Anyway, if I was to implement it, I would trust the developer consuming it, and further relax the constructor mapping to support a params-based constructor. This could be done by expanding the sorting used to identify a constructor in DefaultTypeMap.FindConstructor().

Currently, Dapper sorts constructors in the following order:

1. accessor (first public, then protected, then private)
2. number of parameters (more parameters to less parameters)

I would change this to go by the following order instead:

1. presence of params (non-params first, params second)
2. accessor (first public, then protected, then private)
3. number of parameters (more parameters to less parameters)

Then, for each of the params cases, I would check if it was possible for the data provided via the SQL query to fit into the constructor. So if the constructor signature looks liked this...

MyImmutable(String foo, Guid bar, params int[] values) { /* [ ... ] */ }

... I would expect it to be compatible with a query like this:

SELECT Bar, Foo, Bizz, Buzz FROM MyImmutable;

DESCRIBE MyImmutable;
+--------------+-------------------+------+-----+---------+-------+
| Field        | Type              | Null | Key | Default | Extra |
+--------------+-------------------+------+-----+---------+-------+
| Bar          | char(36)          | NO   |     | NULL    |       |
| Foo          | varchar(255)      | NO   |     | NULL    |       |
| Bizz         | int (11)          | NO   |     | NULL    |       |
| Buzz         | int (11)          | NO   |     | NULL    |       |
+--------------+-------------------+------+-----+---------+-------+

This means a constructor of MyImmutable(params object[] values) would match all queries, but it would only be used as a last resort.

---

By the way, I am more than happy to contribute to this change, though I think I would struggle mightily with the explicit IL updates. I have always used compiled expressions to create cached delegates instead (like this).

[NickCraver]

Thanks for all the work here @carusology - added to the v2 list to see if we can support it.