MyFaces Core
  1. MyFaces Core
  2. MYFACES-1328

UISelectOne and UISelectMany fail with custom converter that returns java.lang.String from getAsObject() method.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.4-SNAPSHOT
    • Fix Version/s: 1.1.5
    • Component/s: None
    • Labels:
      None

      Description

      The problem seems to be in javax.faces.component._SelectItemsUtil.matchValue(FacesContext context, Object value, Iterator selectItemsIter, _ValueConverter converter) method.

      Line 63-72:
      Object itemValue = item.getValue();
      if(converter != null && itemValue instanceof String)

      { itemValue = converter.getConvertedValue(context, (String)itemValue); }

      if (value==itemValue || value.equals(itemValue))

      { return true; }

      If item's value is java.lang.String then this code does duplicate conversion making matchValue() return false and subsequently resulting in error in calling method (validateValue() in javax.faces.component.UISelectOne and javax.faces.component.UISelectMany).

      1. reproducer.zip
        4 kB
        Alexey Maslov

        Activity

        Hide
        Alexey Maslov added a comment -

        This is a simple reproducer of the problem. It contains one page (selectOnePage.jsp) with <h:selectOneMenu> with custom converter that converts java.lang.String to java.lang.String of different kind.

        Show
        Alexey Maslov added a comment - This is a simple reproducer of the problem. It contains one page (selectOnePage.jsp) with <h:selectOneMenu> with custom converter that converts java.lang.String to java.lang.String of different kind.
        Hide
        Nikolay Petrov added a comment -

        I really don't see a reason to put conversion:
        if(converter != null && itemValue instanceof String)

        { itemValue = converter.getConvertedValue(context, (String)itemValue); }

        in this case. A possible solution would be to remove conversion at all (as it is within ri).

        May be there is some special case to handle out. Can anybody made something up?

        Show
        Nikolay Petrov added a comment - I really don't see a reason to put conversion: if(converter != null && itemValue instanceof String) { itemValue = converter.getConvertedValue(context, (String)itemValue); } in this case. A possible solution would be to remove conversion at all (as it is within ri). May be there is some special case to handle out. Can anybody made something up?
        Hide
        Martin Marinschek added a comment -

        You're absolutely right, Nikolay. Thanks for tracking this down.

        regards,

        Martin

        Show
        Martin Marinschek added a comment - You're absolutely right, Nikolay. Thanks for tracking this down. regards, Martin
        Hide
        Martin Marinschek added a comment -

        Grant has had problems with this - conversion errors which are not logged and stop his application dead in conversion and validation.

        regards,

        Martin

        Show
        Martin Marinschek added a comment - Grant has had problems with this - conversion errors which are not logged and stop his application dead in conversion and validation. regards, Martin
        Hide
        Grant Smith added a comment -

        It turns out my application was missing <h:messages>, so of course, no validation message was reaching me. I'll close this, although I'm sure we're going to have a lot of users that got spoiled by having the convenient conversion. Still, we must follow the behaviour of the RI...

        Show
        Grant Smith added a comment - It turns out my application was missing <h:messages>, so of course, no validation message was reaching me. I'll close this, although I'm sure we're going to have a lot of users that got spoiled by having the convenient conversion. Still, we must follow the behaviour of the RI...
        Hide
        Daniel Campagnoli added a comment -

        We are one of the many spoilt users that rely on the convenient conversion! We would be dissapointed to see this behaviour go as convenience is a good thing. We had just updated to the latest nightly build and it's broken our application.

        I've had a look at the _SelectItemsUtil class, and it appears the problem could be solved by re-ordering the extra conversion to be after the initial equals check, which wouldn't break with custom converters that return String's. So the code snipped in this issues description would be replaced with:

        Object itemValue = item.getValue();
        if (value==itemValue || value.equals(itemValue))

        { return true; }

        if(convenientConversion) // optional if statement so the default behaviour could match the RI
        {
        if(converter != null && itemValue instanceof String)
        {
        itemValue = converter.getConvertedValue(context, (String)itemValue);
        if (value==itemValue || value.equals(itemValue))

        { return true; }

        }
        }

        The faces context and converter arguments would have to be added back to the matchValue() method and creation of the _ValueConverter in UISelectMany added back in.

        Dan

        Show
        Daniel Campagnoli added a comment - We are one of the many spoilt users that rely on the convenient conversion! We would be dissapointed to see this behaviour go as convenience is a good thing. We had just updated to the latest nightly build and it's broken our application. I've had a look at the _SelectItemsUtil class, and it appears the problem could be solved by re-ordering the extra conversion to be after the initial equals check, which wouldn't break with custom converters that return String's. So the code snipped in this issues description would be replaced with: Object itemValue = item.getValue(); if (value==itemValue || value.equals(itemValue)) { return true; } if(convenientConversion) // optional if statement so the default behaviour could match the RI { if(converter != null && itemValue instanceof String) { itemValue = converter.getConvertedValue(context, (String)itemValue); if (value==itemValue || value.equals(itemValue)) { return true; } } } The faces context and converter arguments would have to be added back to the matchValue() method and creation of the _ValueConverter in UISelectMany added back in. Dan
        Hide
        Paul Devine added a comment -

        This comment is not intended as a bug report of any kind, but might other users with a possible 1.1.5 upgrade problem. In upgrading to 1.1.5 the faces validation phase was causing the lifecycle to exit on some of our pages that were working just fine in 1.1.3. It turns out we were relying on the conversion discussed above, because our pages used selectOneRadio's in a manner illustrated below :

        <h:selectOneRadio value="#

        {myBean.someBooleanProperty}" converter="javax.faces.Boolean" ...>
        <f:selectItem itemValue="true" itemLabel="Yes" />
        <f:selectItem itemValue="false" itemLabel="No"/>
        </h:selectOneRadio>

        The _SelectItemsUtil.matchValue method in myfaces prior to 1.1.5 would handle this by converting the "true" and "false" on the fly to Boolean.TRUE and Boolean.FALSE because we specified the BooleanConverter. The validation phase would be ok with the submitted values and the whole lifecycle would complete right through the action method. The code change to _SelectItemsUtil.matchValue methodfor 1.1.5 no longer does the conversion, so the validation phase basically says the submitted value is not one of the values in the selection items, so causes the lifecycle to exit without executing the action method.

        As the change made to Myfaces brings it in line with the spec, our jsp-code was not compatible with the spec, so we will change our jsp accordingly. Now we are like this:
        <h:selectOneRadio value="#{myBean.someBooleanProperty}

        " converter="javax.faces.Boolean" ...>
        <f:selectItems value="#

        {myBean.yesNoSelectItems}

        "/>
        </h:selectOneRadio>

        getYesNoSelectItems() returns SelectItem[] , each whose item value is Boolean object.

        Show
        Paul Devine added a comment - This comment is not intended as a bug report of any kind, but might other users with a possible 1.1.5 upgrade problem. In upgrading to 1.1.5 the faces validation phase was causing the lifecycle to exit on some of our pages that were working just fine in 1.1.3. It turns out we were relying on the conversion discussed above, because our pages used selectOneRadio's in a manner illustrated below : <h:selectOneRadio value="# {myBean.someBooleanProperty}" converter="javax.faces.Boolean" ...> <f:selectItem itemValue="true" itemLabel="Yes" /> <f:selectItem itemValue="false" itemLabel="No"/> </h:selectOneRadio> The _SelectItemsUtil.matchValue method in myfaces prior to 1.1.5 would handle this by converting the "true" and "false" on the fly to Boolean.TRUE and Boolean.FALSE because we specified the BooleanConverter. The validation phase would be ok with the submitted values and the whole lifecycle would complete right through the action method. The code change to _SelectItemsUtil.matchValue methodfor 1.1.5 no longer does the conversion, so the validation phase basically says the submitted value is not one of the values in the selection items, so causes the lifecycle to exit without executing the action method. As the change made to Myfaces brings it in line with the spec, our jsp-code was not compatible with the spec, so we will change our jsp accordingly. Now we are like this: <h:selectOneRadio value="#{myBean.someBooleanProperty} " converter="javax.faces.Boolean" ...> <f:selectItems value="# {myBean.yesNoSelectItems} "/> </h:selectOneRadio> getYesNoSelectItems() returns SelectItem[] , each whose item value is Boolean object.

          People

          • Assignee:
            Martin Marinschek
            Reporter:
            Alexey Maslov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development