Stripes

options-collection tag is rendering incorrectly when inside the loop

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.5.1
  • Component/s: Tag Library
  • Labels:
    None
  • Environment:
    JDK 1.6.0_07, Resin 3.1.2

Description

Code in question (class InputOptionsCollectionTag, method doEndTag(), lines 345,346) :

this.value = null;
this.label = null;

Problem description:

I have the following JSP code:

<c:forEach ...>
...
<stripes:select ...>
<stripes:options-collection collection="some_collection_here" label="name" value="id" />
</stripes:select>
...
</c:forEach>

As a result of the JSP translation Resin generates the following java code:

while (_jsp_iter_269.hasNext()) {

...skip...

if (_jsp_InputOptionsCollectionTag_18 == null) { _jsp_InputOptionsCollectionTag_18 = new net.sourceforge.stripes.tag.InputOptionsCollectionTag(); _jsp_InputOptionsCollectionTag_18.setPageContext(pageContext); _jsp_InputOptionsCollectionTag_18.setParent((javax.servlet.jsp.tagext.Tag) _jsp_InputSelectTag_17); _jsp_InputOptionsCollectionTag_18.setLabel("name"); _jsp_InputOptionsCollectionTag_18.setValue("id"); }

_jsp_InputOptionsCollectionTag_18.setCollection(_caucho_expr_5.evalObject(_jsp_env));
_jsp_InputOptionsCollectionTag_18.doStartTag();
_jsp_InputOptionsCollectionTag_18.doEndTag();

...skip...

}

As you can see setLabel(), setValue() are only called once in the loop right after tag instance is lazily instantiated inside "if" statement.
Once doEndTag() method is executed during the first iteration "value" and "label" properties are nulled and then the next iteration results in incorrect HTML rendering.

Apparently Resin applies the same paradigm of tag initialization to every jsp tag, so this particular problem may potentially exist with other stripes's tags if they have similar "clean up" code at the end of the doEndTag().

It appears that Stripes 1.4.3 does not have this problem and problem only appeared in 1.5

I have not tested this particular test case with other web containers, and not sure if this sort of the "optimization" performed by Resin is appropriate, but on the other hand from the pure theoretical point of view since we do not control initialization of the tag properties shouldn't we be cleaning them ourselves?

Activity

Hide
Nick Yantikov added a comment - 29/Oct/08 2:15 AM

For the sake of the others who run into the same problem here is the workaround that seems to be suppressing Resin's optimization and forces setValue() and setLabel() be called on every loop iteration:

<c:forEach...>
<c:set var="resinFixName" value="name"/>
<c:set var="resinFixId" value="id"/>
<stripes:select ...>
<stripes:options-collection collection="${some_collection_here}" label="${resinFixName}" value="${resinFixId}" />
</stripes:select>
</c:forEach>

Show
Nick Yantikov added a comment - 29/Oct/08 2:15 AM For the sake of the others who run into the same problem here is the workaround that seems to be suppressing Resin's optimization and forces setValue() and setLabel() be called on every loop iteration: <c:forEach...> <c:set var="resinFixName" value="name"/> <c:set var="resinFixId" value="id"/> <stripes:select ...> <stripes:options-collection collection="${some_collection_here}" label="${resinFixName}" value="${resinFixId}" /> </stripes:select> </c:forEach>
Hide
Frederic Daoud added a comment - 29/Oct/08 5:14 AM

I've also had tag pooling-related problems with Resin. I verified that these problems do not occur with Tomcat and Jetty.

Maybe I'm misunderstanding something, but when you say "shouldn't we be cleaning them ourselves?", isn't that what we're doing by nulling the properties of the tag after we're done? Isn't the problem the other way around, that Resin isn't initializing the tag properly for each iteration of the loop?

Just to add some info, in my case, the problem occurs not when I have the tag in a loop, but rather when I have more than one <stripes:option-collection> tag in the same page. Probably the same end result. Resin sees that the label= and name= properties are the same and thinks that it's appropriate to set them only once.

