Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      I have a use case which can't be done with the Digester's standard rules. (I
      hope I haven't overlooked Digester's API.) After some experiment, I realized
      with some enhancement I could make CallMethodRule/CallParamRule work. I think
      the use case is pretty common and generic.

      The use case is like this: I need to call a 3rd party API

      Parent.add(Child child, Condition condition)

      with the XML data:

      <parent>
      <child .../>
      <condition .../>
      </parent>

      I can't use SetNextRule as it takes only one object from the stack.
      CallMethodRule/CallParamRule comes close, but it allows only parameters coming
      from body texts or attributes, not generic objects from the stack.

      So, I made the following changes to CallMethodRule/CallParamRule:

      • Changed the param stack frame datatype from String[] to Object[]
      • Overloaded CallParamRule to allow configuration to get the parameter from
        the stack
      • Made CallParamRule stateless so that it works probably in nesting

        Activity

        Hide
        John Yu added a comment -

        Created an attachment (id=3109)
        Diff of CallMethodRule

        Show
        John Yu added a comment - Created an attachment (id=3109) Diff of CallMethodRule
        Hide
        John Yu added a comment -

        Created an attachment (id=3110)
        Diff of CallParamRule

        Show
        John Yu added a comment - Created an attachment (id=3110) Diff of CallParamRule
        Hide
        John Yu added a comment -

        Created an attachment (id=3111)
        Diff of Digester

        Show
        John Yu added a comment - Created an attachment (id=3111) Diff of Digester
        Hide
        John Yu added a comment -

        It seems the fix from Bug#11693 has addressed (partly?) the last issue that
        CallMethodRule didn't work correctly in nested situation.

        Show
        John Yu added a comment - It seems the fix from Bug#11693 has addressed (partly?) the last issue that CallMethodRule didn't work correctly in nested situation.
        Hide
        Emmanuel Bourg added a comment -

        CallMethodRule should work fine in any nested situation now. I haven't tried
        mixed nestings like the following though

        <foo id="f1">
        <bar id="b1">
        <foo id="f2">
        <bar id="b2"/>
        </foo>
        </bar>
        </foo>

        Is there any other nesting scenario ?

        Show
        Emmanuel Bourg added a comment - CallMethodRule should work fine in any nested situation now. I haven't tried mixed nestings like the following though <foo id="f1"> <bar id="b1"> <foo id="f2"> <bar id="b2"/> </foo> </bar> </foo> Is there any other nesting scenario ?
        Hide
        John Yu added a comment -

        I haven't tried the latest build with the fix from Bug#11693 yet.

        However, this bug report is more than fixing the nesting bug. I'm suggesting to
        enchance CallMethodRule/CallParamRule such that it can use objects of arbitrary
        type from the stack as parameters. Currently, only xml attributes or body
        contents, both of which are strings, can be used as parameters.

        Show
        John Yu added a comment - I haven't tried the latest build with the fix from Bug#11693 yet. However, this bug report is more than fixing the nesting bug. I'm suggesting to enchance CallMethodRule/CallParamRule such that it can use objects of arbitrary type from the stack as parameters. Currently, only xml attributes or body contents, both of which are strings, can be used as parameters.
        Hide
        Christopher Lenz added a comment -

        Created an attachment (id=3234)
        Test case for the enhancement

        Show
        Christopher Lenz added a comment - Created an attachment (id=3234) Test case for the enhancement
        Hide
        Christopher Lenz added a comment -

        I second this feature request.

        I'm not 100% sure why you would ever want to specify stackPos other than 0, but
        other than that I like the provided patch.

        I've attached a test case for this feature. The test case will add a new file
        CallMethodRuleTestCase.java, which contains all the CallMethodRule/CallParamRule
        related tests, some of which moved from RuleTestCase.java, and added an entry to
        build.xml. (this is because I believe that the tests concentrating on a single
        rule should be in their own dedicated test case).

        Show
        Christopher Lenz added a comment - I second this feature request. I'm not 100% sure why you would ever want to specify stackPos other than 0, but other than that I like the provided patch. I've attached a test case for this feature. The test case will add a new file CallMethodRuleTestCase.java, which contains all the CallMethodRule/CallParamRule related tests, some of which moved from RuleTestCase.java, and added an entry to build.xml. (this is because I believe that the tests concentrating on a single rule should be in their own dedicated test case).
        Hide
        John Yu added a comment -

        Thanks for the test case!

        You're right that in practice you don't need stackPos other than 0.
        I added that in so that I could overload CallMethodRule's constructor without
        clashing with an existing constructor signature. Very dirty indeed. >

        Show
        John Yu added a comment - Thanks for the test case! You're right that in practice you don't need stackPos other than 0. I added that in so that I could overload CallMethodRule's constructor without clashing with an existing constructor signature. Very dirty indeed. >
        Hide
        Christopher Lenz added a comment -

        Then how about making that

        public CallParamRule(int paramIndex, boolean fromStack)

        i.e. using a boolean instead of the stackPos.

        The motivation would be that the stackPos adds complexity that is not needed in
        practice, both from the API client and the API maintainers perspective. What do
        you think?

        Show
        Christopher Lenz added a comment - Then how about making that public CallParamRule(int paramIndex, boolean fromStack) i.e. using a boolean instead of the stackPos. The motivation would be that the stackPos adds complexity that is not needed in practice, both from the API client and the API maintainers perspective. What do you think?
        Hide
        rdonkin@apache.org added a comment -

        i've committed a fix for this issue.

        the patch i committed is substantially that provide by John Yu but i have replaced stackPos
        with the boolean fromStack suggested by Christopher Lenz. i think that there's a good
        chance that someone sooner or later will end up needing the extra functionality provided
        orginally but using fromStack wlll simplify maintenance and make the API more intuitive.
        we can always add it later if someone has a pressing need.

        the comments on this issue by all parties were extremely helpful. my thanks to everyone
        who contributed.

        it'd be cool if someone submitted a documentation patch describing this new functionality.

        • robert
        Show
        rdonkin@apache.org added a comment - i've committed a fix for this issue. the patch i committed is substantially that provide by John Yu but i have replaced stackPos with the boolean fromStack suggested by Christopher Lenz. i think that there's a good chance that someone sooner or later will end up needing the extra functionality provided orginally but using fromStack wlll simplify maintenance and make the API more intuitive. we can always add it later if someone has a pressing need. the comments on this issue by all parties were extremely helpful. my thanks to everyone who contributed. it'd be cool if someone submitted a documentation patch describing this new functionality. robert
        Hide
        Henri Yandell added a comment -

        (Fixing JIRA import error)

        Show
        Henri Yandell added a comment - (Fixing JIRA import error)

          People

          • Assignee:
            Unassigned
            Reporter:
            John Yu
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development