0

can someone help me with my code, i need to check first if record exist. Well i actually passed that one, but when it comes to inserting new record. im getting the error "There is already an open DataReader associated with this Command which must be closed first." can some help me with this? thanks. 

  Protected Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click

    Dim reg_con As SqlConnection
    Dim reg_cmd, chk_cmd As SqlCommand
    Dim checker As SqlDataReader
    Dim ID As Integer
    Dim fname_, mname_, lname_, gender_, emailadd_, college_, password_ As String

    ID = idnumber.Value
    fname_ = fname.Value.ToString
    mname_ = mname.Value.ToString
    lname_ = lname.Value.ToString
    gender_ = gender.Value.ToString
    college_ = college.Value.ToString
    emailadd_ = emailadd.Value.ToString
    password_ = reg_password.Value.ToString

    reg_con = New SqlConnection("Data Source=JOSH_FLYHEIGHT;Initial Catalog=QceandCceEvaluationSystemDatabase;Integrated Security=True")
    reg_con.Open()

    chk_cmd = New SqlCommand("SELECT IDnumber FROM UsersInfo WHERE IDnumber = '" & ID & "'", reg_con)
    checker = chk_cmd.ExecuteReader(CommandBehavior.CloseConnection)

    If checker.HasRows Then

        MsgBox("Useralreadyexist")

    Else

        reg_cmd = New SqlCommand("INSERT INTO UsersInfo([IDnumber], [Fname], [Mname], [Lname], [Gender], [Emailadd], [College], [Password]) VALUES ('" & ID & "', '" & fname_ & "', '" & mname_ & "', '" & lname_ & "', '" & gender_ & "', '" & emailadd_ & "', '" & college_ & "', '" & password_ & "')", reg_con)
        reg_cmd.ExecuteNonQuery()

    End If
    reg_con.Close()

End Sub
user3367225
  • 23
  • 1
  • 8
  • The issue is that you are using the same connection that's reading the data to insert the data. You need to create a new connection. reg_con is opened up and used with the data reader so it cant use the same open connection to write the data. – logixologist Apr 07 '14 at 20:44

3 Answers3

1

Add this string to your connection string

...MultipleActiveResultSets=True;";

Starting from Sql Server version 2005, this string allows an application to maintain multiple active statements on a single connection. Without it, until you close the SqlDataReader you cannot emit another command on the same connection used by the reader.

Apart from that, you insert statement is very dangerous because you use string concatenation. This is a well known code weakness that could result in an easy Sql Injection vulnerability

You should use a parameterized query (both for the insert and for the record check)

reg_cmd = New SqlCommand("INSERT INTO UsersInfo([IDnumber], ......) VALUES (" & _
                         "@id, ......)", reg_con)
reg_cmd.Parameters.AddWithValue("@id", ID)
.... add the other parameters required by the other field to insert.....
reg_cmd.ExecuteNonQuery()

In a parameterized query, you don't attach the user input to your sql command. Instead you put placeholders where the value should be placed (@id), then, before executing the query, you add, one by one, the parameters with the same name of the placeholder and its corresponding value.

Steve
  • 213,761
  • 22
  • 232
  • 286
  • Im getting this error, "Keyword not supported: 'multipleactiveresults'." – user3367225 Apr 07 '14 at 20:48
  • _MultipleActiveResultSets_ – Steve Apr 07 '14 at 20:49
  • What do you mean by this one? "Apart from that, you insert statement is very dangerous because you use string concatenation. This is a well known code weakness that could result in an easy Sql Injection vulnerability" – user3367225 Apr 07 '14 at 20:57
  • An SQL Injection Attack is based on the string concatenation of sql code with user input. The user prepares a particular text and when this text is blindly attached to the sql command and sent to the database its results is often very dangerous. [See this link](http://stackoverflow.com/questions/332365/how-does-the-sql-injection-from-the-bobby-tables-xkcd-comic-work) – Steve Apr 07 '14 at 20:59
  • Thanks for the information, can you advice me? and i will read the link that you gave me. thanks for that. – user3367225 Apr 07 '14 at 21:02
  • I have added something about parameterized queries to the answer. – Steve Apr 07 '14 at 21:05
  • THANKS!, I'll change it. for the better! Thanks very much! – user3367225 Apr 07 '14 at 21:08
  • Should i remove this? if im going to these things "ID = idnumber.Value fname_ = fname.Value.ToString mname_ = mname.Value.ToString lname_ = lname.Value.ToString gender_ = gender.Value.ToString college_ = college.Value.ToString emailadd_ = emailadd.Value.ToString password_ = reg_password.Value.ToString" if im going to use what you said? – user3367225 Apr 08 '14 at 05:18
  • You could just pass the value of the controls directly as second parameter in the AddWithValue. However pay attention to this fact. AddWithValue creates a parameter looking at the datatype of the value. So a string parameter is created if you pass a string, or an integer if you pass an integer. These datatypes should match the datatypes of the underlying fields that they want to update. – Steve Apr 08 '14 at 06:06
0

You need to close your reader using checker.Close() as soon as you're done using it.

rory.ap
  • 34,009
  • 10
  • 83
  • 174
0

Quick and dirty solution - issue checker.Close() as a first command of both IF and ELSE block.

But (better) you don't need a full blown data reader to check for record existence. Instead you can do something like this:

chk_cmd = New SqlCommand("SELECT TOP (1) 1 FROM UsersInfo WHERE IDnumber = '" & ID & "'", reg_con)
Dim iExist as Integer = chk_cmd.ExecuteScalar()

If iExist = 1 Then
....

This approach uses ExecuteScalar method that returns a single value and doesn't tie the connection.

Side note: Instead of adding parameters like you do now - directly to the SQL String, a much better (and safer) approach is to use parametrized queries. Using this approach can save you a lot of pain in the future.

Yuriy Galanter
  • 38,833
  • 15
  • 69
  • 136