SQL Server : Update query not working in ASP.Net

Question

I'm having trouble with the update query.

I couldn't run the second method.

This works:

if (Methods.Update("TABLE1",
    "Name", TextBoxName.Text,
    "Column1", "1"
    ) == 1)

This does not work:

if (Methods.Update("TABLE1",
    "Name", TextBoxName.Text,
    "Surname", TextBoxSurname.Text,
    "Column1", "2"
    ) == 1)

I get this error:

Conversion failed when converting the nvarchar value 'a4' to data type int.

Working method:

public static int Update(string Table1, string Column1, string Column1Value, string WhereColumn, string WhereValue)
{
    SqlConnection connection = new SqlConnection(WebConfigurationManager.ConnectionStrings["connection"].ConnectionString);
    SqlCommand command = new SqlCommand("UPDATE " + Table1 + " SET " + Column1 + "= @Column1Value " + " WHERE " + WhereColumn + "=@WhereValue", connection);
    command.Parameters.AddWithValue("@Column1Value", Column1Value);
    command.Parameters.AddWithValue("@WhereValue", WhereValue);

    try
    {
        if ((connection.State == ConnectionState.Closed) || (connection.State == ConnectionState.Broken))
        {
            connection.Open();
        }

        int i = Convert.ToInt16(command.ExecuteNonQuery());

        return i;
    }
    finally
    {
        connection.Close();
    }
}

Not working method:

public static int Update(string Table1, string Column1, string Column1Value, string Column2, string Column2Value, string WhereColumn, string WhereValue)
{
    SqlConnection connection = new SqlConnection(WebConfigurationManager.ConnectionStrings["connection"].ConnectionString);
    SqlCommand command = new SqlCommand("UPDATE " + Table1 + " SET " + Column1 + "= @Column1Value," + Column2 + " = @Column2Value WHERE " + WhereColumn + "=" + WhereValue, connection);
    command.Parameters.AddWithValue("@Column1Value", Column1Value);
    command.Parameters.AddWithValue("@Column2Value", Column2Value);
    command.Parameters.AddWithValue("@WhereValue", WhereValue);

    try
    {
        if ((connection.State == ConnectionState.Closed) || (connection.State == ConnectionState.Broken))
        {
            connection.Open();
        }

        int i = Convert.ToInt16(command.ExecuteNonQuery());

        return i;
    }
    finally
    {
        connection.Close();
    }
}

My table content:

TABLE1
------------------------
Name   Surname   Column1
Name1  Surname1  NULL
Name2  Surname2  NULL
Name3  Surname3  a4
Name4  Surname4  NULL
Name5  Surname5  1
Name6  Surname6  2
Name7  Surname7  3

My table structure :

CREATE TABLE [User1].[TABLE1] 
(
    [ID]            INT            IDENTITY (1, 1) NOT NULL,
    [Name]          NVARCHAR (50)  NOT NULL,
    [Surname]       NVARCHAR (50)  NOT NULL,    
    [Column1]       NVARCHAR (50)  NULL,

    PRIMARY KEY CLUSTERED ([ID] ASC)
);

I couldn't understand the reason.

Thanks in advance.

Aside from the issue at hand you should read this and start defining your datatypes for parameters. blogs.msmvps.com/jcoehoorn/blog/2014/05/12/… — Sean Lange
– Sean Lange, Commented Aug 5, 2015 at 16:40
If you're not careful this could leave you wide open to SQL injection. This specific usage doesn't because values are hard-coded, but the method which actually interacts with the database ensures no protection that consuming code won't abuse it. — David
– David, Commented Aug 5, 2015 at 16:41
Not a big fan of a generic update method. The challenges to prevent sql injection are a real pain to deal with. Why not create the CRUD procedures for your tables. It is a little more work initially but also start the path to separating the data and business layers. — Sean Lange
– Sean Lange, Commented Aug 5, 2015 at 16:42
@David actually this implementation is very vulnerable to sql injection. It receives string values, builds that up as a sql command and executes it. — Sean Lange
– Sean Lange, Commented Aug 5, 2015 at 16:43
@SeanLange: Yes, that's what I said. The implementation is vulnerable. This specific usage of the implementation isn't because the vulnerable parts aren't exposed to user input, but the core implementation is. If always used carefully, there would be no net vulnerability. But I highly recommend to the OP that he ensure security explicitly rather than assume the code will always be used carefully. — David
– David, Commented Aug 5, 2015 at 16:46

Dan Field · Accepted Answer · 2015-08-05 16:37:36Z

The problem in the non-working method is you aren't parameterizing the WhereValue:

WhereColumn + "=" + WhereValue,

Instead of

WhereColumn + "=@WhereValue"

So SQL is casting it to an INT and then failing to compare it properly to the a4 row.

While changing the second method would fix your issue, you have some security issues in your SQL here. It'd be very possible to suffer from SQL injection like so if someone passed you a bad table or column name. At the very least, you should check to make sure the values being passed are in a list of actual table and column names - so someone couldn't pass you a table that really shouldn't be updated. Imagine what would happen if a malicious user passed this for your table name parameter: "TableName SET Column1 = 1; SELECT * FROM INFORMATION_SCHEMA.TABLES --".

Really, it'd probably be best to have specific functions for updating specific tables and values, and making sure you always parameterize the values to avoid SQL Injection

Collectives™ on Stack Overflow

SQL Server : Update query not working in ASP.Net

1 Answer 1

Comments

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Related