RawSqlQuery minimizing risk of field name confusion

By Posted on

Problem

Is there a way to make the following code more maintainable?

public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    const string selectList = "Quantity, NumDone";
    var sql =
        @$"select  convert(int,numordered) as Quantity , convert(int,sum(isnull(numDone,0)))  as NumDone
            from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
            inner join job j on k.jobid = j.jobid 
            where k.jobtaskid = {jobTaskId}
            group by k.jobtaskid, j.NumOrdered";

    var tokens = selectList.Tokenize();
    var quantityOrdinal = Array.IndexOf(tokens, "Quantity");
    var numdoneOrdinal = Array.IndexOf(tokens, "NumDone");
    var dtos = DataHelpers.RawSqlQuery(sql, x => new DtoNumOrderedNumDone()
    {
        NumDone = x.GetInt32(numdoneOrdinal),
        Quantity = x.GetInt32(quantityOrdinal)
    }
    );
    var dto = dtos.FirstOrDefault();
    return dto;
}

I aren’t worried about sql injection because jobTaskId is an int, however I am worried that the code is vulnerable to being altered later and the field order being made incorrect.

[Update]

The helper code is

public static class DataHelpers
{
    public static List<T> RawSqlQuery<T>(string query, Func<DbDataReader, T> map, params SqlParameter[] parameters)
    {
        try
        {
            using var context = MakeDbContext();
            return RunQuery(context, query, map, parameters);
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            throw;
        }
    }
    public static List<T> RunQuery<T>(MyDbContext context, string query, Func<DbDataReader, T> map,
        params SqlParameter[] parameters)
    {
        try
        {
            var cn = context.Database.GetDbConnection();
            var oldState = cn.State;
            if (cn.State.Equals(ConnectionState.Closed)) cn.Open();
            using var command = cn.CreateCommand();
            command.CommandText = query;
            command.CommandType = CommandType.Text;
            foreach (var param in parameters) command.Parameters.Add(param);
            if (cn.State.Equals(ConnectionState.Closed)) cn.Open();
            var entities = new List<T>();
            using (var result = command.ExecuteReader())
            {
                while (result.Read())
                {
                    var r = map(result);
                    entities.Add(r);
                }
            }

            if (oldState.Equals(ConnectionState.Closed) && cn.State == ConnectionState.Open) cn.Close();
            return entities;
        }
        catch (Exception e)
        {
            MessageBox.Show($"RunQuery inner: {e.InnerException}, ex:{e} rn {query}");
            Console.WriteLine(e);
            throw;
        }

    }

[Update]

Exploring ISR5’s answer I get

this error message

Solution

If you look at the GetNumOrderedNumDone method then you can see that you have repeated the column names three times. It can cause problem when you need to rename one of them, because you have to update it in multiple places.

There are several ways to fix this:

  • Define consts for column names
  • Define a custom attribute, decorate the DTO’s properties and use reflection to retrieve column names
  • etc.

I will show you the first approach, but in order to get there we need to do some refactoring.

1st iteration

As discussed in the comments section if you change the RunQuery to receive a Func<DbDataReader, List<T>> parameter then the column indexes can be cached (on the map function implementation side) and be reused

public static List<T> RunQuery<T>(MyDbContext context, string query, 
    Func<DbDataReader, List<T>> map,
    params SqlParameter[] parameters)
{
    try
    {
        //...
        if (cn.State.Equals(ConnectionState.Closed)) cn.Open();

        using var reader = command.ExecuteReader();
        var entities = reader.HasRows ? map(reader) : new List<T>();
                    
        if (oldState.Equals(ConnectionState.Closed) && cn.State == ConnectionState.Open) 
            cn.Close();
        return entities;
    }
    catch (Exception e)
    {
        //...
    }
}
  • Since some responsibility has been shifted to the consumer of your helper method that’s why the DbDataReader usage became cleaner
  • I’ve added here the HasRows check to avoid calling map function unnecessary if there is nothing to map

Then your GetNumOrderedNumDone can be rewritten like this:

public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    const string QuantityColumnName = "Quantity", NumDoneColumnName = "NumDone";
    var sql =
        @$"select  convert(int,numordered) as {QuantityColumnName} , convert(int,sum(isnull(numDone,0)))  as {NumDoneColumnName}
    from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
    inner join job j on k.jobid = j.jobid 
    where k.jobtaskid = {jobTaskId}
    group by k.jobtaskid, j.NumOrdered";

