FxCop Suppressions For Designer Generated Code?

Topics: EF Designer, EF Power Tools
Oct 20, 2012 at 5:50 PM
Edited Oct 20, 2012 at 5:50 PM

When using EF to create a model for a database, the out-of-the-box generated code generates a number of FxCop warnings.  For now I've worked around them by adding suppressions to the .tt templates in my project, but ideally the generated code would be clean to start with.

Creating a simple model against the SQL 2012 AdventureWorks database generates the following warnings:

  • CA1506: Warning for excessive class-coupling due to the fact there's a class referenced for each table in the database due to the one-to-one relationship of tables to types.
  • CA2214: Warning for using virtual members in the constructor of each model where it contains at least one navigation property.
  • CA2227: Warning for each navigation property as it is a collection property with a getter and a setter.

EF 5.0 generates some other warnings (CA1063), but these don't appear to be issues in EF 6.0.

I've submitted a pull request to suppress these messages, but it's been suggested that these may wish to be fixed instead by changing the generated code.  Personally I think they should be suppressed as they appear to be design decisions made to improve testability or are inherent to the EF approach to database modelling.  My reasoning for each is as below.

CA1506 should be suppressed, as if you're working with a genuinely large database (50+) tables, then it would be overkill to pull the context apart into different context classes, as you'd end up with a fair amount of duplicate code, and may end up having to use more than one context to perform operations between them, which would probably cause a lot of headaches.

CA2214 is fired due to the navigation properties being virtual.  I'm guessing they were made virtual to allow tools like Moq to use setups on the navigation properties for use in unit tests.  If this is the reason why they are designed as such, it would seem to be safe to suppress the warning on the grounds that someone is unlikely to derive a class from a model type and override the properties manually in such a way that caused undesirable behaviour.

CA2227 is fired as the navigation properties are of type ICollection<T>, which causes FxCop to suggest that instead they should have a private setter to prevent them being set by public callers.  Again, the current design appears to be sensible to that the navigation properties can be easily accessed for setup in unit tests, otherwise you'd need to perform constructs such as:

Model foo = new Model();
foo.NavProperty.Clear();

foreach (var item in myMockCollection)
{
    foo.NavProperty.Add(item);
}

which is much more verbose that the current usage of:

Model foo = new Model();
foo.NavProperty = myMockCollection;

Based on the above, personally I think these three warnings should be suppressed so that the models that are generated are FxCop clean.

Any other thoughts on whether they should be suppressed or the design changed instead to eliminate the warnings in the first place?

Martin

Nov 27, 2012 at 8:07 PM

You get the same problem if you use VS' Static Code Analysis tool.  If you enable Code Analysis on Build and set the rule set to Microsoft Basic Correctness Rules, you get a ton of errors from the Code-Based Migrations generated (designer) files.  I had to go into the designer files and add the GeneratedCode attribute to them.  Since these files are generated, this attribute should be on those classes.  J

Dec 18, 2012 at 9:51 AM

Should definitely be fixed. MANY of those warnings are quality warnings that are useless for generated code (i.e. they point to possible human error, which can not happen there). Not marking generated code as such is IMHO an error.

Coordinator
Jan 14, 2013 at 11:29 PM

Hey,

We've been discussing this here at Microsoft and agree that suppressing some of these is the right thing to do. Changing the code not to violate CA2214 and CA2227 would result in messier code that is harder to test.

We agree that any code that is not to be maintained by the developer (i.e. migrations designer files, code generated from an edmx model, etc.) should be marked as generated. This code is a little different because once it has been scaffolded it becomes part of the developers code base. That's why we want to suppress rather than blanket mark it as generated.

In the near future we're planning to merge the Power Tools functionality into the EF Designer (see this spec for more details - http://entityframework.codeplex.com/wikipage?title=Tooling%20Consolidation). Given that, rather than taking accepting a pull request for the Power Tools we will take care of dealing with the suppressions in the new code base.

~Rowan