Feedback

Dec 14, 2009 at 12:06 AM
Edited Dec 14, 2009 at 1:32 AM
First up, Matt, thanks a lot for your efforts and for open sourcing this project.
You asked for feedback, so I recorded my thoughts during implementation, please fire away if you would like more information or would like me to track these as separate issues, I've also created this as a Google wave if you want more dynamic discussion. Some of the issues are minor, I'm a little OCD so please forgive me :)

Things I like:

  • It works! This is the most important thing right?
  • Custom database schema is great
  • Using byte for AuditAction enum is nice way to save some space over normal int

Things I had issues with:

  • Assembly is not signed, so will always force users to build from source if they sign
  • Dependency on System.Web (Databinder.Eval for getting property value, HTTPContext for setting user name (this changed now though?))
  • Unwanted build artifacts under version control (.suo, .user, bin, obj)
  • Missing (public) and incorrect documentation (invalid parameter references) - Just turn on all build errors. May be fixed in new code, but Ghostdoc FTW!
  • AssemblyInfo probably should contain license and copyright details, ambiguous at the moment.
  • AuditedEntity (was EntityAuditRecord) is not CLS Compliant (base type 'Doddle.Linq.Audit.LinqToSql.AuditableDataContext' is not CLS-compliant)
  • Consider null object pattern for AuditedEntity.AssociationTableKey to avoid NullReferenceException? (current example on codeplex will throw NRE if no association, a default NullEntityKey object would solve that)
  • Issues with AuditPropertyResolver.GetAuditValue when database schema uses UPPERCASE_FOR_FIELD_NAMES. Just needs more unit tests.

Things I'm not sure about:

  • Extensibility model for IAuditableContext. Our audit logs use a UserId rather than UserName, and we also link to event logs, things which happen in the system (e.g. user failed to log in) which may not generate audit data, but are still audit-related. I ended up adding my own properties to IAuditableContext to handle this, and I'll need to add more, but not sure if there is a better way? 
If you would like patches for any of my changes (I use SVN though, not TFS), or are keen for other developers to help I can try to pitch in. Disclaimer: we're in crunch mode at the moment.
thanks again!
si
Dec 14, 2009 at 12:54 AM
Edited Dec 14, 2009 at 1:49 AM

As a start here's a change for StringExtensions to handle all upper case field names:

 

