CODEWITHSUNDEEP
c#asp.netsqlsql-server
Kyle Noland
Kyle Noland

Reputation: 4798

What is wrong with this simple update query?

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();

Upvotes: 2

Views: 496

Answers (6)

Sky Sanders
Sky Sanders

Reputation: 37074

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.

Upvotes: 9

Christopher
Christopher

Reputation: 10627

If contact_id is an integer, try not putting it in single quotes.

Change:

"WHERE contact_id = '" + contact_id + "'";

to

"WHERE contact_id = " + contact_id;

Update: As John pointed out in a comment, there is no significant difference in these two clauses. It's probably faster to query without the quotes because you're skipping a cast/convert in that case, though.

Upvotes: 0

Christopher
Christopher

Reputation: 10627

Another thing you can try for quick troubleshooting purposes is to use the SQL Server profiler. It's easier to use than you think. I promise!

  1. Run the Sql Server Profiler (comes with Sql server)
  2. Click File -> New Trace
  3. Connect to your DB
  4. Accept the default trace properties.
  5. Boom! It's recording!
  6. Run your app. If you're already on the page that does the update, refresh it, or hit the button or whatever that causes the update.
  7. Examine the records that appear in the trace. It's all real-time!
  8. Optional: Click the erase button and try again. You will be able to see the exact SQL that is executing in the profiler.
  9. When you're done, hit the stop button.

Upvotes: 2

Stephen Jennings
Stephen Jennings

Reputation: 13234

Looking at the other discussion in this question, it seems like your query works but something unrelated is wrong here. Some things to check that bite me in the ass sometimes:

  • Are you really sure you're running the query you think you are? Break into the debugger before you hit cmd.ExecuteNonQuery(). Are the parameters what you think they are? Is the command text still what you think it should be?

  • Are you updating the right database? Break into the debugger and check your connection string (cs). Perhaps you're still connected to a production database when you want a development one, or vice versa?

  • What happens if you run the query manually in SQL Management Studio? Did the row update to what you think it should?

Upvotes: 1

John Rasch
John Rasch

Reputation: 63435

If it's still not working, here's some debugging questions:

  • Where is this code located and at which part of the page lifecycle is it being called?
  • What is Lib.GetConnectionString(null) returning?
  • Have you set any breakpoints before the update query to ensure the correct data is there?

My best guess is that since you are updating an existing record, it's very likely that your postback values are being overwritten on Page_Load by existing data in the database, which would mean the query is in fact updating, but it's updating with the same exact data that's already in there.

What you'll want to do is check if(!IsPostBack)... before you populate any of your textboxes on the page. This way, if there is a postback, then the data in the textboxes will populate with the postback values instead of values from your database.

Upvotes: 3

Neil Moss
Neil Moss

Reputation: 6818

For one thing, it's wildly susceptible to injection attack.

On a more day-to-day level, any input containing a single apostrophe character is going to break the syntax. Does any of your data happen to have an apostrophe in it?

EDIT - 2nd idea As you're just learning, be aware that you MUST call conn.Close(); to close the connection to the database once you're done with it, otherwise the connection will remain open on the database server until garbage collection gets round to disposing your conn instance. Alternatively, see the using () {} construct.

Slightly grasping at straws here, but try an explicit transaction.

SqlTransaction trans;
conn.Open(); 
try 
{
    trans = conn.BeginTransaction();
    int rows = cmd.ExecuteNonQuery(); 
    trans.Commit();
}
catch 
{
     trans.Rollback();
}
conn.Close();

Upvotes: 1

Related Questions