Using OracleDataReader and OracleConnection (or any .Net connection and reader object).

It's always important to close connections and readers. Aside from it being sloppy coding not to do so. Failure to do so can prevent the release of connections back to the connection pool. Here is an example of using a reader that using an OracleConnection and OracleDataReader object. I use the “using“ statement to make sure everything is cleaned up. Instead of a “using“ statement you can also use a try/finally, but the syntax of “using“ is just so much cleaner.

try
{
     using(OracleConnection oracleConn = new OracleConnection(_connectionString))
     {
          // open connection
          try{oracleConn.Open();}
          catch{throw new LogonException();}
          // create the sql statement....i always prefer StringBuilder over string
          StringBuilder sql = new StringBuilder();
          sql.AppendFormat( "select NVL(SUM(SOME_COLUMN),0) from SOME_TABLE where COLUMN_NAME=",name );
          // build the command object
          OracleCommand cmd = new OracleCommand(sql.ToString(),oracleConn);
          cmd.CommandType = CommandType.Text;
          using(OracleDataReader reader=cmd.ExecuteReader(CommandBehavior.CloseConnection))
          {
               // read to get first (and only) record.
               reader.Read();

               // return a new object instance. i could check for DBNull, but my sqlstatement will guarantee a value.
               return reader.GetDouble(0);
          }
     }
}
catch( LogonException )
{
     // LogonException is an exception defined in my DataTier.
     // just rethrow it. it is from our internal code block
     throw
;
}
catch( Exception ex )
{
     // DataSourceException is an exception defined in my DataTier.
     throw new DataSourceException( ex );
}

