Sitecore / Habitat

Sitecore Modular Architecture Example
Other
413 stars 397 forks source link

InvalidOperationException during indexing due to rule-based device resolution #368

Open gtryf opened 6 years ago

gtryf commented 6 years ago

Issue Description

If you enable rule-based device resolution on any of the devices, you'll notice millions of errors like this one in your log files:

Exception: System.InvalidOperationException Message: deviceRuleContext.HttpContext is null Source: Sitecore.CES.DeviceDetection.Rules at Sitecore.CES.DeviceDetection.Rules.RuleDeviceInformationManager.GetUserAgentFromDeviceRuleContext(DeviceRuleContext deviceRuleContext) at Sitecore.CES.DeviceDetection.Rules.RuleDeviceInformationManager.ResolveObject[T](RuleContext ruleContext, String key, Func`2 objectFactory, String trackerDisabledMessageFormat, String deviceDetectionExceptionMessageFormat, Object[] formatArgs) at Sitecore.CES.DeviceDetection.Rules.RuleDeviceInformationManager.GetExtendedProperty(RuleContext ruleContext, String propertyName) at Sitecore.CES.DeviceDetection.Rules.Conditions.DevicePropertyCondition1.Execute(T ruleContext) at Sitecore.Rules.Conditions.``WhenCondition1`.Evaluate(T ruleContext, RuleStack stack) at Sitecore.Data.Items.DeviceItem.MatchesRules(HttpContextBase httpContext, SafeDictionary2 customData) at Sitecore.Data.Items.DeviceItem.ResolveHelper.FindBestMatch(Database database, HttpContextBase httpContext)

You'll have to enable first-chance exception trapping to see why this happens. The issue stems from this piece of code inside /Foundation/LocalDataSource/Infrastructure/Indexing/LocalDataSourceContentField.cs:

private bool ShouldIndexItem(Item item)
{
    return item.HasLayout() && !item.Paths.LongID.Contains(ItemIDs.TemplateRoot.ToString());
}

where the HasLayout() extension method (/Foundation/SitecoreExtensions/Extensions/ItemExtensions.cs) looks like this:

public static bool HasLayout(this Item item)
{
    return item?.Visualization?.Layout != null;
}

Using a decompiler you'll find that the Layout property at some point calls into this:

ResolveDevice(database, (HttpContext.Current != null) ? new HttpContextWrapper(HttpContext.Current) : null);

and during indexing, HttpContext is null, causing the exception once for each item being indexed.

Suggested Workaround

Add a second argument to the HasLayout() extension method, namely bool resolveDevice, as follows:

        public static bool HasLayout(this Item item, bool resolveDevice = true)
        {
            if (resolveDevice)
            {
                return item?.Visualization?.Layout != null;
            } else 
            {
                DeviceItem defaultDevice = ... // Use the API to retrieve the instance's default device
                return item?.Visualization?.GetLayout(defaultDevice) != null;
            }
        }

and change the ShouldIndexItem() method inside the computed field like so:

    private bool ShouldIndexItem(Item item)
    {
      return item.HasLayout(false) && !item.Paths.LongID.Contains(ItemIDs.TemplateRoot.ToString());
    }

Please let me know if this sounds like something I should create a pull request for.

Thanks, George

nickwesselman commented 6 years ago

Hi @gtryf, thanks for the report. Which version of Sitecore was this? I'm unable to repro currently on the v9 branch. This actually looks like a bug in Device Detection to me -- it should gracefully handle the lack of HTTP context in the condition, and return false. If you still think a workaround is needed, I think a better approach would be to check the layout fields directly, and just determine whether there is any Shared or Final layout on the item (or inherited from standard values).

gtryf commented 6 years ago

Hi @nickwesselman, I have witnessed this on versions 8.0 and 8.2 (both Initial and Update-2). Have you enabled rule based detection on at least one of your devices? You won't reproduce it otherwise.

Agreed that this is a bug in the Device Detection subsystem, however since I don't see this changing any time soon, it looks like a workaround should be implemented. The reason I suggested the approach I did is that if you statically check the shared/final layout item fields, you'll miss out on rule-based device selection when run from within a valid HttpContext (e.g. from within a rendering).

Regards, George

nickwesselman commented 6 years ago

Hi @gtryf -- I'm unable to replicate on 8.2 rev 170614 with a device rule. Can you provide more specific repro steps / the specific rules you are using?

image