locked
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

    Friday, February 17, 2012 10:39 PM

Answers

  • 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

    • Marked as answer by Harald Kogler Monday, March 05, 2012 4:17 PM
    Friday, February 24, 2012 9:33 AM
  • 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

    Tuesday, February 28, 2012 10:30 AM
    Moderator

All replies

  • 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

    Sunday, February 19, 2012 2:17 PM
  • 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

    Monday, February 20, 2012 2:20 AM
    Moderator
  • 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

    • Edited by Harald Kogler Monday, February 20, 2012 12:26 PM Added Help Source
    Monday, February 20, 2012 12:18 PM
  • 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

    Monday, February 20, 2012 1:58 PM
  • 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)

    Monday, February 20, 2012 5:07 PM
  • 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

    Monday, February 20, 2012 5:36 PM
  • Thanks, the bug has been posted. I'll keep you informed with the results.

    Tuesday, February 21, 2012 12:05 AM
  •  [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

    Tuesday, February 21, 2012 7:02 AM
    Moderator
  • 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

    Tuesday, February 21, 2012 1:17 PM
  • 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


    Thursday, February 23, 2012 2:09 AM
    Moderator
  • 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.

    Thursday, February 23, 2012 9:01 AM
  • 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

    Thursday, February 23, 2012 10:06 AM
    Moderator
  • 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

    Thursday, February 23, 2012 10:28 AM
  • @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()) ... } } }

    Thursday, February 23, 2012 1:15 PM
  • 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


    Thursday, February 23, 2012 1:42 PM
  • 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

    Friday, February 24, 2012 7:28 AM
    Moderator
  • 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

    • Marked as answer by Harald Kogler Monday, March 05, 2012 4:17 PM
    Friday, February 24, 2012 9:33 AM
  • 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.)
    Friday, February 24, 2012 1:45 PM
  • 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

    Tuesday, February 28, 2012 10:30 AM
    Moderator
  • 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

    Thursday, March 01, 2012 6:20 AM
    Moderator