14 Comments

  • using on a SqlConnection object at least physically closes the connection thus disabling connection pooling. Also note that you should not have catch blocks that simply rethrow the exception (LogonException).

  • The example is good, to make it better (yes, very nitpicky)



    - You could use ExecuteScalar

    - VB.Net doesn't have using (yet), so try/finally would be more global

    -Connection could be opened a bit later



    Also, instead of putting the conn.open() in its own try catch, I rather just add:

    if (conn.state != ConnectionState.Closed) conn.close()



    or something

  • Good point on the exception, didn't catch on to that right away.



    I just ran a test on our test database, it appears they work the same. Maybe this was a v1 problem. But I know I had this problem once upon a time because page load times were extremely high because connection pooling was not working and it was tracked down to use Dispose instead of Close.

  • Per executescaler. yes. i agree. if you are only returning a single result executescaler is worthy and your comment is correct.



    the example i pulled it from originally had multiple columns and i removed all columns but but one for the post (brevity)....oh well.



    Per your conn.Close() code.....Yes, you could put that in a finally block but the using statement makes it unncessary. So why? It's so much extra code. As we all know, the using statement calls Dispose() and Dispose() calls Close(). For example, if you decompile the SqlConnection dispose method using my favorite decompiler"Reflector" ;), you get the following:



    protected override void Dispose(bool disposing)

    {

    ConnectionState state1;

    if(!disposing)

    {

    goto L_0027;

    }

    state1=this._objectState;

    switch(state1)

    {

    case 0:

    {

    goto L_0020;

    }

    case 1:

    {

    goto L_001A;

    }

    }

    goto L_0020;

    L_001A:

    this.Close();

    L_0020:

    this._constr=null;

    L_0027:

    base.Disposing(disposing);

    }



    The OracleClient has quite a bit more code describing similar behavior. It's also interesting that it shows the code that puts it back into the pool upon Dispose() and therefore Close().



    Overall, the using statement has less code and therefore there is less of a chance of a bug showing up.



    -Mathew Nolton

  • And of course the typical SQL injection problem :) You really should not build queries as a string (or string builder, no difference) but use parameterized commands.

  • +1 on the SQL injection.



    Also, design-wise, do you REALLY want to throw a LogonException if the call to oracleConn.Open() throws an exception ? This has nothing to do with logging on, besides the fact that that's what the method does, so you'd get that info anyway from the stack trace. It's actually a connectivity problem with your DB. IMO, that's what you should be reporting.



    I'd reserve the LogonException for problems with the datareader code. For instance, what if you don't get a row back ? Would returning a success/failure be more appropriate than an exception in that case ?



    Finally, +1 on the "using" statement, it really does clean things up, and it's a pity more developers don't use it.

  • Per the SqlInjection problem....If you expose your database server, then yes you have the problem and its always a debate as to whether or not you use dynamic sql or procs. In my case and with certain databases getting a procedure built takes an act of congress. Therefore, dynamic sql is really the only thing i can do.



    Per the LogonException. I don't agree. This is an exception defined in my DAL. I never throw the platform specific exception because that implies a certain level of intimacy with my underlying rdbms that I don't want to expose to my clients. It is always wrapped and it has a specific meaning to my clients.



    It could just as easily be called NoLoginException. The whole point is that I want to provide enough information about where the problem is without exposing to much sensitive information....The reality is that this exception never makes it past by business logic layer. I like to have a few well defined exceptions in my DAL that can be easily trapped in my BLL....it just makes life so much simpler.



    Yes. I also agree that the using statement is an underused syntax....it does make life so much easier.



    -Mathew Nolton

  • I most commonly use custom exceptions for cases where client code SHOULD do something with them. If I can't open a connection to the DB, I throw a CannotConnectToDatabaseException. This allows my business code to differentiate between different kinds of DAL problems. Actually, I try to check the error code in my provider specific wrappers, and throw more meaningfull exceptions if possible.



    On the other hand, when it comes to checking if a user is authenticated, my Login method returns a boolean. This is the common case.

  • Ok then I am not sure that I understand the distinction you are trying to make. I throw a LogonException and you throw an CannotConnectToDatabase() for a similar condition. And we both try to use custom exceptions to throw meaningful exceptions.



    -Mathew Nolton

  • I wasn't talking about sprocs, I was talking about using IDbCommand object and using parameters. That's a little different than using sprocs.



    And I don't understand why having a database server will leave you vulnerable to SQL injection problems, if the user your code runs under is only allowed to execute sprocs then that's the only thing it will be allowed to do. So even if somebody manages to login to your db server as your code they will only be able to execute sprocs you wrote. If you allow the code to make modifications to tables (because you're using ad-hoc queries) than the attacker will be able to execute any query your permissions allow (which may be more than you'd like).

  • to make sure we are discussing agreeing and/or disagreeing on a set of points. let me get this straight.



    i agree from a security standpoint that a proc call is the best approach and all of the stuff you discuss about security. again i agree. but we can't always have use procedures ( for whatever reasons) as i discussed above. so putting the proc approach aside, we are left w/ dynamic sql.



    now we can 1) execute dynamic sql using a command object with raw sql where the parameter values are passed into the method and formatted by us(e.g. developer) into the sql stream we send to the server. Or 2) Execute dynamic sql using a command object but instead of formatting the parameters into

    the stream we use the parameter collection which is the approach you were talking about in your first post. Fair statement? From a logging into the database and access rights, both approaches need the same credentials. Also a fair statement?



    from everything i have read, the dbcommand w/ parameters approach is all about treating parameters as literals so that sql injection cannot occur. Now if i do a great job validating my parameters and i make sure that the parameters being combined with my sql are treated as literals with option 1. the two approaches yield the same result. true statement?



    so, at the end of the day. dbcommand with parameters is A solution but not THE ONLY solution.



    -Mathew NOlton

  • By the way....Good discussion everyone.



    -Mathew Nolton

  • Jonathan,

    Interesting approach. how has it worked for you? any issues you've had?

    -Mahtew Nolton

  • Business logic code that catches a CannotConnectToDatabase exception would probably behave differently than if it caught a general LogonException that only reports that logon wasn't able to be performed, without giving any reason why. That is the difference.

Comments have been disabled for this content.