CA2000 on returning SqlCommand with many predefined parameters

Question

Hi!

I'm using following piece of code - and other similar methods - for creating SQLCommands for INSERT, UPDATE, or SELECT-Commands.

        private SqlCommand CreateInsertCommand()
        {
            SqlCommand tmpCmd = null;
            SqlCommand result = null;
            try
            {
                string command =
                    "INSERT INTO TABLE (" +
                          "PL_AREA " +
                        ", PL_PART " +
                        ", PL_VALUE " +
                        ", PL_VALMAX " +
                        ", PL_ISVAL " +
                        ", PL_TEXT " +
                        ", ZUORDNUNG " +
                        ", KNSWEB " +
                        ", CAR4YOU " +
                        ", EXT1 " +
                        ", EXT2 " +
                        ", EXT3 " +
                        ", EXT4 " +
                        ", EXT5 " +
                        ", PL_TEXT2 " +
                        ", PL_TEXT3 " +
                        ", SysDefaultText " +
                        ", ERF_USER " +
                        ", ERF_TIME " +
                        ", MOD_USER " +
                        ", MOD_TIME " +
                    ") VALUES (" +
                          "@PLArea " +
                        ", @PLPart " +
                        ", @PLValue " +
                        ", @PLValMax " +
                        ", @PLIsVal " +
                        ", @PLText " +
                        ", @Zuordnung " +
                        ", @KNSWeb " +
                        ", @Car4You " +
                        ", @Ext1 " +
                        ", @Ext2 " +
                        ", @Ext3 " +
                        ", @Ext4 " +
                        ", @Ext5 " +
                        ", @PLText2 " +
                        ", @PLText3 " +
                        ", @SysDefaultText " +
                        ", @User " +
                        ", GetDate() " +
                        ", @User " +
                        ", GetDate() " +
                    ") ";

                tmpCmd = new SqlCommand(command);
                tmpCmd.Parameters.Add(new SqlParameter("PLArea", SqlDbType.VarChar, 15) { IsNullable = false });
                tmpCmd.Parameters.Add(new SqlParameter("PLPart", SqlDbType.VarChar, 10) { IsNullable = false });
                tmpCmd.Parameters.Add(new SqlParameter("PLValue", SqlDbType.VarChar, 20) { IsNullable = false });
                tmpCmd.Parameters.Add(new SqlParameter("PLValMax", SqlDbType.VarChar, 10) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("PLIsVal", SqlDbType.VarChar, 4) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("PLText", SqlDbType.VarChar, 200) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Zuordnung", SqlDbType.VarChar, 8) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("KNSWeb", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Car4You", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Ext1", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Ext2", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Ext3", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Ext4", SqlDbType.Text) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("Ext5", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("PLText2", SqlDbType.VarChar, 35) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("PLText3", SqlDbType.VarChar, 35) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("SysDefaultText", SqlDbType.VarChar, 200) { IsNullable = true, Value = DBNull.Value });
                tmpCmd.Parameters.Add(new SqlParameter("User", SqlDbType.VarChar, 70) { IsNullable = true, Value = DBNull.Value });

                result = tmpCmd;
                tmpCmd = null;
            }
            finally
            {
                if (tmpCmd != null)
                    tmpCmd.Dispose();
            }
            return result;
        }

For some of these Methods I'm getting

CA2000 : Microsoft.Reliability : In method 'Class1.CreateInsertCommand()', call System.IDisposable.Dispose on object 'tmpCmd' before all references to it are out of scope.

In the above example Method, if I remove 3 Parameters (no matter which 3), the warning is gone, other Methods seem to have another "total count of  parameters" for this warning to occur.

Is there another way to avoid this than suppress the message?

Regards

Harald

Answer 1

You should not dispose the SqlCommand at all! You're returning it to be used elsewhere. Also, no need to create both a tmpCmd and a result variable to further confuse FxCop and other readers of your code.