    var dtos = DataHelpers.RawSqlQuery(sql, reader => {
        int quatityColumnIndex = reader.GetOrdinal(QuantityColumnName);
        int numDoneColumnIndex = reader.GetOrdinal(NumDoneColumnName);

        return reader.Cast<IDataRecord>().Select(record => new DtoNumOrderedNumDone
        {
            NumDone = record.GetInt32(quatityColumnIndex),
            Quantity = record.GetInt32(numDoneColumnIndex)
        }).ToList();
    }
    );
    var dto = dtos.FirstOrDefault();
    return dto;
}
  • I’ve defined two constants that are storing the column names
    • Since you are already using string interpolation for your query it is really convenient to use these there as well
  • Inside the RawSqlQuery first I’ve cached the column indexes to be able to reuse them multiple times
  • The second part of the map implementation is a bit tricker
    • If you have a DbDataReader then you can use the Read method to iterate through it or you can treat it as IEnumerable
      • If you are unaware of this capability then I highly recommend to read these SO topics: 1, 2
    • So, after we have converted the DbDataReader to IEnumerable<IDataRecord> we can use Linq

2nd iteration

What is the problem with the first one?

The RunQuery is generic enough to be used for queries which might return 0, 1 or multiple rows. In your particular case the cardinality is 0..1. So, calling the ToList inside the RawSqlQuery then calling the FirstOrDefault on the returned collection seems a bit overkill.

So, if you introduce an overload for the RunQuery (and for the RawSqlQuery as well) which can return 0 or 1 entity then the usage could be more streamlined.

Let’s start with the RunQuery

public static T RunQuery<T>(MyDbContext context, string query,
    Func<DbDataReader, T> map,
    params SqlParameter[] parameters)
{
    try
    {
        //...
        if (cn.State.Equals(ConnectionState.Closed)) cn.Open();

        using var reader = command.ExecuteReader();
        T entity = reader.Cast<IDataRecord>().Count() switch {
            1 => map(reader),
            0 => default,
            _ => throw new InvalidOperationException("Query returned more than 1 rows")
        };
                    
        if (oldState.Equals(ConnectionState.Closed) && cn.State == ConnectionState.Open) 
           cn.Close();
        return entity;
    }
    catch (Exception e)
    {
        //...
    }
}
  • Here I’ve used the Count() instead of HasRows to be able to handle each cases accordingly
    • If there is only one record then it should call the map function
    • If there is zero matching record then it should return default(T)
    • If there are more than one matching records then it should throw exception

The usage could be simplified like this

public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    const string QuantityColumnName = "Quantity", NumDoneColumnName = "NumDone";
    var sql =
        @$"select  convert(int,numordered) as {QuantityColumnName} , convert(int,sum(isnull(numDone,0)))  as {NumDoneColumnName}
    from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
    inner join job j on k.jobid = j.jobid 
    where k.jobtaskid = {jobTaskId}
    group by k.jobtaskid, j.NumOrdered";

    return DataHelpers.RawSqlQuery(sql, reader =>
        reader.Cast<IDataRecord>().Select(record => new DtoNumOrderedNumDone
        {
            NumDone = record.GetInt32(record.GetOrdinal(QuantityColumnName)),
            Quantity = record.GetInt32(record.GetOrdinal(NumDoneColumnName))
        }).FirstOrDefault()
    );
}
  • For the sake of brevity I’ve omitted the exception handling but you should do that
  • Since we know that the Select will be called only once that’s why we do not need to cache the column indices

UPDATE #1: map(reader) problem

I have to confess that I have rarely used DbDataReader in the past decade so I have forgotten that Cast<IDataRecrod> creates a forward-only iterator. There is no real Reset function which can rewind that IEnumerable. So, after calling the Count method the iteration reaches its end. That’s why it can’t be reiterated.

