How to Write UPDATE SQL query in C#

Question

The following query throws an exception and I don't know why:

Message: Test method MainTest.MySkillDevelopmentTests.SupervisorFieldAndNamePresent threw exception: System.Data.SqlClient.SqlException: Incorrect syntax near ' where UserName = '. Unclosed quotation mark after the character string ''.

Code:

public static List<List<string>> SetMyGpidAsSupervisorGPID(string gpid)
{
     string query = string.Format("UPDATE [" + dbName + "].[dbo].[MasterDatas] "
                                + "SET SupervisorGPID = " + gpid + "'"
                                + " WHERE UserName = strenev");
    return ConnectToDB(query);
}

Why use string.Format when you're still using the basic level of concatenation? — ThePerplexedOne, Jan 31 '18 at 15:11
What is not clear about the error message? Enclose the text of the username with '. And use proper sql parameters! — Peter Bons, Jan 31 '18 at 15:11
i cant even look at string.format anymore I always use `$""` now — WhatsThePoint, Jan 31 '18 at 15:12
Even the name of this is worrisome... Are you allowing the user to make their user a supervisor? Hopefully that doesn't include elevated permissions... — Jacob H, Jan 31 '18 at 15:13
@ThePerplexedOne that's it, can never remember the name, I just say dollar formatting :D:D — WhatsThePoint, Jan 31 '18 at 15:42
@AleksandarTodorov - You may like to accept the answer as well, if it helped you. See how to accept an answer -> https://meta.stackexchange.com/questions/5234/how-does-accepting-an-answer-work. — Am_I_Helpful, Jul 19 '18 at 05:42

score 2 · Answer 1 · edited Jan 31 '18 at 15:50

Don't use string.Format() in this case, when what you already have is a string.

Your query is missing an enclosing single-quote over 2 places. Use the following code:

string query = "UPDATE [" + dbName + "].[dbo].[MasterDatas] "
                                + "SET SupervisorGPID = '" // here ' is missing
                                + gpid + "'"
                                + " WHERE UserName = 'strenev'"; // here surrounding '' is missing

By the way, you need to study more about SQL-Injection, etc. to know that this is not a good/safe practice.

EDIT based on the comments (thanks for the suggestion):

Code without SQL-Injection (reference: What are good ways to prevent SQL injection?):

string query = "UPDATE [dbname].[dbo].[MasterDatas] "
  //// assuming dbName is not a variable which a user-decides as it generally can't be, and rather a fixed string.
                + "SET SupervisorGPID = @gpid"+
               + " WHERE UserName = 'strenev'"; // here surrounding '' is missing
using (SqlConnection connection = new SqlConnection(connectionString))
{
    SqlCommand command = new SqlCommand(query, connection);
    command.Parameters.Add("@dbName", SqlDbType.NVarchar);
    command.Parameters["@dbName"].Value = dbName;
    command.Parameters.Add("@gpID", SqlDbType.Int);
    command.Parameters["@gpID"].Value = gpid;

    try
    {
        connection.Open();
        Int32 rowsAffected = command.ExecuteNonQuery();
        Console.WriteLine("RowsAffected: {0}", rowsAffected);
    }
    catch (Exception ex)
    {
        //catch and handle OR throw;
    }
}

I think you can do better than this. Why don't you add the code to avoid the SQL Injection? Giving this kind of answer will not help the OP. — Steve, Jan 31 '18 at 15:22
do not `throw ex;`. Write `throw;` instead as this will preserve the stack trace. In the case above, since you are not doing anything with the exception, just omit the try/catch completely. — Igor, Jan 31 '18 at 15:39
There is still a problem. The dbname cannot be parameterized. We can only hope that this value is not typed by the user but choosen from a whitelist of allowed database names (perhaps it is not required at all) However this is a step in the right direction. — Steve, Jan 31 '18 at 15:39
A better choice would be to set the [Database](https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection.database(v=vs.110).aspx) in the `SqlConnection` instead of setting it in the query itself. That would avoid the possibility of a sql injection attack. — Igor, Jan 31 '18 at 15:43
@Igor - About the throwing of exception, it was supposed to be a comment. Regarding the database name setting, it should be part of connectinString, which has been not shared as the same is missing in the question. Thanks for the suggestion though. — Am_I_Helpful, Jan 31 '18 at 15:47

score 0 · Answer 2 · edited Jan 31 '18 at 16:45

0

String constants must be surrounded by single quotes.

where UserName='strenev';      // surround with quotes.

edited Jan 31 '18 at 16:45

Am_I_Helpful

18,735
7
49
73

answered Jan 31 '18 at 15:13

Not sure why this was downvoted as it's correct – Sean T Jan 31 '18 at 15:20
Glad your answer is better :-) – Jan 31 '18 at 15:22
@Am_I_Helpful there's another constant that isn't surrounded by single quotes – Sean T Jan 31 '18 at 15:22
@buffjape - It's not about whose answer is better, but rather make OP/others things clear so that they have a good learning curve. I've also grown on SO, and so I'm rendering what I've learnt. – Am_I_Helpful Jan 31 '18 at 15:37
@Am_I_Helpful - clearer is better. Just take the compliment :-) – Jan 31 '18 at 17:59

Sampanna Pokhrel · Answer 3 · 2018-01-31T15:59:15.280

0

Use parameterized queries instead for example in your case UserName = @name and use sqlcommand inside which you could reference the sql parameter @name and the value 'strenev' in it. Refer to Why do we always prefer using parameters in SQL statements?

"UPDATE [" + dbName + "].[dbo].[MasterDatas] "
                                    + "SET SupervisorGPID = " + gpid 
                                    + " WHERE UserName = 'strenev'");

edited Jan 31 '18 at 15:59

answered Jan 31 '18 at 15:16

Sampanna Pokhrel

3
1
5

This is still wrong at all levels. – Steve Jan 31 '18 at 15:20
@Steve he said he had error at UserName. If UserName is varchar2 it will go through. – Sampanna Pokhrel Jan 31 '18 at 15:23
2

No this is totally wrong. You have a single quote after gpid and before the WHERE statement. But even if you fix it, it is still wrong because you leave the SQL Injection problem unresolved – Steve Jan 31 '18 at 15:25
1

@steve keep fighting the good fight! – djs Jan 31 '18 at 15:27
@Steve you are right I edited it. But sometimes you may need a quick fix for intranet application to show it to the client. But I am totally with you regarding sql injections – Sampanna Pokhrel Jan 31 '18 at 15:38
and then your POC intranet library gets used in a publicly facing production application, and then your company is in the news for a data breach. – djs Jan 31 '18 at 15:41

How to Write UPDATE SQL query in C#

3 Answers3