ExecuteNonQuery() doesn't work

Question

Every time I try to run my code, I get this exception:

An unhandled exception of type 'System.Data.SqlClient.SqlException' occurred in System.Data.dll

Additional information: Incorrect syntax near ')'.

Tried multiple workarounds, but I never get past the ExectueNonQuery line. Can someone tell me what's wrong with it?

private void button1_Click(object sender, EventArgs e)
{
    SqlConnection con = new SqlConnection(@"Data Source=CHARLIE-PC\MSSQLSERVER1;Initial Catalog=Tema;Integrated Security=True;");
    con.Open();

    SqlCommand cmd = new SqlCommand("INSERT INTO Fisier (idFisier, Nume, idFolder) VALUES ('"+idFis.Text+ "','"+ numeFis.Text + "','" +idFoldFis.Text +"',)", con);

    cmd.ExecuteNonQuery();
    con.Close();
}

Answer 1

The comma is your problem, but I would recommend a few other changes at least before moving on:

1. Don't embed your connection strings into each db connection request. Use app.config/web.config or anything else :)
2. Ensure your connections are commands are properly disposed of
3. Parameterize any SQL queries to prevent injection attacks
4. Abstract database commands into separate business layer

1. Utilize an "app.config" for connection strings

There are many docs out there on keeping a connection string secure, but at a minimum, don't embed straight into each of your connection code.

Add an "app.config" to your client project (or utilize the web.config of web projects). At a minimum, this looks like this:

<configuration>
  <appSettings>
      <add key="db" value="Data Source=CHARLIE-PC\MSSQLSERVER1;Initial Catalog=Tema;Integrated Security=True;" />
  </appSettings>
</configuration>

Then add a reference to "System.Configuration" to your project, and you can reference it like this in your code:

var con = new SqlConnection(ConfigurationManager.AppSettings["db"]);

2. Ensure your connections ard commands are properly disposed

Wrap connections and commands in using. Here is an example:

using (var con = new SqlConnection(ConfigurationManager.AppSettings["db"]))
{
    con.Open();
    var sql = "/* My command here */";
    using (var cmd = new SqlCommand(sql, con))
    {
        // SQL execution here
    }
} // Closing is now handled for you (even if errors occur)

3. Parameterize any SQL queries to prevent injection attacks

Concatenating strings are very dangerous for SQL commands (just google "SQL Injection"). This is how to protect yourself.

using (var con = new SqlConnection(ConfigurationManager.AppSettings["db"]))
{
    con.Open();
    var sql = "INSERT INTO Fisier (idFisier, Nume, idFolder) VALUES (@idFisier, @nume, @idFolder)";
    using (var cmd = new SqlCommand(sql, con))
    {
        cmd.Parameters.Add("@idFisier", SqlDbType.VarChar).Value = idFis.Text;
        cmd.Parameters.Add("@nume", SqlDbType.VarChar).Value = numeFis.Text;
        cmd.Parameters.Add("@idFolder", SqlDbType.VarChar).Value = idFoldFis.Text;
        cmd.ExecuteNonQuery();
    }
} // Closing is now handled for you (even if errors occur)

4. Abstract database commands into separate business layer

It is usually best practice and will save you many headaches by writing separate classes (even class library) as your business layer that only contain your data commands. Then your UI would only handle calling the business layer methods.

If your database ever changes or you need to do similar functionality in other parts of your UI, it won't be very fun updating the same query all over your UI as opposed to just updating a single spot in your business layer.

Answer 2

Make SQL being readable and parametrized and you'll find the routine easy to implement:

// Extract a method (or even a class): do not mix UI and business logic/storage
// Just RDBMS logic: no UI controls or something at all
private static void CoreInsertFisier(string idFisier, nume, idFolder) {
  // Do not hardcode the connection string, but read it (from settings)
  // wrap IDisposable into using
  using (SqlConnection con = new SqlConnection(ConnectionStringHere)) {
    con.Open();

    // Make sql readable (use verbatim strings @"...")
    // Make sql parameterized 
    string sql = 
      @"INSERT INTO Fisier (
           idFisier, 
           Nume, 
           idFolder)
        VALUES (
           @prm_idFisier, 
           @prm_Nume, 
           @prm_idFolder)";

     // wrap IDisposable into using
     using (SqlCommand cmd = new SqlCommand(sql, con)) {
       // Parameters.Add(...) is a better choice, but you have to know fields' types
       cmd.Parameters.AddWithValue("@prm_idFisier", idFisier); 
       cmd.Parameters.AddWithValue("@prm_Nume", nume);
       cmd.Parameters.AddWithValue("@prm_idFolder", idFolder); 

       cmd.ExecuteNonQuery(); 
     }
  }
}
...

private void button1_Click(object sender, EventArgs e) {
  // UI: just one call - please insert these three textbox into db
  CoreInsertFisier(idFis.Text, numeFis.Text, idFoldFis.Text);
}

Answer 3

While the other questions state the root problem, your trailing comma, you really must do better about your queries. Do not glue your query together like that, use parameters instead. If you do not you are opening yourself to huge security problems. Also you really must put the connection in a using statement so when a error does happen the connection will still be closed.

private void button1_Click(object sender, EventArgs e)
{
    using(SqlConnection con = new SqlConnection(@"Data Source=CHARLIE-PC\MSSQLSERVER1;Initial Catalog=Tema;Integrated Security=True;"))
    {
        con.Open();
        SqlCommand cmd = new SqlCommand("INSERT INTO Fisier (idFisier, Nume, idFolder) VALUES (@idFis,@numeFis,@idFoldFis)",con);
        cmd.Parameters.Add("@idFis", SqlDbType.NVarChar, -1).Value = idFis.Text;
        cmd.Parameters.Add("@numeFis", SqlDbType.NVarChar, -1).Value = numeFis.Text;
        cmd.Parameters.Add("@idFoldFis", SqlDbType.NVarChar, -1).Value = idFoldFis.Text;
        cmd.ExecuteNonQuery();
    }
}

Answer 4

You have an extra trailing comma:

private void button1_Click(object sender, EventArgs e)
    {
        SqlConnection con = new SqlConnection(@"Data Source=CHARLIE-PC\MSSQLSERVER1;Initial Catalog=Tema;Integrated Security=True;");
        con.Open();
        SqlCommand cmd = new SqlCommand("INSERT INTO Fisier (idFisier, Nume, idFolder) VALUES ('"+idFis.Text+ "','"+ numeFis.Text + "','" +idFoldFis.Text +"')",con);
        cmd.ExecuteNonQuery();
        con.Close();
    }

Anyway as others said, it is a very bad idea to concatenate your query that way, since it could lead you to have sql injection on your code.

Answer 5

Try removing the , before the closing )

   SqlCommand cmd = new SqlCommand("INSERT INTO Fisier (idFisier, Nume, idFolder) VALUES ('"+idFis.Text+ "','"+ numeFis.Text + "','" +idFoldFis.Text +"')",con);