Tiles
  1. Tiles
  2. TILES-339

JSP tags do not reset attributes when reused

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1.0
    • Fix Version/s: 2.0.7, 2.1.1
    • Labels:
      None
    • Flags:
      Important

      Description

      It is simply a misinterpretation of the Tag.release method:
      http://java.sun.com/j2ee/1.4/docs/api/javax/servlet/jsp/tagext/Tag.html#release()
      Attribute reset must be done in the "doEndTag"!

      1. tiles-2.1-reset.patch
        23 kB
        Zach Bailey
      2. tiles-jsp-tags-reset.patch
        17 kB
        Zach Bailey
      3. TilesTag.java
        2 kB
        Zach Bailey
      4. TilesBodyTag.java
        2 kB
        Zach Bailey

        Issue Links

          Activity

          Hide
          Antonio Petrelli added a comment -

          Closed due to the release of Tiles 2.0.7.

          Show
          Antonio Petrelli added a comment - Closed due to the release of Tiles 2.0.7.
          Hide
          Antonio Petrelli added a comment -

          Applied patch posted by Zach Bailey. Merged to TILES_2_0_X branch too.
          Thanks a lot Zach, it was very well written!

          Show
          Antonio Petrelli added a comment - Applied patch posted by Zach Bailey. Merged to TILES_2_0_X branch too. Thanks a lot Zach, it was very well written!
          Hide
          Zach Bailey added a comment -

          The other file missed by the patch - TilesBodyTag

          Show
          Zach Bailey added a comment - The other file missed by the patch - TilesBodyTag
          Hide
          Zach Bailey added a comment -

          TilesTag.java which the patch missed

          Show
          Zach Bailey added a comment - TilesTag.java which the patch missed
          Hide
          Zach Bailey added a comment -

          Argh, somehow Eclipse missed the two unversioned files (TilesTag and TilesBodyTag) when generating this latest patch. I'll go ahead and attach them individually. These should go in the base taglib package.

          Show
          Zach Bailey added a comment - Argh, somehow Eclipse missed the two unversioned files (TilesTag and TilesBodyTag) when generating this latest patch. I'll go ahead and attach them individually. These should go in the base taglib package.
          Hide
          Zach Bailey added a comment -

          Yep, here is that same changeset patched against trunk.

          filename: tiles-jsp-tags-reset.patch

          Show
          Zach Bailey added a comment - Yep, here is that same changeset patched against trunk. filename: tiles-jsp-tags-reset.patch
          Hide
          Antonio Petrelli added a comment -

          Can you create a patch against the trunk please?

          Show
          Antonio Petrelli added a comment - Can you create a patch against the trunk please?
          Hide
          Zach Bailey added a comment -

          Here's my first attempt at a patch which incorporates the changes I outlined above.

          It is against the 2.1.0 release tag in subversion.

          Let me know if I missed/broke anything, or if this needs to be reworked.

          Show
          Zach Bailey added a comment - Here's my first attempt at a patch which incorporates the changes I outlined above. It is against the 2.1.0 release tag in subversion. Let me know if I missed/broke anything, or if this needs to be reworked.
          Hide
          Antonio Petrelli added a comment -

          +1 to everything, Zach.
          I thought that TryCatchFinally interface was too much, but in fact it is the best approach to ensure that the reset method is always called.

          Show
          Antonio Petrelli added a comment - +1 to everything, Zach. I thought that TryCatchFinally interface was too much, but in fact it is the best approach to ensure that the reset method is always called.
          Hide
          Zach Bailey added a comment - - edited

          I am thinking of the following modifications:

          1.) Add an abstract TilesTag and TilesBodyTag which extend TagSupport and BodyTagSupport and both implement TryCatchFinally.
          2.) Existing tag classes which extend TagSupport will now extend TilesTag. Existing tag classes which extend BodyTagSupport will extend TilesBodyTag.
          3.) Both TilesTag and TilesBodyTag will implement doCatch() in the following manner:

          public void doCatch(Throwable throwable) throws Throwable

          { throw throwable; }

          This is a default (no-op) implementation and will result in no change in handling exceptions thrown from tags.

          4.) Both TilesTag and TilesBodyTag will implement doFinally in the following manner:

          public void doFinally()

          { //reset any per-invocation resources reset(); }

          5.) TilesTag and TilesBodyTag will now define the reset method:

          /**

          • Resets the state of the tag, preparing it for another invocation.
          • Called every invocation after doEndTag via {@link TryCatchFinally#doFinally()}

            .
            */
            protected void reset() { }

          6.) The release() method will be defined as follows on TilesTag and TilesBodyTag:

          /**

          • Release all allocated resources.
            */
            @Override
            public void release() { super.release(); reset(); }

          7.) All logic found in existing release() methods will be moved to the reset() method and the release() methods removed.

          Does anyone see any pitfalls or problems with this approach, or see a better way to resolve this issue?

          Show
          Zach Bailey added a comment - - edited I am thinking of the following modifications: 1.) Add an abstract TilesTag and TilesBodyTag which extend TagSupport and BodyTagSupport and both implement TryCatchFinally. 2.) Existing tag classes which extend TagSupport will now extend TilesTag. Existing tag classes which extend BodyTagSupport will extend TilesBodyTag. 3.) Both TilesTag and TilesBodyTag will implement doCatch() in the following manner: public void doCatch(Throwable throwable) throws Throwable { throw throwable; } This is a default (no-op) implementation and will result in no change in handling exceptions thrown from tags. 4.) Both TilesTag and TilesBodyTag will implement doFinally in the following manner: public void doFinally() { //reset any per-invocation resources reset(); } 5.) TilesTag and TilesBodyTag will now define the reset method: /** Resets the state of the tag, preparing it for another invocation. Called every invocation after doEndTag via {@link TryCatchFinally#doFinally()} . */ protected void reset() { } 6.) The release() method will be defined as follows on TilesTag and TilesBodyTag: /** Release all allocated resources. */ @Override public void release() { super.release(); reset(); } 7.) All logic found in existing release() methods will be moved to the reset() method and the release() methods removed. Does anyone see any pitfalls or problems with this approach, or see a better way to resolve this issue?

            People

            • Assignee:
              Antonio Petrelli
              Reporter:
              Antonio Petrelli
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development