4

As part of my common utilities I used in all my line of business applications, I have this code...

using System.Web.UI.WebControls;

public class Database
    {
    /// <summary>
    /// Creates a DataView object using the provided query and an SqlDataSource object.
    /// </summary>
    /// <param name="query">The select command to perform.</param>
    /// <returns>A DataView with data results from executing the query.</returns>
    public static DataView GetDataView(string query)
        {
        SqlDataSource ds = GetDBConnection();
        ds.SelectCommand = query;
        DataView dv = (DataView)ds.Select(DataSourceSelectArguments.Empty);
        return dv;
        }

     /// <summary>
     /// Creates a SqlDataSource object with initialized connection string and provider
     /// </summary>
    /// <returns>An SqlDataSource that has been initialized.</returns>
    public static SqlDataSource GetDBConnection()
        {
        SqlDataSource db = new SqlDataSource();
        db.ConnectionString = GetDefaultConnectionString(); //retrieves connection string from .config file
        db.ProviderName = GetDefaultProviderName(); //retrieves provider name from .config file
        return db;
        }
   }

Then, in my projects, to retrieve data from databases I'll have some code like..

DataView dv=Database.GetDataView("select mycolumn from my table");
//loop through data and make use of it

I have taken some heat from people for using SqlDataSource in this manner. People don't seem to like that I'm using a Web control purely from code instead of putting it on an ASPX page. It doesn't look right to them, but they haven't been able to tell me a downside. So, is there a downside? This is my main question. Because if there's a lot of downsides, I might have to change how I'm doing many internal applications I've developed.

My Database class even works from non-ASP.NET situations, so long as I add the System.Web assembly. I know it's a slight increase in package size, but I feel like it's worth it for the type of application I'm writing. Is there a downside to using SqlDataSource from say a WPF/Windows Forms/Console program?

mason
  • 31,774
  • 10
  • 77
  • 121
  • 1
    Is this code a simplified version of your real code, or do you never pass parameters to your query? Because the code you've posted seems to invite [SQL Injection](http://www.troyhunt.com/2013/07/everything-you-wanted-to-know-about-sql.html) if you need to pass parameters. – Richard Deeming Dec 19 '13 at 16:50
  • @RichardDeeming Simplified, I have other code that scrubs for SQL Injection. I suppose I should mention that this code is used for internal line of business applications inside our 60 person company. It's not publicly accessible. – mason Dec 19 '13 at 16:56
  • And there's no possibility of internal users trying to hack your databases, or malware getting inside your network, right? http://www.troyhunt.com/2013/10/your-corporate-network-is-already.html – Richard Deeming Dec 19 '13 at 17:17
  • @RichardDeeming I'm not saying there's no possibility. I'm saying I take steps to protect against it and that it's less of a concern than it would be for a public website. – mason Dec 19 '13 at 17:21

3 Answers3

1

Well, there are no hard rules stopping anyone from doing such implementation.

