SPWeb.AssociatedGroups.Contains Lies

While working on SPExLib (several months ago), I revisited this post, which presented a functional approach to a solution Adam describes here. Both posts include logic to add an SPWeb group association, which most simply could look something like this:

SPGroup group = web.SiteGroups[groupName];
if (!web.AssociatedGroups.Contains(group))
{
    web.AssociatedGroups.Add(group);
    web.Update();
}

While testing on a few groups, I noticed that the Contains() call lies, always returning false. This behavior can also be verified with PowerShell:

PS > $w.AssociatedGroups | ?{ $_.Name -eq 'Designers' } | select Name

Name
----
Designers

PS > $g = $w.SiteGroups['Designers']
PS > $w.AssociatedGroups.Contains($g)
False

Of course, it’s not actually lying—it just doesn’t do what we expect. Behind the scenes, AssociatedGroups  is implemented as a simple List<SPGroup> that is populated with group objects retrieved by IDs stored in the SPWeb‘s vti_associategroups property. The problem is that List<T>.Contains() uses EqualityComparer<T>.Default to find a suitable match, which defaults to reference equality for reference types like SPGroup that don’t implement IEquatable<T> or override Equals().

To get around this, SPExLib provides a few extension methods to make group collections and SPWeb.AssociatedGroups easier to work with and more closely obey the Principle of Least Surprise:

public static bool NameEquals(this SPGroup group, string name)
{
    return string.Equals(group.Name, name, StringComparison.OrdinalIgnoreCase);
}

public static bool Contains(this SPGroupCollection groups, string name)
{
    return groups.Any<SPGroup>(group => group.NameEquals(name));
}

public static bool HasGroupAssociation(this SPWeb web, string name)
{
    return web.AssociatedGroups.Contains(name);
}

public static bool HasGroupAssociation(this SPWeb web, SPGroup group)
{
    if (group == null)
        throw new ArgumentNullException("group");
    return web.HasGroupAssociation(group.Name);
}

public static void EnsureGroupAssociation(this SPWeb web, SPGroup group)
{
    if (web.HasGroupAssociation(group))
        web.AssociatedGroups.Add(group);
}

The code should be pretty self-explanatory. The name comparison logic in NameEquals() is written to align with how SharePoint compares group names internally, though they use their own implementation of case insensitivity because the framework’s isn’t good enough. Or something like that.

There should be two lessons here:

  1. Don’t assume methods that have a notion of equality, like Contains(), will behave like you expect.
  2. Use SPExLib and contribute other extensions and helpers you find useful. :)
Advertisements
Posted in Object Model, SharePoint. Tags: , . Comments Off on SPWeb.AssociatedGroups.Contains Lies

Extension Methods on Types You Own?

It’s no secret that I’m a fan of using extension methods to make code more concise and expressive. This is particularly handy for enhancing APIs outside of your control, from the base class library to ASP.NET MVC and SharePoint. However, there are certain situations where it might be useful to use extension methods even though you have the option to add those methods to the class or interface itself. Consider this simplified caching interface:

public interface ICacheProvider
{
    T Get<T>(string key);
    void Insert<T>(string key, T value);
}

And a simple application of the decorator pattern to implement a cached repository:

public class CachedAwesomeRepository : IAwesomeRepository
{
    private readonly IAwesomeRepository awesomeRepository;
    private readonly ICacheProvider cacheProvider;

    public CachedAwesomeRepository(IAwesomeRepository awesomeRepository, ICacheProvider cacheProvider)
    {
        this.awesomeRepository = awesomeRepository;
        this.cacheProvider = cacheProvider;
    }

    public Awesome GetAwesome(string id)
    {
        var awesome = cacheProvider.Get<Awesome>(id);
        if(awesome == null)
            cacheProvider.Insert(id, (awesome = awesomeRepository.GetAwesome(id)));
        return awesome;
    }
}

So far, so good. However, as caching is used more often it becomes clear that there’s a common pattern that we might want to extract:

    T ICacheProvider.GetOrInsert<T>(string key, Func<T> valueFactory)
    {
        T value = Get<T>(key);
        if(value == default(T))
            Insert(key, (value = valueFactory()));
        return value;
    }

Which would reduce GetAwesome() to a single, simple expression:

    public Awesome GetAwesome(string id)
    {
        return cacheProvider.GetOrInsert(id, () => awesomeRepository.GetAwesome(id));
    }

Now I just need to decide where GetOrInsert() lives. Since I control ICacheProvider, I could just add another method to the interface and update all its implementers. However, after starting down this path, I concluded this was not desirable for a number of reasons:

  1. The implementation of GetOrInsert within each cache provider was essentially identical.
  2. Tests using a mocked ICacheProvider now needed to know if the code under test used GetOrInsert() or Get() + Insert(), coupling the test too tightly to the implementation. Furthermore, natural tests along the lines of “Should return cached value” and “Should insert value from repository if not cached” were replaced with a single implementation-specific test: “Should return value from GetOrInsert”.
  3. Most importantly, I came to realize that GetOrInsert() really just isn’t something that a cache does, so why should it be part of the interface?

So instead I have a handy GetOrInsert() extension method (conversion is left as an exercise for the reader) that I can use to clean up my caching code without needing to change any of my cache providers or tests for existing consumers.

The question is really analogous to whether or not Select() and Where() should be part of IEnumerable<T>. They are certainly useful ways to consume the interface, just as GetOrInsert() is, but they exist outside of what an IEnumerable<T> really is.

Posted in .NET. Tags: . Comments Off on Extension Methods on Types You Own?

Safely Process SPSite.AllWebs

Probably the ugliest scenario for properly disposing SPWeb objects is cleaning up when you enumerate SPSite.AllWebs. To simplify that task, I present a pair of LINQ-inspired extension methods:

public static void ForEach(this SPWebCollection webs, Action<SPWeb> action)
{
    foreach (SPWeb web in webs)
    {
        action(web);
        web.Dispose();
    }
}

public static IEnumerable<TResult> Select<TResult>(this SPWebCollection webs, Func<SPWeb, TResult> action)
{
    List<TResult> res = new List<TResult>(webs.Count);
    webs.ForEach(w => res.Add(action(w)));
    return res;
}

public static IEnumerable<TResult> Select<TResult>(this SPWebCollection webs, Func<SPWeb, TResult> selector)
{
    foreach (SPWeb web in webs)
    {
        TResult ret = selector(web);
        web.Dispose();
        yield return ret;
    }
}

Combined with lambda expressions, we can cleanly handle tasks that would ordinarily require more code and explicit disposal. Want a list of your web URLs?

var urls = site.AllWebs.Select(w => { return w.Url; });

How about updating a property on every web?

site.AllWebs.ForEach(w =>
{
    w.Properties["MyProp"] = DateTime.Now.ToString();
    w.Properties.Update();
});

We can even leverage anonymous types:

var props = site.AllWebs.Select(w =>
{
    return new
    {
        w.Title,
        w.Url,
        MyProp = w.Properties["MyProp"]
    };
});

Speaking of LINQ and SharePoint, check out Adam Buenz‘s post on using LINQ’s IEnumerable.Cast<T> with SharePoint collections to get IQueryable support. And while using LINQ for filtering and such may be prettier, resist the urge to skip CAML altogether: there is definitely a performance advantage in filtering your SPListItemCollection with an SPQuery, especially for large lists. I can’t seem to find any hard data on this, so I nominate Waldek Mastykarz to investigate – his analyses of other performance topics were great.

Update 12/10/2008: New, improved Select! Discussed here.