-1

I am getting this error message:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''D:\AirMaintStorage\' where id=1' at line 1

I run this in WorkBench and it works correctly but from my code in my app I get the error above. Here is my code:

string SQL = "Update Company_Table set Company_Table_Default_Storage='";
SQL += ra.EscFunction(DefaultStorage);
SQL += "' where id=";
SQL += cp.id.ToString();
MySql.Data.MySqlClient.MySqlConnection  conn = new MySql.Data.MySqlClient.MySqlConnection (ra.conn_String1);
MySql.Data.MySqlClient.MySqlCommand cmd = new MySql.Data.MySqlClient.MySqlCommand(SQL, conn);
conn.Open();
cmd.ExecuteNonQuery();
conn.Close();

The query is:

Update Company_Table set Company_Table_Default_Storage='D:\\AirMaintStorage\\' where id=1

ra is a utilities class that holds the connection information and escfunction escapes apostrophes in a string. cmp.cp.id is the id field of my company table.

Sammy Hale
  • 11
  • 4
  • There's not enough information here to make your question meaningfully answerable here. I don't know what type `ra` is, nor what `EscFunction` does. The title of your question has some good advice, however. I would note that your backslashes \ probably need escaping. \\ – Robert Harvey Feb 21 '22 at 20:26
  • as in `'D:\\AirMaintStorage\\'` – Robert Harvey Feb 21 '22 at 20:27
  • What you were looking at was the actually error message that popped up in the IDE. It removed the D:\\AirMaintStorage\\ and changed it to D:\AirMaintStorage\ in the error message but the actual SQL Statement has the escape in it for the \ – Sammy Hale Feb 21 '22 at 20:35
  • 1
    I would strongly advise you to use parameterized queries though... it's safer and easier to get right too. – Jon Skeet Feb 21 '22 at 20:38
  • "the actual SQL Statement has the escape in it for the \" - how have you determined that? If you've been looking at a string in the debugger, then that escapes the string as if you were writing a C# string literal. In other words, if it shows up as D:\\AirMaintStorage\\ in the debugger, then the *real* string is D:\AirMaintStorage\. – Jon Skeet Feb 21 '22 at 20:39
  • Nope. `EscFunction()` is not good enough. You need to use **parameterized queries**! – Joel Coehoorn Feb 21 '22 at 20:42
  • Yep, I am sure. Just to bring you up to speed on this, this application was originally created to work with Microsoft SQL Server and it works correctly in that environment. I have been trying to move to a more multi-platform environment so I have started moving my app to MySQL. – Sammy Hale Feb 21 '22 at 20:42
  • "Yep, I am sure." Not sure what that's in response to, unfortunately. But regardless of whether it's SQL Server or MySQL, parameterized queries are the best way forward. – Jon Skeet Feb 21 '22 at 20:46

1 Answers1

2

I expect there's a bug in EscFunction(). You should remove that function from your code base completely; it's entirely the wrong way to approach the issue. It is not correct to sanitize your database inputs!

Rather, the only correct approach is to QUARANTINE your database inputs using parameterized queries, as demonstrated below:

string SQL = "
UPDATE Company_Table 
SET Company_Table_Default_Storage= @DefaultStorage 
WHERE ID= @ID";

using (var conn = new MySqlConnection(ra.conn_String1))
using (var cmd = new MySqlCommand(SQL, conn))
{
    cmd.Parameters.AddWithValue("@DefaultStorage", DefaultStorage);
    cmd.Parameters.AddWithValue("@ID", cp.id);
    conn.Open();
    cmd.ExecuteNonQuery();
} // No need to even call conn.Close(); The using block takes care of it.

This is different than "sanitizing", because the quarantined values are never merged back into the SQL command text, even on the server.

Also note the use of using blocks to manage the connection lifetime.

Finally, I want to address this comment:

I have been trying to move to a more multi-platform environment so I have started moving my app to MySQL.

I'm getting outside the realms of the both the question scope and the fact-based verifiable-answers we like to write for Stack Exchange here, and more into an expression of my personal opinion on a specific technology. Nevertheless, I didn't want to leave that comment alone.

I do get wanting to support a broader set of database or hosting technologies, but you should know the MySql product spent many years (from ~2005 to ~2018) almost completely stagnant. It has fallen significantly behind other options and is the least standards compliant of the major products in the space. The good news is it seems to be progressing again, but right now if I needed to move an app to a new open source DB today I'd pick Postgresql instead, and it wouldn't be a close decision.

Of course, your use case may be different, but I think MySql has a reputation as the default option that no longer reflects that actual state of the technologies, and hasn't for some time now. At the same time, SQL Server is now perfectly happy to run in linux, meaning it's not required to switch to MySql or anything else in order to enable multiplatform hosting.

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
  • Note that you don't need actual blocks for the using statements any more - assuming the end of the block is okay for disposal, then just standalone using statements (e.g. `using var conn = new MySqlConnection(ra.conn_String);`) would be fine. – Jon Skeet Feb 21 '22 at 20:50
  • Thank you Joel Coehorn, that worked. And yes I will definately look into moving to Postgresql. The reason I chose MySQL is because it is used on most web hosting platforms and I want to add a mobile web version (that will run on a Linux server) to my app. – Sammy Hale Feb 21 '22 at 21:09