SharePoint Disposal Wish List

I was almost off my SharePoint disposal kick when Jeremy Jameson had to pull me back in with this post. Resisting the temptation to rehash some stuff I’ve covered before, I thought it might be therapeutic to spend some time discussing what the dispose story should look like. I’m sure the API for WSS 4.0 is more or less locked at this point, but it still might be useful to frame how I think about ownership of these blasted IDisposables.

In my SharePoint fantasy world, the dispose story can be explained in four bullets:

  1. Use of a disposed object throws an exception; failures to dispose properly are logged.
  2. IDisposable references retrieved from an instance property are owned by the instance.
  3. IDisposable references retrieved from a constructor, method or collection indexer are owned by the developer and should always be disposed.
  4. IDisposable references from an enumerator are owned and disposed by the enumerator.

Let’s examine these a bit more closely.

1. Disposed = Invalid = Use is Exceptional; Discoverability = Good

The leaked object logging is already pretty good, but that’s more than made up for by the epic failure to actually close objects that have been closed. Fixing discoverability would make all other points a matter of convenience.

2. Instance Properties

An instance property suggests “mine”. My context’s Site, my site’s RootWeb, my feature’s Parent, etc. If something is “mine,” then I should be responsible for it. This behavior is already consistent with three exceptions:

SPWeb.ParentWeb

This property is tricky because the instance is shared for all consumers of the SPWeb (think SPContext.Web), but will not be cleaned up if the SPWeb is disposed and has no mechanism to notify the child that it has been disposed to null out the reference to it. So what to do? Well in accordance with my fantasy guidelines, there are two options, either of which would resolve the ambiguity:

  1. Leave ParentWeb as a property and require that the SPWeb dispose its parent as part of the clean-up process.
  2. Provide SPWeb.GetParentWeb() instead, returning a new SPWeb on each call that is owned by the developer.

Since GetParentWeb() can be implemented as an extension method, I guess I would prefer option 1.

SPList.ParentWeb

This property is almost fine, but for some reason has an edge case returns a new SPWeb (details here). It should be fine as a property, but that edge case needs to disappear. Why can’t it just return SPList.Lists.Web?

MOSS UserProfile.PersonalSite

This property returns a new SPSite on each call, so ideally it would be a method instead, perhaps GetPersonalSite. That seems much easier than having UserProfile implement IDisposable to facilitate cleanup of shared instance.

3. Constructors & Methods & Indexers, Oh My!

If properties are “mine”, then everything else is “yours”. Again, the vast majority of the API already fits this behavior. The few discrepancies:

SPControl.GetContextSite() & SPControl.GetContextWeb()

I believe these are the only methods that return IDisposables that SharePoint owns. MSDN clearly indicates that behavior, but for consistency the preferred usage (consistent with our fantasy guidelines) would be to use an SPContext’s Site and Web properties instead.

MOSS PublishingWeb

I’ve already discussed the semi-disposability of PublishingWeb, which just needs a few tweaks to fit nicely in my fantasy model:

  • Implement IDisposable! Close() is already implemented, just need the interface to take advantage of our languages’ using constructs.
  • I believe ParentPublishingWeb would be fixed if SPWeb.ParentWeb were disposed with the child. Alternatively, change to GetParentPublishingWeb and ensure that the returned PublishingWeb will close the internal SPWeb on dispose.
  • PublishingWebCollection.Add() is fine, but the indexer works in such a way that an internal SPWeb is created that is not cleaned up by PublishingWeb.Close(). I would consider this a bug in the current code base, which would naturally be fixed in my fantasy.

4. Enumerating Collections

This isn’t quite as simple as it would seem. When dealing with collections and enumerators, there are essentially two classes of operation:

  1. Enumerate over all or part of the collection and perform an operation. (LINQ Select, GroupBy, OrderBy)
  2. Enumerate over the collection to pick an object from the collection to return. (LINQ First, Last, ElementAt)

