-1

I have a SqlDB.dll that has the function:

         public SqlDataReader getEnumValues(int enumId)
    {
        SqlDataReader reader = null;
        using (SqlConnection connection = new SqlConnection(connectionString))
        {
            connection.Open();
            SqlCommand command =
                new SqlCommand(
                    "SELECT * FROM [EnumValue] WHERE enumId LIKE '" + enumId + "';",
                    connection);
            reader = command.ExecuteReader();
            //if(reader.Read())
            //    Debug.WriteLine("Inside sqlDb->getEnumValues command = " + command.CommandText + " reader[name] = " + reader["name"].ToString() + " reader[value] = " + reader["value"].ToString() + " reader[description] = " + reader["description"].ToString());
        }
        //reader.Close();
        return reader;
    }

As you can see I have tried closing the reader before returning it, and also I read the data inside and it's ok. I'm using the function like this:

 using (SqlDataReader getEnumValuesReader = (SqlDataReader)getEnumValues.Invoke(sqlDB, getEnumValuesForEnumParam))
                    {
                        Debug.WriteLine("Success getEnumValues -- ");

                        if (getEnumValuesReader.HasRows)
                        {
                            while (getEnumValuesReader.Read())        //Loop throw all enumValues and add them to current enum
                            {
                                try
                                {
                                    values.Add(new Model.EnumValue(getEnumValuesReader["name"].ToString(), getEnumValuesReader["value"].ToString(), getEnumValuesReader["description"].ToString()));
                                    Debug.WriteLine("Value[0].name = " + values[0].Name);
                                }
                                catch (Exception ex)
                                {
                                    Debug.WriteLine("Error in building new EnumValue: " + ex.Message);
                                }
                            }
                        }

                    }

and I'm getting exception of type 'System.InvalidOperationException'

I'm guessing it has something to do with the Sqldatareader being passed.

Yogevnn
  • 1,430
  • 2
  • 18
  • 37
  • 1
    [SQL Injection alert](http://msdn.microsoft.com/en-us/library/ms161953%28v=sql.105%29.aspx) - you should **not** concatenate together your SQL statements - use **parametrized queries** instead to avoid SQL injection – marc_s Apr 20 '14 at 20:32
  • I dont have a security problem at all, it does not have any user \ other developers \ plans for the future but thank you for the answer – Yogevnn Apr 20 '14 at 20:35
  • If this code is used for web sites - **any** of those web sites is in high danger of being exploited by SQL injection. Just let me know what web sites those are so I can make sure to never ever visit any of them .... – marc_s Apr 20 '14 at 20:37
  • 1
    No it is not for web, it is a VSPackage program I'm building. I'm familiar with the SQL injection problem but I'm not worried in this case. – Yogevnn Apr 20 '14 at 20:39
  • No matter what the end product is - it's just accepted **best practice** to avoid concatenating together your SQL. You should really use **parametrized queries** - ***ALWAYS, no exceptions***. – marc_s Apr 20 '14 at 20:41

1 Answers1

3

The SQL reader cannot exist outside the context of the SQL connection. Your connection is being disposed in your method, so your returned reader cannot fetch any data in the calling method.

the best option is to return the values from the reader instead of the reader. It is not good practice to pass around readers, which means the underlying connection etc. Is open and undisposed.

Raja Nadar
  • 9,409
  • 2
  • 32
  • 41
  • Thank you for the answer, my problem is how to return all the values? because not all of the values are from the same type, some are strings and some are int – Yogevnn Apr 20 '14 at 20:24
  • you just need to declare a class with all your types (string, int etc.), and return a List from the method.. also called as DTOs. Data Transfer Objects. – Raja Nadar Apr 21 '14 at 19:48