There are several workarounds for that, like:

  • Issue two select statements where the 2nd is SELECT @@ROWCOUNT and then use NextResult to move the iterator to next result set
  • Load the DataReader into a DataTable and pass that around
  • Use SingleOrDefault on the map implementation side to enforce 0..1 cardinality
  • Pass IDataRecord to map instead of DbDataReader
  • etc.

Let me show you the last two options

SingleOrDefault

public static T RunQuery<T>(MyDbContext context, string query, 
    Func<DbDataReader, T> map,
    params SqlParameter[] parameters)
{
    try
    {
        //...
        if (cn.State.Equals(ConnectionState.Closed)) cn.Open();

        using var reader = command.ExecuteReader();
        var entity = map(reader);
                    
        if (oldState.Equals(ConnectionState.Closed) && cn.State == ConnectionState.Open) 
           cn.Close();
        return entity;
    }
    catch (Exception e)
    {
        //...
    }
}
  • The RunQuery is simplified since the cardinality enforcement is shifted to the map function
public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    const string QuantityColumnName = "Quantity", NumDoneColumnName = "NumDone";
    var sql =
        @$"select  convert(int,numordered) as {QuantityColumnName} , convert(int,sum(isnull(numDone,0)))  as {NumDoneColumnName}
    from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
    inner join job j on k.jobid = j.jobid 
    where k.jobtaskid = {jobTaskId}
    group by k.jobtaskid, j.NumOrdered";

    return DataHelpers.RawSqlQuery(sql, reader =>
       reader.Cast<IDataRecord>().Select(record => new DtoNumOrderedNumDone
       {
           NumDone = record.GetInt32(record.GetOrdinal(QuantityColumnName)),
           Quantity = record.GetInt32(record.GetOrdinal(NumDoneColumnName))
       }).SingleOrDefault()
    );
}

IDataRecord

NOTE: I have tested this approach with sqlite, but I hope this approach works as well with MSSQL as well.

public static T RunQuery<T>(MyDbContext context, string query,
    Func<IDataRecord, T> map,
    params SqlParameter[] parameters)
{
    try
    {
        //...
        if (cn.State.Equals(ConnectionState.Closed)) cn.Open();

        using var reader = command.ExecuteReader();
        var rawEntities = reader.Cast<IDataRecord>().ToList();
        T entity = rawEntities.Count switch
        {
            1 => map(rawEntities.First()),
            0 => default,
            _ => throw new InvalidOperationException("Query returned more than 1 rows")
        };

        if (oldState.Equals(ConnectionState.Closed) && cn.State == ConnectionState.Open)
            cn.Close();
        return entity;
    }
    catch (Exception e)
    {
        //...
    }
}
  • Here we retrieve the IDataRecords into a helper variable and make the branching on its Count property
  • Please note that here we are passing the IDataRecord to the map function not the reader
public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    const string QuantityColumnName = "Quantity", NumDoneColumnName = "NumDone";
    var sql =
        @$"select  convert(int,numordered) as {QuantityColumnName} , convert(int,sum(isnull(numDone,0)))  as {NumDoneColumnName}
    from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
    inner join job j on k.jobid = j.jobid 
    where k.jobtaskid = {jobTaskId}
    group by k.jobtaskid, j.NumOrdered";

    return DataHelpers.RawSqlQuery(sql, record => new DtoNumOrderedNumDone
    {
        NumDone = record.GetInt32(record.GetOrdinal(QuantityColumnName)),
        Quantity = record.GetInt32(record.GetOrdinal(NumDoneColumnName))
    });
}

I suggest the second approach since

  • You can’t enforce the map implementor to call SingleOrDefault instead of First or FirstOrDefault
  • The map implementation is way more concise compare to the SingleOrDefault approach
  • You are not leaking implementation detail (DataReader) so the map implementor for example can’t dispose the reader
  • Your intent is better expressed since you are passing a single record to map to an entity type

UPDATE #2: Share RawSqlQuery method