public static string[] SplitUpperCase(this string source)
{
if (source == null)
return new string[] { }; //Return empty array.

if (source.Length == 0)
return new string[] { "" };

// Don't split if all upper case e.g. COMPANY_NAME
if (source == source.ToUpper())
return new string[]{ source };

// Don't split if just last character is lower case e.g. PRODUCTs
if (source.Length > 1)
{
string ignoreLastChar = source.Substring(0, source.Length - 2);
if (ignoreLastChar == ignoreLastChar.ToUpper())
return new string[] { source };
}

//...

 

Sorry I can add tests, we use NUnit or MBUnit and don't have the Visual Studio assemblies under our source control, so I had to remove the test project from our source, since the missing assemblies were breaking our build server.

EDIT:  After thinking about it a little more, I guess some folks would want COMPANY_NAME converted to Company Name.  However for us, we'll probably end up using a lookup table to convert table and field names into descriptions.  So I'm wondering now would it be better to use a dependency injection or IoC (like Autofac) or some sort of extensibility framework (MEF?) to handle the areas that change?

 

Regarding the NullReferenceException on AuditedEntity.AssociationTableKey, not even sure you really need a null object (it's probably cleaner though), but this fixed it for me:

internal void UpdateKeys()
{
    if (AuditAssociation != null)
    {
        // ..
    }
    else
    {
        object pk = AuditDefinition.PkSelector.Compile().DynamicInvoke(Entity);
        EntityTableKey = new EntityKey(pk);
        AssociationTableKey = new EntityKey(new Object()); // Avoid NullReferenceException
    }
}

cheers
si

Coordinator
Dec 14, 2009 at 3:35 PM

@CombatWomat,

Thanks for the great feedback. A lot of the issues you mentioned are exactly the same issues I've been concerned about as well. Some of them I believe still have // TODO comments in the source (i.e., the remove dependency on System.Web)

I really want to put some time into fixing these and other issues soon, and given your feedback would be more than happy to incorporate patches into the next release. Perhaps I should set myself a deadline so I stop putting it off :)

I like your recommendation about the extensibility points (specifically the SplitUpperCase implementation. Perhaps I can take a page from the MVC manual regarding custom model binders, etc, and create extension hooks for users to customize this process without being forced to change the source code. For example, AuditContext.NameFormatter.Default = new MyCustomNameFormatter();

I'll put some more thought into that. Are there any other extensibility points in the library that would be most beneficial for you right away? Regarding those other fixes I will work on those soon as well.

Please keep it coming

-Matt

Dec 15, 2009 at 12:22 AM
Edited Dec 15, 2009 at 12:23 AM

Hey Matt,  thanks for the prompt reply, I think you can tell that quite a few people are interested in this project; the issue tracker contains some useful fixes.

As far as extensibility points, the only two areas I've hit are the parsing of column names (custom formatter seems ideal!) and adding custom properties to AuditableDataContext, or ideally IAuditableContext (or an abstract base class) as we are considering moving from L2S to EF. 

I'm not a fan of having to build 3rd party code from source (can be a maintenance hassle), so the non-signing is a road block.  If you are interested in the various approaches, here's a good stackoverflow question.

Also, I may have hit another bug yesterday when testing associations.  The ExcludePropertyIf extension method looks perfect for excluding from auditing the Table<T> property added by Linq2Sql for associations, but either the method is incorrectly named or the logic is inverted.  For example, I have test tables TESTS_AUDIT_PARENT -> TESTS_AUDIT_CHILD -> TESTS_AUDIT_GRANDCHILD, and DoddleAudit was auditing the TESTS_AUDIT_CHILDs property on parent table for inserts (new value was System.Data.Linq.EntitySet`1[TESTS_AUDIT_CHILD]).  So I added:

context.ExcludePropertyIf((mi, p) => mi.Name == "TESTS_AUDIT_CHILDs");

But that excluded everything except Table<TESTS_AUDIT_CHILDs> from auditing, swapping the logic over fixed it:

context.ExcludePropertyIf((mi, p) => mi.Name != "TESTS_AUDIT_CHILDs");
I'm not even sure why you would want to audit this anyway, and it appears that you attempt  to filter it out in SubmitChanges overload:
PropertyAuditRules.Add((m, e) => m.HasAttribute(typeof(ColumnAttribute)));

So maybe it's 2 issues instead of 1?  This is all based on HEAD revision in source code.

Tracing Func<> based code is fun, I couldn't find where the problem is, but a little performance optimization, fail fast as soon as any rule fails (instead of testing all rules):

 

internal static bool ShouldAuditProperty(this IAuditableContext context, MemberInfo member, object entity)
{
foreach (var rule in context.PropertyAuditRules)
{
// Break as soon as one rule fails
if (!rule(member, entity))
return false;
}
return true;
}

 

Finally, having had to create an audit log against a DataSet based system, the one thing I recommend above everything else is writing lots of unit and functional tests to cover as many scenarios and edge cases as possible. The next most useful thing I found was logging.

Regards
Si

Dec 15, 2009 at 3:13 AM

Here's a fix for SubmitChanges auditing non-column based properties, in the ReflectionExtensions class:

public static bool HasAttribute(this MemberInfo mi, Type attrType)
{
     return Attribute.GetCustomAttribute(mi, attrType) != null;
}

Also, the overload for this HasAttribute(this Type t, Type attrType) isn't called in the code.

Just let me know if you'd prefer these logged as issues to keep track of them.

cheers
si

Dec 17, 2009 at 3:34 AM
Edited Dec 17, 2009 at 12:41 PM

Another minor fix - I noticed that on Insert operations, the Primary Key column was being audited, even though it is being captured in EntityTableKey. We're expecting our audit table to get huge, so any space saving is good (which is why I liked your byte style for AuditAction)

Code is in ContextAuditor.AddModifiedPropertiesToRecord:

else if (action == AuditAction.Insert)
{
PropertyInfo[] props = entity.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.FlattenHierarchy);
foreach (PropertyInfo pi in props)
{
if (_context.ShouldAuditProperty(pi, entity))
{
AuditedEntityField values = resolver.GetAuditValue(pi, null, pi.GetValue(entity, null));
if (values != null)
{
// Don't audit primary keys on inserts, it's already in EntityTableKey
var col = Attribute.GetCustomAttribute(pi, typeof(ColumnAttribute)) as ColumnAttribute;
if (col != null && col.IsPrimaryKey) continue;

record.ModifiedFields.Add(values);
}
}
}
}

Also, to add some more feedback about usage, I ended up re-implementing AuditableDataContext and inheriting from my own DataContext which contained my audit related entities. I would've liked to stick with DoddleAudits AuditableDataContext, but .NET doesn't support multiple inheritance and I couldn't think of another way to combine my LinqToSql audit entities with DoddleAudit AuditableDataContext. We wanted to bake auditing into our LinqToSql usage at a low level, so this approach allowed us to make the auditing automated (except for wiring up the Audit() to entities, which is good, since this gives the programmer choice).  The only real change was making InsertAuditRecordToDatabase virtual and implementing persistence inside the abstract class. 

I also added some logging and a Stopwatch to measure performance of the various processes in SubmitChanges, because my manager was concerned about cost of reflection based approach.  So far it's look pretty good!  Rough estimates (YMMV) are around 2-5ms (after initial connection, i.e. connection pooling active) for total audit cost for a single entity, including persisting to db.  The only other thing I changed was because our application is for Healthcare industry, there are strict audit requirements, so the audit submit changes fail mode was changed, so that the transaction would be rolled back if the auditing failed.

Regards
Si