private SqlCommand CreateInsertCommand()
        {
            SqlCommand result = null;
            try
            {
                string command =
                    "INSERT INTO TABLE (" +
                          "PL_AREA " +
                        ", PL_PART " +
                        ", PL_VALUE " +
                        ", PL_VALMAX " +
                        ", PL_ISVAL " +
                        ", PL_TEXT " +
                        ", ZUORDNUNG " +
                        ", KNSWEB " +
                        ", CAR4YOU " +
                        ", EXT1 " +
                        ", EXT2 " +
                        ", EXT3 " +
                        ", EXT4 " +
                        ", EXT5 " +
                        ", PL_TEXT2 " +
                        ", PL_TEXT3 " +
                        ", SysDefaultText " +
                        ", ERF_USER " +
                        ", ERF_TIME " +
                        ", MOD_USER " +
                        ", MOD_TIME " +
                    ") VALUES (" +
                          "@PLArea " +
                        ", @PLPart " +
                        ", @PLValue " +
                        ", @PLValMax " +
                        ", @PLIsVal " +
                        ", @PLText " +
                        ", @Zuordnung " +
                        ", @KNSWeb " +
                        ", @Car4You " +
                        ", @Ext1 " +
                        ", @Ext2 " +
                        ", @Ext3 " +
                        ", @Ext4 " +
                        ", @Ext5 " +
                        ", @PLText2 " +
                        ", @PLText3 " +
                        ", @SysDefaultText " +
                        ", @User " +
                        ", GetDate() " +
                        ", @User " +
                        ", GetDate() " +
                    ") ";

                result = new SqlCommand(command);
                result.Parameters.Add(new SqlParameter("PLArea", SqlDbType.VarChar, 15) { IsNullable = false });
                result.Parameters.Add(new SqlParameter("PLPart", SqlDbType.VarChar, 10) { IsNullable = false });
                // ... other parameters
                return result;
            }
            catch
            {
                if (result != null)
                     result.Dispose();
                throw;
            }
        }

As for the number of parameters, it's better to create a DTO object or use EntityFramework, DataRows or any other method to abstract away all these parameters into one object. That's much easier to update and maintain.

---

My blog: blog.jessehouwing.nl

Answer 2

I am writing to check the status of the issue on your side. 
What about this problem now? 
Would you mind letting us know the result of the suggestions?

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 3

I'm a bit confused now, because the way I implemented the method is just the way as it is suggested in the online help to CA2000, but I see that your code would just do the same - only without the second SqlCommand variable.

I tried your approach, but the behavior is still the same - when the parameter count exceeds a certain value the Warning CA2000 is thrown. - If I remove some Parameters the warning will not be thrown...

Entity Framework, Data Rows or other don't meet my requirements so i need to write my own classes and generators - and I'd like them to be "warning free" as well as "suppressed warning minimized"

For completeness - I'm referring to this Help article:

http://msdn.microsoft.com/en-us/library/ms182289.aspx

Answer 4

There has been a lot of disussion on this, and there are basically two camps. But I don't like the null assignment to a vaiable to control flow, it's very untransparant. And it's very easy for someone to add a "if (result != null) result.Dispose(); to it, just because it looks like it was missed.

As for the reason why you're getting the warning, I find that very interesting, would you be able to send me the actual assembly so that I can have a look at the generated IL? I suspect the compiler might be doing some interesting things in the background which are causing this issue. If possible send me the assembly (compressed with 7zip or rar) to jesse.houwing (at) avanade.com.

Also, just for completeness, which version of Visual Studio/FxCop are you using? The Code Analysis version that ships with Visual Studio 2010 Premium and Ultimate has a completely re-worked version of this rule, the older versions are a lot less efficient. 

---

My blog: blog.jessehouwing.nl

Answer 5

I agree with you: with the try-catch version it's more obvious what's happening.

I'm using Visual Studio 2010 Premium Version 10.0.40219.1 SP1Rel / C# / and Visual Studio Code Analysis

A demo project where I can reproduce the issue should be in your mailbox.

- Just remove or comment-out two of the parameters to make the warning disappear... (It seems, with the removal of the "tmpCmd" we raised the command-limit)

Answer 6

I get it too. Looks like a bug in the rule (or they are takign a shortcut to improve performance).