In my experience, the vast majority fall into the first category, including most of LINQ. These operations shouldn’t need to know that the objects being enumerated are IDisposable; it’s up to the enumerator to behave appropriately. Rather than sacrifice these many useful operations, I suggest that the enumerator should include proper disposal of the objects it creates—this is precisely what my AsSafeEnumerable() iterator does.

The second category can either be handled using for loops and indexers, which return objects that the caller must dispose, or through higher-order functions that allow dispose-safe operation on or selection from the desired element. But again, these seem to be the exception rather than the rule, and can be handled accordingly.

Wishful Thinking

The unfortunate reality is that most of these adjustments could never happen because breaking changes are generally frowned upon. But perhaps in framing our disposal thought process around some simple rules with documented exceptions, it may be easier to get things right without leaning on tools like SPDisposeCheck.

Posted in Object Model, SharePoint. Tags: , . Comments Off

Community Stuff, Apr 2009

First, thanks to the folks at Cedar Valley INETA for coming out to my PowerShell 101 talk earlier this month. The deck, scripts and demo transcript are available for download here. Among other things, we looked at a script to use System.Speech from PowerShell. The results were outstanding:

Recognized [0.2717747] “some of”
Recognized [0.2246611] “us in the all the rest of us are”
Recognized [0.302918] “the Mike Tyson > Microsoft software”
Recognized [0.4645212] “of Mike Tyson is”
Recognized [0.4163939] “this still undecided vote”
Recognized [0.3286791] “-Mike Tyson did not understand its electricity can”

I’m not sure what I was trying to say, but I’m pretty sure it wasn’t that. Thanks to Josh and Travis for the great work they’re doing with that up-and-coming group.

Next up is the Iowa SharePoint User Group on Apr. 1st with Corey Erkes:

SharePoint and Social Networking

We will explore how to use SharePoint for social computing that will power the next generation of business productivity. During the session we will highlight the following components and how they can be extended using customization, Codeplex and partner solutions:

  • MySites
  • Blogs
  • Wikis
  • Podcasting
  • Unified Communications

Details:

As usual, if you have ideas or requests for future IASPUG topics, please send them to info@sharepointia.com.

Then on Saturday, Apr. 4th, I’ll be presenting at Twin Cities Code Camp 6:

Introduction to F#

With Visual Studio 2010, F# will officially become a first-class citizen in the .NET development ecosystem. This session will explore functional programming, why it matters in an object-oriented world, and how F# bridges the gap. I will also discuss some lessons learned from using F# with OpenCV, an unmanaged computer vision library.

And finally on Tuesday, Apr. 7th, I’ll be at the inaugural meeting of the new Dubuque INETA presenting an overview of what’s new in Visual Studio 2008, .NET 3.5 and the language features of C# 3.0 and Visual Basic 9. Thanks to Elizabeth Groom for her hard work getting that group started.

Also, the next Iowa Code Camp is confirmed for May 2 at Kirkwood College in Cedar Rapids. The last two have been fantastic, so definitely looking forward to it.

Introducing SPWeb.GetParentWeb()

Official Microsoft guidance is to never explicitly Dispose() SPWeb.ParentWeb. I generally agree with this advice, given that my Rule #1 of SharePoint disposal is that “Using a disposed object can cause more problems than failing to dispose.” To understand why, I’ll borrow my explanation from SPDevWiki:

This property will allocate an SPWeb object the first time it is called. The caveat is that once it is disposed, any reference to the property will return the disposed object. If an SPWeb is not owned by the developer, its ParentWeb should be considered not owned as well. For example, there could be a problem if two components both depend on SPContext.Current.Web.ParentWeb and one calls Dispose() before the other is done with it.

However, this can result in memory pressure in cases involving enumeration or where the parent SPSite has a long lifetime. For example:

SPSite contextSite = SPContext.Current.Site;
foreach(SPWeb web in contextSite.AllWebs.AsSafeEnumerable())
{
    SPWeb webParent = web.ParentWeb; // Internal OpenWeb()
    // Do something with web and webParent
}

The web references are disposed by my safe iterator, but every webParent will remain open until the context SPSite is disposed. Not that I would recommend using code like this (in fact I would strongly urge against it), but you can never say never.

