vb.net Module for data access - how bad is this?

Question

I've inherited some code that has a Public Module for data access:

Code looks like:

Public Module Foo
    Dim ds As New DataSet

    Public Function GetDataSet(ByVal sqlQuery As String) As DataSet
     ...
       Fill(ds)...
     ...
    return ds
    End Function
End Module

And all the pages call GetDataSet(sql). Am I correct is assuming that this is a bad idea? Worst case is that concurrent callers could get each others data?

Answer 1

Very bad. With that architecture there is absolutely no way to use a parameterized query or stored procedure to protect yourself from SQL Injection...and that's just a start.

You're also going to be missing out on any kind of strongly typed data sets (which make life a whole lot easier).

Answer 2

i use something similar to this and have never had any problems. makes data access sooo easy. i don't mind writing SQL and in fact it kind of helps me understand exactly what data i'm getting. so long as data access is clearly separated (ie, you pull data consistently from the same location), and you sanitize all data in the SQL string, i don't see anything wrong w/this. i do agree that public variable is bad, though.

sigh... or maybe i'm a bad programmer :\ let the downvoting begin...

Answer 3

1. Your concerns about concurrent callers is well founded. The solution is to just move the "Dim ds.." from the module into the GetDataSet routine. Then its local, on the stack, and theres no chance of stepping on each other.

2. A larger problem is that composing SQL queries as string in client code is not good application design, it can be OK for SQL utilities, but nothing else. This approach will keep you from using Linq to SQL, Linq to Entities, Stored Procedures, most of the more desirable DB-access security models and probably exposes you to SQL Injection attacks.

Answer 4

Do you even have to ask?