File a bug on connect (https://connect.microsoft.com/VisualStudio/feedback/CreateFeedbackForm.aspx?FeedbackFormConfigurationID=4861&FeedbackType=1),  csome experimenting leads me to believe that it's being caused by the initializer syntax, if you create either your own local SQLParameter locals and set their properties or use the following syntax:

                result.Parameters.Add(new SqlParameter("PLArea", SqlDbType.VarChar, 15));
                result.Parameters[7].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLPart", SqlDbType.VarChar, 10));
                result.Parameters[8].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLValue", SqlDbType.VarChar, 20));
                result.Parameters[9].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLValMax", SqlDbType.VarChar, 10));
                result.Parameters[10].IsNullable = true;
                result.Parameters[11].Value = DBNull.Value;

the issue clears up magically.

---

My blog: blog.jessehouwing.nl

Answer 7

Thanks, the bug has been posted. I'll keep you informed with the results.

Answer 8

 [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]

I think we just can use this attribute statement to suppress this warning message form your method.

And, Harald, can you post the MS Connect case link here if you had submitted this issue.

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 9

Yes, the Warning can be suppressed, but this leads to the assumption that something is wrong in the code - not in the code analysis.

Also, as I asked in first place:

Is there another way to avoid this than suppress the message?

This way to the connect case

Answer 10

We can code it like this way, then the warning will be disappear.

The "using block" is a try-catch block for the SqlCommand instance, it will handle the exception and dispose the object.

        private SqlCommand CreateInsertCommand()
        {
            
            SqlCommand result = null;
            //try
            //{
                string command =
                    "INSERT INTO TABLE (" +
                          "PL_AREA " +
                        ", PL_PART " +
                        ", PL_VALUE " +
                        ", PL_VALMAX " +
                        ", PL_ISVAL " +
                        ", PL_TEXT " +
                        ", ZUORDNUNG " +
                        ", KNSWEB " +
                        ", CAR4YOU " +
                        ", EXT1 " +
                        ", EXT2 " +
                        ", EXT3 " +
                        ", EXT4 " +
                        ", EXT5 " +
                        ", PL_TEXT2 " +
                        ", PL_TEXT3 " +
                        ", SysDefaultText " +
                        ", ERF_USER " +
                        ", ERF_TIME " +
                        ", MOD_USER " +
                        ", MOD_TIME " +
                    ") VALUES (" +
                          "@PLArea " +
                        ", @PLPart " +
                        ", @PLValue " +
                        ", @PLValMax " +
                        ", @PLIsVal " +
                        ", @PLText " +
                        ", @Zuordnung " +
                        ", @KNSWeb " +
                        ", @Car4You " +
                        ", @Ext1 " +
                        ", @Ext2 " +
                        ", @Ext3 " +
                        ", @Ext4 " +
                        ", @Ext5 " +
                        ", @PLText2 " +
                        ", @PLText3 " +
                        ", @SysDefaultText " +
                        ", @User " +
                        ", GetDate() " +
                        ", @User " +
                        ", GetDate() " +
                    ") ";
 
                using (SqlCommand tmpCmd = new SqlCommand(command))
                {
                //SqlCommand tmpCmd = new SqlCommand(command);
                    tmpCmd.Parameters.Add(new SqlParameter("PLArea", SqlDbType.VarChar, 15) { IsNullable = false });
                    tmpCmd.Parameters.Add(new SqlParameter("PLPart", SqlDbType.VarChar, 10) { IsNullable = false });
                    tmpCmd.Parameters.Add(new SqlParameter("PLValue", SqlDbType.VarChar, 20) { IsNullable = false });
                    tmpCmd.Parameters.Add(new SqlParameter("PLValMax", SqlDbType.VarChar, 10) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("PLIsVal", SqlDbType.VarChar, 4) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("PLText", SqlDbType.VarChar, 200) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Zuordnung", SqlDbType.VarChar, 8) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("KNSWeb", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Car4You", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Ext1", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Ext2", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Ext3", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Ext4", SqlDbType.Text) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("Ext5", SqlDbType.VarChar, 50) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("PLText2", SqlDbType.VarChar, 35) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("PLText3", SqlDbType.VarChar, 35) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("SysDefaultText", SqlDbType.VarChar, 200) { IsNullable = true, Value = DBNull.Value });
                    tmpCmd.Parameters.Add(new SqlParameter("User", SqlDbType.VarChar, 70) { IsNullable = true, Value = DBNull.Value });
 
                    result = tmpCmd;
                    //tmpCmd = null;
                }
            //}
            //finally
            //{
            //    //if (tmpCmd != null)
            //    //    tmpCmd.Dispose();
            //}
            return result;
        }

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 11

In my understanding the using-block automatically disposes the resource referenced with it when leaving the block, like a try-finally block, not try-catch.

The variable "result" also receives a reference on the resource of the using-block - so "result" would in fact return a already disposed SqlCommand-object.

