Commons Digester
  1. Commons Digester
  2. DIGESTER-3

[digester] CallMethodRule doesn't call with null parameters, String-ifies them first.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: All

      Description

      Hi all!

      First, thank you for creating such great software as the whole Jakarta project.

      I have a very specific problem regarding Commons Digester, in particular
      CallMethodRule

      The javadoc states:
      --------------

      • If a CallMethodRule is expecting more than one parameter,
      • then it is always invoked, regardless of whether the parameters were
      • available or not (missing parameters are passed as null values).</p>
        ---------------

      Please note the last phrase: "missing parameters are passed as null values".

      This is an excerpt from the logs of my application (client class names had been
      cut out):

      -----------------
      DEBUG main org.apache.commons.digester.Digester - Fire end() for
      CallMethodRule[methodName=setUtenzeBT, paramCount=5,
      paramTypes=

      {java.lang.Float, java.lang.Integer, java.lang.Float, java.lang.Integer, java.lang.Float}

      ]
      DEBUG main org.apache.commons.digester.Digester - Popping params
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule](0)null
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule](1)null
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule](2)null
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule](3)null
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule](4)null
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Convert string 'null' to
      class 'java.lang.Float'
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Using converter
      org.apache.commons.beanutils.converters.FloatConverter@288051
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Convert string 'null' to
      class 'java.lang.Integer'
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Using converter
      org.apache.commons.beanutils.converters.IntegerConverter@10045eb
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Convert string 'null' to
      class 'java.lang.Float'
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Using converter
      org.apache.commons.beanutils.converters.FloatConverter@288051
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Convert string 'null' to
      class 'java.lang.Integer'
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Using converter
      org.apache.commons.beanutils.converters.IntegerConverter@10045eb
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Convert string 'null' to
      class 'java.lang.Float'
      DEBUG main org.apache.commons.beanutils.ConvertUtils - Using converter
      org.apache.commons.beanutils.converters.FloatConverter@288051
      DEBUG main org.apache.commons.digester.Digester - [CallMethodRule]

      {rl/i/ria}

      Call
      it.setUtenzeBT(0.0/java.lang.Float,0/java.lang.Integer,0.0/java.lang.Float,0/java.lang.Integer,0.0/java.lang.Float)
      DEBUG main org.apache.commons.beanutils.MethodUtils - Matching name=setUtenzeBT
      on class it
      DEBUG main org.apache.commons.beanutils.MethodUtils - Found straight match:
      public void
      it.setUtenzeBT(java.lang.Float,java.lang.Integer,java.lang.Float,java.lang.Integer,java.lang.Float)
      throws it.DataValidationException
      DEBUG main org.apache.commons.beanutils.MethodUtils - isPublic:true
      DEBUG main org.apache.commons.digester.Digester - Fire end() for
      SetNextRule[methodName=add, paramType=java.lang.Object]
      -----------------

      If you look at the first lines, you see the five "[CallMethodRule](0)null"
      entries. That's correct, because the data passed in hasn't got the inner element
      needed for the parameter.

      But, after that, instead of calling it.setUtenzeBT(null, null, null, null,
      null), Digester calls
      it.setUtenzeBT(0.0/java.lang.Float,0/java.lang.Integer,0.0/java.lang.Float,
      0/java.lang.Integer,0.0/java.lang.Float), that is with zeros, having converted
      the nulls to Strings before, and then to Floats.

      I think the problem lies around here:

      From:
      /jakarta/commons/proper/digester/tags/DIGESTER_1_6/src/java/org/apache/commons/digester/CallMethodRule.java
      Revision: 132717

      -----------------
      // Construct the parameter values array we will need
      // We only do the conversion if the param value is a String and
      // the specified paramType is not String.
      Object paramValues[] = new Object[paramTypes.length];
      for (int i = 0; i < paramTypes.length; i++) {
      // convert nulls and convert stringy parameters
      // for non-stringy param types
      if(
      parameters[i] == null ||
      (parameters[i] instanceof String &&
      !String.class.isAssignableFrom(paramTypes[i])))

      { paramValues[i] = ConvertUtils.convert((String) parameters[i], paramTypes[i]); }

      else

      { paramValues[i] = parameters[i]; }

      }
      -----------------

      I don't think 'parameters[i] == null' should be ||'ed with the other condition.
      It should call convert only when a real object gets passed. I could be wrong,
      however. It's the first time I glance at Commons' source code, and even if it is
      extra clear, I may miss something.

      However, I think the logged behaviour doesn't match what advertised in the
      javadocs, and I need the latter one.
      I'v searched BugZilla without finding anything similar to this, so I posted this
      bug.
      If more info/test data is needed, I'll prepare it (that means i'll have to
      delete the details I can't make public).

      Thanks in advance to anyone who can help me.

      Michele Mauro

        Activity

        Hide
        Simon Kitching added a comment -

        Javadoc updated, so I'm closing this entry.

        Show
        Simon Kitching added a comment - Javadoc updated, so I'm closing this entry.
        Hide
        Michele Mauro added a comment -

        Registering the Converters with my preferred default (null) was really trivial,
        and now I have 2 more tests passing
        There's no need of additional CallMethodRule parameters; just correct the
        javadoc, maybe adding a brief reference to this discussion (if you want nulls,
        register an appropriate Converter).

        Thanks again.

        Mauro Michele

        Show
        Michele Mauro added a comment - Registering the Converters with my preferred default (null) was really trivial, and now I have 2 more tests passing There's no need of additional CallMethodRule parameters; just correct the javadoc, maybe adding a brief reference to this discussion (if you want nulls, register an appropriate Converter). Thanks again. Mauro Michele
        Hide
        Michele Mauro added a comment -

        Thanks for the prompt answer!

        I'll give it a try, and test which option is the easiest for me. My app is a
        standalone client, so I don't have issues with other components in the same JVM.

        As far as if "(String) parameters[i] doesn't generate a string when
        parameters[i] is null", well, I'm not as sure as you are that I knew it already
        . If you make me think about it, yes, it seems logical to me (that a cast
        doesn't generate an Object), but when I read the code I didn't noticed it. I was
        just thinking, "nulls shouldn't be converted, so the conditions shouldn't be in
        the if clause"... However, I didn't know that passing through ConvertUtils you
        can set your preferred defaults for the conversion.

        I think I'll test the Custom Converter first, and if I have time, try to add an
        option to CallMethodRule. So, on to the BeanUtils site I go!

        You can consider this to be a 'documentation bug', and close it with the javadoc
        correction.

        If you think it's worth it, you can make the flag thing an RFE with low priority.

        Thank you very much for your good work.

        Mauro Michele

        Show
        Michele Mauro added a comment - Thanks for the prompt answer! I'll give it a try, and test which option is the easiest for me. My app is a standalone client, so I don't have issues with other components in the same JVM. As far as if "(String) parameters [i] doesn't generate a string when parameters [i] is null", well, I'm not as sure as you are that I knew it already . If you make me think about it, yes, it seems logical to me (that a cast doesn't generate an Object), but when I read the code I didn't noticed it. I was just thinking, "nulls shouldn't be converted, so the conditions shouldn't be in the if clause"... However, I didn't know that passing through ConvertUtils you can set your preferred defaults for the conversion. I think I'll test the Custom Converter first, and if I have time, try to add an option to CallMethodRule. So, on to the BeanUtils site I go! You can consider this to be a 'documentation bug', and close it with the javadoc correction. If you think it's worth it, you can make the flag thing an RFE with low priority. Thank you very much for your good work. Mauro Michele
        Hide
        Simon Kitching added a comment -

        Just a little more detail. The constructor for ConvertUtilsBean calls the
        "deregister" function to build up its default set of converters. And the
        converters registered there for Integer.class, Float.class, etc. are set up to
        return "default" values of 0, 0.0 etc. when passed nulls.

        Note, by the way, that the code
        (String) parameters[i]
        doesn't generate a string when parameters[i] is null; the cast still results in
        a null reference (I'm sure you knew that already).

        BTW, when I stated that "ConvertUtils is a Singleton" it's actually a
        singleton-per-context-classloader, so it is possible to register custom
        converters in a webapp without affecting other webapps.

        Show
        Simon Kitching added a comment - Just a little more detail. The constructor for ConvertUtilsBean calls the "deregister" function to build up its default set of converters. And the converters registered there for Integer.class, Float.class, etc. are set up to return "default" values of 0, 0.0 etc. when passed nulls. Note, by the way, that the code (String) parameters [i] doesn't generate a string when parameters [i] is null; the cast still results in a null reference (I'm sure you knew that already). BTW, when I stated that "ConvertUtils is a Singleton" it's actually a singleton-per-context-classloader, so it is possible to register custom converters in a webapp without affecting other webapps.
        Hide
        Simon Kitching added a comment -

        I think you're right that the javadoc does not match the code (my mistake; I
        wrote that piece of javadoc!).

        However the current behaviour (nulls get passed through ConvertUtils) is
        deliberate, and can't be changed.

        I will correct the javadoc.

        To address your issue, one option is to register custom ConvertUtils converters
        that leave nulls unchanged. This way, digester calls ConvertUtils.convert, but a
        null just comes straight back. See the BeanUtils library, method
        ConvertUtils.register for more details. Note that setting a converter on
        ConvertUtils changes global behaviour for the whole JVM (ConvertUtils is a
        Singleton) so if you are using any other libs that use ConvertUtils you may need
        to think carefully about the implications of doing this.

        The other option would be to add a flag to the CallMethodRule class to indicate
        whether it should use the current behaviour or leave nulls alone. If you wish to
        provide a patch for this (and unit tests) I would be happy to review and commit
        it. Or I might get around to implementing this sometime (it seems a worthy
        request) but I can't give you any particular date this might be done by.

        Show
        Simon Kitching added a comment - I think you're right that the javadoc does not match the code (my mistake; I wrote that piece of javadoc!). However the current behaviour (nulls get passed through ConvertUtils) is deliberate, and can't be changed. I will correct the javadoc. To address your issue, one option is to register custom ConvertUtils converters that leave nulls unchanged. This way, digester calls ConvertUtils.convert, but a null just comes straight back. See the BeanUtils library, method ConvertUtils.register for more details. Note that setting a converter on ConvertUtils changes global behaviour for the whole JVM (ConvertUtils is a Singleton) so if you are using any other libs that use ConvertUtils you may need to think carefully about the implications of doing this. The other option would be to add a flag to the CallMethodRule class to indicate whether it should use the current behaviour or leave nulls alone. If you wish to provide a patch for this (and unit tests) I would be happy to review and commit it. Or I might get around to implementing this sometime (it seems a worthy request) but I can't give you any particular date this might be done by.

          People

          • Assignee:
            Unassigned
            Reporter:
            Michele Mauro
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development