Update SQL table from VB multiple where condition

Question

I am attempting to update an SQL table from VBA (with the below code)and cant seem to get it correct. I would like to update the columns one, two, three and four based on the search conditions of A, B, C, D, E. What am I getting wrong here? There is no error given, but the table just does not update? thank

Sub UpdateData(A As String, B As String, C As String, D As String, E As String, one As Double, two As Double, three As Double, four As Double)

Dim sA As String, sB As String, sC As String, sD As String, sDesk As String, sE As String
Dim sone As Double, stwo As Double, sthree As Double, sfour As Double

Dim objConn As ADODB.Connection
Set objConn = New ADODB.Connection
objConn.ConnectionString = "Provider=SQLOLEDB;Data Source=source;Initial Catalog=Model_table;Integrated Security=SSPI"
objConn.Open
Set objRec = New ADODB.Recordset


Date = Format(Range("date").Value, "YYYY-MM-DD")
sA = A
sB = B
sC = C
sD = D
sE = E
sone = one
stwo = two
sthree = three
sfour = four
     StrSQL = "UPDATE pnl_results SET (" & sone & "," & stwo & "," & sthree & "," & sfour & ") Where  ('" & date =  sDate & "', '" & AA= sA & _
              "','" & BB= sB & "','" CC= & sC & "','" & DD= sD & "','" & EE=sE & ")"
     Set objRec = objConn.Execute(StrSQL)

objConn.Close
Set objConn = Nothing

End Sub

One large concern here is that this query is wide open to sql injection. Firstly, you should be looking to parametrise this. — Thom A, Feb 02 '18 at 15:31
I agree that parameterising it would be better, but there really is no need if the coder controls all the variables. — braX, Feb 02 '18 at 15:34
@braX I will strongly disagree with your comment. Controlling the input variables is a horrible approach to this. Parameterizing the queries is the way to go. Consider if somebody else uses your code as an example and the query was not using parameters. It is so simple to do it the right way that anything else is negligent. — Sean Lange, Feb 02 '18 at 15:48
@MrNemo you can read more about sql injection here. http://bobby-tables.com/ — Sean Lange, Feb 02 '18 at 15:49
@SeanLange - I understand, but it's a big learning curve for inexperienced coders, at least from what I've seen. — braX, Feb 02 '18 at 15:50
@braX so.... an inexperienced coder that never heard about SQL injection would have an easier time validating/sanitizing user inputs than parameterizing their queries? — Mathieu Guindon, Feb 02 '18 at 15:53
@braX inexperienced coders are exactly the people who need to learn how to use parameters. It is not all that difficult to learn if there is some teaching going on. Anything other than doing it the right way is a sign of being content with dangerous code. This is not acceptable. If you know how to use parameters and you don't teach people how to protect themselves you are doing them a disservice. — Sean Lange, Feb 02 '18 at 15:54
@SeanLange I like your thinking (I'm not a big believer of "dumbing it down"). That said this comment thread is dragging... we should probably all leave it at that, ...or move it to chat. — Mathieu Guindon, Feb 02 '18 at 15:56
@braX I disagree with you. Learning how to use parameterized variables is just as easy, if not easier (Because it's really easy to mess up validation/sanitization and not realize it), than validating/sanitizing. It is _not_ a hard technology to learn by any means. — GrumpyCrouton, Feb 02 '18 at 15:57
I think people are missing my meaning. I am referring to updates that do not require user input. That's all I meant. — braX, Feb 02 '18 at 16:00
And besides, with how all of you are so strict about answering the actual question, this one was about SQL syntax. Telling the person to use parameters instead doesn't actually answer the question. — braX, Feb 02 '18 at 16:03
@braX true, but OP learned about SQL injection in the process, and now they know a better, safer solution exists :) — Mathieu Guindon, Feb 02 '18 at 16:06
And if we all we do is ignore the horrible practice we are doing them a disservice. Helping them understand how to do it correctly is what they are looking for. They want to know how to update the table. And telling them the right way to do it (with parameters) is the correct and complete answer. — Sean Lange, Feb 02 '18 at 16:07
I'm not saying it's bad to make them aware of a better alternative, guys... geez, sorry for having an opinion. :P — braX, Feb 02 '18 at 16:09
@braX meh.. comments (even chat) just aren't ideal for this... that's a nice discussion I wish we could all be having around a beer! cheers! — Mathieu Guindon, Feb 02 '18 at 16:14
I second the notion for this discussion in person....and of course beer!!! — Sean Lange, Feb 02 '18 at 16:20
Thanks guys, a really interesting debate. But, any recommended links on Parameterizing Sql queries.? Can't find a decent example for say inserting or updating as in the below the below solution. — , Feb 02 '18 at 22:33

Racil Hilan · Accepted Answer · 2018-02-02T15:41:05.180

3

There are several errors in your code:

You don't need parentheses.
Put the column names directly in the string, don't surround them with apostrophes.
Surround all string values with apostrophes.
Use AND or OR operator to combine different conditions, not a comma.

Try this:

StrSQL = "UPDATE pnl_results SET " & _
         " one = '" & sone & _
         "', two = '" & stwo & _
         "', three = '" & sthree & _
         "', four = '" & sfour & _
         "' Where date = '" & sDate & _
         "' AND AA = '" & sA & _
         "' AND BB = '" & sB & _
         "' AND CC = '" & sC & _
         "' AND DD = '" & sD & _
         "' AND EE = '" & sE & "'"

As mentioned in the comments, using inline query with values coming directly from user input makes your code open to SQL injection attacks. Either validate user input before using it in the query, or better use a parameterized query.

edited Feb 02 '18 at 15:41

answered Feb 02 '18 at 15:34

Racil Hilan

24,690
13
50
55

1

Validating/sanitizing user input boils down to "rolling your own" and I would strongly and vehemently advise against it. Parameterize, period. =) – Mathieu Guindon Feb 02 '18 at 15:48
@Mat'sMug Generally, yes, but not always. In some scenarios input validation is enough, and in rare scenarios parameterized queries are not possible. In all cases, even when using parameterized queries, input validation is still strongly recommended. – Racil Hilan Feb 02 '18 at 15:53
Depending on what "validation" stands for, I definitely agree. If it means escaping single quotes and stripping SQL keywords, then... big fat "nope" ;-) – Mathieu Guindon Feb 02 '18 at 15:54
NO. Input validation is a joke. Parameters are so simple to use. I can't figure out why so many people fall into the trap of input validation. – Sean Lange Feb 02 '18 at 15:55
@SeanLange I took *input validation* as, e.g. you have a text field that expects a 5-digit string, then you put the `TextBox.MaxLength` to 5 and actively validate that every character is a digit before you even begin to think assigning it to a query parameter value. Note, it ends up as a parameter anyway =) – Mathieu Guindon Feb 02 '18 at 15:58
@Mat'sMug Escaping apostrophes is not an input validation, so yeah, we agree :). Input validation means ensuring that the values are what they should be. Example, the age must be a number between... Hmmm, let's say 0 and 200 :). – Racil Hilan Feb 02 '18 at 15:59
@Mat'sMug sure that works for simple inputs. How do you handle a comment box with that type of validation? – Sean Lange Feb 02 '18 at 16:00
@RacilHilan the number validation should not be done prior to entry. If that is a rule of the data it should be a constraint in the database. – Sean Lange Feb 02 '18 at 16:00
1