Or maybe we /shouldn't/ be nulling the values? I don't know enough about the intricacies of tag pooling to know the answer.

Show
Frederic Daoud added a comment - 29/Oct/08 5:14 AM I've also had tag pooling-related problems with Resin. I verified that these problems do not occur with Tomcat and Jetty. Maybe I'm misunderstanding something, but when you say "shouldn't we be cleaning them ourselves?", isn't that what we're doing by nulling the properties of the tag after we're done? Isn't the problem the other way around, that Resin isn't initializing the tag properly for each iteration of the loop? Just to add some info, in my case, the problem occurs not when I have the tag in a loop, but rather when I have more than one <stripes:option-collection> tag in the same page. Probably the same end result. Resin sees that the label= and name= properties are the same and thinks that it's appropriate to set them only once. Or maybe we /shouldn't/ be nulling the values? I don't know enough about the intricacies of tag pooling to know the answer.
Hide
Frederic Daoud added a comment - 29/Oct/08 5:48 AM

My understanding is that we should be putting the properties back to the way they were, not to null. That is, if we change the value of the label= and name= properties to do some work, we should remember the original value and set the properties back to the original value when we're done.

In the case of InputOptionsCollectionTag it doesn't look like we're changing the values of the properties themselves, but instead we're using local variables. So maybe it's just a matter of removing these lines:

this.collection = null;
this.value = null;
this.label = null;

Show
Frederic Daoud added a comment - 29/Oct/08 5:48 AM My understanding is that we should be putting the properties back to the way they were, not to null. That is, if we change the value of the label= and name= properties to do some work, we should remember the original value and set the properties back to the original value when we're done. In the case of InputOptionsCollectionTag it doesn't look like we're changing the values of the properties themselves, but instead we're using local variables. So maybe it's just a matter of removing these lines: this.collection = null; this.value = null; this.label = null;
Hide
Nick Yantikov added a comment - 29/Oct/08 11:52 PM

Frederic, I do not want to pollute JIRA with my theoretical reasoning. In short what I was trying to say is that I think Stripes should not be nulling those properties. In my opinion the fix is in removing these lines as you suggested but I am going to leave it up to you to decide.

It would also be great if you guys could verify that other Stripes tags do not follow the same pattern.

Show
Nick Yantikov added a comment - 29/Oct/08 11:52 PM Frederic, I do not want to pollute JIRA with my theoretical reasoning. In short what I was trying to say is that I think Stripes should not be nulling those properties. In my opinion the fix is in removing these lines as you suggested but I am going to leave it up to you to decide. It would also be great if you guys could verify that other Stripes tags do not follow the same pattern.
Hide
Frederic Daoud added a comment - 03/Nov/08 8:39 AM

Hi Nick,

I've committed a fix in the 1.5.x branch, SVN build 996. Could you please retest?

Thanks!
Frederic Daoud

Show
Frederic Daoud added a comment - 03/Nov/08 8:39 AM Hi Nick, I've committed a fix in the 1.5.x branch, SVN build 996. Could you please retest? Thanks! Frederic Daoud
Hide
Nick Yantikov added a comment - 15/Nov/08 9:17 PM

Tested against 1.5.x branch. It's working fine now

Thanks a lot.

Show
Nick Yantikov added a comment - 15/Nov/08 9:17 PM Tested against 1.5.x branch. It's working fine now Thanks a lot.
Hide
Frederic Daoud added a comment - 15/Nov/08 9:30 PM

Thanks for verifying the fix.

Show
Frederic Daoud added a comment - 15/Nov/08 9:30 PM Thanks for verifying the fix.

People

Vote (0)
Watch (0)

Dates

  • Created:
    29/Oct/08 2:06 AM
    Updated:
    15/Nov/08 9:30 PM
    Resolved:
    03/Nov/08 8:39 AM