umbraco / Umbraco.UIBuilder.Issues

Back office UI builder for Umbraco
3 stars 2 forks source link

Nullref error when searching a searchable property which has a null value #66

Closed jemayn closed 10 months ago

jemayn commented 10 months ago

Describe the bug

If you add a searchable property which is a nullable string and then have data which has null for that value it throws an exception.

Steps To Reproduce Set up a new UI Builder dashboard, something like this:

public class KonstruktConfigurator : IConfigurator
{
    public void Configure(UIBuilderConfigBuilder builder)
    {
        builder.AddSection("Dealers", sectionConfig => sectionConfig
            .Tree(treeConfig => treeConfig
                .AddCollection<Dealer>(
                    x => x.UniqueId,
                    "Dealer",
                    "Dealers",
                    "List of dealers",
                    "icon-location-near-me",
                    "icon-users",
                    collectionConfig => collectionConfig
                        .SetRepositoryType<DealerKonstruktRepository>()
                        .SetNameProperty(x => x.Name)

                        // Searchable properties
                        .AddSearchableProperty(x => x.Email)

Where Email is of type string?.

Then have data which contains classes where atleast one of them has null for the email property.

In the Repository I have code like this:

protected override PagedResult<Dealer> GetPagedImpl(int pageNumber, int pageSize, Expression<Func<Dealer, bool>> whereClause, Expression<Func<Dealer, object>> orderBy,
    SortDirection orderByDirection)
{
    var allItems = _dealerRepository.GetAllCached().GetAwaiter().GetResult();

    var res = new PagedResult<Dealer>(0, pageNumber, pageSize);

    if (allItems is null)
    {
        return res;
    }

    if (whereClause is not null) // Despite the compiler saying otherwise whereClause can be null
    {
        allItems = allItems.Where(whereClause.Compile());
    }

    if (orderBy is not null) // Despite the compiler saying otherwise orderBy can be null
    {
        allItems = orderByDirection is SortDirection.Ascending 
            ? allItems.OrderBy(orderBy.Compile()) 
            : allItems.OrderByDescending(orderBy.Compile());
    }

    List<Dealer> items;

    var totalCount = items.Count;
    items = items.Skip((pageNumber -1) * pageSize).Take(pageSize).ToList();

    return new PagedResult<Dealer>(totalCount, pageNumber, pageSize)
    {
        Items = items
    };
}

It fails on the allItems = allItems.Where(whereClause.Compile()); if the dataset has a searchable property which is null. If I remove that property as searchable or if I set the value to fx an empty string instead of null it works as expected.

The error it throws is this:

An error occurred
Object reference not set to an instance of an object.

Exception Details
System.NullReferenceException, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e: Object reference not set to an instance of an object.
Stacktrace
at Umbraco.UIBuilder.Persistence.Repository`2.Umbraco.UIBuilder.Persistence.IRepository.GetPaged(Int32 pageNumber, Int32 pageSize, LambdaExpression whereClause, LambdaExpression orderBy, SortDirection orderDirection, Boolean raiseEvents)
   at Umbraco.UIBuilder.Services.EntityService.FindEntities(CollectionConfig collection, Object parentId, Int32 pageNumber, Int32 pageSize, String query, IDictionary`2 filters, String dataViewAlias, String orderBy, String orderDirection)
   at Umbraco.UIBuilder.Web.Controllers.Api.UIBuilderApiController.FindEntities(FindEntitiesPostModel postModel)
   at lambda_method538(Closure, Object, Object[])
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.SyncObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Logged|12_1(ControllerActionInvoker invoker)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)

Expected behavior I would expect it not to throw and return the relevant search results.

Environment (please complete the following information):

mattbrailsford commented 10 months ago

I'm not entirely sure this is our issue. Given you are implementing your own repository, it's up to you to ensure the where clause executes on your dataset. I'm guessing there is an issue in how EF is parsing the where clause expression tree 🤔

What happens if you were to execute the same where clause directly on your EF dataset?

mattbrailsford commented 10 months ago

Hmm, maybe it is within the where clause it's an issue 🤔

image

I'm guessing maybe we need to try and null check before performing the ToUpper and StartsWith

jemayn commented 10 months ago

Hmm, maybe it is within the where clause it's an issue 🤔

image

I'm guessing maybe we need to try and null check before performing the ToUpper and StartsWith

Yes that sounds right, it is when calling the .Where(whereClause.Compile()) it fails. The data is just an IEnumerable, so seems the problems come within the func for whereClause.

This syntax is quite new to me so I may be using it wrong, but it looked like the way to go.

mattbrailsford commented 10 months ago

Ok, I've quickly added a null check. I'll push to the nightly server if you want to test it? https://docs.umbraco.com/umbraco-cms/fundamentals/setup/install/installing-nightly-builds (give me a few minutes, but it will be a 12.0.1 preview)

PS Yea, expression trees are soooooo confusing :D

jemayn commented 10 months ago

That would be great, I'll test it later off the nightly build then 🙂

mattbrailsford commented 10 months ago

Should be there now 12.0.1--preview.1 🤞

jemayn commented 10 months ago

Tested with 12.0.1--preview.1 and my issue disappeared and the search works as expected - thanks!

mattbrailsford commented 10 months ago

Excellent! I'll make sure this is in the next patch release 👍

mattbrailsford commented 10 months ago

Resolved in 10.0.1 and 12.0.1