What is wrong with this simple update query?

Question

Besides being unsafe... I get no error message, but the row is not updated. The rows integer is set 1 following the query, indicating that 1 row was affected.

String query = "UPDATE contacts SET contact_name = '" + ContactName.Text.Trim() + "', " +
            "contact_phone = '" + Phone.Text.Trim() + "', " +
            "contact_fax = '" + Fax.Text.Trim() + "', " +
            "contact_direct = '" + Direct.Text.Trim() + "', " +
            "company_id = '" + Company.SelectedValue + "', " +
            "contact_address1 = '" + Address1.Text.Trim() + "', " +
            "contact_address2 = '" + Address2.Text.Trim() + "', " +
            "contact_city = '" + City.Text.Trim() + "', " +
            "contact_state = '" + State.SelectedValue + "', " +
            "contact_zip = '" + Zip.Text.Trim() + "' " + 
            "WHERE contact_id = '" + contact_id + "'";

String cs = Lib.GetConnectionString(null);
SqlConnection conn = new SqlConnection(cs);

SqlCommand cmd = conn.CreateCommand();
cmd.CommandText = query;
cmd.Connection.Open();
int rows = cmd.ExecuteNonQuery();

Answer 1

You should use a parameterized query for several reasons:

• eliminate the possibility of an embedded apos breaking your query
• protecting your database/website from sql injection

Also, if the cmd returns 1 then the row was updated. You may need to examine your expectations...

String query = @"
    UPDATE contacts 
    SET contact_name = @contact_name, contact_phone = @contact_phone, contact_fax = @contact_fax, 
        contact_direct = @contact_direct , company_id = @company_id, contact_address1 = @contact_address1, 
        contact_address2 =@contact_address2, contact_city = @contact_city , contact_state = @contact_state,  
        contact_zip = @contact_zip 
    WHERE contact_id = @contact_id";

String cs = Lib.GetConnectionString(null);
using (SqlConnection conn = new SqlConnection(cs))
{
    using (SqlCommand cmd = conn.CreateCommand())
    {
        cmd.Parameters.AddWithValue("@contact_name", ContactName.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_phone", Phone.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_fax", Fax.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_direct", Direct.Text.Trim());
        cmd.Parameters.AddWithValue("@company_id", Company.SelectedValue);
        cmd.Parameters.AddWithValue("@contact_address1", Address1.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_address2", Address2.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_city", City.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_state", State.SelectedValue);
        cmd.Parameters.AddWithValue("@contact_zip", Zip.Text.Trim());
        cmd.Parameters.AddWithValue("@contact_id", contact_id);
        cmd.CommandText = query;
        cmd.Connection.Open();
        int rows = cmd.ExecuteNonQuery();
    }
}

Now, doesn't that look much cleaner? And when you understand why, it will give you a warm fuzzy feeling and let you sleep well at night. ;-)

Clarification:

Your stated question was "What is wrong with this query". The answer is nothing. Nothing is wrong with the query.

The problem is with your page code, which you did not post even after 5 people suggested that there is nothing wrong with the query.

What I meant by 'You may need to examine your expectations...' and 'you need to examine the data going into the parameters BEFORE executing the query to ensure they are what you want' is more clearly described by John, but let me briefly point it out again:

The query as shown is ostensibly correct and should perform as expected. What you are likely experiencing is a flaw in the logic of the surrounding code.

Most likely you are databinding in page_load without an IsPostback guard thus overwriting your input values with the original data.

You need to load and bind your controls/page once only upon the first load. Thereafter the state of the controls are persisted in viewstate, which is stored in a hidden field in the html. If you simply bind to the data source on each page load you will overwrite any input

So lets examine how this works.

Here is a proper logical flow

protected void Page_Load(object sender, EventArgs e)
{
    System.Diagnostics.Trace.WriteLine("Page_Load");
    if (!IsPostBack)
    {
        System.Diagnostics.Trace.WriteLine("\tBind TextBox1");
        TextBox1.Text = "Initial Value";
    }
    System.Diagnostics.Trace.WriteLine("\tTextBox1.Text = " + TextBox1.Text);
}

protected void Button1_Click(object sender, EventArgs e)
{
    System.Diagnostics.Trace.WriteLine("Button1_Click");
    System.Diagnostics.Trace.WriteLine("\tTextBox1.Text = " + TextBox1.Text);
}

If you load this page, enter 'New Value' into the textbox and click button one, this is what Trace shows:

    Page_Load
        Bind TextBox1
        TextBox1.Text = Initial Value

    Page_Load
        TextBox1.Text = New Value

    Button1_Click
        TextBox1.Text = New Value

But without the guard:

protected void Page_Load(object sender, EventArgs e)
{
    System.Diagnostics.Trace.WriteLine("Page_Load");
    System.Diagnostics.Trace.WriteLine("\tBind TextBox1");
    TextBox1.Text = "Initial Value";
    System.Diagnostics.Trace.WriteLine("\tTextBox1.Text = " + TextBox1.Text);
}

        protected void Button1_Click(object sender, EventArgs e)
{
    System.Diagnostics.Trace.WriteLine("Button1_Click");
    System.Diagnostics.Trace.WriteLine("\tTextBox1.Text = " + TextBox1.Text);
}

You get the results that you are likely experiencing...

    Page_Load
        Bind TextBox1
        TextBox1.Text = Initial Value

    Page_Load
        Bind TextBox1
        TextBox1.Text = Initial Value

    Button1_Click
        TextBox1.Text = Initial Value

But again, a practical debugging technique that would help you is to break in the update method just before the query is executed and examine the values to verify that they are what you think they are and then go from there.