Simple, ordinary SQL Server insert method

Question

This had to be a simple, ordinary SQL insert method but when I run it and I click "button1" I get the error

An unhandled exception of type 'system.data.sqlclient.sqlexception' occurred in system.data.dll

Does anyone know what the problem is?

namespace InsertDeleteUpdate_Login
{
    public partial class Form1 : Form
    {
        SqlConnection cn = new SqlConnection(@"Data Source=(LocalDB)\v11.0;AttachDbFilename=E:\C #\InsertDeleteUpdate-Login\InsertDeleteUpdate-Login\Database1.mdf;Integrated Security=True");

        SqlCommand cmd = new SqlCommand();
        SqlDataReader dr;

        public Form1()
        {
            InitializeComponent();
            cmd.Connection = cn;
        }

        private void button1_Click(object sender, EventArgs e)
        {
            if (textBox1.Text != "" && textBox2.Text != "")
            {
                cn.Open();
                cmd.CommandText = "INSERT INTO info (ID,Name,Password)" + " VALUES ('" + textBox1.Text + "','" + textBox2.Text + "','" + textBox3.Text + "')'";

                cmd.ExecuteNonQuery();

                cmd.Clone();

                MessageBox.Show("Inserare reusita");

                cn.Close();
            }
        }
    }
}

Catch the exception to see what the error message is. The system is trying to tell you what's wrong, don't ignore exceptions. Also, obligatory... Your code is ***highly vulnerable*** to **SQL injection attacks**. Use parameterized queries instead of *executing user input as code*. And you are ***storing user passwords in plain text***. This is **grossly irresponsible** to your users. User passwords should be 1-way obscured using a hashing algorithm and should *never* be retrievable. — David, Aug 03 '15 at 15:57
You have an SQL Injection vulnerability, `'` in textbox values will cause errors or worse corruption. [Use a parametrized insert instead](http://stackoverflow.com/questions/8389803/insert-data-into-database-in-c-sharp) — Alex K., Aug 03 '15 at 15:57
use parameterised queries!!! instead of those nasty strings. — Philip Stuyck, Aug 03 '15 at 15:58
Not to beat a dead horse, but the only way this code is even anywhere NEAR acceptable, is if this is for a throwaway prototype, or if this is perhaps code for a class assignment and they haven't taught you about the things that other people have brought attention to (SQL injection and to always hash your passwords with something like SHA or so - even md5 is more acceptable than plain text), or I suppose if you are going to be THE only user of the program, although then you're still coding yourself into a corner, as you're disallowing characters like apostrophes to be entered. — user2366842, Aug 03 '15 at 16:19
@CraigW. remember you were beginner once. I personally never learnt to debug in school, of course that is 30 years ago. I hope nowadays it is better and that schools pay attention to it because it is as important as programming itself. — Philip Stuyck, Aug 03 '15 at 16:27
@user2366842 , yes it is a prototype, i m a begginner in c# and i hadonly one class about databases in school, which was much more like a presentation of sql databases not a class in which to learn something practical.Moreover , in the high school i attend, the computer science classes focuses EXCLUSSIVELY on algortihmic(there's nothing about OOP) .Fortunately a computer science teacher of the high school offered to make a course(of about 8 classes), for students from all over the high school ,whose objective was to introduce us to oop(c#).Before that, i didn't even know about oop. — Lazu Razvan, Aug 03 '15 at 17:36
@user2366842 you see, the academic help for oop is almost NULL , so what i have to do is to learn by myself from tutorials or whatever. i didn't want to simply copy codes from the internet, i don t beieve this is a good idea for learning code, **so if u have some ideas of how to get better at OOP/DATABASES in a faster ,most effiicient way ,please tell me ** — Lazu Razvan, Aug 03 '15 at 17:42
For starters, take advice that everyone's given so far...Seems like you've got a reasonable grip on most of it (outside of the security stuff). Everyone has to start somewhere. Take this question as a learning experience. — user2366842, Aug 03 '15 at 17:45

score 3 · Accepted Answer · answered Aug 03 '15 at 16:05

The root cause of your problem is that you are not using parameterized queries and are trying to create an sql string on the fly. As a result you make an error in the assembling code of that string. But if you use a parameterized query the chance of running into an issue like that is a lot lower because you don't have to mess about with quotes and the like. On top of this, you cannot have a sql injection attack if you use parameters and it makes the code more readable too.

Read http://www.dotnetperls.com/sqlparameter on how to use a parameterized query the way it should be done and don't just fix the textual error in the querystring. It is not the way it is supposed to be done.

This is a good explanation too : http://www.dreamincode.net/forums/topic/268104-parameterizing-your-sql-queries-the-right-way-to-query-a-database/

score 1 · Answer 2 · answered Aug 03 '15 at 16:00

1

I can't add comments yet, but it looks like you might have an extra single quote after the last close bracket that shouldn't be there.

answered Aug 03 '15 at 16:00

MarioAna

815
7
11

1

yes that is definitely also a problem ;-) But if the op were to use paremeterized queries that problem would not be there in the first place nevertheless i upvoted. – Philip Stuyck Aug 03 '15 at 16:01
1

Although this is correct, I'm not going to upvote, as leaving the code in its current state is dangerous at best. – user2366842 Aug 03 '15 at 16:22
@user2366842 that is probably a smart, we should not encourage this kind of coding in any way, not even by correcting the problem in the string handling. I got carried away because Mario found the error in the string ;-) – Philip Stuyck Aug 03 '15 at 16:25

score 1 · Answer 3 · answered Aug 03 '15 at 16:35

1

As mentioned by several people above, you should ALWAYS parameterise your queries, and you also have a trailing single quote, which is most likely what SQL Server is choking on.

Try something like this:

cmd.CommandText = "INSERT INTO info (ID, Name, Password) VALUES (@ID, @Name, @Password)";
cmd.Parameters.AddWithValue("@ID", textBox1.Text);
cmd.Parameters.AddWithValue("@Name", textBox2.Text);
cmd.Parameters.AddWithValue("@Password", textBox3.Text);
cmd.ExecuteNonQuery();

answered Aug 03 '15 at 16:35

Ed B

785
4
9

Better....although the OP would still be storing passwords in plain text, which is an issue in and of itself. – user2366842 Aug 03 '15 at 16:46
yes it is a prototype, i m a begginner in c# and i hadonly one class about databases in school, which was much more like a presentation of sql databases not a class in which to learn something practical.Moreover , in the high school i attend, the computer science classes focuses EXCLUSSIVELY on algortihmic(there's nothing about OOP) .Fortunately a computer science teacher of the high school offered to make a course(of about 8 classes), for students from all over the high school ,whose objective was to introduce us to oop(c#).Before that, i didn't even know about oop. – Lazu Razvan Aug 03 '15 at 17:58

Simple, ordinary SQL Server insert method

3 Answers3

Linked