CODEWITHSUNDEEP
c#sql
Adam
Adam

Reputation: 51

SQL C# INSERT query sent doesn't actually add any new entries to the table

I'm currently using C# to try and execute queries on a SQL database. The SELECT query works fine but the INSERT one doesn't.

I've launched the same query from SQL Server manager itself and it works fine but when I send it from the C# (using an ASP page with text boxes to specify the values being sent for the columns) the page simply refreshes and doesn't show any errors.

The fact that the SELECT query works tells me that server connects fine and that it isn't the problem. I've used breakpoints in the C# code to check if the variables send correctly from the text boxes and they are they should be.

Any ideas what's going wrong?

Here's the Code:

  protected void SendMyData_OnClick(object sender, EventArgs e)
    {
        //(NO RECORD SET IS NEEDED IN THIS FUNCTION SEEING AS HOW WE'RE NOT FETCHING ANYTHING) 

         SqlConnection conServer = new SqlConnection(CDatabase.ConnectionString());

        // Database variables
        string Track_Name   = TextBox8.Text;

        string Track_No     = TextBox10.Text;
        long TN             = Convert.ToInt64(Track_No);

        string Artist_ID    = TextBox11.Text;
        long ArtID          = Convert.ToInt64(Artist_ID);

        string Album_ID     = TextBox12.Text;
        long AlbID          = Convert.ToInt64(Album_ID);
        //SUB THESE INTO THE SQL QUERY 

        try
        {
            string qrySQL = string.Format("INSERT INTO Tracks ([Track_Name],[Track_No],[Artist_ID],[Album_ID]) VALUES ({0}, {1}, {2}, {3})", Track_Name, TN, ArtID, AlbID);

            //This is the string Used to send (Note the NonReturn(As we aren't fetching anything) : " conServer.Open();CDatabase.ExecuteNonReturnQuery(qrySQL);"

            conServer.Open();
            //(NO RECORD SET IS NEEDED IN THIS FUNCTION SEEING AS HOW WE'RE NOT FETCHING ANYTHING) 
            CDatabase.ExecuteNonReturnQuery(qrySQL);
        }
        catch
        {
        }

        finally
        {
           conServer.Close();
        }

Upvotes: 0

Views: 1741

Answers (6)

Tim
Tim

Reputation: 28530

Shouldn't the first value (Track_Name) be quoted, since it's a string?

string qrySQL = string.Format("INSERT INTO Tracks ([Track_Name],[Track_No],[Artist_ID],[Album_ID]) VALUES ('{0}', {1}, {2}, {3})", Track_Name, TN, ArtID, AlbID);

EDITED TO ADD

As others have said (in their better and more complete answers, IMO), you're wide open to SQL Injection attacks with the code as is - the only thing my answer did was fix the query so it would actually insert.

Of course, as Jon Skeet pointed out, you need to be doing parmaterized SQL. I appreciate the accepted answer, but I Jon's was the better overall answer, and you should really accept that one - other users may come across this question in their own searches, and should have the most accurate and complete answer (IMO).

Upvotes: -1

Jon Skeet
Jon Skeet

Reputation: 1503200

Well, there are at least two things seriously wrong here:

  • You're embedding values directly in your SQL. Welcome to SQL Injection Attack Land. Don't do that. Use parameterized SQL instead, specifying the values separately. (See the docs for SqlCommand.Parameters for an example.)
  • You're catching all exceptions and just swallowing them, with no logging. It's entirely possible that this code is throwing an exception which explains exactly what's going wrong - but you'd never know, because your code has its metaphorical fingers in its ears.

My guess is that it is throwing an exception due to the track name neither being parameterized nor quoted - you've got an arbitrary string in the middle of your SQL.

Catching all exceptions is almost always wrong, and particularly at this level. Usually you should let exceptions propagate up to higher levels, where you may well have a top-level catch-all block for "something went wrong with the request and we don't want to bring down the whole server" - ASP.NET supplies one of these for you, of course, and allows you to specify how errors should be handled.

Additionally, rather than explicitly closing the SQL connection, I would wrap the whole thing in a using statement (and only after parsing the input, too).

using (SqlConnection con = new SqlConnection(...))
{
    con.Open();
    using (SqlCommand command = ...)
    {
        // Execute the command
    }
}

Finally, your variable naming is inconsistent - sometimes you're using camelCase and sometimes you're using PascalCase. It's unusual to see Pascal cased variables, to be honest - likewise variables with underscores in. Abbreviating "Artist" and "Album" to "Art" and "Alb" just makes the code less readable too.

Upvotes: 6

RonnyKnoxville
RonnyKnoxville

Reputation: 6404

Is this a coursework? It definitely looks like it :P

Also, are the square brackets around the parameters an SQL Server thing? Ive not seen it written like that before. I would have thought it was just "INSERT INTO Tracks (Track_Name, Track_Number, etc... Try printing out the exception... such as 

catch Exception e{
print e
}

So you can see whats happening

Not sure on the exact syntax

Upvotes: 0

StefanE
StefanE

Reputation: 7630

Following will work:

SqlConnection sqlConnection = new SqlConnection(ConnectionString);
string qrySQL = string.Format("INSERT INTO Tracks ([Track_Name],[Track_No],[Artist_ID],[Album_ID]) VALUES ({0}, {1}, {2}, {3})", Track_Name, TN, ArtID, AlbID);
SqlCommand sqlCommand = new SqlCommand(qrySQL, sqlConnection);
sqlConnection.Open();
sqlCommand.ExecuteNonQuery();

You would be better of using parameters to avoid problems with Data Types and Security issues.

string qrySQL = "INSERT INTO Tracks ([Track_Name],[Track_No],[Artist_ID],[Album_ID]) VALUES (@Track_Name,@Track_No,@Artist_ID,@Album_ID)";
sqlCommand.Parameters.AddWithValue("@Track_Name", "Value");

and so on..

Upvotes: 0

Rune FS
Rune FS

Reputation: 21752

Your code is throing an exception that you are not seeing because of the empty catch. Your text values have no ' around them which makes the SQL fail. AS mentioned by others you should used parameterized SQL which would not only fix this problem but also keep you safe(r) from SQL injection attacks

Upvotes: 0

Wouter de Kort
Wouter de Kort

Reputation: 39898

The page will refresh because you have an empty catch clause. So if there is an error you will catch it and swallow the error.

Try removing the catch clause or adding a new 'throw' in it which will retrow the exception

Upvotes: 0

Related Questions