0

I have the issue with the following function:

Public Function collectuserid(conn, username)
    Dim sql As String = "select ID from tblUserDetails where Username =" & username
    Dim usersid As String = Nothing
    Using connection As New OleDbConnection(conn)
        Using command As New OleDbCommand(sql, connection)
            connection.Open()
            usersid = CStr(command.ExecuteNonQuery())
            connection.Close()
        End Using
    End Using
    Return usersid
End Function

The problem occurs in the following line:

usersid = CStr(command.ExecuteNonQuery())

The variable conn holds the connection string, and username holds a value for the username present in the database. I want to collect the userid of the record, but cant seem to get it right. I have the exact same function open in another program with a different database and it works perfectly. All table and variable names are correct also. Any help?

Program for generating the record:

Sub insertuservalues(conn, a, b, c, d)
    Dim sql As String = "INSERT INTO tblUserDetails(Name,Username,[Password],Email) VALUES (@name, @username, @password, @email)"
    Using connection As New OleDbConnection(conn)
        Using command As New OleDbCommand(sql, connection)
            connection.Open()
            command.Parameters.Add("@name", OleDbType.VarWChar).Value = a
            command.Parameters.Add("@username", OleDbType.VarWChar).Value = b
            command.Parameters.Add("@password", OleDbType.VarWChar).Value = c
            command.Parameters.Add("@email", OleDbType.VarWChar).Value = d
            command.ExecuteNonQuery()
            connection.Close()
        End Using
    End Using
End Sub
Kxpr
  • 23
  • 5
  • 2
    You *really* need to make `username` into an SQL Parameter. – Andrew Morton Apr 17 '20 at 15:58
  • 1
    Also, there is some chance that it should be `ExecuteScalar`, not `ExecuteNonQuery`. – Andrew Morton Apr 17 '20 at 16:04
  • 1
    Me again! You forgot to use [`Option Strict On`](https://stackoverflow.com/a/29985039/1115360). It will help make sure that variable types match up instead of VB making things up as it goes along, possibly not as you wished. – Andrew Morton Apr 17 '20 at 16:07
  • Turned on option strict now, and tried ExecuteScalar, but it gives me the same error. You did mention paramaters.. They are something completely new to me and i believe i have created the record using them. Ive updated the post with the code for that also. – Kxpr Apr 17 '20 at 16:28
  • Yikes, this looks scary-vulnerable to sql injection issues. – Joel Coehoorn Apr 17 '20 at 16:29
  • Also, post the updated code after you turned Option Strict On. We know you must have changed it, because that code won't compile with Option Strict. It's missing type names for the Function return and arguments. – Joel Coehoorn Apr 17 '20 at 16:30

2 Answers2

0

Strictly speaking, the mistake you made was that you didn't wrap your literal text value within the SQL code in single quotes. Just as VB literal text must be wrapped in double quotes, so SQL literal text must be wrapped in single quotes:

Dim sql As String = "SELECT ID FROM tblUserDetails WHERE Username = '" & username & "'"

or:

Dim sql As String = String.Format("SELECT ID FROM tblUserDetails WHERE Username = '{0}'", username)

or:

Dim sql As String = $"SELECT ID FROM tblUserDetails WHERE Username = '{username}'"

If you do it the right way though, and follow the advice to use parameters, this becomes redundant. ALWAYS use parameters to avoid formatting issues, special character issues and, most importantly, SQL injection issues.

jmcilhinney
  • 50,448
  • 5
  • 26
  • 46
  • Thanks for the help, but i believe i am using parameters already. I didn't put it into the original post, but ive eddited it now, adding the sub that generates the record – Kxpr Apr 17 '20 at 16:33
  • Ive changed the line, works perfectly now. Thank you very much – Kxpr Apr 17 '20 at 16:34
  • @Kxpr You have to use parameters **EVERYWHERE** you accept input data. The insert command has parameters, but you're missing them in the `collectuserid()` method, and that's _**not okay**_. – Joel Coehoorn Apr 17 '20 at 16:36
  • Also, very bad form ever suggesting string concatenation to put values into a query like this. – Joel Coehoorn Apr 17 '20 at 16:41
  • @JoelCoehoorn, where did I ever suggest that? Did you miss the part where I said "ALWAYS use parameters"? Not sure how that qualifies as a suggestion to do anything else. – jmcilhinney Apr 18 '20 at 04:28
0

You need single quotes around string values in the SQL. But don't do that! Instead, define this as a query parameter:

Public Function collectuserid(conn As String, username As String) As String
    'No string concatentation!
    ' Also, OleDb tends to use ? placeholders for positional parameters, rather than parameter names.
    Dim sql As String = "select ID from tblUserDetails where Username = ?" 

    Using connection As New OleDbConnection(conn)
        Using command As New OleDbCommand(sql, connection)

            'Use the actual type and length from the database here
            command.Parameters.Add("username", OleDbtype.VarWChar, 20).Value = username
            connection.Open()
            Return CStr(command.ExecuteNonQuery())
            'No need to call connection.Close()
            ' The Using block guarantees the connection will close, even though we Return before reaching the end of it.
        End Using
    End Using
End Function
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794