However, following are few questions that need to be answered before doing that implementation.

  1. Is this usage thread safe? (because there is every possibility the same call can be made by multiple consuming applications.
  2. Will there be a layered differentiation (UI.Control being used in a Data layer)?
  3. What if that control becomes obsolete / restricted in the next framework releases?
Consult Yarla
  • 1,150
  • 10
  • 22
1

Given how easy it is to replace this code, whilst removing the temptation to use dynamic SQL queries to pass parameters, I think the question should be: is there any benefit to keeping the code as-is?

For example:

public static class Database
{
    private static readonly Func<DbCommandBuilder, int, string> getParameterName = CreateDelegate("GetParameterName");
    private static readonly Func<DbCommandBuilder, int, string> getParameterPlaceholder = CreateDelegate("GetParameterPlaceholder");

    private static Func<DbCommandBuilder, int, string> CreateDelegate(string methodName)
    {
        MethodInfo method = typeof(DbCommandBuilder).GetMethod(methodName, BindingFlags.Instance | BindingFlags.NonPublic, Type.DefaultBinder, new Type[] { typeof(Int32) }, null);
        return (Func<DbCommandBuilder, int, string>)Delegate.CreateDelegate(typeof(Func<DbCommandBuilder, int, string>), method);
    }

    private static string GetDefaultProviderName()
    {
        ...
    }

    private static string GetDefaultConnectionString()
    {
        ...
    }

    public static DbProviderFactory GetProviderFactory()
    {
        string providerName = GetDefaultProviderName();
        return DbProviderFactories.GetFactory(providerName);
    }

    private static DbConnection GetDBConnection(DbProviderFactory factory)
    {
        DbConnection connection = factory.CreateConnection();
        connection.ConnectionString = GetDefaultConnectionString();
        return connection;
    }

    public static DbConnection GetDBConnection()
    {
        DbProviderFactory factory = GetProviderFactory();
        return GetDBConnection(factory);
    }

    private static void ProcessParameters(
        DbProviderFactory factory, 
        DbCommand command, 
        string query, 
        object[] queryParameters)
    {
        if (queryParameters == null && queryParameters.Length == 0)
        {
            command.CommandText = query;
        }
        else
        {
            IFormatProvider formatProvider = CultureInfo.InvariantCulture;
            DbCommandBuilder commandBuilder = factory.CreateCommandBuilder();
            StringBuilder queryText = new StringBuilder(query);

            for (int index = 0; index < queryParameters.Length; index++)
            {
                string name = getParameterName(commandBuilder, index);
                string placeholder = getParameterPlaceholder(commandBuilder, index);
                string i = index.ToString("D", formatProvider);

                command.Parameters.AddWithValue(name, queryParameters[index]);
                queryText = queryText.Replace("{" + i + "}", placeholder);
            }

            command.CommandText = queryText.ToString();
        }
    }

    public static DataView GetDataView(string query, params object[] queryParameters)
    {
        DbProviderFactory factory = GetProviderFactory();

        using (DbConnection connection = GetDBConnection(factory))
        using (DbCommand command = connection.CreateCommand())
        {
            command.CommandType = CommandType.Text;
            ProcessParameters(factory, command, query, queryParameters);

            DbDataAdapter adapter = factory.CreateDataAdapter();
            adapter.SelectCommand = command;

            DataTable table = new DataTable();
            adapter.Fill(table);
            return table.DefaultView;
        }
    }
}

With this version, you can now pass in parameters simply and safely, without relying on custom code to try to block SQL injection:

DataView dv = Database.GetDataView(
   "select mycolumn from my table where id = {0} and name = {1}",
   1234, "Robert');DROP TABLE Students;--");

EDIT
Updated to support parameters for different providers, with help from this answer.

Community
  • 1
  • 1
Richard Deeming
  • 29,830
  • 10
  • 79
  • 151
  • FYI, SqlDataSource SelectCommand (and the other commands) do not allow you to put a semicolon in your query outside of an internal string. But I do appreciate your code. It's probably doing something very similar to what SqlDataSource is doing internally. – mason Dec 19 '13 at 17:27
  • @msm8bball: Are you sure? I've just tried your code with the query `"select * from sys.databases;select * from sys.objects;"`, and it executed both commands. – Richard Deeming Dec 19 '13 at 17:38
  • I tried it with System.Data.OracleClient and it gives a runtime exception. Now that I think about it, I can't recall having tried that out from SQL Server or any other data provider. Maybe that was System.Data.OracleClient specific. – mason Dec 19 '13 at 17:54
  • @msm8bball: I've not used `OracleClient`, and AFAIK, it's been discontinued. However, I've updated my answer to account for the different parameter name syntaxes used by the different providers. – Richard Deeming Dec 19 '13 at 18:15
-1

The only issues I see are

(1) this is like reinventing the wheel. There is Enterprise library v5 for FW3.5 and v6 for FW4.5, which has data access components. Use that.

With EL you can make a call and have 2,3,4 tables loaded in Dataset. With your method this is not possible, only one at the time.

Enterprise library is a complete Data Access suite provided by Microsoft. It takes care of all the little details and all you need is to call your data. This is complete data access layer. And if you look deeper, EL allows for integration of Data and Caching, and other things. But you don't have to use what you don't need. If you need data access you can use only that.

And (2) Generally, this is not a good idea to write low level assembly with high-level assembly in reference. Anything System.Web.... is UI and client related stuff. In a layered cake design this is like the top of it and Data Access is on the bottom. All references [save for "common"] should travel from bottom to the top and you have it in opposite direction.

Look at this picture:

enter image description here

This is from Microsoft. You see the layers of the "cake". All references are going up. What you've done - you took UI-related component and wrote Data Access in it.

You can call it opinion-based - but this opinion is standard practice and pattern in software development. Your question is also opinion based. Because you can code everything in single file, single class, and it will work. You can set references to System.Windows.Forms in Asp.net application, if you want to. Technically, it is possible but it is really bad practice.

Your application now have limited reusability. What if you write WPF component or service that need to use same Data Access. You have to drag all System.Web into it?

T.S.
  • 18,195
  • 11
  • 58
  • 78
  • What advantage does Enterprise library have over what I'm already doing? Your second point carries no weight. You're saying "but, it's not meant to be used that way!" without saying _why_ that's a bad idea. If you can provide specific examples of why this would be a bad idea I would appreciate it. – mason Dec 18 '13 at 23:47
  • @msm8bball - see my edits. Why? Because you have no separation of concerns. Read about "SOLID". You created UI-DataAccess mix. This is technically possible (in this case, because Microsoft provided components for "fast fixes"), but in good application design this is not going to happen. – T.S. Dec 19 '13 at 15:44
  • Regarding your edit, I fail to see how that picture makes a difference. You are not explaining _why_ it's a bad idea. You're saying it's not _meant_ to be used that way without explaining _why_ it's a bad practice. Although SqlDataSource is in a UI related namespace, it's hardly UI related. In fact, the only reason I can see that they bothered making it a Control in the first place is so Microsoft can say "ASP.NET can show data without writing a single line of code!" – mason Dec 19 '13 at 15:45
  • Ok, here is another big "Why". Because your application now have limited reusability. What if you write WPF component or service that need to use same Data Access. You have to drag all System.Web into it? As I said before, many of these controls really are to say "ASP.NET can show data without writing a single line of code!". No serious design uses any of these. People write their frameworks because this control doesn't take care of crosscutting concerns in the application. – T.S. Dec 19 '13 at 15:54
  • Like I said in the post, I simply add a reference to System.Web. Bam, it's reusable. I have successfully used this class in ASP.NET, console, and Windows Forms projects. How is that limited reusability? – mason Dec 19 '13 at 15:57
  • Have you tried? I have suspicion that once you out of Http Context you can't use it. But even if you can today, nobody guaranties that tomorrow, Microsoft will change something in it and it will not work without `HttpContext`. Another thing, it connects to `SqlClient`, `OleDb`, `Odbc`, `OracleClient` but not ODP.NET – T.S. Dec 19 '13 at 16:09
  • Like I just said, I used this in ASP.NET, console, and Windows Forms projects. It does WORK. I am here looking for tangible reasons why it would be a bad idea. SqlDataSource logically doesn't use anything from HttpContext. Why would it need to? It's a class used for data access. And again, you're making guesses about the future that aren't based on anything. And yes, it can use ODP.NET. – mason Dec 19 '13 at 16:13
  • Last post: odp.net? MS doesn't say that. – T.S. Dec 19 '13 at 16:29