Wicket
  1. Wicket
  2. WICKET-606

AbstractTextComponent#setConvertEmptyInputStringToNull(true) does not work with IObjectClassAwareModels (affects TextField, etc.)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.3.0-beta1
    • Fix Version/s: 1.3.0-beta2
    • Component/s: None
    • Labels:
      None

      Description

      The unit tests ought to cover this, but apparently don't. A TextField should convert empty strings to nulls if you call setConvertEmptyInputStringToNull(true) on it (which the constructors do by default).

      However, this doesn't currently work:

      • AbstractTextComponent#getConvertEmptyInputStringToNull() is only called from AbstractTextComponent#convertValue(String[]).
      • AbstractTextComponent#convertValue(String[]) is called from FormComponent#convert(), but only if the component doesn't have a type assigned to it.
      • As soon as you use a PropertyModel (which implements IObjectClassAwareModel), this means empty strings are no longer converted to nulls.

      This is obviously a great big blocker and needs fixing ASAP.

        Issue Links

          Activity

          Hide
          Johan Compagner added a comment -

          that is not that simple because if you have a type then the type converter is in control how to handle nulls and empty strings
          for example if the type is a Number then an empty string will always return null (no matter what it is)

          os if you are saying false in convert empty string to null and the type is not String then something really goes wrong.

          So the only use of the getConvertEmptyInput is if the type is a String, so the question is how are we going to give that behavior
          to the StringConverter??

          the only thing i can think of is that AbstractTextComponent does override:

          public IConverter getConverter(Class/* <?> */type)

          { return getSession().getConverter(type); }

          and if the type is String return a wrapper around the converter
          that is given by the session. And handle the value based on the conver empty input setting.

          any other ideas?

          Show
          Johan Compagner added a comment - that is not that simple because if you have a type then the type converter is in control how to handle nulls and empty strings for example if the type is a Number then an empty string will always return null (no matter what it is) os if you are saying false in convert empty string to null and the type is not String then something really goes wrong. So the only use of the getConvertEmptyInput is if the type is a String, so the question is how are we going to give that behavior to the StringConverter?? the only thing i can think of is that AbstractTextComponent does override: public IConverter getConverter(Class/* <?> */type) { return getSession().getConverter(type); } and if the type is String return a wrapper around the converter that is given by the session. And handle the value based on the conver empty input setting. any other ideas?
          Hide
          Johan Compagner added a comment -

          by the way. The problem with this is IF the developer overrides getConverter again
          he has to honer this them selfs.

          Show
          Johan Compagner added a comment - by the way. The problem with this is IF the developer overrides getConverter again he has to honer this them selfs.
          Hide
          Matej Knopp added a comment -

          If you really think that this problem only happens with Strings, then the solution can be easy. We simply won't set component type to String if the target type is string. We can just ignore the type.

          E.g.

          protected void onBeforeRender()
          {
          super.onBeforeRender();
          if (getType() == null)
          {
          Class type = getModelType(getModel());
          if (String.class.equals(type) == false)

          { setType(getModelType(getModel())); }

          }
          }

          WDYT?

          Show
          Matej Knopp added a comment - If you really think that this problem only happens with Strings, then the solution can be easy. We simply won't set component type to String if the target type is string. We can just ignore the type. E.g. protected void onBeforeRender() { super.onBeforeRender(); if (getType() == null) { Class type = getModelType(getModel()); if (String.class.equals(type) == false) { setType(getModelType(getModel())); } } } WDYT?
          Hide
          Johan Compagner added a comment -

          i think that would work yes
          It should be allowed to set the type yourself (but that is still the case) because i could think of some string->string conversions (mask textfields and so on)
          and getConverter is i think only used when a type is set.

          Show
          Johan Compagner added a comment - i think that would work yes It should be allowed to set the type yourself (but that is still the case) because i could think of some string->string conversions (mask textfields and so on) and getConverter is i think only used when a type is set.
          Hide
          Alastair Maw added a comment -

          Fixed in r544135.

          Show
          Alastair Maw added a comment - Fixed in r544135.
          Hide
          Timo Rantalaiho added a comment -

          After I updated to a fresh 1.3.0 snapshot today (and these changes got applied), one of our tests started failing because a TextField got its input trimmed even though we didn't want it to happen.

          What does the trimming is FormComponent.convertValue():

          protected Object convertValue(String[] value) throws ConversionException

          { return value != null && value.length > 0 && value[0] != null ? value[0].trim() : null; }

          So I think that a side effect of these changes has been that now FormComponent.convertValue() gets always called to String-type text components (at least when using a PropertyModel).

          I find it suspicious that the framework would always trim input by default; at least it could be overridable (though you can already override the whole convertValue()). What puzzles me a bit more is how this side effect was caused by these changes.

          What do you think?

          Show
          Timo Rantalaiho added a comment - After I updated to a fresh 1.3.0 snapshot today (and these changes got applied), one of our tests started failing because a TextField got its input trimmed even though we didn't want it to happen. What does the trimming is FormComponent.convertValue(): protected Object convertValue(String[] value) throws ConversionException { return value != null && value.length > 0 && value[0] != null ? value[0].trim() : null; } So I think that a side effect of these changes has been that now FormComponent.convertValue() gets always called to String-type text components (at least when using a PropertyModel). I find it suspicious that the framework would always trim input by default; at least it could be overridable (though you can already override the whole convertValue()). What puzzles me a bit more is how this side effect was caused by these changes. What do you think?
          Hide
          Eelco Hillenius added a comment -

          I agree with Timo that trimming the value doesn't sound right (though I must admit that I'm not sure what we were doing before. We should temporarily trim to e.g. check for an 'empty' string, but we should keep the input as provided.

          Show
          Eelco Hillenius added a comment - I agree with Timo that trimming the value doesn't sound right (though I must admit that I'm not sure what we were doing before. We should temporarily trim to e.g. check for an 'empty' string, but we should keep the input as provided.
          Hide
          Johan Compagner added a comment -

          we always trimmed. So this is not new behavior this is behavior we always had just because convertValue() wasn't called anymore for String values so it didn't show this for a while in 1.3
          i think in 99% of the cases i want trimming.

          Show
          Johan Compagner added a comment - we always trimmed. So this is not new behavior this is behavior we always had just because convertValue() wasn't called anymore for String values so it didn't show this for a while in 1.3 i think in 99% of the cases i want trimming.
          Hide
          Alastair Maw added a comment -

          This bug is fixed - behaviour is back to 1.2.x-style and trunk prior to the patch that created this particular issue.

          If you think trimming whitespace is a bug then please open a separate issue for it.

          Show
          Alastair Maw added a comment - This bug is fixed - behaviour is back to 1.2.x-style and trunk prior to the patch that created this particular issue. If you think trimming whitespace is a bug then please open a separate issue for it.

            People

            • Assignee:
              Alastair Maw
              Reporter:
              Alastair Maw
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development