To that end, I propose a simple extension method whose contract is clear: always dispose me! We can still follow MS guidance regarding SPWeb.ParentWeb, but have convenient access to a developer-owned parent SPWeb as well:

[SPDisposeCheckIgnore(SPDisposeCheckID.SPDisposeCheckID_120, "By Design")]
public static SPWeb GetParentWeb(this SPWeb web)
{
    if(web == null)
        throw new ArgumentNullException("web");
    return web.Site.OpenWeb(web.ParentWebId);
}

And our “Best Practice” memory pressure can be revised slightly to achieve much better memory use:

SPSite contextSite = SPContext.Current.Site;
foreach(SPWeb web in contextSite.AllWebs.AsSafeEnumerable())
{
    using(SPWeb webParent = web.GetParentWeb())
    {
        // Do something with web and webParent
    }
}

Trivial? Obvious? Perhaps. But often the most useful code is.

Update: Included appropriate SPDisposeCheckIgnore attribute for “leaked” SPWeb from OpenWeb(); we know what we’re doing. That said, you could certainly implement higher-order functions to invoke an action or selector against our imitation ParentWeb without returning it—I’ll leave those as an exercise for the reader.

More SharePoint Higher-Order Functions

Though I haven’t actually used the term before, I’ve discussed a number of higher-order functions in the past. Simply put, a higher-order function either accepts a function as a parameter, returns a function, or both. The terminology might be foreign, but the technique is used all over the place:

Another use of higher-order functions is to ensure the existence of a SharePoint resource. For example, I often need to fetch a SharePoint list and create it if doesn’t exist. A standard implementation might look something like this:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     string description, SPListTemplate template)
{
    SPListCollection webLists = web.Lists;
    SPList list = webLists.Cast<SPList>()
                          .FirstOrDefault(l => l.Title == listName);
    if (list == null)
    {
        Guid newListID = webLists.Add(listName, description, template);
        list = webLists[newListID];
    }
    return list;
}

While there’s nothing wrong with this implementation, per se, it’s not exceedingly flexible. What if we want to use a different overload of SPListCollection.Add? What if we need to elevate privileges to create the list? We could certainly create a dozen variations based on this pattern, but that’s a bunch of duplicate code that we would much rather avoid. Instead, we can use a single higher-order function:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     Func<SPWeb, string, SPList> listBuilder)
{
    SPList list = web.Lists.Cast<SPList>()
                     .FirstOrDefault(l => l.Title == listName);
    if(list == null && listBuilder != null)
        list = listBuilder(web, listName);
    return list;
}

And then specify exactly how the list should be created. We could redefine our original method like this:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     string description, SPListTemplate template)
{
    return GetOrCreateList(web, listName, (builderWeb, builderName) =>
    {
        var builderLists = builderWeb.Lists;
        Guid newListID = builderLists.Add(builderName, description, template);
        return builderLists[newListID];
    });
}

Or we can just as easily specify a builder that uses elevated privileges and a different Add overload:

public static SPList GetOrCreateTasksList(this SPWeb web)
{
    return GetOrCreateList(web, "Tasks", (builderWeb, builderName) =>
    {
        Guid newListId = web.SelectAsSystem(sysWeb =>
            sysWeb.Lists.Add(builderName, null, SPListTemplateType.Tasks));

        return builderWeb.Lists[newListId];
    });
}

Or my preference is to define a (testable) builder method and just use the higher-order function without a wrapper:

private static SPList CreateGenericList(SPWeb web, string name)
{
    var id = web.Lists.Add(name, null, SPListTemplateType.GenericList);
    return web.Lists[id];
}

void DoSomething(SPWeb web)
{
    SPList list = web.GetOrCreateList("Some List", CreateGenericList);
    if (list == null)
        throw new SPException("Some List does not exist and could not be created.");
    // Do something
}

GetOrCreateGroup

Another use for this pattern is the creation of SharePoint groups, inspired by Adam Buenz’s recent post. His code is correct (though I believe an ordinal comparison is more appropriate than invariant culture), but it can’t easily handle scenarios requiring elevation, AllowUnsafeUpdates, etc. Instead, we can define a higher-order function like this:

public static SPGroup GetOrCreateGroup(this SPWeb web, string groupName,
                                       Func<SPWeb, string, SPGroup> groupBuilder,
                                       Action<SPWeb, SPGroup> associateGroup)
{
    SPGroup group = web.SiteGroups.Cast<SPGroup>()
                        .FirstOrDefault(g =>
                            string.Equals(g.Name, groupName,
                                StringComparison.OrdinalIgnoreCase));
    if (group == null && groupBuilder != null)
        group = groupBuilder(web, groupName);
    if (group != null && associateGroup != null)
        associateGroup(web, group);
    return group;
}

With which the original method is easily rewritten:

public static SPGroup GetGroupOrCreate(SPWeb web, string name,
                                       string description, SPUser owner,
                                       SPUser defaultUser, bool associate)
{
    return web.GetOrCreateGroup(name,
        (builderWeb, builderName) =>
        {
            var builderGroups = builderWeb.SiteGroups;
            builderGroups.Add(builderName, owner, defaultUser, description);
            return builderGroups[name];
        },
        (assocWeb, assocGroup) =>
        {
            if (associate && !assocWeb.AssociatedGroups.Contains(assocGroup))
            {
                web.AssociatedGroups.Add(assocGroup);
                web.Update();
            }
        });
}

Again, the advantage is that we can easily tweak how the group is created and associated independent from the common get-or-create logic.

Why You Need Reflector: SPList.DefaultView

I love Reflector. It has been invaluable in my various continuous learning efforts, poking around interesting code to improve my own. However, the need for Reflector spans beyond curiosity: when working with the SharePoint object model, it is an essential tool to understand what’s going on behind the scenes. It would be great if the API always did what you expect, or at least documented some of its less-than-obvious behavior, but it doesn’t and probably never will. It’s simply too big and “what you expect” is often a matter of opinion.

An excellent example is the behavior of SPList.DefaultView. As recently discovered by Andy Burns and earlier documented by Daniel Pavelescu, DefaultView returns a new SPView instance on every call. So instead of this:

list.DefaultView.ViewFields.Add("SomeField");
list.DefaultView.Update();

You need to do this:

SPView defView = list.DefaultView;
defView.ViewFields.Add("SomeField");
defView.Update();

Daniel was able to verify this behavior with the VS debugger, but it’s not clear why. Enter Reflector:

  1. SPList.DefaultView:
    public SPView DefaultView
    {
        get
        {
            return this.Views.DefaultView;
        }
    }
  2. SPViewCollection.DefaultView:
    internal SPView DefaultView
    {
        get
        {
            if (this.m_iDefaultViewIndex != -1)
            {
                return this[this.m_iDefaultViewIndex];
            }
            return null;
        }
    }
  3. SPViewCollection.Item[Int32]
    public SPView this[int iIndex]
    {
        get
        {
            if ((iIndex < 0) || (iIndex >= this.Count))
            {
                throw new ArgumentOutOfRangeException();
            }
            return new SPView(this, this.m_arrViewSchema, iIndex);
        }
    }

In three clicks we see exactly what the problem was and why. Since I usually have Reflector open, this is almost always faster than asking Google or MSDN. Of course some methods are obfuscated (code can’t be disassembled), but fortunately these are exceptions rather than the rule.

Exploring SPView.ViewFields

As another example of code not behaving as expected, consider the implementation of SPView.ViewFields:

public SPViewFieldCollection get_ViewFields()
{
    if (this.m_xdView != null)
    {
        while (this.m_bFullBlownXmlDoc)
        {
            return new SPViewFieldCollection(this, this.Node);
        }
    }
    return new SPViewFieldCollection(this, this.GetInnerXmlForNode("ViewFields"));
}

Given that a new collection is created each time, one might expect the following to add only the second field:

SPView defView = list.DefaultView;
defView.ViewFields.Add("FirstField");
defView.ViewFields.Add("SecondField");
defView.Update();

