Wicket
  1. Wicket
  2. WICKET-330

CheckBox incorrectly converts its model value when a custom Boolean converter is installed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0 branch (discontinued), 1.3.0-beta3
    • Fix Version/s: 1.3.0-rc1
    • Component/s: wicket
    • Labels:
      None

      Description

      When I use a custom localized Boolean converter (using my custom IConverterLocator) that converts "Ano" to true and "Ne" to false (Czech words for "yes" and "no") the CheckBox with 'true' getModelObject() will render unchecked. The problem is that CheckBox uses Strings.isTrue() to convert a value returned by getModelObjectAsString() which uses converters. The result is that true is incorrectly converted to false...

      true -> convertToString() -> "Ano" -> Strings.isTrue() -> false

      1. CheckBox.patch
        0.6 kB
        Martin Benda
      2. checkbox-getModelValue.patch
        0.6 kB
        Martin Benda

        Activity

        Hide
        Johan Compagner added a comment -

        fixed the CheckBox wil always use the default boolean converter (or somebody has to override that again)

        Show
        Johan Compagner added a comment - fixed the CheckBox wil always use the default boolean converter (or somebody has to override that again)
        Hide
        Johan Compagner added a comment -

        ahh ok, now it is clear to me then it is a global boolean converter that shouldn't be used by the checkbox.. will look into it.

        Show
        Johan Compagner added a comment - ahh ok, now it is clear to me then it is a global boolean converter that shouldn't be used by the checkbox.. will look into it.
        Hide
        Martin Benda added a comment -

        The proposed patch.

        Show
        Martin Benda added a comment - The proposed patch.
        Hide
        Martin Benda added a comment -

        > So as far as i can see setting a converter on a Checkbox is weird. The converter really only works for text input fields.
        Yes, but I want to use the converter for TextFields and not for CheckBoxes! The problem is that there is a side-effect of the new converter: all CheckBoxes become broken.

        Now the CheckBox relies on the default implementation of the Boolean converter (via the getModelObjectAsString method). What I propose is to get rid of this dependency and make CheckBoxes work independently of Boolean converters.

        Show
        Martin Benda added a comment - > So as far as i can see setting a converter on a Checkbox is weird. The converter really only works for text input fields. Yes, but I want to use the converter for TextFields and not for CheckBoxes! The problem is that there is a side-effect of the new converter: all CheckBoxes become broken. Now the CheckBox relies on the default implementation of the Boolean converter (via the getModelObjectAsString method). What I propose is to get rid of this dependency and make CheckBoxes work independently of Boolean converters.
        Hide
        Johan Compagner added a comment -

        looking at this bug again, If i implement such a getModelValue() then what does the converter do?
        Because it won't be called at all for Boolean to a String then

        And when the data comes back your converter will also not get the converted value. So what is the use of setting the converter? Where is it used then?

        So as far as i can see setting a converter on a Checkbox is weird. The converter really only works for text input fields.

        Show
        Johan Compagner added a comment - looking at this bug again, If i implement such a getModelValue() then what does the converter do? Because it won't be called at all for Boolean to a String then And when the data comes back your converter will also not get the converted value. So what is the use of setting the converter? Where is it used then? So as far as i can see setting a converter on a Checkbox is weird. The converter really only works for text input fields.
        Hide
        Martin Benda added a comment -

        A CheckBox can have only o Boolean model (CheckBox extends FormComponent<Boolean>) and I believe that wicket converters are designed to convert model Object to its String display representation and vice versa, not boolean <> model objects. That's why they have a locale as the second conversion parameter. IMHO wicket converters should be completely decoupled from database or any other backend system. Models are responsible to communicate with database and they should handle conversions like 1 <> Boolean.TRUE and 0 <-> Boolean.FALSE.

        I believe that this could be the fix, it works for me:

        public class CheckBox extends FormComponent<Boolean> implements IOnChangeListener
        {
        ....
        @Override
        protected String getModelValue()

        { return Boolean.toString(Boolean.TRUE.equals(getModelObject())); }

        ...
        }

        Show
        Martin Benda added a comment - A CheckBox can have only o Boolean model (CheckBox extends FormComponent<Boolean>) and I believe that wicket converters are designed to convert model Object to its String display representation and vice versa, not boolean < > model objects. That's why they have a locale as the second conversion parameter. IMHO wicket converters should be completely decoupled from database or any other backend system. Models are responsible to communicate with database and they should handle conversions like 1 < > Boolean.TRUE and 0 <-> Boolean.FALSE. I believe that this could be the fix, it works for me: public class CheckBox extends FormComponent<Boolean> implements IOnChangeListener { .... @Override protected String getModelValue() { return Boolean.toString(Boolean.TRUE.equals(getModelObject())); } ... }
        Hide
        Johan Compagner added a comment -

        typo: A Display Value of the Checkbox is just something he CAN convert to a simple true/false.

        Show
        Johan Compagner added a comment - typo: A Display Value of the Checkbox is just something he CAN convert to a simple true/false.
        Hide
        Johan Compagner added a comment -

        Yes that i know. i explained that with my previous comment.

        So the fix is that we need to call the converter twice when rendering: in getModelObjectAsString, and then directly in onComponenTag again to convert it to an object. (must be then Boolean)
        and then also set that value in the output of the page.

        But then we can encounter a big other problem
        What if people has it in the database just around? So not true or false but yes/no or Ano/Ne??

        And they use the converter to give the checkbox a boolean so that it know that by Ano it must select and by Ne it shouldnt?
        And on submit the checkbox sends a boolean true or false to the converter that converts it then what the underlying model wants (Ano or Ne)

        So i still don't know what the right fix would be. I am still a bit inclined to say that the current behavior is fine. Because the converter in this case
        must just provide boolean -> model object or model object -> boolean..
        Because if the database can't store booleans (you have those) and they do it just with 1 and 0 .. what then? What should your converter do then?
        are you making from 1 -> Ano ? But what does then the other way do? Ano -> 1 ? But that is still not a boolean that Checkbox component can handle. He wants True or False..
        to be able to know if it checked or not.

        So how to fix this without breaking other things.. i don't know yet. A Display Value of the Checkbox is just something he can't convert to a simple true/false.

        Show
        Johan Compagner added a comment - Yes that i know. i explained that with my previous comment. So the fix is that we need to call the converter twice when rendering: in getModelObjectAsString, and then directly in onComponenTag again to convert it to an object. (must be then Boolean) and then also set that value in the output of the page. But then we can encounter a big other problem What if people has it in the database just around? So not true or false but yes/no or Ano/Ne?? And they use the converter to give the checkbox a boolean so that it know that by Ano it must select and by Ne it shouldnt? And on submit the checkbox sends a boolean true or false to the converter that converts it then what the underlying model wants (Ano or Ne) So i still don't know what the right fix would be. I am still a bit inclined to say that the current behavior is fine. Because the converter in this case must just provide boolean -> model object or model object -> boolean.. Because if the database can't store booleans (you have those) and they do it just with 1 and 0 .. what then? What should your converter do then? are you making from 1 -> Ano ? But what does then the other way do? Ano -> 1 ? But that is still not a boolean that Checkbox component can handle. He wants True or False.. to be able to know if it checked or not. So how to fix this without breaking other things.. i don't know yet. A Display Value of the Checkbox is just something he can't convert to a simple true/false.
        Hide
        Martin Benda added a comment -

        The problem is that getValue() returns either raw input (not converted) or model value converted by getConverter(...).convertToString(). In the first case, Strings.isTrue() is fine, but getConverter(...).convertToObject should be used in the latter case...

        Show
        Martin Benda added a comment - The problem is that getValue() returns either raw input (not converted) or model value converted by getConverter(...).convertToString(). In the first case, Strings.isTrue() is fine, but getConverter(...).convertToObject should be used in the latter case...
        Hide
        Johan Compagner added a comment -

        What is the getModelObject() of the checkbox? so without conversion of that checkbox?

        Is that Boolean.TRUE or Boolean.FALSE?

        If that is the case. Why have a custom converter for the checkbox? There shouldn't be anything converted.

        so for this to work in your situation if you want a true-> XXX and a false -> YYY

        onComponentTag:

        final String value = getValue(); <<< if rendered the first time: getModelObjectAsString() that goes through the converter so that converts true -> XXXX
        if (value != null)
        {
        try
        {
        if (Strings.isTrue(value)) << So this is wrong and should again use the converter to go from a string value to the real object???

        { tag.put("checked", "checked"); }

        But getValue after a submit returns "on" (default value of html)
        So that is totally not what your getModelObjectAsString() returns..

        So i guess what you want is something like this:

        protected final void onComponentTag(final ComponentTag tag)
        {
        checkComponentTag(tag, "input");
        checkComponentTagAttribute(tag, "type", "checkbox");

        final String value = getValue();
        if (value != null)
        {
        tag.put("value",value);
        try
        {
        if (getConverter().convertToObject(value) == Boolean.TRUE)
        { tag.put("checked", "checked"); }

        else

        { // In case the attribute was added at design time tag.remove("checked"); }

        }

        protected Object convertValue(String[] value)
        {
        String tmp = value != null && value.length > 0 ? value[0] : null;
        try

        { return getConverter().convertToObject(tmp); //return Strings.toBoolean(tmp); }

        catch (StringValueConversionException e)

        { throw new ConversionException("Invalid boolean input value posted \"" + getInput() + "\"", e).setTargetType(Boolean.class); }

        }

        Show
        Johan Compagner added a comment - What is the getModelObject() of the checkbox? so without conversion of that checkbox? Is that Boolean.TRUE or Boolean.FALSE? If that is the case. Why have a custom converter for the checkbox? There shouldn't be anything converted. so for this to work in your situation if you want a true-> XXX and a false -> YYY onComponentTag: final String value = getValue(); <<< if rendered the first time: getModelObjectAsString() that goes through the converter so that converts true -> XXXX if (value != null) { try { if (Strings.isTrue(value)) << So this is wrong and should again use the converter to go from a string value to the real object??? { tag.put("checked", "checked"); } But getValue after a submit returns "on" (default value of html) So that is totally not what your getModelObjectAsString() returns.. So i guess what you want is something like this: protected final void onComponentTag(final ComponentTag tag) { checkComponentTag(tag, "input"); checkComponentTagAttribute(tag, "type", "checkbox"); final String value = getValue(); if (value != null) { tag.put("value",value); try { if (getConverter().convertToObject(value) == Boolean.TRUE) { tag.put("checked", "checked"); } else { // In case the attribute was added at design time tag.remove("checked"); } } protected Object convertValue(String[] value) { String tmp = value != null && value.length > 0 ? value [0] : null; try { return getConverter().convertToObject(tmp); //return Strings.toBoolean(tmp); } catch (StringValueConversionException e) { throw new ConversionException("Invalid boolean input value posted \"" + getInput() + "\"", e).setTargetType(Boolean.class); } }
        Hide
        Martin Benda added a comment -

        >Where do you use those 2 "Ano" and "Ne" as a string?
        >Does the checkbox also print those?

        CheckBox prints nothing, it renders either checked or unchecked...

        >You do store true and false in the database? So why are you converting that to a string for the checkbox?
        >
        >I would say you need a special converter if you store the "Ano" and "Ne" in the database and you need that to convert to true/false for the checkbox.

        It doesn't matter if I store anything in the database... I just created my custom implementation of IConverterLocator which returns custom BooleanConverter that converts true <> "Ano" and false <> "ne" so that TextFiled<Boolean> with a boolean model shows "Ano" for "true" model value and "Ne" for false model value. And it works fine.

        The problem is that a CheckBox uses internally a boolean converter to convert Boolean to a String and Strings.isTrue to convert String to Boolean.

        This issue was discussed on the wicket-dev mailing list - subject "Insane conversions inside CheckBox#onComponentTag (wicket-2.0)".

        Show
        Martin Benda added a comment - >Where do you use those 2 "Ano" and "Ne" as a string? >Does the checkbox also print those? CheckBox prints nothing, it renders either checked or unchecked... >You do store true and false in the database? So why are you converting that to a string for the checkbox? > >I would say you need a special converter if you store the "Ano" and "Ne" in the database and you need that to convert to true/false for the checkbox. It doesn't matter if I store anything in the database... I just created my custom implementation of IConverterLocator which returns custom BooleanConverter that converts true < > "Ano" and false < > "ne" so that TextFiled<Boolean> with a boolean model shows "Ano" for "true" model value and "Ne" for false model value. And it works fine. The problem is that a CheckBox uses internally a boolean converter to convert Boolean to a String and Strings.isTrue to convert String to Boolean. This issue was discussed on the wicket-dev mailing list - subject "Insane conversions inside CheckBox#onComponentTag (wicket-2.0)".
        Hide
        Johan Compagner added a comment -

        Where do you use those 2 "Ano" and "Ne" as a string?
        Does the checkbox also print those?

        You do store true and false in the database? So why are you converting that to a string for the checkbox?

        I would say you need a special converter if you store the "Ano" and "Ne" in the database and you need that to convert to true/false for the checkbox.

        Show
        Johan Compagner added a comment - Where do you use those 2 "Ano" and "Ne" as a string? Does the checkbox also print those? You do store true and false in the database? So why are you converting that to a string for the checkbox? I would say you need a special converter if you store the "Ano" and "Ne" in the database and you need that to convert to true/false for the checkbox.

          People

          • Assignee:
            Johan Compagner
            Reporter:
            Martin Benda
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development