Database Model Security For Book Inventory

Question

This code is one of the models developed for the Book Inventory application that is open for review. It is also based on a comment on this answer to a C# question. Based on the comment I did some research and found this stackoverflow question.

The code presented for review is the database model for a series of books in the Book Inventory. A series of books has an author and a title. The author is represented by a key into the author table. To add a series to the database the user has to select the author from a list of authors already in the database. They then have to add the title or name of the series.

Below I present a before and after, the before is one function that was used in the answer above, the after is the entire code to be reviewed including the refactored function.

Questions:

Is there anything else I can do to prevent SQL Injection attacks?

What else can I do to improve the code, keep in mind I've been on this site for a while, I'm not asking "Does my code suck?", all code sucks, I'm asking how does my code suck.

Before

From my answer

    private DataRow GetRawSeriesData(uint seriesId)
    {
        DataRow rawData = null;

        if (seriesId > 0)
        {
            try
            {
                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    string queryString = "SELECT * FROM series WHERE idSeries = '" + seriesId.ToString() + "';";
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    conn.Open();
                    using (MySqlCommand cmd = new MySqlCommand())
                    {
                        cmd.Connection = conn;
                        cmd.CommandType = CommandType.Text;
                        cmd.CommandText = queryString;

                        MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                        ResultCount = sda.Fill(Dt);
                        if (ResultCount > 0)
                        {
                            rawData = Dt.Rows[0];
                        }
                    }
                }
            }
            catch (Exception ex)
            {
                string errorMsg = "Database Error: " + ex.Message;
                MessageBox.Show(errorMsg);
            }
        }

        return rawData;
    }

After: Code to be Reviewed: SeriesTableModel.cs

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

namespace pacsw.BookInventory.Models
{
    public class SeriesTableModel : CDataTableModel
    {
        private int seriesTitleIndex;
        private int seriesKeyIndex;
        private int seriesAuthorKeyIndex;

        public SeriesTableModel() : base("series", "getAllSeriesData", "addAuthorSeries")
        {
            seriesTitleIndex = GetDBColumnData("SeriesName").IndexBasedOnOrdinal;
            seriesKeyIndex = GetDBColumnData("idSeries").IndexBasedOnOrdinal;
            seriesAuthorKeyIndex = GetDBColumnData("AuthorOfSeries").IndexBasedOnOrdinal;
        }

        public DataTable Series { get { return DataTable; } }

        public bool AddSeries(ISeriesModel iSeriesData)
        {
            SeriesModel seriesModel = (SeriesModel)iSeriesData;
            return addItem(seriesModel);
        }

        public bool AddSeries(SeriesModel seriesModel)
        {
            return addItem(seriesModel);
        }

        public List<string> SeriesSelectionListCreator(AuthorModel author)
        {
            List<string> seriesSelectionList = new List<string>();

            if (author != null && author.IsValid)
            {
                DataTable currentSeriesList = Series;
                string filterString = "LastName = '" + author.LastName + "' AND FirstName = '" + author.FirstName + "'";
                DataRow[] seriesTitleList = currentSeriesList.Select(filterString);

                foreach (DataRow row in seriesTitleList)
                {
                    seriesSelectionList.Add(row[seriesTitleIndex].ToString());
                }

            }

            return seriesSelectionList;
        }

