Re: Proper jsp:setProperty behavior for Tomcat 3.2
Larry Isaacs wrote:
When applying Gareth Morgan's bug fix for JspRuntimeLibrary in the Tomcat MAIN
branch, a difference between the tomcat_32 and MAIN versions revealed that
Tomcat 3.2 still has the bug discovered by Glenn Nielsen. This is where:
jsp:setProperty name="bean" property="prop" value=""/
leaves the property unchanged instead of setting it to an empty string. As Glenn
noted, the spec calls for ignoring an empty string when param is used, i.e.:
jsp:setProperty name="bean" property="prop" param="prop"/
with a query string of "?prop=". But doesn't say to ignore it for value. This
is easily fixed.
1) Please beware that the current fix to 3.3 did not quite do it properly
(I ran more tests this weekend).
It did fix the fact that an empty value is jsp:setProperty does go through
properly (i.e. the property is set using the empty string), but it introduced
the bug that an empty value for a request parameter also does go through
(while it should not).
However, its not clear to me what the spec calls for with respect to empty
strings and indexed properties. Also, Tomcat 3.2's current behavior seems
inconsistent on this issue. I tested the following cases where the
property is a String array:
1) jsp:setProperty name="bean" property="prop" param="prop"/
2) jsp:setProperty name="bean" property="prop"/
3) jsp:setProperty name="bean" property="*"/
For an empty query string, or a query string with mutlitple non-empty values,
you get the expected result. However, for the following query strings
which involve empty values, I get the following results for how the String
array is set.
Query String Result1 Result2 Result3 Spec
?prop="" "" not changed not changed
?prop=prop= "","" "","" not changed ???
?prop=prop=text "","text" "","text" not changed ???
I would expect the results to be the same instead of different. The spec
doesn't say that ignoring an empty value shouldn't apply to an indexed
property. But if that's true, what about "?prop=prop="? Should this be
ignored too? And should "?prop=prop=text" set the property to a single
element array containing "text"?
Am I missing something in the spec? Can someone shed some light on what
proper behavior should be?
I discussed the issue with Eduardo (JSP spec lead)
on Friday afternoon.
He agreed that the spec is unclear and has filed a bug
against the spec to trigger further discussions with the
expert group.
In the meantime, here is what we can do:
-
2) Bug fix: When using property="*", indexed properties are
not properly handled when
first property is null or empty
Your test cases exposed this bug. Only the first parameter of an
indexed property is considered when figuring out if it is
null or empty.
This is a simple fix. This will bring your test cases
to all have a consistent behavior (i.e. do not ignore the
request parameters for an indexed property as soon as first
parameter value is empty).
-
3) Pick one behavior for 'indexed properties' in the case of
empty values and document it properly.
I see 3 types of behavior that could make sense:
3A) All request parameter values are passed 'intact' as an array
to the indexed property setter method.
This means that the following query strings would yield the following
arrays being passed to the setter method:
query string: ?prop=prop=oneprop=prop=threeprop=prop=
array: {"","one","","three","",""}
query string: ?prop=prop=prop=prop=prop=prop=
array: {"","","","","",""}
3B) Same behavior as 3A), with the exception that the setter
method with the array argument is called only if at least
one parameter value is non-empty.
query string: ?prop=prop=oneprop=prop=threeprop=prop=
array: {"","one","","three","",""}
query string: ?prop=prop=prop=prop=prop=prop=
- setter method not called; indexed property unchanged
3C) Only set the properties that have a non-empty value
query string: ?prop=prop=oneprop=prop=threeprop=prop=
setIndexed(1, "one")
setIndexed(3, "three")
query string: ?prop=prop=prop=prop=prop=prop=
- no setter called
-
3A is identical in behavior as what happens when an indexed property
is set with the "value" attribute in jsp:setProperty
jsp:setProperty property="indexed" value=%=new String[]{"","",""}%/
3C has the same behavior as for non-indexed properties; only non-empty
values in request parameters trigger a call to the setter method
for the specific index.
3B is half-way solution: it has the same behavior as for non-indexed
properties only if all values of the indexed property are empty.
I'm +1 on fixing the jsp:setProperty name="bean" property="prop" value=""/
bug in Tomcat 3.2. If the desired behavior for indexed properties can be
determined, I
RE: Proper jsp:setProperty behavior for Tomcat 3.2
I would argue that in such a case, the developer should not use an "indexed" property, but rather a simple property that takes a String array as argument. (i.e. the only difference with an "indexed property" is that the index setter method would not be defined). I had not noticed the distinction of a simple property that takes an array, and one that with an indexed setter becomes a true "indexed" property. I thought that if the setter setprop() took an array as an argument, it was considered an indexed property. [I'd personally propose to the expert group to throw an exception if a request parameter is specified for an indexed property] Consider the following where ColorBean has a "colors" property which takes String array: === some HTML === ... form action="a.jsp" method="get" input type="checkbox" name="colors" value="red"/Red input type="checkbox" name="colors" value="green"/Green input type="checkbox" name="colors" value="blue"/Blue input type="submit"/ /form ... === a.jsp === ... jsp:useBean id="mycolors" class="ColorBean"/ jsp:setProperty name="mycolors" property="colors"/ ... I wouldn't expect this to throw an exception if ColorBean had an index setter and not throw one if it didn't. It's not clear to me from the spec, how important the index of an indexed property is intended to be. It only says that the setter for an indexed property needs to take an array. With respect to the "multi-valued request parameter" case, I'd personally vote for throwing an exception. As a developer I'd rather know right up front that something is fishy rather that find out about it much later (after losing a bunch of hair). [especially if the order in which parameters are reported is unreliable] +1 But this is something that should be decided by the expert group, and for the time being we should leave the behavior as it currently stands in tomcat. Agreed. With all this being said, whether or not the empty values are passed in the array for the setter method then does not become as important an issue as before. Only passing the non-empty values is however nicer. I'll try to commit a patch that fixes just the following: 1) Fix value="" without allowing param="..." to set. 2) For a String array property, make "?prop=prop=text" set the property for property="*". Currently property="*" handling assumes only simple properties and skips the set because the first argument is empty. Since proper behavior with respect to empty array element is still unclear, I'll wait on the removal of empty array elements. This should be addressed in a separate patch anyway. Cheers, Larry
Re: Proper jsp:setProperty behavior for Tomcat 3.2
Larry Isaacs wrote: ... I'll try to commit a patch that fixes just the following: 1) Fix value="" without allowing param="..." to set. 2) For a String array property, make "?prop=prop=text" set the property for property="*". Currently property="*" handling assumes only simple properties and skips the set because the first argument is empty. Since proper behavior with respect to empty array element is still unclear, I'll wait on the removal of empty array elements. This should be addressed in a separate patch anyway. Sounds good. Thanks. -- Pierre
Proper jsp:setProperty behavior for Tomcat 3.2
When applying Gareth Morgan's bug fix for JspRuntimeLibrary in the Tomcat MAIN branch, a difference between the tomcat_32 and MAIN versions revealed that Tomcat 3.2 still has the bug discovered by Glenn Nielsen. This is where: jsp:setProperty name="bean" property="prop" value=""/ leaves the property unchanged instead of setting it to an empty string. As Glenn noted, the spec calls for ignoring an empty string when param is used, i.e.: jsp:setProperty name="bean" property="prop" param="prop"/ with a query string of "?prop=". But doesn't say to ignore it for value. This is easily fixed. However, its not clear to me what the spec calls for with respect to empty strings and indexed properties. Also, Tomcat 3.2's current behavior seems inconsistent on this issue. I tested the following cases where the property is a String array: 1) jsp:setProperty name="bean" property="prop" param="prop"/ 2) jsp:setProperty name="bean" property="prop"/ 3) jsp:setProperty name="bean" property="*"/ For an empty query string, or a query string with mutlitple non-empty values, you get the expected result. However, for the following query strings which involve empty values, I get the following results for how the String array is set. Query String Result1 Result2 Result3 Spec ?prop="" "" not changed not changed ?prop=prop= "","" "","" not changed ??? ?prop=prop=text "","text" "","text" not changed ??? I would expect the results to be the same instead of different. The spec doesn't say that ignoring an empty value shouldn't apply to an indexed property. But if that's true, what about "?prop=prop="? Should this be ignored too? And should "?prop=prop=text" set the property to a single element array containing "text"? Am I missing something in the spec? Can someone shed some light on what proper behavior should be? I'm +1 on fixing the jsp:setProperty name="bean" property="prop" value=""/ bug in Tomcat 3.2. If the desired behavior for indexed properties can be determined, I can try to fix that too. Cheers, Larry __ Larry Isaacs [EMAIL PROTECTED] SAS Institute Inc.
Re: Proper jsp:setProperty behavior for Tomcat 3.2
Larry Isaacs wrote: I'm +1 on fixing the jsp:setProperty name="bean" property="prop" value=""/ bug in Tomcat 3.2. +1 If the desired behavior for indexed properties can be determined, I can try to fix that too. Sent email to Eduardo to try to get a clarification. -- Pierre
Re: Proper jsp:setProperty behavior for Tomcat 3.2
Larry Isaacs wrote:
When applying Gareth Morgan's bug fix for JspRuntimeLibrary in the Tomcat MAIN
branch, a difference between the tomcat_32 and MAIN versions revealed that
Tomcat 3.2 still has the bug discovered by Glenn Nielsen. This is where:
jsp:setProperty name="bean" property="prop" value=""/
leaves the property unchanged instead of setting it to an empty string. As Glenn
noted, the spec calls for ignoring an empty string when param is used, i.e.:
jsp:setProperty name="bean" property="prop" param="prop"/
with a query string of "?prop=". But doesn't say to ignore it for value. This
is easily fixed.
+1 to incorporate my setProperty value="" jasper 3.3dev patch into 3.2
glenn 00/08/07 12:04:37
We should also merge in to 3.2 the below patch I did for jasper in the 3.3dev
branch.
Modified:src/share/org/apache/jasper/compiler TagLibraryInfoImpl.java
Log:
According to the JSP 1.1 spec a TLD tag attribute required and rtexprvalue
elements should accept "yes" in addition to "true". Jasper was not
recoginizing "yes" as meaning "true". Patched.
Revision ChangesPath
1.25 +11 -5
jakarta-tomcat/src/share/org/apache/jasper/compiler/TagLibraryInfoImpl.java
Index: TagLibraryInfoImpl.java
===
RCS file:
/home/cvs/jakarta-tomcat/src/share/org/apache/jasper/compiler/TagLibraryInfoImpl.java,v
retrieving revision 1.24
retrieving revision 1.25
diff -u -r1.24 -r1.25
--- TagLibraryInfoImpl.java 2000/08/04 14:57:19 1.24
+++ TagLibraryInfoImpl.java 2000/08/07 19:04:37 1.25
@@ -1,7 +1,7 @@
/*
- * $Header:
/home/cvs/jakarta-tomcat/src/share/org/apache/jasper/compiler/TagLibraryInfoImpl.java,v
1.24
2000/08/04 14:57:19 jiricka Exp $
- * $Revision: 1.24 $
- * $Date: 2000/08/04 14:57:19 $
+ * $Header:
/home/cvs/jakarta-tomcat/src/share/org/apache/jasper/compiler/TagLibraryInfoImpl.java,v
1.25
2000/08/07 19:04:37 glenn Exp $
+ * $Revision: 1.25 $
+ * $Date: 2000/08/07 19:04:37 $
*
* The Apache Software License, Version 1.1
*
@@ -492,12 +492,18 @@
name = t.getData();
} else if (tname.equals("required")) {
Text t = (Text) e.getFirstChild();
-if (t != null)
+if (t != null) {
required = Boolean.valueOf(t.getData()).booleanValue();
+ if( t.getData().equalsIgnoreCase("yes") )
+ required = true;
+ }
} else if (tname.equals("rtexprvalue")) {
Text t = (Text) e.getFirstChild();
-if (t != null)
+if (t != null) {
rtexprvalue =
Boolean.valueOf(t.getData()).booleanValue();
+if( t.getData().equalsIgnoreCase("yes") )
+rtexprvalue = true;
+ }
} else if (tname.equals("type")) {
Text t = (Text) e.getFirstChild();
if (t != null)
--
Glenn Nielsen [EMAIL PROTECTED] | /* Spelin donut madder|
MOREnet System Programming | * if iz ina coment. |
Missouri Research and Education Network | */ |
--
