Fix InvalidCastException on multi mapping

[azyobuzin]

When I used Query with multi-mapping, I found that InvalidCastException was thrown when the mapping type is a primitive type for example int, long, string, and so on.

Here is the reproducing code. MySQL Connector/.NET (ver. 8.0.13) returns the integer values in the SELECT statement as long. In the test cases of int and string, the expected behavior is that the method returns values converted to the specified type, but the actual behavior is that the method throws InvalidCastException.

using System;
using System.Collections.Generic;
using Dapper;
using MySql.Data.MySqlClient;

class Program
{
    static void Main(string[] args)
    {
        using (var connection = new MySqlConnection("server=localhost;uid=root"))
        {
            connection.Open();

            const string query = "SELECT 1 AS c1, 2 AS c2";
            //  ---------
            // | c1 | c2 |
            // |----|----|
            // |  1 |  2 |
            //  ---------

            Console.Write("long: ");
            Try(() => connection.Query(query, (long c1, long c2) => (c1, c2), splitOn: "c2"));

            Console.Write("int: ");
            Try(() => connection.Query(query, (int c1, int c2) => (c1, c2), splitOn: "c2"));

            Console.Write("string: ");
            Try(() => connection.Query(query, (string c1, string c2) => (c1, c2), splitOn: "c2"));
        }
    }

    static void Try<T>(Func<IEnumerable<T>> action)
    {
        try
        {
            var result = action();
            Console.WriteLine("OK ({0})", string.Join(", ", result));
        }
        catch (Exception ex)
        {
            Console.WriteLine("{0} ({1})", ex.GetType().Name, ex.Message);
        }
    }
}

Output:

long: OK ((1, 2))
int: InvalidCastException (Unable to cast object of type 'System.Int64' to type 'System.Int32'.)
string: InvalidCastException (Unable to cast object of type 'System.Int64' to type 'System.String'.)

This pull request fixes this problem by inserting a call to Convert.ChangeType in GenerateMapper method. And I wrote a test case and it passed on SQL Server 2016 LocalDB.

[NickCraver]

While this is one way to fix the problem, it adds extreme overhead converting every value coming back to try a coerce things. The fundamental issue underneath is we're trying to deserialize the wrong type. We either need to return integers for integers, or use longs if we expect longs. The alternative (in the PR here) it to add a lot of overhead to paper over some type mismatches, which are ultimately that: mismatches. In such cases, it's better for us to fix the mismatch, rather than globally incurring penalties on performance for all users.

I know that answer may such (as the work has been done here), but I wanted to lay out alternative thinking plainly on tradeoffs here - thoughts?

[azyobuzin]

I drew on the existing implementation of normal Query to write this PR.

https://github.com/StackExchange/Dapper/blob/f486848e88a240708b473bdc67d7a12bc6586809/Dapper/SqlMapper.cs#L1098-L1106

I think the overhead is reasonable because this method was taken to by the original implementation.

[azyobuzin]

I have updated my branch and made it use SqlMapper.GetValue<T>, which is same way how QueryImpl converts values from the reader.

The fundamental issue underneath is we're trying to deserialize the wrong type. We either need to return integers for integers, or use longs if we expect longs.

I think Dapper should convert values to the right type. I will introduce our use cases for multi mapping.

Use Case 1: Get a column of joined tables

The first is to get all columns of a table and a column of joined tables or a calculated value at one time.

using Dapper;
using Microsoft.Data.Sqlite;

class Program
{
    static void Main(string[] args)
    {
        using var connection = new SqliteConnection("Data Source=:memory:");
        connection.Open();

        connection.Execute(@"
CREATE TABLE T1 (Id, Value);
CREATE TABLE T2 (Id, Count);
INSERT INTO T1 (Id, Value) VALUES (@id, @value);
INSERT INTO T2 (Id, Count) VALUES (@id, @count);",
            new { id = 1, value = "string", count = 1 });

        // Get all columns of T1 and some columns or calculated values from joined tables
        var tuples = connection.Query(
            "SELECT T1.*, T2.Count FROM T1 INNER JOIN T2 ON T1.Id = T2.Id",
            (T1 x, int y) => (x, y),
            splitOn: "Count");
    }
}

public class T1
{
    public int Id { get; set; }
    public string Value { get; set; }
}

public class T2
{
    public int Id { get; set; }
    public int Count { get; set; }
}

Use Case 2: Check whether there is a record joined by LEFT JOIN

The second is to get all columns of a table and check whether joined record is available at one time.

using var connection = new SqliteConnection("Data Source=:memory:");
connection.Open();

connection.Execute(@"
CREATE TABLE T1 (Id, Value);
CREATE TABLE T2 (Id, Count);
INSERT INTO T1 (Id, Value) VALUES (@id, @value);",
    new { id = 1, value = "string" });

// Get all columns of T1 and check whether T2 record is available
var tuples = connection.Query(
    "SELECT T1.*, T2.Id IS NOT NULL AS T2Available FROM T1 LEFT JOIN T2 ON T1.Id = T2.Id",
    (T1 x, bool y) => (x, y),
    splitOn: "T2Available");

In both cases, y should not be long. In the first case, T2.Count is defined as an int field in the POCO. In the second case, y is a boolean value obviously. However the database provider may return that values as long and the current Dapper throws InvalidCastException. In addition, connection.Query<int>("SELECT Count FROM T2") works and the behavior of multi mapping seems to be inconsistent.

Overhead

https://github.com/azyobuzin/DapperMultimapBenchmark

The benchmark shows that the more columns we split, the more overhead it takes.

In common cases, I think the number of divisions is less than 4 and this overhead is acceptable in common cases.

[NickCraver]

We will continue to support conversion for most reasonable things as it does today, e.g. int -> long, and the reverse if it fits and such, but coercing other types adds additional overhead as well as soon boxing overhead compared to v3 plans.

If you need to bring back types, doing so at the SQL level and requiring no conversion on the client will remain the most optimal approach. Going to close this out since the situation hasn't changed here - this just isn't a performance regression we'd want to take. There are more lenient (and less performant) ORM options out there if this behavior is more desired than performance - that's just not in line with the goals of Dapper.