        public uint GetSeriesKey(AuthorModel author, string seriesTitle)
        {
            uint key = 0;

            if (author != null && author.IsValid)
            {
                string SqlQuery = "SELECT series.idSeries FROM series WHERE series.SeriesName = @title AND series.AuthorOfSeries = @authorid;";

                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    try
                    {
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = SqlQuery;
                            cmd.Parameters.Add("@title", MySqlDbType.String);
                            cmd.Parameters["@title"].Value = seriesTitle;
                            cmd.Parameters.Add("@authorid", MySqlDbType.UInt32);
                            cmd.Parameters["@authorid"].Value = author.AuthorId;

                            cmd.ExecuteNonQuery();
                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                key = Dt.Rows[0].Field<uint>(0);
                            }
                        }
                    }
                    catch (Exception ex)
                    {
                        string errorMsg = "Database Error: " + ex.Message;
                        MessageBox.Show(errorMsg);
                        key = 0;
                    }
                }
            }

            return key;
        }

        public string GetSeriesTitle(uint seriesId)
        {
            string title = string.Empty;

            if (seriesId > 0)
            {
                string SqlQuery = "SELECT series.SeriesName FROM series WHERE series.idSeries = @seriesid;";

                using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                {
                    int ResultCount = 0;
                    DataTable Dt = new DataTable();
                    try
                    {
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = SqlQuery;
                            cmd.Parameters.Add("@seriesid", MySqlDbType.UInt32);
                            cmd.Parameters["@seriesid"].Value = seriesId;

                            cmd.ExecuteNonQuery();
                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                title = Dt.Rows[0].Field<string>(0);
                            }
                        }
                    }
                    catch (Exception ex)
                    {
                        string errorMsg = "Database Error: " + ex.Message;
                        MessageBox.Show(errorMsg);
                    }
                }
            }

                return title;
        }

        public SeriesModel GetSeriesModel(uint seriesId)
        {
            SeriesModel seriesData = null;
            DataRow rawSeriesData = GetRawSeriesData(seriesId);

            if (rawSeriesData != null)
            {
                seriesData = ConvertDataRowToSeriesModel(rawSeriesData);
            }

            return seriesData;
        }

        protected override void InitializeSqlCommandParameters()
        {
            AuthorTableModel authorTable = ((App)Application.Current).Model.AuthorTable;
            MySqlParameterCollection parameters = AddItemParameters;

            _addSqlCommandParameter("First Name", authorTable.GetDBColumnData("FirstName"), parameters["@authorFirst"]);
            _addSqlCommandParameter("Last Name", authorTable.GetDBColumnData("LastName"), parameters["@authorLast"]);
            _addSqlCommandParameter("Series Title", GetDBColumnData("SeriesName"), parameters["@seriesTitle"]);
        }

        private SeriesModel ConvertDataRowToSeriesModel(DataRow rawSeriesData)
        {
            uint authorId;
            uint.TryParse(rawSeriesData[seriesAuthorKeyIndex].ToString(), out authorId);
            string title = rawSeriesData[seriesTitleIndex].ToString();

            AuthorModel author = ((App)Application.Current).Model.AuthorTable.GetAuthorFromId(authorId);

            SeriesModel seriesModel = new SeriesModel(author, title);

            return seriesModel;
        }

        private DataRow GetRawSeriesData(uint seriesId)
        {
            DataRow rawData = null;

            if (seriesId > 0)
            {
                try
                {
                    using (MySqlConnection conn = new MySqlConnection(_dbConnectionString))
                    {
                        string queryString = "SELECT * FROM series WHERE idSeries = @seriesid;";
                        int ResultCount = 0;
                        DataTable Dt = new DataTable();
                        conn.Open();
                        using (MySqlCommand cmd = new MySqlCommand())
                        {
                            cmd.Connection = conn;
                            cmd.CommandType = CommandType.Text;
                            cmd.CommandText = queryString;
                            cmd.Parameters.Add("@seriesid", MySqlDbType.UInt32);
                            cmd.Parameters["@seriesid"].Value = seriesId;

                            MySqlDataAdapter sda = new MySqlDataAdapter(cmd);
                            ResultCount = sda.Fill(Dt);
                            if (ResultCount > 0)
                            {
                                rawData = Dt.Rows[0];
                            }
                        }
                    }
                }
                catch (Exception ex)
                {
                    string errorMsg = "Database Error: " + ex.Message;
                    MessageBox.Show(errorMsg);
                }
            }

            return rawData;
        }

    }
}

Answer 1

Anytime I see something like these I wonder "is there a better way we can do this?":

cmd.Parameters.Add("@title", MySqlDbType.String);
cmd.Parameters["@title"].Value = seriesTitle;
cmd.Parameters.Add("@authorid", MySqlDbType.UInt32);
cmd.Parameters["@authorid"].Value = author.AuthorId;

Well, in this case, I'm going to suggest a slightly-functional approach to this process.

C# has a (mostly) handy feature called "Extension Methods". In general, I try to avoid them unless there's a good application for them, such as here.

I would define an extension method here quick:

public static MySqlCommand AddParameter<T>(this MySqlCommand command, string name, MySqlDbType type, T value) {
    command.Parameters.Add(name, type);
    command.Parameters[name].Value = value;
    return command;
}

Now, here, I returned the MySqlCommand on purpose, because I want to chain this:

command.AddParameter("@title", MySqlDbType.String, seriesTitle)
       .AddParameter("@authorid", MySqlDbType.UInt32, author.AuthorId);

Since you aren't using an ORM (and I suspect you have reasons for that, I definitely won't try to persuade you to use one) I would do a couple of these small things to make life a little easier on myself. It's pretty cheap, and it allows us to work our code more easily.

---

I would take some of these shorter functions and apply some expression-bodied members:

public DataTable Series { get { return DataTable; } }

public bool AddSeries(ISeriesModel iSeriesData)
{
    SeriesModel seriesModel = (SeriesModel)iSeriesData;
    return addItem(seriesModel);
}

public bool AddSeries(SeriesModel seriesModel)
{
    return addItem(seriesModel);
}

To:

public DataTable Series => DataTable;
public bool AddSeries(ISeriesModel iSeriesData) => addItem((SeriesModel)iSeriesData);
public bool AddSeries(SeriesModel seriesModel) => addItem(seriesModel);

For simple functions like those, it's trivial to do and saves you some vertical space.

Answer 2

public bool AddSeries(ISeriesModel iSeriesData)
{
    SeriesModel seriesModel = (SeriesModel)iSeriesData;
    return addItem(seriesModel);
}

public bool AddSeries(SeriesModel seriesModel)
{
    return addItem(seriesModel);
}

If SeriesModel implements ISeriesModel interface (which I suppose it does) then you need only the overload taking the interface.

Also casting an interface into a class is rarely a good idea because we then loose the advantages of having the interface in the first place like using a mock type.