Web form is not updating tables, why?

Question

I have a web application and on page is an update page to update some profile information. Below is the code I am using to update the table. But I think it is wrong. Does anything stick out? The connection string works cause it is used to read the database to get the profile information, I just removed it due to it containing password/login info for the db.

player is the class of properties that contains player information and ds is the dataset, but I would like to update the database itself online...

 Dim connectionString As String = ""

 Dim GigsterDBConnection As New System.Data.SqlClient.SqlConnection(connectionString)
 GigsterDBConnection.Open()
 Dim updatetoursql As String = "UPDATE PLAYERS SET FIRSTNAME = '" & player.FIRSTNAME & "', LASTNAME =  '" & player.LASTNAME & "', ADDRESS = '" & player.ADDRESS & "', CITY =  '" & player.CITY & "', ZIP = '" & player.ZIP & "', PHONE =  '" & player.PHONE & "', EMAIL = '" & player.EMAIL & "', REFFEREDBY =  '" & player.REFEREDBY & "' "

 updatetoursql = updatetoursql & "PLAYERID = '" & player.PLAYERID & "';"

 Dim cmd As New System.Data.SqlClient.SqlCommand(updatetoursql, GigsterDBConnection)
 Dim sqlAdapter As New System.Data.SqlClient.SqlDataAdapter(cmd)
 sqlAdapter.Update(ds, "PLAYERS")

I think the issue is something the 3 last lines of the code. am I doing it right or is their a better way?

Thanks

Answer 1

Well, apart from the glaring SQL injection issues waiting to bite you ..... (hint: use parametrized queries instead of concatenating together your SQL statement!!)

Dim cmd As New SqlCommand(updatetoursql, GigsterDBConnection)
Dim sqlAdapter As New SqlDataAdapter(cmd)

The problem here is: if you call the SqlDataAdapter constructor this way, what you're passing in is the select command (of the data adapter) - not the update command!

You need to do it this way:

Dim cmd As New SqlCommand(updatetoursql, GigsterDBConnection)
Dim sqlAdapter As New SqlDataAdapter()
sqlAdapter.UpdateCommand = cmd;

Now you've associated your UPDATE statement with the SqlDataAdapter.UpdateCommand and now it should work.

About the SQL injection: I'd strongly recommend using parametrized queries all the time - at least in production code. So instead of concatenating together your query, use this:

Dim updatetoursql As String = 
   "UPDATE PLAYERS SET FIRSTNAME = @FirstName, LASTNAME = @LastName, " & 
   "ADDRESS = @Address, CITY = @City, ZIP = @Zip, PHONE = @Phone " &
   "EMAIL = @EMail, REFFEREDBY = @ReferredBy, PLAYERID = @PlayerID"

and then before you execute the command or the SqlDataAdapter.Update statement, set those parameters to the values you have. This is much safer and gives you less headaches and possibly even speed improvements (if that single Update query is only cached once in SQL Server memory).

Also, why go the long and complicated way of a SqlDataAdapter at all??

After you've created the SqlCommand and set all the parameters, just call cmd.ExecuteNonQuery(); and you're done!

Dim cmd As New SqlCommand(updatetoursql, GigsterDBConnection)

// set up the parameters here.....
cmd.Parameters.AddWithvalue("@FirstName", FirstName);
... etc.

// just call ExecuteNonQuery - and you're done!
cmd.ExecuteNonQuery();

Answer 2

The big thing that jumps up at me is how open to SQL Injection attacks this code is.

You should not build a SQL string in this manner, but use parameterized queries.

Other then that, you are constructing your adapter incorrectly, as the constructor will take the select command, not the update command. Create the command with the parameterless constructor then assign the command you have created to the UpdateCommand property.