Thou Shalt Name Thy Parameter “command”

22 commentsWritten on January 31st, 2011 by
Categories: ASP.NET MVC, Telerik

I'm currently doing a proof of concept with Telerik's ASP.NET MVC Extensions and ran into something that i find rather troubling. I was trying to get the grid to work with custom binding, so i can take care of paging/sorting serverside for AJAX requests. It was pretty easy to set up, but for some reason my controller method which is invoked for the AJAX requests wasn't being passed the relevant sorting information.

The method definition looks like this:

        [GridAction(EnableCustomBinding = true)]
        public ActionResult GetModels(GridCommand gridCommand)

When i go to a different page in the grid, the gridCommand instance contains the relevant paging information (pagesize, current page). The GridCommand type also exposes a SortDescriptors collection which contains all the information you'd need to correctly sort the data you're supposed to return. Except that in my case, the SortDescriptors collection was always empty. I went through the documentation and looked over everything to make sure that i wasn't doing anything stupid, which is always my first assumption. Unfortunately, everything looked alright. I googled and didn't really find anything, until i found a thread on the Telerik support forum where someone else mentioned the exact same problem. Unfortunately, the thread didn't get an answer so that didn't offer any help.

Then i turned to Reflector to check out the code of Telerik's GridAction attribute. I noticed the following code in the constructor of the argument:

public GridActionAttribute()
{
    this.ActionParameterName = "command";
    this.adapterFactory = DI.Current.Resolve<IGridActionResultAdapterFactory>();
}

Cue the "you've gotta be shitting me" response. I went back to my controller method and changed it to this:

        [GridAction(EnableCustomBinding = true)]
        public ActionResult GetModels(GridCommand command)

And suddenly, it worked.

I can't believe i ran into something like this, in the year 2011. This isn't exactly a good first impression that Telerik is leaving on me, and i can only hope that this proof of concept is not going to turn into a Daily WTF discovery.

« Walk Out On Sources Of NegativityRecommended Book: jQuery In Action »

This entry was posted on Monday, January 31st, 2011 at 14:56 and is filed under ASP.NET MVC, Telerik. You can subscribe to any comments to this entry via RSS 2.0. You can leave a response, or trackback from your own site.

  • Flash

    In Teleriks defence, they do mention in the documentation for GridActionAttribute that the default value for ActionParameterName is “command”. You can change it if you need to… But don’t worry, I normally don’t read the documentation either :)

    • http://davybrion.com Davy Brion

      why do they need to know the name of the parameter? it would be much cleaner if they’d just access it based on its type since you’ll never have 2 of them in the same method definition anyway

  • Roy

    If you just want to do a simple grid proof-of-concept you’re probably much better off with rolling your own simple server-side code and doing the client-side with something like [url=datatables.net]DataTables[/url].

    • Roy

      Well, with a non-malformed URL anyway :)

    • http://davybrion.com Davy Brion

      actually the POC is to see whether we’re going to ‘standardize’ on the Telerik control set :)

      have already looked at DataTables and it seems nice, though i liked jqGrid more

  • Bobjennings

    Davy -

    We played around with Telerik’s MVC Extensions early on and quickly left them behind. Not to diminish the hard work that their team has put into the extensions, but we found them to be a tad bloated, in some cases sluggish (grid), and not terribly easy to extend. Perhaps I should give them another look, but that was our experience a year or so ago.

  • http://www.google.com/profiles/verheyen.koen Koen

    That’s a typical one. ASP.NET AJax (+ Toolkit) also requires exact parameter names at some points (e.g. web service to load auto complete text box). I’m not sure why they do that but it had a reason…

  • http://twitter.com/ntcoding Nick

    What do you boys reckon to the controls overall? Are they useful to all developer and all projects? Or are they/aimed suited for a particular niche of developer/project type?

    • http://davybrion.com Davy Brion

      i’m leaning towards: no, not useful to all devs/projects

  • Leon Breedt

    Principle of most surprise.

  • http://twitter.com/korchev Atanas Korchev

    Hi Davy,

    I work for Telerik and I am part of the MVC team.

    We need to know the name of the GridCommand parameter because we must populate it with the right values. If you check the implementation of the GridActionAttribute you will see this in the OnActionExecuting method:

    if (filterContext.ActionParameters.ContainsKey(ActionParameterName))
    {
    GridCommand command = new GridCommand
    {
    Page = filterContext.Controller.ValueOf(Prefix(GridUrlParameters.CurrentPage)),
    PageSize = filterContext.Controller.ValueOf(Prefix(GridUrlParameters.PageSize))
    };
    /* some other code which is not relevant here*/
    filterContext.ActionParameters[ActionParameterName] = command;
    }

    If we don’t have this code the command parameter will stay empty as ASP.NET MVC will not do model binding for something for which there is no value in the POST collection. The grid posts only stuff like “Grid-page=1&Grid-orderBy=something”. Perhaps this is not the best approach to implement this but that’s what we ended up with. If you have any suggestions how to improve this I will be more than happy to hear your feedback.

    There is another thing which you can try though. You can omit the GridCommand parameter and change your action method signature like this:

    [GridAction(EnableCustomBinding = true)]
    public ActionResult GetModels(int page, string orderBy)
    {
    }

    The only thing else which you need to do is to prevent the grid from prefixing the url parameters:

    Of course then you have to follow the right naming of the parameters – page, orderBy etc. All are listed in the GridUrlParameters class.

    • http://davybrion.com Davy Brion

      Atanas,

      i’m no expert on ActionFilters, but does the filterContext.ActionParameters dictionary already contain instances for the values in the OnActionExecuting method? If so, it would’ve been very easy to just get the parameter instance out of the dictionary whose type is GridCommand and then there wouldn’t have been any requirement regarding the name of the parameter

      If not, i would even resort to reflection to pick the right parameter based on the type

      In short, i’d go through hell to avoid forcing my customers to name their parameters a certain way because the impression that such a requirement leaves is something i would find unacceptable

      • http://twitter.com/korchev Atanas Korchev

        Davy,

        Thank you for the input! ActionParameters is an IDictionary so we could in theory find the one parameter which is of GridCommand type and use it. The only tricky situation we need to deal with is the case when there is more than one GridCommand parameter. Perhaps then we could rely on the ActionParameterName. Does this make more sense to you? If yes – we will refactor the implementation it the suggested way and release a hotfix build.

        • http://davybrion.com Davy Brion

          Atanas,

          i can’t really think of a situation where there could be more than one parameter of that type in a controller method, though i’m sure you bump into a lot more usage edge-cases when it comes to reusable controls than i do :)

          either way, i’d provide a configurable fallback approach for that scenario because that is an edge-case… the typical scenario should just work however, regardless of the name of the parameter

          • http://twitter.com/korchev Atanas Korchev

            Davy,

            Thanks again. We will implement it in the way suggested. I really appreciate your feedback and you can feel free to ping me should you have other comments/questions/problems.

  • P51dfltln

    hope you went back and posted your answer on the support forum…

    • http://davybrion.com Davy Brion

      sure did :)

  • Ruleit

     Thanks Man! You Save me a loooot of time!

Meta

Recent Posts

Categories

Archives