Velocity Tools
  1. Velocity Tools
  2. VELTOOLS-88

LinkTool could easily be modified to support current-request parameter duplication

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.x, 1.4, 2.x
    • Component/s: None
    • Labels:
      None

      Description

      When implementing a pager a long time ago, I created a subclass of the StrutsLinkTool that added two methods: ignore(String paramName) and addAllParameters(). Together, they could be used to copy all the parameters from the current request into the link currently being built, and you could ignore a parameter from the current request using the ignore method.

      I hadn't seen any demand for such a feature out in the wild, so I kept it to myself.

      Now that I'm looking into Struts 2, I can see that their link builder tool has the same capability. Since that team decided to add this feature, I'm offering to add it to the LinkTool, which will provide the same capability to all subclasses such as StrutsLinkTool and SecureLinkTool.

      The change would not affect any existing code out in the wild: this would merely be a new method which supports the new feature. Its use is entirely optional and should not disturb any other code.

      The behavior of the Struts 2 link builder is to replace any current-request parameter explicitly set in the link builder, while my implementation requires that you explicitly ignore those parameters. My enhancement can follow either strategy (and I'd appreciate feedback on this item).

      1. LinkToolTests.java
        10 kB
        Christopher Schultz
      2. VELTOOLS-88a.diff
        4 kB
        Christopher Schultz
      3. VELTOOLS-88.diff
        2 kB
        Christopher Schultz

        Activity

        Hide
        Christopher Schultz added a comment -

        Patch to implement this feature. It implements the ignore/add capability rather than the auto-ignore as described in the initial description.

        Any and all comments are appreciated.

        Show
        Christopher Schultz added a comment - Patch to implement this feature. It implements the ignore/add capability rather than the auto-ignore as described in the initial description. Any and all comments are appreciated.
        Hide
        Nathan Bubna added a comment -

        Nice feature. It sorta overlaps the self() method when selfIncludeParameters is changed to true, but as the default for that is false, this is a cleaner way to add them than doing $link.params($request.parameterMap), especially with the ability to ignore certain params. That said, here's some critiques before we put this in:

        • The self() method will need to respect the paramsToIgnore when it is configured to include params.
        • I'm not thrilled about using addQueryData(name,value) for each individual parameter. If there's a lot of params, that will needlessly create and discard a lot of LinkTool clones. I think it would be better if it copied the request.getParameterMap(), removed any params to be ignored from that and then passed the result to params(Map).
        • The javadoc references ignore(String) which is not in the patch.
        • addIgnore() should return a copy, not this. Which means you'll probably need to add a new copyWithIgnore(String) method and make sure that the paramsToIgnore are always copied to new copies.

        Sorry if that seems like a lot, but i'd like to avoid introducing inconsistencies in how LinkTool behaves. speaking of which, i just noticed a few things in it that need attention.

        let me know if you have questions about my feedback. i'd be happy to help, as i think this is a nice new feature.

        Show
        Nathan Bubna added a comment - Nice feature. It sorta overlaps the self() method when selfIncludeParameters is changed to true, but as the default for that is false, this is a cleaner way to add them than doing $link.params($request.parameterMap), especially with the ability to ignore certain params. That said, here's some critiques before we put this in: The self() method will need to respect the paramsToIgnore when it is configured to include params. I'm not thrilled about using addQueryData(name,value) for each individual parameter. If there's a lot of params, that will needlessly create and discard a lot of LinkTool clones. I think it would be better if it copied the request.getParameterMap(), removed any params to be ignored from that and then passed the result to params(Map). The javadoc references ignore(String) which is not in the patch. addIgnore() should return a copy, not this. Which means you'll probably need to add a new copyWithIgnore(String) method and make sure that the paramsToIgnore are always copied to new copies. Sorry if that seems like a lot, but i'd like to avoid introducing inconsistencies in how LinkTool behaves. speaking of which, i just noticed a few things in it that need attention. let me know if you have questions about my feedback. i'd be happy to help, as i think this is a nice new feature.
        Hide
        Christopher Schultz added a comment -

        The use of addQueryData for each of the parameters was a carryover from the implementation of StrutsLinkTool, which was intended to use its superclass as a black box. I will look at the use of the params() method to streamline this code.

        I will make the other changes and re-submit.

        I have a unit test working without too much funny business. I'll include that unit test along with the patch.

        Show
        Christopher Schultz added a comment - The use of addQueryData for each of the parameters was a carryover from the implementation of StrutsLinkTool, which was intended to use its superclass as a black box. I will look at the use of the params() method to streamline this code. I will make the other changes and re-submit. I have a unit test working without too much funny business. I'll include that unit test along with the patch.
        Hide
        Christopher Schultz added a comment -

        Okay, I've made all those changes, and I think I fixed a bug along the way. The existing implementation of getSelf() had this at the bottom:

        if (this.selfParams)

        { dupe.params(request.getParameterMap()); }

        return dupe;

        Note that the parameters copied in the dupe.params() call will be entirely lost, since dupe.params() returns a copy. The code is useless.

        My new implementation properly does:

        dupe = dupe.copyWith(params);

        ...which should result in the proper behavior.

        Another odd thing I noticed in the class... the duplicate() method makes arrangements for non-cloneable implementations of LinkTool, yet LinkTool itself implements Cloneable. Why should we have to have any backup code in case LinkTool suddenly isn't cloneable?

        Show
        Christopher Schultz added a comment - Okay, I've made all those changes, and I think I fixed a bug along the way. The existing implementation of getSelf() had this at the bottom: if (this.selfParams) { dupe.params(request.getParameterMap()); } return dupe; Note that the parameters copied in the dupe.params() call will be entirely lost, since dupe.params() returns a copy. The code is useless. My new implementation properly does: dupe = dupe.copyWith(params); ...which should result in the proper behavior. Another odd thing I noticed in the class... the duplicate() method makes arrangements for non-cloneable implementations of LinkTool, yet LinkTool itself implements Cloneable. Why should we have to have any backup code in case LinkTool suddenly isn't cloneable?
        Hide
        Christopher Schultz added a comment -

        Replaced deleted patch for historical pusposes.

        Show
        Christopher Schultz added a comment - Replaced deleted patch for historical pusposes.
        Hide
        Christopher Schultz added a comment -

        Updated patch addresses Nathan's concerns.

        Take a look.

        Show
        Christopher Schultz added a comment - Updated patch addresses Nathan's concerns. Take a look.
        Hide
        Christopher Schultz added a comment -

        Unit test to exercise new "ignore" and "add all parameters" capability.

        Show
        Christopher Schultz added a comment - Unit test to exercise new "ignore" and "add all parameters" capability.
        Hide
        Nathan Bubna added a comment -

        Ok, i've committed the patch and the tests with a few modifications (mostly stylistic, but also a few simplifications) to the trunk (1.x branch) and ported them over to the 2.x branch with a few more tweaks, including a refactor of addAllParameters() that i'll port back to 1.x momentarily...

        Regarding the overzealous backup code in LinkTool.duplicate()... yeah, it's probably unnecessary. feel free to post a patch to excise it, if you feel so inclined.

        Show
        Nathan Bubna added a comment - Ok, i've committed the patch and the tests with a few modifications (mostly stylistic, but also a few simplifications) to the trunk (1.x branch) and ported them over to the 2.x branch with a few more tweaks, including a refactor of addAllParameters() that i'll port back to 1.x momentarily... Regarding the overzealous backup code in LinkTool.duplicate()... yeah, it's probably unnecessary. feel free to post a patch to excise it, if you feel so inclined.
        Hide
        Christopher Schultz added a comment -

        Fix verified.

        Show
        Christopher Schultz added a comment - Fix verified.

          People

          • Assignee:
            Unassigned
            Reporter:
            Christopher Schultz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development