# Database Model Security For Book Inventory

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

    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;

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; } }

{
SeriesModel seriesModel = (SeriesModel)iSeriesData;
}

{
}

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)
{
}

}

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["@title"].Value = seriesTitle;
cmd.Parameters["@authorid"].Value = author.AuthorId;

cmd.ExecuteNonQuery();
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["@seriesid"].Value = seriesId;

cmd.ExecuteNonQuery();
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;

}

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["@seriesid"].Value = seriesId;

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;
}

}
}


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["@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[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)


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; } }

{
SeriesModel seriesModel = (SeriesModel)iSeriesData;
}

{
}


To:

public DataTable Series => DataTable;


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

• If I implement the first suggestion in the abstract class CDataTableModel in my original question will it work in all the inherited DataTable models classes? – pacmaninbw Apr 9 '19 at 18:34
• No, you will need to make a new public static class for the extension methods, but then as long as the namespace containing that public static class is available, it will work in any class. – Der Kommissar Apr 9 '19 at 18:37
• Usually it's a better idea to cast a class into an interface rather the the other way around as in your last two lines but I guess this is just a blind copy/paste of OP's code and turning the methods into expression-bodied ones but in this case the overload taking a SeriesModel is no longer necessary. – t3chb0t Apr 10 '19 at 10:35
• @t3chb0t Yeah I didn't look at any of the dependency trees, just went off what was in the question. – Der Kommissar Apr 10 '19 at 11:37
public bool AddSeries(ISeriesModel iSeriesData)
{
SeriesModel seriesModel = (SeriesModel)iSeriesData;
}


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.