Update query not throwing error

Question

Thanks for the help in advance.

Im trying to use update query in C#

Error : command is getting executed even if I use incorrect values

protected void Button1_Click(object sender, EventArgs e)
{
    try
    {
        con.Open();

        cmd = new SqlCommand("update Comcast_AvayaID set Status='Inactive' where Employee_Id='" + TxtEMPID.Text + "' and AvayaID ='" + TxtAvayaID.Text + "'", con);
        cmd = new SqlCommand("UPDATE Avaya_Id SET Status = 'UnAssigned' where Avaya_ID ='" + TxtAvayaID.Text + "'", con);
        cmd.ExecuteNonQuery();
        LBLSuccess.Visible = true;
        LBLSuccess.Text = "Deactivation Successfull";
        con.Close();
    }
    catch (SqlException ex)
    {
        LBLSuccess.Visible = true;
        LBLSuccess.Text = "Deactivation Unsuccessfull";
    }

Your code is wide open to SQL injection attacks. Use parameterized queries! — maccettura, Apr 19 '18 at 19:49
What do you mean by `incorrect values`? Are you wanting to add input validation? — Dan Wilson, Apr 19 '18 at 19:52
Instead of writing an useless message when you get the exception write out the _ex.Message_ property. This will be necessary to understand what happens here (and of course tell us what do you get there) — Steve, Apr 19 '18 at 19:52
Why do you want to transfer message you don't about to user? @Steve Don't do what previous comment says. SqlException can be anything about, user not for sure supposed to see it, and if you show it it can be +1 to threat — Evgeny Gorbovoy, Apr 19 '18 at 20:04
@EugeneGorbovoy we are trying to debug its problem. Better if he could write the exception in a log file but for the purpose to understand what's happening then let the exception be visible. In a production code you don't have the exception handler at all if you don't plan to handle it. — Steve, Apr 19 '18 at 20:12

score 0 · Answer 1 · answered Apr 19 '18 at 19:54

0

If you put "incorrect" values it just updates zero of records. No errors/exception expected here.

answered Apr 19 '18 at 19:54

Evgeny Gorbovoy

674
3
19

score 0 · Accepted Answer · answered Apr 19 '18 at 20:08

your code would look better like this, it not the most optimal, but ia already a way better piece of code then your snippet

1) added parameters using a helper function for the sql injection issue

2) an ExecuteNonQuery returns the rows affected, so if you are expecting that 1 row was updated, you can check on that

3) if you update a row with an id that not exists, it will not throw a SqlException like you are expecting in your code, this happens e.g. when locking occurs

 public void Update()
        {
            var con = new SqlConnection();
            try
            {

                var empId = TxtEMPID.Text
                var avayaId = TxtAvayaID.Text
                con.Open();

                var cmd1 = new SqlCommand("update Comcast_AvayaID set Status='Inactive' where Employee_Id=@empId and AvayaID = @avayaId", con);              
                cmd1.Parameters.Add(AddParameter("@empId",empId));
                cmd1.Parameters.Add(AddParameter("@avayaId", avayaId));

                var cmd2 = new SqlCommand("UPDATE Avaya_Id SET Status = 'UnAssigned' where Avaya_ID =avayaId", con);
                cmd2.Parameters.Add(AddParameter("@avayaId", avayaId));

                var rowsaffected1 = cmd1.ExecuteNonQuery();

                var rowsAffected2 = cmd2.ExecuteNonQuery();

                if (rowsaffected1 == 1 && rowsAffected2 == 1)
                {
                    //success code goes here
                    //--------
                    LBLSuccess.Visible = true;
                    LBLSuccess.Text = "Deactivation Successfull";
                }
                else
                {
                    // failure code goes here
                    //-----------------------
                    LBLSuccess.Visible = true;
                    LBLSuccess.Text = "Deactivation Unsuccessfull";
                }
            }
            catch (SqlException ex)
            {
                //handle errors
            }
            finally
            {
                con.Close();
            }


            Console.ReadLine();
        }

        private SqlParameter AddParameter(string name, object value) {
            var par = new SqlParameter();
            par.ParameterName = name;
            par.Value = value;
            return par;
        }

The solution worked great. But why this function is used private SqlParameter AddParameter(string name, object value) { var par = new SqlParameter(); par.ParameterName = name; par.Value = value; return par; } — Mohammed Salim, Apr 19 '18 at 20:22
it is just a helper function since there is no default constructor to create a SqlParameter with name and value. By using this you will you cleaner code and less repeats. And i use the parameters to prevent SQL injection in your queries like your sample was doing see https://stackoverflow.com/questions/601300/what-is-sql-injection for more info on sql injection — Ben Croughs, Apr 19 '18 at 20:27
@BenCroughs we have [SqlParameterCollection.AddWithValue](https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlparametercollection.addwithvalue(v=vs.110).aspx) from ages now. However, when adding a parameter, it always good practice to specify its type and size. — Steve, Apr 19 '18 at 20:57
@Steve thx forgot about that one, i am still old school sometimes :-) but indeed with the helper function you can easily add type and size — Ben Croughs, Apr 19 '18 at 21:01
Why did you put catch(SqlException) - which exceptions do you expect here? — Evgeny Gorbovoy, Apr 20 '18 at 08:25
If you write no code inside catch - better remove this catch and use using construction — Evgeny Gorbovoy, Apr 20 '18 at 08:25
@BenCroughs , The code is working Fine. The If Condition is not working. It loops if we enter wrong data. It doesnt throw error — Mohammed Salim, Apr 23 '18 at 12:45
what do you mean with wrong data ? and the if only checks how many rows affected, it will never throw an exception, if you want one, you must throw an exception in the else of the if — Ben Croughs, Apr 23 '18 at 12:49
@BenCroughs The If section is not working. It Loops for long time. Nothing is displayed even if the update command executes — Mohammed Salim, Apr 23 '18 at 15:08

Update query not throwing error

2 Answers2