Sitecore and FxCop – ItemAxes and Database Rules

This is the fourthpost in the Sitecore and FxCop series

I’ve been working on a bunch of easier to implement rules, which I figured I’d run through here. As always, I’m trying to improve the code, either increasing the performance of the code or making it a little more safe to use.

There’s a couple of categories in which I’ve added rules, and I’ll be creating a post per category, as it’ll be a huge post otherwise.

The categories I added rules in are:

ItemAxes & Database

This is one of the most important areas for performance issues. When we are using the ItemAxes to get all ancestors or descendants that can take a very big performance hit. It is often much better and faster to use Sitecore.ContentSearch (granted, this can only be done using Sitecore 7 and up). The same goes for the SelectItems() with Xpath as well.

So the four rules I put in on the ItemAxes are:

  1. Don’t use SelectSingleItem (accessible as Sitecore.Data.Items.ItemAxes.SelectSingleItem as well as Sitecore.Data.Database.SelectSingleItem)
  2. Don’t use SelectItems (accessible as Sitecore.Data.Items.ItemAxes.SelectItems as well as Sitecore.Data.Database.SelectItems)
  3. Don’t use GetAncestors
  4. Don’t use GetDescendants

On this blogpost I saw some stats for Lucene and fast query, which is not really a fair comparison I think because fast query will always go straight to the database, I figured I’d get a bit of data together.

Consider the following code:

var index = Sitecore.ContentSearch.ContentSearchManager.GetIndex("sitecore_master_index")
{
    using (var context = index.CreateSearchContext())
    {
        // ContentSearch
        Stopwatch sw = new Stopwatch();
        sw.Start();
        var indexed = context.GetQueryable<Sitecore.ContentSearch.SearchTypes.SearchResultItem>().Where(item => item.Name.Contains("s"));
        sw.Stop();
        Sitecore.Diagnostics.Log.Info(string.Format("ContentSearch. Elapsed time: {0}. Items found: {1}", sw.Elapsed, indexed.ToList().Count));
    }

    // Fast query
    Stopwatch sw2 = new Stopwatch();
    sw2.Start();
    var fQuery = Sitecore.Configuration.Factory.GetDatabase("master").SelectItems("fast://*[@@name='%s%']");
    sw2.Stop();
    Sitecore.Diagnostics.Log.Info(string.Format("Fast query. Elapsed time: {0}. Items found: {1}", sw2.Elapsed, fQuery.Length), this);

    // Normal query
    Stopwatch sw3 = new Stopwatch();
    sw3.Start();
    var query = Sitecore.Configuration.Factory.GetDatabase("master").SelectItems("//*[contains(@@name, 's')]");
    sw3.Stop();
    Sitecore.Diagnostics.Log.Info(string.Format("Normal query. Elapsed time: {0}. Items found: {1}", sw3.Elapsed, query.Length), this);
}

This gave me the following output in my Sitecore log file:

INFO ContentSearch. Elapsed time: 00:00:00:0178173. Items found: 8723
INFO Fast query. Elapsed time: 00:00:05.2718443. Items found: 5194
INFO Normal query. Elapsed time: 00:00:09.7881149. Items found: 4320

Those are some pretty big differences. These results are for just under 13000 items. Of course, it’s still not completely honest because the normal query does need to call the Contains function, which I don’t know the performance implications for…

Another big plus of using ContentSearch is that Linq is a lot easier to read in more complex queries than Xpath / Fast query.

From a custom rule perspective, those four rules are extremely simple to create. Almost all rules I added to my project so far use a lot of the same code – If the corresponding classes get called, flag them as warnings on the page.

Of course, as with all rules, there’s bound to be some exceptions and actual legitimate good uses of these classes. The rules are only there to point out that it might be a good idea to re-think that.

Sitecore and FxCop – Field Rules

This is the fourthpost in the Sitecore and FxCop series

I’ve been working on a bunch of easier to implement rules, which I figured I’d run through here. As always, I’m trying to improve the code, either increasing the performance of the code or making it a little more safe to use.

There’s a couple of categories in which I’ve added rules, and I’ll be creating a post per category, as it’ll be a huge post otherwise.

The categories I added rules in are:

Fields

I actually added a couple of field rules. In a previous post I was talking about accessing raw field value properly and getting items from the database with IDs rather than a path.

Now, of course it would also make sense to get the field values based not on the fieldname, but the GUID of the particular fields. This of course has the advantage of getting the correct field always, even when the fieldname changes.

I’ll admit this is a rule I had some difficulty with, although it ended up being very easy.

