ViewBag properties if condition shouldn't throw exception on null check if the property doesn't exists

[amro93]

Describe the bug simply using this template "@if(ViewBag.Namee == null) ...."

expected: when the ViewBag properties does not exists the condition in the template should returns true

actual: when the ViewBag properties does not exists the condition in the template will throw an exception

To Reproduce Steps to reproduce the behavior:

Expected behavior A clear and concise description of what you expected to happen.

Information (please complete the following information):

• OS: [ e.g Ubuntu 16.04, Windows Server 2016, etc ]
• Platform [.NET Framework 4.x / .NET Core 2.x , etc]
• RazorLight version [ e.g 2.0-beta1 ]
• Are you using the OFFICIAL RazorLight package? https://www.nuget.org/packages/razorlight
• Visual Studio version [ e.g Visual Studio Community 17.8.5 ]

Additional context Add any other context about the problem here.

[toddams]

In our case - ViewBag is a dynamic ExpandoObject, and it throws in case of missing properties. ASP.NET case - ViewBag is a dynamic wrapper around ViewData. And ViewData itself retrieves the values as follows:

public object this[string key]
{
    get
    {
        object value;
        _innerDictionary.TryGetValue(key, out value);
        return value;
    }
    set { _innerDictionary[key] = value; }
}

This is why it gracefully returns null in case of missing property.

[jzabroski]

This is why it gracefully returns null in case of missing property.

@toddams I assume "it" refers to ASP.NET - Are you suggesting we match ASP.NET behavior here? Or just explaining the difference in behavior?

[amro93]

I'd prefer following up the ASP.NET approach for checking nulls as the templates are coded in razor files

[toddams]

Are you suggesting we match ASP.NET behavior here?

Correct. This should not be a breaking change, I doubt somebody really relies on ViewBag throwing exceptions.

[jzabroski]

Well, it would require breaking the API to no longer use ExpandoObject and instead use something like Orchard CMS's old Clay API, as well as changing the interface from ExpandoObject to dynamic and converting the dynamic object into a custom ExpandableObject . Unless you think internally we should wrap the instance passed into the compiler. But the current code contract is clear that its an ExpandoObject. It would be a silent breaking change to convert it to a dynamic type.

What does ASP.NET do? Wrap the instance after you pass it in?

I think the PR should have the following:

1. ViewBag_NullConditionalExpressions.cshtml
   1. Test ViewBag?.Data
   2. Test ViewBag.IntIndexer?[0]
   3. Test ViewBag.StringIndexer?["key"]
   4. Test ViewBag?.Method(Value?[0]?.More)?["key"]

[doxxx]

I've encountered this regression myself during a migration from RazorEngine to RazorLight. Some of our pages expect this behaviour of returning null for unknown properties. For example, it's used in default layouts like so:

@{
    ViewBag.HasBackButton = ViewBag.HasBackButton ?? false;
    ViewBag.HasConnectStatus = ViewBag.HasConnectStatus ?? false;
}

Those properties are then referenced later in the layout to include/exclude HTML.

I also tried proxying the ViewBag passed to RenderTemplateAsync with Castle DynamicProxy so that I could intercept the invocations and return null if the binding exception is thrown, but ExpandoObject is a sealed class so the generated proxy subclass fails to load.

The workaround that I eventually got working is to define a method on a custom template base class (which subclasses TemplatePage) that checks whether the ViewBag has a property:

public bool ViewBagHasProperty(string propertyName)
{
    return ((IDictionary<string, object>)ViewBag).ContainsKey(propertyName);
}

And then the original example will have to be transformed into:

ViewBag.HasBackButton = ViewBagHasProperty("HasBackButton") ? ViewBag.HasBackButton : false;
ViewBag.HasConnectStatus = ViewBagHasProperty("HasConnectStatus") ? ViewBag.HasConnectStatus : false;

It's not great, but it should work.