Tapestry
  1. Tapestry
  2. TAPESTRY-1978

When supplying an empty parameter binding, indicate problem parameter in error message.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.0.7
    • Fix Version/s: 5.0.11
    • Component/s: Framework
    • Labels:
      None

      Description

      While prototyping a page recently, I decided to hold off on passing in parameters until I had a good sense of what their names should be. So, I had something similar to:

      <t:textfield value=""/>

      Clearly this is a broken construct and Tapestry generally recognized it. I was greeted with the following error message:

      "Parameter expression was null or contained only whitespace."

      And a highlighted block of my template. Unfortunately, the highlighted block is not where the problem was. Likewise, the error message really didn't indicate what parameter was broken of what component. After a little bit of guess-and-check, I caught the obvious error. It'd be nice if Tapestry handled this for me though.

        Activity

        Hide
        Kevin Menard added a comment -

        I've just delved into the issue a bit more. The source of the problem is:

        BindingSourceImpl:50
        notBlank(expression, "expression");

        While the error message is technically accurate, it lacks any context to help the user figure out what's wrong.

        The description value supplied includes the source parameter that is in error, but it is appended to the string "parameter " in PageElementFactoryImpl:305. A hackish way of dealing with this is to do a substring to extract what we want. That's highly caller dependent, however. What we really should do is pass the parameter name as a separate parameter, which would necessitate either an interface change or addition.

        It should be noted that the location value would help immensely as well, although it only shows the component in error, not the parameter value. Due to the fail fast nature of notBlank, the location value is essentially discarded. So, that should be wrapped up nicely as well.

        Show
        Kevin Menard added a comment - I've just delved into the issue a bit more. The source of the problem is: BindingSourceImpl:50 notBlank(expression, "expression"); While the error message is technically accurate, it lacks any context to help the user figure out what's wrong. The description value supplied includes the source parameter that is in error, but it is appended to the string "parameter " in PageElementFactoryImpl:305. A hackish way of dealing with this is to do a substring to extract what we want. That's highly caller dependent, however. What we really should do is pass the parameter name as a separate parameter, which would necessitate either an interface change or addition. It should be noted that the location value would help immensely as well, although it only shows the component in error, not the parameter value. Due to the fail fast nature of notBlank, the location value is essentially discarded. So, that should be wrapped up nicely as well.
        Hide
        Howard M. Lewis Ship added a comment -

        There's no essential reason to use Defense.notBlank() if a more custom error message would be more useable. I'm just surprised that a blank expression could make it that far; i.e., I'd expect an earlier check, before we invoke BindingSource.

        Show
        Howard M. Lewis Ship added a comment - There's no essential reason to use Defense.notBlank() if a more custom error message would be more useable. I'm just surprised that a blank expression could make it that far; i.e., I'd expect an earlier check, before we invoke BindingSource.
        Hide
        Kevin Menard added a comment -

        I was able to convey the location information by wrapping up in a custom Tapestry exception. I avoided an interface change by using the "description" parameter to my advantage. Since it's largely an open-ended value describing the binding, I confined it in my case to the actual parameter name. I couldn't find any place that this narrowing meaning would affect and all existing tests pass, so I'm going with it. If it turns out we need to actually split the description from the parameter name, we can do it when it becomes an actual problem.

        Show
        Kevin Menard added a comment - I was able to convey the location information by wrapping up in a custom Tapestry exception. I avoided an interface change by using the "description" parameter to my advantage. Since it's largely an open-ended value describing the binding, I confined it in my case to the actual parameter name. I couldn't find any place that this narrowing meaning would affect and all existing tests pass, so I'm going with it. If it turns out we need to actually split the description from the parameter name, we can do it when it becomes an actual problem.

          People

          • Assignee:
            Kevin Menard
            Reporter:
            Kevin Menard
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development