1

I realize IDataReader is outdated and some view it as dirty code, but on the site I am working on this is what they use. I have an IDataReader statement to run a query to get a specific id from a table using multiple joins. Now this site has a DAL but it only supports the ability to select from one table at a time, so using select statements with joins do not work with it. This is why I am forced to use IDataReader with this.

 if (Request.QueryString["CategoryId"].ToString() == "0")
                {
                    using (IDataReader getCategoryID = DB.GetRS("SELECT ItemCatalogCategory.CategoryID FROM UserCustomerCatalog INNER JOIN ItemCatalogCategory ON UserCustomerCatalog.ItemProfileCatalogID = ItemCatalogCategory.ItemProfileCatalogID " +
                              "INNER JOIN ItemCategory ON ItemCatalogCategory.CategoryID = ItemCategory.CategoryID INNER JOIN StoreCatalog ON UserCustomerCatalog.StoreCatalogID = StoreCatalog.StoreCatalogID " +
                              "WHERE UserCustomerCatalog.ItemProfileCatalogID = '" + Request.QueryString["CatalogID"] + "' AND UserCustomerCatalog.CustomerID =' " + Session["Customer"].ToString() + "' AND ItemCategory.ProductID = '" + productis + "'"))
                    {

                        if (getCategoryID.Read())
                        {
                            string categoryID = getCategoryID["ItemCatalogCategory.CategoryID"].ToString();

                            string lookmike = Request.Url.AbsolutePath + "?CatalogID=" + catalogis + "&ProductID=" + productis + "&CatalogIndex=" + Request.QueryString["CatalogIndex"] + "&CategoryID=" + categoryID;
                            Response.Redirect(Request.Url.AbsolutePath + "?CatalogID=" + catalogis + "&ProductID=" + productis + "&CatalogIndex=" + Request.QueryString["CatalogIndex"] + "&CategoryID=" + categoryID);

                        }
                        else
                        {
                            Response.Redirect(Request.Url.AbsolutePath + "?CatalogID=" + catalogis + "&ProductID=" + productis + "&CatalogIndex=" + Request.QueryString["CatalogIndex"] + "&CategoryID=" + Request.QueryString["CategoryId"]);
                        }

                    }//end using getCategoryID
                }

this is what I have, but when it gets to:

if (getCategoryID.Read())

it renders as false, there are no exceptions thrown, and no errors or warnings. I have done this type of select in the past with no problems, but I cannot figure out why .Read() is returning false.

Can anyone suggest possible reasons for it not reading? If more code is needed, I can provide as needed. Any help is appreciated, thank you in advance.

  • 1
    Is it possible that there really are no results? How many results are returned when you run the SQL query you're creating against whatever DB system you're using? – Michael Todd Nov 07 '12 at 22:57
  • 2
    "I realize IDataReader is outdated and some view it as dirty code" not in the least... DataTable, maybe - but data-reader is fine. Personally I'd hide it away a *little* (not right next to the UI) though. – Marc Gravell Nov 07 '12 at 23:03
  • 2
    You *have*, however,got some ***huge*** SQL injection holes there. Very unsafe code. (Still looking...) – Marc Gravell Nov 07 '12 at 23:05
  • I'd recommend suggesting to the DAL code maintainers that their code forces a vulnerability to SQL injection attacks. – prprcupofcoffee Nov 07 '12 at 23:06
  • If it is returning false fromRead, then it isn't returning data (assuming the DAL didn't eat the data). In which case,mothered a problem in the SQL you've generated. You're just going to have to capture the SQL you've generated, and test it in SSMS. And then rewrite it with parameters. – Marc Gravell Nov 07 '12 at 23:09
  • You can try use debugger to display the whole SQL query after it's evaluated and then copy-paste and execute it directly in the database manager. You've many INNER JOINs, maybe it's true that no records are found? (it's enough one of them has no match). You can also temporary change INNER JOIN to LEFT JOIN and see if still no records exists. By the way - records ID are mostly numbers - you use ' ' for them, are they a text fields? – mj82 Nov 07 '12 at 23:12
  • 2
    HERE ->`' " + Session["Customer"].ToString() + "'` there is a space that brokes your query. Is it a typo? – Steve Nov 07 '12 at 23:16
  • SQL INJECTION: http://stackoverflow.com/search?q=sql+injection – Hardrada Nov 08 '12 at 00:16
  • @MichaelTodd I know for sure there is exactly 1 result with this query, I have tested it in MS SQL Management Studio with all the necessary variables. –  Nov 08 '12 at 13:47
  • @Steve This is what was causing it! a small typo on my part thank you very much for finding that, if you would like credit for it just fill out the answer box and I will mark you as the answer. Again thank you for your help –  Nov 08 '12 at 13:48

1 Answers1

1

Looking at your SQL text there is a little typo that could wreak havoc with the results

 "WHERE UserCustomerCatalog.ItemProfileCatalogID = '" + Request.QueryString["CatalogID"] + 
 "' AND UserCustomerCatalog.CustomerID =' " + Session["Customer"].ToString() + "' AND ..... "
                                    here ^

That space mangles your query and give no result.

Let me also repeat that you have a problem with SQL Injection as other members have already said. You could add an overload to your actual implementation of GetRS that receive also a SQLParameter collection to add to the command used to build your SqlDataReader. Something like this

public SqlDataReader GetRS(string sqlText, SqlParameter[] prm)
{
      ....
      SqlCommand cmd = new SqlCommand(sqlText, conn);
      cmd.Parameters.AddRange(prm);
      .....
}

and then start to upate the calling code.

Steve
  • 213,761
  • 22
  • 232
  • 286
  • Thank you for your answer and advice to help avoid SQL Injection, I will try to implement a parameter structure for this and future coding similar to this. Again thank you for your help, I really do appreciate it. –  Nov 08 '12 at 14:03
  • 2
    Glad to be of help. And, while we are at it, let's take a pause and smile a little. http://xkcd.com/327/ – Steve Nov 08 '12 at 14:08