4

Coding Platform: ASP.NET 2.0 WebForms with C# with MySQL as backend

Background

I am currently working on a bug fixing a website.
One of the table "registrations" have 80 columns.

The Insert/ Update is done through simple sql statements without any parameterized queries.

Problem

At registration, the user can vary many parameters resulting in atleast 15 kinds of INSERT query. My problem is how to ensure all fields are inserted with their correct value.

So, I created a

Dictionary<string, string> fields = new Dictionary<string, string>();

fields.Add("LoginEmail", MySQL.SingleQuoteSQL(txtLoginEmail.Text));
fields.Add("Password", MySQL.SingleQuoteSQL(txtLoginPassword.Text));

fields.Add("ContactName", MySQL.SingleQuoteSQL(txtContactName.Text));
fields.Add("City", MySQL.SingleQuoteSQL(txtCity.Text));

My idea was to make a simple insert query like this

INSERT INTO registrations("all keys as comma separated string") VALUES ("all keys as comma separated string") 

My questions are

  1. Is Dictionary the best data structure to implement this?
  2. Does the sorting of keys by generic Dictionary changes key-value indices at the query?
  3. Best method to fetch all keys into an array and the corresponding values to another matching array.

And also, what are the other better approaches?

P.S: I am maintaining the code and Making an Entity Class mapping the columns to properties and storing the values is not an option in this.

3 Answers 3

12
 string list<T>(IEnumerable<T> enumerable)
 {
   List<T> list = new List<T>(enumerable);
   return string.Join(",", list.ToArray());
 } 

//...
string sql= String.Format("INSERT INTO registrations({0}) VALUES({1})",
                list(fields.Keys),
                list(fields.Values));
4
  • Yes, the order of the keys in Keys are the same as the associated values in Values. See "Remarks" in msdn.microsoft.com/en-us/library/yt2fy5zk.aspx Apr 17, 2011 at 17:43
  • I was assuming .NET 4.0. I've updated my answer for .NET 3.5 (using System.Linq;). Apr 17, 2011 at 18:31
  • @mark: thanks a lot i ended up foreach on each keyvaluepair and storing the key/value intwo arrays Apr 18, 2011 at 20:34
  • Adding any wrong charterer in values will lead into error, and it will be target for SQL Injections.
    – Anas Naim
    Mar 26, 2016 at 10:02
2

I think the dictionary structure is ok in this case but

building and executing the string as is allows for SQL Injection atacks

xkcd.com/327/ Exploits Of A Mom

I am using .NET 3.5 but the idea can be applied to .NET 2.0 also

If MySQL.SingleQuoteSQL does more than it's name suggest then please let me know

Otherwise add the values as parameters to prevent this

var values = new Dictionary<string, string>() {
  {"LoginEmail", "LoginEmail"},
  {"Password", "Password"},
  {"ContactName", "ContactName"},
  {"City", "City"}
};

System.Func<string, string> key = p => String.Concat("?", p);

var statement = string.Format("INSERT INTO registrations ({0}) VALUES ({1})", 
  string.Join(",", values.Values.ToArray()),
  string.Join(",", values.Keys.Select(key).ToArray())
);

//"INSERT INTO registrations (LoginEmail,Password,ContactName,City) VALUES (?LoginEmail,?Password,?ContactName,?City)"  

foreach(var p in values) {
  command.Parameters.Add(key(p.Key), p.Value, SqlDbType.Text);
}

command.Prepare();
command.ExecuteNonQuery();

They may be other better classes for .NET mysql, apologies if so - I haven't used mysql in .NET
1
  • MySQL.SingleQuoteSQL comments all the single quotes Apr 18, 2011 at 19:59
1

I know the description says you are not using parameterized sql, but IMHO, use of parameterized sql is a better approach to prevent sql injection. It wouldn't take much effort at all to throw some SQL into txtLoginEmail and submit the form wreaking havoc on your situation.

private static string CreateInsertSql(string table, 
                                      IDictionary<string, string> parameterMap)
{
    var keys = parameterMap.Keys.ToList();
    // ToList() LINQ extension method used because order is NOT
    // guaranteed with every implementation of IDictionary<TKey, TValue>

    var sql = new StringBuilder("INSERT INTO ").Append(table).Append("(");

    for (var i = 0; i < keys.Count; i++)
    {
        sql.Append(keys[i]);
        if (i < keys.Count - 1)
            sql.Append(", ");
    }

    sql.Append(") VALUES(");

    for (var i = 0; i < keys.Count; i++)
    {
        sql.Append('?').Append(keys[i]);
        if (i < keys.Count - 1)
            sql.Append(", ");
    }

    return sql.Append(")").ToString();
}
private static void SqlInsert(string table, IDictionary<string, string> parameterMap)
{
    using (var connection = AcquireConnection())
    {
        connection.Open();
        using (var command = connection.CreateCommand())
        {
            command.Connection = connection;
            command.CommandText = CreateInsertSql(table, parameterMap);
            foreach (var pair in parameterMap)
                command.Parameters.Add(pair.Key, pair.Value);
            command.ExecuteNonQuery();
        }
    }
}

Calling would be something like:

SqlInsert("registrations", new Dictionary<string, string>()
{
    { "LoginEmail", txtLoginEmail.Text },
    { "Password", txtLoginPassword.Text },
    { "ContactName", txtContactName.Text },
    { City", txtCity.Text }
});
5
  • thanks. your first function could be written in a line. see marks code. also i am using C# 2.0 Apr 17, 2011 at 18:10
  • True, but it would be less efficient as that code employs 2 StringBuilder instances (1 in each string.Join call) and 4 concatenations creating 3 unnecessary strings in memory. Mine leverages a single StringBuilder creating 0 unnecessary strings. Apr 17, 2011 at 18:16
  • Also, you couldn't use string.Join to prefix all the parameter names with a delimiter (?, in this case). Apr 17, 2011 at 18:19
  • Sorry, 5 unnecessary strings in memory, as I forgot the result strings of the string.Join calls. Sorry.. dead horse. Apr 17, 2011 at 18:26
  • I think this is the best solution to be safe from SQL Injections, but it only works if you change Append('?') to Append('@').. Good job
    – Anas Naim
    Mar 26, 2016 at 9:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.