public static T RawSqlQuery<T>(string query, Func<IDataRecord, T> map, params SqlParameter[] parameters)
{
    try
    {
        using var context = MakeDbContext();
        return RunQuery(context, query, map, parameters);
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        throw;
    }
}

Instead of doing the mapping manually each time, we can use Reflection to get the property name or ColumnAttribute name, and map the SELECT based on the given model.

A quick modification on DataHelper (Untested):

public static class DataHelpers
{
    // simple cache 
    private static Dictionary<Type, Dictionary<string, PropertyInfo>> _cache = new Dictionary<Type, Dictionary<string, PropertyInfo>>();
    
    // get columns mapping from cache or create if not exists
    private static Dictionary<string, PropertyInfo> GetColumnMapping<T>()
    {
        var type = typeof(T);
        
        if(_cache.ContainsKey(type))
        {
            return _cache[type];
        }
        else
        {
            
            var dictionary = new Dictionary<string, PropertyInfo>();
            
            foreach(var property in type.GetProperties())
            {
                var name = property.GetCustomAttribute<ColumnAttribute>()?.Name ?? property.Name;
                dictionary.Add(name, property);     
            }
            
            _cache.Add(type, dictionary);
            
            return dictionary;
        }
    }
      
    private static List<T> RunQuery<T>(MyDbContext context, string query,params SqlParameter[] parameters) where T : class, new()
    {
        try
        {
            using var connection = context.Database.GetDbConnection();
            
            var oldState = connection.State;
                        
            using var command = connection.CreateCommand(); 
            command.CommandText = query;
            command.CommandType = CommandType.Text;
            
            if(parameters?.Length > 0)
                command.Parameters.AddRange(parameters);
            
            if (connection.State.Equals(ConnectionState.Closed)) 
                connection.Open();

            var mapping  = GetColumnMapping<T>();
            
            var entries = new List<T>();
                        
            using (var reader = command.ExecuteReader())
            {           
                if(reader.HasRows)
                {
                    while (reader.Read())
                    {
                        var entry = new T();

                        for(int x = 0; x < reader.FieldCount; x++)
                        {
                            var columnName = reader.GetName(x);
                            
                            if(mapping.ContainsKey(columnName))
                            {
                                var property = mapping[columnName];
                                
                                var value = reader.GetValue(x);

                                if(value != null)
                                {
                                    property.SetValue(entry, Convert.ChangeType(value , property.PropertyType));
                                }                               
                            }
                        }

                        entries.Add(entry);
                    }                   
                }
            }   

            if (oldState.Equals(ConnectionState.Closed) && connection.State == ConnectionState.Open) 
                connection.Close();
            
            return entries; // if no rows, then empty list.
        }
        catch (Exception e)
        {
            MessageBox.Show($"RunQuery inner: {e.InnerException}, ex:{e} rn {query}");
            Console.WriteLine(e);
            throw;
        }

    }


    public static List<T> RawSqlQuery<T>(string query, , params SqlParameter[] parameters) where T : class, new()
    {
        try
        {
            using var context = MakeDbContext();
            return RunQuery<T>(context, query, parameters);
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            throw;
        }
    }
}

Now your GetNumOrderedNumDone(int jobTaskId) would be like :

public static DtoNumOrderedNumDone GetNumOrderedNumDone(int jobTaskId)
{
    var sql =
        @$"select  convert(int,numordered) as Quantity , convert(int,sum(isnull(numDone,0)))  as NumDone
            from task k left outer join taskdone d on k.jobtaskid = d.jobtaskid 
            inner join job j on k.jobid = j.jobid 
            where k.jobtaskid = {jobTaskId}
            group by k.jobtaskid, j.NumOrdered";

    return DataHelpers.RawSqlQuery<DtoNumOrderedNumDone>(sql).FirstOrDefault();
}

This would map the results to the model without the need to sepecify the mapping manually. if you need to change the property name but you still need to keep the column name as is, then apply ColumnAttribute on the property to read the column name from the attribute.

It might need some improvements, but at least it is enough demonstration to boost your thoughts.

Leave a Reply

Your email address will not be published. Required fields are marked *