public override void VisitMethodCall(MethodCall methodCall)
{
    base.VisitMethodCall(methodCall);

    var memberBinding = methodCall.Callee as MemberBinding;
    if (memberBinding != null)
    {
        var methodCalled = memberBinding.BoundMember as Method;
        if (methodCalled != null)
        {
            if (methodCalled.FullName == "Sitecore.Collections.FieldCollection.get_Item(System.String)")
            {
                Expression parameter = methodCall.Operands[0];
                CheckValue(methodCall, parameter);
            }
        }
    }
}

In which CheckValue just tries to match the value of the parameter with a GUID, using a Regular Expression. The thing I had difficulties with was that I was accessing a FieldCollection, and could not inspect the actual line which was making the call.

Another rule which I think will be particularly useful is one that flags where people are trying to get the value from a CheckboxField – to check whether it’s been checked or not. The utils that come with Sitecore seem to be a little under-used, and one of the things that’s possible with the utils is exactly this: Get the value of a checkbox.

Consider the following code snippet:

namespace SomeNamespace
{
    public class SomeClass
    {
        public void SomeMethod()
        {
            var item = Sitecore.Context.Database.GetItem("{66EC0398-1E67-4D43-B2EB-ED29E3E8E291}");
            if (item != null)
            {
                CheckboxField field = (CheckboxField)item.Fields["checkbox"];
                if (field != null)
                {
                    var isChecked = field.Checked;
                }
            }
        }

        public void SomeOtherMethod()
        {
            var item = Sitecore.Context.Database.GetItem("{66EC0398-1E67-4D43-B2EB-ED29E3E8E291}");
            if (item != null)
            {
                var isChecked = MainUtil.GetBool(item["checkbox", false);
             }
        }
    }
}

Of course, instead of calling the field with its fieldname we should do that with the ID – but for this example it’s a bit easier to read like this.
We don’t have to first get the checkboxfield, then check whether it even exists followed by finally getting the value, we can just call Sitecore’s MainUtil.GetBool, pass in the value we want to convert to bool and the default value. A lot safer and it’s a couple lines shorter which should appeal to us lazy programmers 😉

I figured I’d skip the same FxCop rule code this time, as it’s the same as the examples in the other posts. The method we’re checking for this time is Sitecore.Data.Fields.CheckboxField.get_Checked . If that method is indeed called, flag that as a possible problem.

Sitecore and FxCop – Security Rules

This is the third post in the Sitecore and FxCop series

I’ve been working on a bunch of easier to implement rules, which I figured I’d run through here. As always, I’m trying to improve the code, either increasing the performance of the code or making it a little more safe to use.

There’s a couple of categories in which I’ve added rules, and I’ll be creating a post per category, as it’ll be a huge post otherwise.

The categories I added rules in are:

  • Security
  • Fields
  • ItemAxes
  • Database

Security

The first new FxCop rule I implemented is to make sure to use the Sitecore.Security.Accounts.UserSwitcher, rather than the Sitecore.SecurityModel.SecurityDisabler.
The SecurityDisabler elevates the users permission (temporarily) to administrator rights, which has the potential to be very dangerous to use and errors to potentially be very costly. An interesting side effect is that anything done with the SecurityDisabler will show up as being done by the sitecore\Anonymous role, messing up the audit trail.

However, if we use a UserSwitcher, we will actually tell the system to do something based on a user, with the access rights of that user. Consider the following code snippet:

namespace SomeNamespace
{
    public class SomeClass
    {
        var home = Sitecore.Context.Database.GetItem("{66EC0398-1E67-4D43-B2EB-ED29E3E8E291}");
        using (new Sitecore.SecurityModel.SecurityDisabler())
        {
            home.Delete();
        }

        Sitecore.Security.Accounts.User someUser = Sitecore.Security.Accounts.User.FromName(@"sitecore\SomeUser", false);
        using (new Sitecore.Security.Accounts.UserSwitcher(someUser))
        {
            home.Delete();
        }
    }
}

Assuming we have set up the access for the SomeUser account correctly, the home item will not be deleted with the UserSwitcher, but will be deleted with the SecurityDisabler, because we have effectively disabled all security there. In our case, because the SomeUser account has been set up correctly, an AccessDeniedException will be thrown.
Although this is a trivial example, it does point out the dangers of the SecurityDisabler.

The rule is actually also very easy, and is very similar to the rules described in earlier posts. The biggest difference is that because we’re now looking for a constructor rather than a method that’s been called, we need to override the VisitConstruct method.

public override void VisitConstruct(Construct construct)
{
    base.VisitConstruct(construct);

    Method method = (Method)((MemberBinding)construct.Constructor).BoundMember;
    if (method.FullName.Equals("Sitecore.SecurityModel.SecurityDisabler.#ctor", StringComparison.OrdinalIgnorecase)
    {
        this.Problems.Add(new Problem(this.GetResolution(method), construct));
    }
}

We don’t really care about anything else – if the SecurityDisabler is getting called, we want to add a warning that the UserSwitcher should be used.