@SeanLange cap the comment box maxlength, and then, well, that's all you can do; send the contents to a nvarchar parameter and call it a day! As for db-vs-app validation/constraints, that's up for debate: as a dev I like a "dumb db" that merely enforces PK's, FK's and NK's, and leave the "business rules" and such, at the client/application level. But someone with a DBA background might disagree. That's kind of an eternal debate ;-) – Mathieu Guindon Feb 02 '18 at 16:03
@SeanLange Strongly disagree. Validation is always business rules and **MUST** be in UI and business code, not database. On top of that and if we ignore it for a moment, your suggestions is sooooo wasteful and inefficient. Why would you try to enter invalid data to the database, just to get it rejected and an exception is thrown and then go back all the way up to the UI to tell the user that they screwed up? – Racil Hilan Feb 02 '18 at 16:04
To be fair that type of validation should be done in both the database and the UI. – Sean Lange Feb 02 '18 at 16:05
No, no, and no... No validation of any type in the database beyond data integrity. Period. The only exception is in some huge solutions where there is a complete separation between the database and code. these cases are super rare. Exception from the database are very expensive. – Racil Hilan Feb 02 '18 at 16:08
@RacilHilan don't you love triggers? #derailing – Mathieu Guindon Feb 02 '18 at 16:08
LOL. We will have to agree to disagree there. A solution does not have to be large to have good separation from the code and the database. Why would you allow invalid data into your database? – Sean Lange Feb 02 '18 at 16:10
Of course I wouldn't have sql in my application anyway. I would have all my updates be in a stored procedure. Saves all this discussion because it would force everything to be parameterized. And only take a few extra minutes during development to do it like that. – Sean Lange Feb 02 '18 at 16:14
@SeanLange I wouldn't. Who said I would? I just prefer to do it way much sooner than when it reaches the database. If it really reaches the database, I'd give up programming and give it to others who know how to program :). By separation between database and code, I didn't mean that in the wider sense as that is required for all projects like you said. I meant when the database is a solution by itself and the apps to access it are done by other teams. Very rare cases. – Racil Hilan Feb 02 '18 at 16:16
@Mat'sMug I'm not sure how to read your question, is it a joke, are you asking for my opinion, are you trying to tell me something? No idea :). Very few of my applications have triggers, but I do use triggers when needed. I don't know if that qualifies as "love" or "hate" :). – Racil Hilan Feb 02 '18 at 16:18
He was commenting that the comments here got derailed. – Sean Lange Feb 02 '18 at 16:19
Sarcasm failed, sorry! I never *needed* triggers, and the only exposure I've had to triggers has been a horrendous spaghetti mess of side-effects. Been advocating business logic in code since then :) – Mathieu Guindon Feb 02 '18 at 16:21
Ahhhh, apparently I'm not a social-media gen X :). I'm really getting old :'(. Now I got what that meant, a "hash tag" :). Yeah, we better stop here and delete our comments. It was good talking to you guys. – Racil Hilan Feb 02 '18 at 16:23

score 1 · Answer 2 · answered Feb 02 '18 at 15:31

1

You are confusing the syntax between an Update and an Insert Into. It should look more like this:

StrSQL = "UPDATE pnl_results SET field1='value1', field2='value2' WHERE field3='value3' AND field4='value4'

answered Feb 02 '18 at 15:31

braX

11,506
5
20
33

Update SQL table from VB multiple where condition

2 Answers2