Please correct me if I'm getting this wrong.

Answer 12

Sorry, the return statement should be safe in the using block.

http://brendan.enrick.com/post/Returning-From-Inside-a-Using-Statement.aspx

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 13

Important remark: As long as you don't do anything asynchronous with the SqlCommand, or as long as you don't assign it to something to use it later on and continue.

---

My blog: blog.jessehouwing.nl

Answer 14

@Mike: I still don't get it...
As i can see in the Blog it is safe to return from within a using block -> the resource will still be disposed..

But I need the resource that would be disposed by the using-block in another method...

This is an example for a usage of the "CreateSelectCommand"-Method and the SqlCommand-Object returned by it.

using (SqlConnection connection = ToolsLib.GetConnection())
{
   using (SqlCommand selectCommand = CreateSelectCommand())
   {
      selectCommand.Connection = connection;
      selectCommand.Parameters[0].Value = "SomeValue";
      
      using (SqlDataReader rd = selectCommand.ExecuteReader())
      {
         while(rd.Read())
         ...
      }
   }
}

Answer 15

Mike,

The using statement is syntactic sugar for try{} finally{ ((IDisposable)target).Dispose(); }, not for a try-catch.  In your example, it will always dispose the SqlCommand before returning it from the method, which is really not something that one would typically wish to do to the return value of a factory method.

The only reason that it does not have a deleterious impact in this particular case is that disposing a SqlCommand doesn't actually do anything that prevents subsequent use of the SqlCommand in .NET Framework versions that have shipped so far.  Needless to say, there is no guarantee that this would continue to be the case in future .NET versions.  It's also far from the case for many other disposable types that one might wish to return from a factory method.

Nicole

Answer 16

Thanks Nicole!

It is the try-finally, its my writing mistake.

About the SqlCommand.Dispose() method, you're right, it is my lucky when I write the above logic for that class.

This CA2000 is a known issue. It seems that this will be fixed in the next version of Visual Studio.

And now, I admit that I have no idea more to skip this warning.

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 17

So, to get the available options together:

One could suppress the Warning by adding this Attribute to the factory method: - Thanks Mike

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]

One could rewrite the parameters and avoid using the Initializer syntax - Thanks Jesse

                result.Parameters.Add(new SqlParameter("PLArea", SqlDbType.VarChar, 15));
                result.Parameters[7].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLPart", SqlDbType.VarChar, 10));
                result.Parameters[8].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLValue", SqlDbType.VarChar, 20));
                result.Parameters[9].IsNullable = false;
                result.Parameters.Add(new SqlParameter("PLValMax", SqlDbType.VarChar, 10));
                result.Parameters[10].IsNullable = true;
                result.Parameters[11].Value = DBNull.Value; 

One can wait for the bug to be resolved

https://connect.microsoft.com/VisualStudio/feedback/details/725836/warning-ca2000-is-fired-on-a-sqlcommand-with-many-sqlparameters#details

Thanks @all

Answer 18

Mike,

The general approach for a factory method returning a disposable should be to dispose then rethrow if any exception is caught after instantiation of the return value.  e.g.:

public DisposableThing CreateThing()
{
	DisposableThing result = new DisposableThing();
	try
	{
		// Do some more setup of the result.
	}
	catch (Exception)
	{
		result.Dispose();
		throw;
	}

	return result;
}

This is actually pretty simple -- it might be worth including in the CA2000 documentation as the recommended resolution for factory methods. (There's already a somewhat similar example in the community content section of the doc topic, but it's unnecessarily complex since it moves initialization inside the try block, thereby requiring a pre-try null assignment, and a null test in the catch block.)

Answer 19

Thanks for your feed back to the document, you can add your comment to the document directly.

And I think change the Initialize statement also cannot solve the problem, I don't know if you have test this way with OP codes, at least it cannot work in my computer.

And one senior engineer told me that this issue would be occur if there are more than 16 possible places where would caused the exception, and the issue would be fixed in feature.

This can explain why the OP code will work if comment out two sql command statements.

If there's any concern, please feel free to let me know.

Best wishes,

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us

Answer 20

I am writing to check the status of the issue on your side. 
What about this problem now? 
Would you mind letting us know the result of the suggestions?

---

Mike Zhang[MSFT]
MSDN Community Support | Feedback to us