Struts 2
  1. Struts 2
  2. WW-3010

s:iterator fails to iterate over collections containing null

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.14
    • Fix Version/s: 2.5
    • Component/s: None
    • Labels:
      None
    • Environment:

      any

      Description

      When using the struts2 taglib's iterator tag to iterate over a collection which contains nulls, the current value ("id") is not set to null, but to the value it had in the last iteration before. This behaviour is explicitly coded without any obvious reason. See IteratorComponent.java from line 219:

                  if ((id != null) && (currentValue != null)) {
                      //pageContext.setAttribute(id, currentValue);
                      //pageContext.setAttribute(id, currentValue, PageContext.REQUEST_SCOPE);
                      stack.getContext().put(id, currentValue);
                  }
      

      Expected behaviour: just iterate over the null values as a plain java iterator would.

      If nulls are forbidden for some important reason, it should throw an execption, but not return a wrong value.

        Issue Links

          Activity

          Hide
          Aleksandr Mashchenko added a comment -

          Fixed in WW-4312

          Show
          Aleksandr Mashchenko added a comment - Fixed in WW-4312
          Hide
          ASF subversion and git services added a comment -

          Commit 1d3d7be4668e66314f7385a2b73f1c3a7dff66dd in struts's branch refs/heads/master from Victor Sosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=1d3d7be ]

          Fix for WW-4312

          A problem on Iterator tag

          and WW-3010 s:iterator fails to iterate over collections containing
          null

          Show
          ASF subversion and git services added a comment - Commit 1d3d7be4668e66314f7385a2b73f1c3a7dff66dd in struts's branch refs/heads/master from Victor Sosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=1d3d7be ] Fix for WW-4312 A problem on Iterator tag and WW-3010 s:iterator fails to iterate over collections containing null
          Hide
          Mark B added a comment -

          It is generally understood, in the programming world, that users of undocumented "features" are absolutely free to use said features but that they do so at their own peril.

          At worst this is a bug, and should be fixed.
          At best, an "undocumented feature". (And that would be very hard to argue as this is documented as an iterator, a notion that transcends APIs and programming languages.)

          In any case, when it finally works according to the documentation no good developer should be surprised, and no well tested or written app should stop working.

          Show
          Mark B added a comment - It is generally understood, in the programming world, that users of undocumented "features" are absolutely free to use said features but that they do so at their own peril. At worst this is a bug, and should be fixed. At best, an "undocumented feature". (And that would be very hard to argue as this is documented as an iterator, a notion that transcends APIs and programming languages.) In any case, when it finally works according to the documentation no good developer should be surprised, and no well tested or written app should stop working.
          Hide
          Daniel Baldes added a comment -

          Well, every bug is bad behavior where someone could possibly rely on... of course there are bugs where the "incorrectness" of the behavior is less obvious, but I don't see why this just-plain-wrong behavior shouldn't be fixed right away. Whatever.

          Show
          Daniel Baldes added a comment - Well, every bug is bad behavior where someone could possibly rely on... of course there are bugs where the "incorrectness" of the behavior is less obvious, but I don't see why this just-plain-wrong behavior shouldn't be fixed right away. Whatever.
          Hide
          Dave Newton added a comment -

          But apps coded to expect "correct" behavior will break, because "correct" behavior isn't what actually happens. Code changes that break existing behavior, "correct" or not, can't just be thrown in willy-nilly, "dangerous mentality" or not.

          Show
          Dave Newton added a comment - But apps coded to expect "correct" behavior will break, because "correct" behavior isn't what actually happens. Code changes that break existing behavior, "correct" or not, can't just be thrown in willy-nilly, "dangerous mentality" or not.
          Hide
          Mark B added a comment - - edited

          Musachy, I think this is a dangerous mentality. There are probably many more apps coded expecting correct behavior than coded to rely on incorrect behavior.

          If we are iterating over a list whose first element is a null, we should get a null as the first value.

          Show
          Mark B added a comment - - edited Musachy, I think this is a dangerous mentality. There are probably many more apps coded expecting correct behavior than coded to rely on incorrect behavior. If we are iterating over a list whose first element is a null, we should get a null as the first value.
          Hide
          musachy added a comment -

          I agree the current value ("id") should be set to null, but changing would potentially break current apps, so this should probably be fixed for 2.2 or so.

          Show
          musachy added a comment - I agree the current value ("id") should be set to null, but changing would potentially break current apps, so this should probably be fixed for 2.2 or so.

            People

            • Assignee:
              Unassigned
              Reporter:
              Daniel Baldes
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development