Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to generate a dataset from a query that has a where statement. If the user passes a value additional things need to be added to the where statement. Is this the best way to go about this?

class TableAdapters
{
    public class FindScriptByTargetVDN
    {
        public static DataSet Fill(SqlConnection connection, string parameter)
        {
            string conditional = string.Empty;
            if (parameter.Length > 0)
            {
                conditional = "and svce.PeripheralNumber= " + parameter + " ";
            }
            string query = "select svce.PeripheralNumber AS Service, " 
                + "ms.EnterpriseName AS [Script Name], "
                + "p.EnterpriseName AS Peripheral, " 
                + "sg.PeripheralNumber AS [Skill Mapping], " 
                + "s.Version AS [Latest Version], "
                + "s.DateTime AS Created, " 
                + "s.Author " 
                + "from Script_Cross_Reference scr " 
                + "LEFT OUTER JOIN Script s on s.ScriptID = scr.ScriptID " 
                + "LEFT OUTER JOIN Master_Script ms on s.MasterScriptID = ms.MasterScriptID "
                + "LEFT OUTER JOIN Service svce on svce.SkillTargetID = scr.ForeignKey " 
                + "LEFT OUTER JOIN Peripheral p on svce.PeripheralID = p.PeripheralID " 
                + "LEFT OUTER JOIN Service_Member sm on svce.SkillTargetID = sm.ServiceSkillTargetID " 
                + "LEFT OUTER JOIN Skill_Group sg on sg.SkillTargetID = sm.SkillGroupSkillTargetID " 
                + "where s.Version = ms.CurrentVersion and scr.TargetType = 1 "
                + conditional
                + "order by svce.PeripheralNumber, ms.EnterpriseName, p.EnterpriseName";
            SqlDataAdapter adapter = new SqlDataAdapter(query, connection);
            DataSet dataSet = new DataSet();
            dataSet.Tables.Add(new DataTable());
            adapter.Fill(dataSet.Tables[0]);
            return dataSet;
        }
    }
}

Here is an example of the call:

this.FindScriptsDataGrid.DataSource = TableAdapters.FindScriptByTargetVDN.Fill(connection, this.ValueTextBox.Text).Tables[0];

The connection is defined before the call. The reason for the some what round about method is the user has the option of defining what server they want to run this query on before they run it.

share|improve this question

3 Answers 3

up vote 8 down vote accepted

This:

+ "where s.Version = ms.CurrentVersion and scr.TargetType = 1 "
+ conditional
+ "order by svce.PeripheralNumber, ms.EnterpriseName, p.EnterpriseName";

Is poison.

conditional came from a string concatenation that involves a parameter string that the method cannot trust:

conditional = "and svce.PeripheralNumber= " + parameter.ToString() + " ";

..Uh, the ToString() call is superfluous here, parameter is already a string! ;)

Looking at the code in your other question, this is where the parameter came from:

string value = this.ValueTextBox.Text;

That is what you shouldn't be doing: concatenating user input into an SQL statement is asking for trouble, whether it's a web or a desktop application doesn't matter, SQL Injection is SQL injection. Not to mention that you just put the calling code in charge of knowing the types for all columns that might be used, whether or not the values should be enclosed in single quotes, etc.

You need to use a parameterized query instead.

share|improve this answer
    
Lol yeah the ToString() got put in there when I was trying something else with the type of parameter, I removed it from my other post guess I didn't catch it here. As far as everything else I'd love to hear your thoughts on my comment on JohnP's answer. –  aaronmallen Jul 2 at 18:31
    
to make it work I had to add adapter.SelectCommand.Parameters.AddWithValue("@param", (parameter.ToString().Length == 0) ? DBNull.Value : parameter); Safe or not? oh and converted parameter type to object. –  aaronmallen Jul 2 at 19:07

Seems like a reasonable approach. It's similar to one that I've used when building a query completely from an XML file, except I used parameters rather than adding in the string.

selectString = string.Format("{0} WHERE {1}='{2}' {3};",
                selectString,
                dateColumn,
                dateSelected.ToString("yyyy-MM-dd"),
                additionalCommandsAfterWhereStatement);
share|improve this answer
    
the problem with using parameters in this instance is if no parameter is passed I need to not include it in my where statement at all. If I simply put and svce.PeripheralNumber= @value directly into the query and the user doesn't pass a value it will either error out or not return anything. –  aaronmallen Jul 2 at 18:28
2  
@aaronmallen give it and svce.PeripheralNumber = ISNULL(@value, svce.PeripheralNumber) instead, and pass NULL to ignore the parameter ;) –  Mat's Mug Jul 2 at 18:34
    
@Mat'sMug LOL DUH god and I actually know SQL –  aaronmallen Jul 2 at 18:34
2  
This is also susceptible to SQL injection! –  Snowbody Jul 2 at 19:23

In my opinion, the BEST way to do this is to use LINQ. Build the query in a modern language, and let the runtime automatically convert it safely to SQL using LINQ-To-SQL. It automatically takes care of putting the user-supplied data into the query safely.

Please check out http://www.linqpad.net/WhyLINQBeatsSQL.aspx

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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