Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add empty list initalizers to help avoid NREs #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

VictorioBerra
Copy link
Contributor

No description provided.

@VictorioBerra
Copy link
Contributor Author

VictorioBerra commented Oct 30, 2018

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
Copy link
Contributor Author

VictorioBerra commented Oct 30, 2018

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.

…when the developer tries to filter on a name that does not exist.
@VictorioBerra
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant