Previously here I posted about a custom FxCop rule that uses the new 3.12 Introspection engine. At the time I didn't post the entire rule because it was very specific to our needs and it wasn't the best way to solve the problem in question.
We have a number of other ongoing issues that we continue to see in code reviews. Rather than having to spend our time pointing out the same problems to multiple teams I've decided to codify the more prominent problems as FxCop rules.
When I wrote the 1st FxCop rule I was a little disappointed in how hard it was. Now, many weeks later, I've gone back and written two more rules and I can honestly say that writing the rules isn't hard at all. My previous difficulties were really a function of the problem I was trying to enforce. It wasn't something that a static code analysis can actually handle very well. Lesson learned: static analysis will only take you so far. If it is too hard to do then probably you are using the wrong tool for the job.
We have a standard set of classes that help create SqlConnection objects. The idea behind these classes is that they separate the decision step of where the connection strings live and how they are encrypted from the developer. All the developer has to do is call GetConnection(key) with some key and they will get the connection object complete with connection string information.
In order to enforce this standard I've created the UseGlobalDataBaseConnections rule.
UseGlobalDataBaseConnections
This rule analyzes the code searching for any OptCode == Newobj. If we find one we check to see what the type of the object being created is. If the type is System.Data.SqlClient.SqlConnection.#ctor then we check to see if the class that is creating the object is in the Connection.Wrapper namespace and if it is it won't flag it as a problem, otherwise it is added to the problem collection.
Here is the rule:
13 public class DoNotCreateSqlConnectionDirectly:BaseEmptyFxCopRule
14 {
15 public DoNotCreateSqlConnectionDirectly():base("DoNotCreateSqlConnectionDirectly"){}
16 #region Check
17 /// <summary>
18 /// Inspects the instructions for potential problems. This is called by the subclass
19 /// check method which in turn is called by the FxCop engine
20 /// </summary>
21 /// <param name="member"></param>
22 /// <returns></returns>
23 public override ProblemCollection Check( CCI.Member member)
24 {
25
26 CCI.Method method = member as CCI.Method;
27 if(method ==null)
28 return null;
29
30 //get the instructions for this method
31 CCI.InstructionList instructions = method.Instructions;
32 if(0 == instructions.Length)
33 return null; // not likely but just in case we don't want to throw an error
34
35 //loop through our instructions and see if we can find any items of interest
36 for(int i=1; i<instructions.Length;i++)
37 {
38 CCI.Instruction instr = instructions[i]; //get the current instruction
39 //if we are creating a new object check for a SqlConnection
40 if(CCI.OpCode.Newobj ==instructions[i].OpCode)
41 {
42
43 if(IsTypeSqlConnection(instructions[i]))
44 {
45 CheckForSqlConnectionProblems(instructions,method, i);
46 }
47 }
48
49 }
50 return base.Problems;
51 }
52 #endregion
53 #region IsTypeSqlConnection
54 /// <summary>
55 /// Determines if this is a SqlConnection object that is being created
56 /// </summary>
57 /// <param name="instruction"></param>
58 /// <returns></returns>
59 private bool IsTypeSqlConnection(CCI.Instruction instr )
60 {
61 CCI.Method method = instr.Value as CCI.Method;
62 if(method==null)
63 return false;
64
65 Debug.WriteLine(String.Format("IsTypeSqlConnection: " + method.FullName));
66 if(method.FullName.IndexOf("System.Data.SqlClient.SqlConnection.#ctor")==0)
67 {
68 return true;
69 }
70 else
71 {
72 return false;
73 }
74
75 }
76 #endregion
77 #region CheckForSqlConnectionProblems
78 /// <summary>
79 /// This method makes sure that we aren't actually in the wrapper class before
80 /// adding the problem item to the problems collection
81 /// </summary>
82 /// <param name="instr"></param>
83 /// <param name="index"></param>
84 private void CheckForSqlConnectionProblems(CCI.InstructionList instr, CCI.Method method, int index)
85 {
86 Debug.WriteLine("CheckForSqlConnectionProblems " + method.GetMethodInfo().DeclaringType.ToString());
87 if(method.GetMethodInfo().DeclaringType.ToString()!= "Connection.Wrapper")
88 {
89 base.Problems.Add(new Problem(GetResolution(method.Name.Name), instr[index]));
90 }
91 }
92 #endregion
93
94 }
This class is based upon the FxCopBaseClass that I wrote about in my previous posting. See link above for it.
Here is the rule Xml
<Rule TypeName="DoNotCreateSqlConnectionDirectly" Category="Connections" CheckId="CK114">
<Name>DoNotCreateSqlConnectionDirectly</Name>
<Description>SqlConnection objects should only be created via calls to Connection.Wrapper</Description>
<LongDescription>Use Connection.Wrapper to return a connection object. This ensures that all connection strings are managed centrally and consistently. You must use the existing classes to retrieve your connection object.</LongDescription>
<FixCategories>Breaking</FixCategories>
<Owner>T Weaver</Owner>
<Url></Url>
<Resolution>Add a reference to Connection.Wrapper and use the Connections class to retrieve the desired connection object</Resolution>
<Email>icodemarine@gmail.com</Email>
<MessageLevel Certainty="99">Error</MessageLevel>
</Rule>
A couple of things I found out while creating these new rules:
1) You can't create a rule that is both Breaking and NonBreaking. For example I had a case where what the developer was doing could be correct in some cases, but in others it was definitely wrong. I wanted to flag each case differently. In order to do that I had to create two rules that are identical except one is Breaking and one is NonBreaking
2) Because I had two rules that went over the same code path and found the same problems I had to structure both rules so that if a Breaking error was found on a specific piece of code a NonBreaking wasn't flagged as well, even though it would meet all the criteria for the NonBreaking. It turns out this wasn't that hard to do, but it did make for a few minutes of grumbling on my part as I tried to figure out an elegant way to handle it.
I didn't want to duplicate the majority of the code for both cases so I created a base class. The base contained an abstract method CheckForProblem() along with all the shared Check(CCI.Method method) logic. Then each subclass determined if the problem in question was something they should add to their own problem collection. It worked out well because all the more brittle checking logic is shared. Now all I need is a way to do TDD with FxCop and I'll be a happy coder.