DataTables.Queryable icon indicating copy to clipboard operation
DataTables.Queryable copied to clipboard

Add empty list initalizers to help avoid NREs

Open VictorioBerra opened this issue 5 years ago • 3 comments

VictorioBerra avatar Oct 30 '18 19:10 VictorioBerra

We need a better way for defaults too.

System.NullReferenceException: Object reference not set to an instance of an object.
   at DataTables.Queryable.DataTablesAjaxPostModel.ToNameValueCollection()
   at DataTables.Queryable.DataTablesRequest`1..ctor(DataTablesAjaxPostModel ajaxPostModel)

This is when I try and POST to my controller with {}. We should be able to set a default length and stuff. Omitting the length means our code will try and divide by zero.

VictorioBerra avatar Oct 30 '18 20:10 VictorioBerra

This is a start for defaults:

public class DataTablesAjaxPostModelDefaultsActionFilter : ActionFilterAttribute
    {
        public override void OnActionExecuting(ActionExecutingContext context)
        {
            var argumentTypes = context.ActionArguments;

            var dataTablesAjaxPostModelArgument = context.ActionArguments
                .SingleOrDefault(x => x.Value.GetType() == typeof(DataTablesAjaxPostModel));

            if (dataTablesAjaxPostModelArgument.Value != null)
            {
                var dataTablesAjaxPostModel = (DataTablesAjaxPostModel)dataTablesAjaxPostModelArgument.Value;

                if(dataTablesAjaxPostModel.Length == default)
                {
                    dataTablesAjaxPostModel.Length = 50;
                }

                if (dataTablesAjaxPostModel.Order == null)
                {
                    dataTablesAjaxPostModel.Order = new List<DataTablesAjaxPostModel.OrderData>();
                }

                if (dataTablesAjaxPostModel.Columns == null)
                {
                    dataTablesAjaxPostModel.Columns = new List<DataTablesAjaxPostModel.ColumnData>();
                }

                if (dataTablesAjaxPostModel.Search == null)
                {
                    dataTablesAjaxPostModel.Search = new DataTablesAjaxPostModel.SearchData()
                    {
                        Value = string.Empty
                    };
                }

            }

        }
    }

We could ship this as a part of the library so people can include that if they want. Maybe rename it to AllowEmptyDataTablesAjaxPostModelActionFilter or something. Even better would be to somehow let people set their own.

I am still struggling with two ugly exceptions:

  1. When you set up defaults like this:
                var ownedByColumn = request.Columns[x => x.OwnedBy];
                ownedByColumn.ColumnSearchPredicate = (x) => x.OwnedBy.Name.Contains(ownedByColumn.SearchValue) ||
                    x.OwnedBy.EmailAddress.Contains(ownedByColumn.SearchValue);
                ownedByColumn.ColumnOrderingProperty = (x) => x.OwnedBy.Name;

And they did not provide OwnedBy, then BOOM it throws. So you have to try/catch every single place you configure some custom column searches. If they do not "ask" for the column in the request, we should find a way to not explode.

  1. Is the opposite way around. If they ask for a column like "IDoNotExist" then DataTablesRequest<T> will explode when you call the CTOR, I am trying to find a global way to detect this and send a graceful error back. Once again I can not try catch everywhere. For now, I wrote my own extension method for the DataTablesAjaxPostModel which returns a new DataTablesRestRequest or null depending on the result of the try/catch.

VictorioBerra avatar Oct 30 '18 20:10 VictorioBerra

The constructor of DataTableRequest is doing a LOT. Can we break this up to better adhere to single responsibility?

VictorioBerra avatar Oct 30 '18 22:10 VictorioBerra