But, much to my surprise, both fields are added! So what’s going on here? Well, let’s explore a bit with PowerShell:

PS 1> $w = spw http://localhost
PS 2> $l = $w.Lists['Test']
PS 3> $dv = $l.DefaultView
PS 4> $vf1 = $dv.ViewFields
PS 5> $vf2 = $dv.ViewFields
PS 6> $vf1
Attachments
LinkTitle
PS 7> $vf1.Add('Editor')
PS 8> $vf2
Attachments
LinkTitle
Editor

So we store two different SPViewFieldCollection objects, update one, and the other is updated as well. In my mind, this begs two questions:

  1. Why doesn’t SPView just store a reference to a shared collection?
  2. How are the collections kept in sync?

It’s tough to guess why, but Reflector can probably help us figure out how. Let’s start with one of the constructors called by ViewFields:

internal SPViewFieldCollection(SPView view, string innerXml)
{
    this.m_View = view;
    this.m_ViewStyle = null;
    this.m_strInnerXml = innerXml;
}

Not much to see here, other than the captured reference to the parent SPView. Using Reflector’s Analyze function (Ctrl-R), we see that m_strInnerXml is also referenced in the SchemaXml property:

Analyzing SPViewFieldCollection.m_strInnerXml

Which disassembles as…

public string get_SchemaXml()
{
    if (this.m_node == null)
    {
        return this.m_strInnerXml;
    }
    return this.m_node.InnerXml;
}

So the value passed in is only used if m_node has not been set. Ctrl-R again:

Analyzing SPViewFieldCollection.m_node

Now we’re getting somewhere. The members rely on EnsureViewFields() to keep up-to-date:

private void EnsureViewFields()
{
    if (this.m_node != null)
    {
        this.m_iCount = this.m_node.ChildNodes.Count;
    }
    else
    {
        this.m_View.EnsureFullBlownXmlDocument();
        this.InitViewFields(this.m_View.Node);
    }
}

And the node connection is set up in InitViewFields using its view’s Node. So SPViewFieldCollection doesn’t even have an internal data store! Instead, everything operates against m_node, which is attached to the parent view. Thus changes to one collection are immediately reflected in other collections created from the same SPView.

I still prefer to capture and reuse the SPViewFieldCollection, but at least now we know that it’s safe not to and, more interestingly, why that’s the case.

Time to share! When has Reflector saved you from SharePoint headaches?

Thinking Functional: Using

In the comments of my last post, Peter Seale pointed me to Matthew Podwysocki‘s implementation of GenerateUsing as functional abstraction of using. I like the idea, but the Generator pattern isn’t particularly useful for common SharePoint tasks. However, I think we can get some value from looking at a more generalized solution.

But first, let me suggest a minor correction to Matt’s version of Generate, at least if it’s going to be used to fulfill an IDisposable contract:

public static IEnumerable<TResult> Generate<T, TResult>(Func<T> opener,
                                                        Func<T, Option<TResult>> generator,
                                                        Action<T> closer)
{
    var openerResult = opener();
    bool stop = false;

    while (true)
    {
        var res = Option<TResult>.None;
        try
        {
            res = generator(openerResult);
        }
        finally
        {
            if (stop = res.IsNone)
                closer(openerResult);
        }
        if (stop)
            yield break;

        yield return res.Value;
    }
}

The stop “hack” is needed because you can’t yield from a finally clause. It seems to me that a closer that might not get called isn’t much of a closer, or am I missing something?

So how else might we use this opener/closer idea? How about something like this:

public static void Process<T>(Func<T> opener,
                              Action<T> action,
                              Action<T> closer)
{
    T openerResult = opener();
    try
    {
        action(openerResult);
    }
    finally
    {
        if (closer != null)
            closer(openerResult);
    }
}

public static void Using<T>(Func<T> opener,
                            Action<T> action
                           ) where T : IDisposable
{
    Process(opener, action, x => x.Dispose());
}

We have now abstracted the idea of a using statement: get the object, do something with it, Dispose(). Abstraction in hand, let’s apply it to SharePoint:

public static void ProcessWeb<TResult>(this SPSite site,
                                       string url,
                                       Action<SPWeb> action)
{
    Using(() => site.OpenWeb(url), action);
}

Now, one could argue that we haven’t gained much over the obvious implementation:

    using(SPWeb web = site.OpenWeb())
        action(web);

But in truth, the vast majority of functional code has a non-functional alternative. It’s just a different thought process. In the former, we specify what we’re trying to do: use the result of site.OpenWeb() to do action. In the latter, we specify how to do it: use an SPWeb named web, assigned from site.OpenWeb(), to perform action. I’m not saying either approach is more correct, just different means to the same end.

Performing actions is all well and good, but we often want to get something back as well:

public TResult Select<T, TResult>(Func<T> opener, Func<T, TResult> selector, Action<T> closer)
{
    T openerResult = opener();
    try
    {
        return selector(openerResult);
    }
    finally
    {

        closer(openerResult);
    }
}

public TResult SelectUsing<T, TResult>(Func<T> opener,
                                       Func<T, TResult> selector,
                                       Action<T> closer
                                      ) where T : IDisposable
{
    return Select(opener, selector, x => x.Dispose());
}
public static TResult SelectFromWeb<TResult>(this SPSite site,
                                             string url,
                                             Func<SPWeb, TResult> selector)
{
    return SelectUsing(() => site.OpenWeb(url), selector);
}

What do you think? Useful?

SPTDD: On Good Vs. Testable Code

First, my position on SharePoint Test Driven Development: I don’t currently use it. I got a free Isolator license (thanks!) that I have yet to install (sorry!). Just like everyone else, I’m trying to figure out where TDD fits in the context of SharePoint. Any assertions in this post about TDD are based on my current understanding, which is incomplete at best.

This post is in response to a post by Eric Shupps: SPTDD: SharePoint and Test Driven Development, Part One. He has a lot to say, so let’s start with this assertion:

…in order to get real value from TDD in SharePoint you must already know how to write good code. All the unit tests in the world won’t change this fact. And the only way to learn how to write good code is to do it over and over again, gathering knowledge along the way from those who have gone before you. This leads the the fundamental problem with TDD as a methodology – it doesn’t teach developers how to write good code; rather, it teaches them how to write testable code.

I agree with the differentiation between good and testable code, but I think Eric underestimates the value of testable code in relation to its goodness. For reference, let’s bring in how his example code “should be written”:

private SPListItemCollection GetListItems(string Url, string ListName)
{
    SPListItemCollection coll = null;
    try
    {
        if (Url != String.Empty && ListName != String.Empty)
        {
            SPSecurity.RunWithElevatedPrivileges(delegate()
            {
                using (SPSite site = new SPSite(Url))
                {
                    using (SPWeb web = site.OpenWeb())
                    {
                        try
                        {
                            SPList list = web.Lists[ListName];
                            if (list.Items.Count > 0)
                            {
                                coll = list.Items;
                            }
                        }
                        catch (System.Exception ex)
                        {
                            LogError(ex.Message);
                        }
                    }
                }
            });
        }
    }
    catch (System.Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

I know “good code” is subjective, but this method has some real problems:

  • SP* objects should not be passed out of a RWEP block.
  • SP* objects should not be used after their parent SPWeb has been disposed.
  • Each call to list.Items will execute a new query. Instead, store it to a variable or use list.ItemCount.

These issues have nothing to do with testability. They’re simply not good SharePoint code. But TDD wouldn’t fix them, so for the purpose of argument let’s use this code instead (essentially Eric’s original plus error handling):

private SPListItemCollection GetListItems(string Url, string ListName)
{
    SPListItemCollection coll = null;
    try
    {
        if (Url != String.Empty && ListName != String.Empty)
        {
            SPSite site = new SPSite(Url);
            SPWeb web = site.OpenWeb();
            try
            {
                SPList list = web.Lists[ListName];
                SPListItemCollection items = list.Items;
                if (items.Count > 0)
                {
                    coll = items;
                }
            }
            catch (System.Exception ex)
            {
                LogError(ex.Message);
            }
        }
    }
    catch (System.Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

So supposing we used TDD, a method like this would have been created based on tests written to verify the following:

  • If Url or ListName are empty, return null.
  • If Url or ListName are null, use them anyway and log the resulting exception.
  • Create SPSite and SPWeb that the caller needs to remember to dispose.
  • If the list has no items, return null, otherwise return all items.
  • If anything goes wrong, log the exception and return null.

I would guess this isn’t exactly what Eric intended the method to do (at least regarding null arguments), but the hypothetical tests drove the TDD implementation; a different set of tests would have led to a more correct version of the method. Herein lies the problem with critiquing TDD based on code developed without tests—TDD would likely yield a different implementation!

Abstraction and Dependency Injection = Artifacts?

I also take issue with this passage:

Look closely – the code itself satisfies all the test requirements without requiring any level of abstraction, dependency injection, nUnit, or other such artifices.

[Irony: artifice - n. Cleverness or skill; ingenuity.]

Regarding satisfaction of the requirements, the obvious question is: How do you know? Without a test, you don’t, and to think you do just because you “already know how to write good code” is more naive than thinking unit-tested SharePoint code is bulletproof. Case in point: null arguments. But more importantly, when did abstraction and dependency injection become TDD things? These are basic programming principles, completely independent from TDD or SharePoint! Just because we’re developing against a specific (and complicated) API doesn’t mean the general best practices don’t apply. We should be .NET developers first, SharePoint developers second. Even without using TDD, I submit that your particular code could benefit quite a bit from dependency injection:

private SPListItemCollection GetListItems(SPWeb web, string listName, SPQuery query)
{
    if (web == null || string.IsNullOrEmpty(listName))
        return null;
    SPListItemCollection coll = null;
    try
    {
        SPList list = web.Lists[listName];
        SPListItemCollection items = query == null ? list.Items : list.GetItems(query);
        if (items.Count > 0)
        {
            coll = items;
        }
    }
    catch (Exception ex)
    {
        LogError(ex.Message);
    }
    return coll;
}

Instead of creating an SPWeb that needs to be disposed by the caller, we just accept one that the caller should already be handling. Instead of always using list.Items (not recommended), we accept an optional SPQuery. The method has a clear purpose, doesn’t have the new SPSite to handle, works just as well with an elevated SPWeb, etc. It’s simply a better design, with improved testability being more of a side benefit than the main goal. I can’t say for sure, but I would guess a TDDed version (given the right requirements) would be similar.

On Mocking

Again, I haven’t found time to try out Isolator so I can’t really speak to the actual practice of testing mocked objects; however, it seems to me that it would be useful to verify that, given certain assumptions about a SharePoint environment, tests pass. If I can copy/paste a few lines of code to fake an SPWeb, with or without a named list, with or without list items, this seems like it would be much easier than actually creating and deleting lists and items to verify that my code behaves accordingly. Then as long as I’m in an environment that matches an assumption I tested, I’m assured the code will work as expected. Isolator seems to enable this sort of test with relative ease, at which point the general arguments for (and against) testing and TDD can be applied to SharePoint without much translation.

All that said, thanks to Eric for posting his thoughts to spark a more public discussion. Also, check out Andrew Woodward‘s first rebuttal and other posts on SharePoint and TDD (particularly Unit Testing SharePoint Solutions – Getting into the SharePoint Object Model).

SharePoint+PowerShell Leak Workarounds

Zach Rosenfield recently posted about an obscure memory leak issue when working with PowerShell and the SharePoint object model. You can read his post for details but in summary:

  1. PowerShell spawns a new thread for each pipeline (essentially any batch of code that runs together).
  2. SharePoint objects should not be used across multiple threads due to mechanics of the unmanaged heap.
  3. SharePoint objects used across multiple pipelines result in unmanaged heap leaks until PowerShell closes or you run out of memory.

This PowerShell behavior is easy enough to verify:

PS 1> function Get-ThreadId { [Threading.Thread]::CurrentThread.ManagedThreadId }
PS 2> set-alias gt Get-ThreadId
PS 3> gt
6
PS 4> gt
4

So we need to ensure that our SharePoint objects are allocated, used and disposed within the same thread. How can we do this? Zach offers two suggestions, but there are actually several options:

Single Line

PS 5> gt; gt; gt
10
10
10
PS 6> gt; `
>> gt; `
>> gt
>>
3
3
3

Script

PS 7> @"
>> gt
>> gt
>> "@ > gt.ps1
>>
PS 8> .\gt.ps1
8
8

Function

PS 9> function gt2 {
>> gt
>> gt
>> }
>>
PS 10> gt2
4
4

Inline Script Block

PS 11> &{
>> gt
>> gt
>> }
>>
7
7

My Using Function

PS 12> using Microsoft.SharePoint
PS 13> gt; using ($s = [Microsoft.SharePoint.SPSite] 'http://moss') {
>> gt
>> $s.Url
>> gt
>> }
>>
5
5

http://moss

5

Cmdlet + Wrapper Class

Gary Lapointe has a set of PowerShell cmdlets that use wrapper objects to hide SPSite/SPWeb objects unless you specifically ask for them.

PS 14> $spi = Get-SPSite-gl 'http://moss'
PS 15> gt; using ($s = $spi.SPBase) { $s.Url; gt }
8

http://moss

8

The severity of this issue depends on your environment and the kinds of scripts you’re running, but in memory-sensitive and production environments these are definitely some techniques to keep in mind.

Faking SPContext

A common question is how to fake SPContext in a non-SharePoint context. An answer is found at the bottom of a great post on Structured and repeatable deployment of Content Query Web Part instances, but Google doesn’t seem to find it so hopefully this will help. The whole article is worth a read, but for those looking for a quick answer:

public static SPContext FakeSPContext(SPWeb contextWeb)
{
  // Ensure HttpContext.Current
  if (HttpContext.Current == null)
  {
    HttpRequest request = new HttpRequest("", web.Url, "");
    HttpContext.Current = new HttpContext(request,
      new HttpResponse(TextWriter.Null));
  }

  // SPContext is based on SPControl.GetContextWeb(), which looks here
  if(HttpContext.Current.Items["HttpHandlerSPWeb"] == null)
    HttpContext.Current.Items["HttpHandlerSPWeb"] = web;

  return SPContext.Current;
}

I’ve made two slight modifications to Waldek’s original code:

  1. Since we don’t care about the response, we can use TextWriter.Null instead of allocating a new StringWriter().
  2. By separating the HttpContext and SPContext logic, we can use this in non-SharePoint web contexts as well as non-web contexts like unit tests and timer jobs.

While this works well for a SPWeb-based SPContext, I have not figured out how to fake a list- or item-level SPContext. It should go without saying that trying to inject a different SPWeb into an existing SPContext is a really bad idea. And finally, it is your responsibility to ensure disposal of the SPSite and SPWeb you create for the fake context.

Faking SPContext might be a necessity to work with others’ code (like CQWP), but if you’re needing to use this to work with code you own, consider refactoring to use dependency injection instead.

Add SPDisposeCheck to Visual Studio External Tools

In Episode 16 of SharePoint Pod Show, Todd Bleeker had a great SPDisposeCheck tip that bears repeating: add it to your Visual Studio External Tools!

Tools > External Tools > Add

SPDisposeCheck as VS External Tool

Here’s some output from the included example solution:

SPDisposeCheck Output in VS

My warning against overdependence notwithstanding, this really is a tool that should be run against any object model code.

Of course, Todd had a lot more to say than just this tip, and the whole show is definitely worth a listen. I will disagree on one point though: I wouldn’t consider returning an SPSite or SPWeb from a helper method to be a “false positive”. Sure, you can document the behavior, but it’s much easier to maintain over time if these objects are always created and disposed in the same place (if only because you can use using). I have yet to see an example where this sort of helper pattern couldn’t be refactored out into something more manageable.

Follow

Get every new post delivered to your Inbox.