Re: EL issues and 6.0.x release

[Konstantin Kolinko <kn...@gmail.com>]

2010/1/31 Mark Thomas <ma...@apache.org>:
> On 30/01/2010 17:41, Mark Thomas wrote:
>> On 30/01/2010 07:33, Konstantin Kolinko wrote:
>>> Regarding the implementation, AttributeParser.java class. I think
>>> that, based on the above, we can fix it to solve bug 48627. Other
>>> parts of the new implementation will remain unchanged.
>>
>> I'll take another look at this. I thought that this wouldn't work but
>> that may because I was doing my testing before I fixed the EL parsing.
>> If this doesn't work I have an alternative plan.
>
> Looks like it will work. Just running the TCK to be sure.
>
>>> 1. In JSP 2.1 spec there is an option to selectively disable '#'
>>> expressions when '$' ones are still enabled. The name of that option
>>> is "deferredSyntaxAllowedAsLiteral".
>>>
>>> As of now, AttributeParser takes care of isELIgnored option, but does
>>> not know about deferredSyntaxAllowedAsLiteral one.
>>
>> Probably a bug. We should write some test cases for this first though to
>> check.
>
> Yep bug. Test cases written. Fixed. Just running the TCK to be sure.
>
>>> 3. EL spec (ch.1.2.3 of EL 2.1 spec) says that "It is illegal to mix
>>> ${} and #{} constructs in a composite expression." though followed by
>>> "This restriction may be lifted in future versions".
>>>
>>> AttributeParser#parseLiteral() has the following clause:
>>>
>>> } else if (ch == type){
>>>
>>> I think it has to process '#' and '$' expressions in the same way, and
>>> the "mix ${} and #{}" rule should be checked either explicitly here,
>>> or elsewhere. I have not researched the question where it is actually
>>> checked.
>>
>> More tests cases required.
>
> Test case added. This is already handled by the EL impl.
>

If we  have "#{foo}${bar}" then parseEL() won't be triggered for the
${bar} part, and so won't correctly parse quotes etc.

Though probably that makes not much of a difference.  I think
${foo}#{bar  will be treated as an error regardless of what garbage or
not that bar is .

As of now
    <tags:echo echo="${foo}#{bar}" />
    <tags:echo echo="${foo}#{bar" />
or "legal"
    <tags:echo echo="#{foo}" />

all result in the same compile-time exception:  "According to TLD or
attribute directive in tag file, attribute echo does not accept any
expressions"

The "any expressions" part of the message text is wrong.

All that is a minor/cosmetic issue. I won't care much if it is
postponed for later or not fixed in a while at all.


>>> Lastly,
>>> when Mark was testing TC7 with JSP 2.2 TCK, he caught several minor EL
>>> evaluation issues. Those are fixed in TC7, and I think some of them
>>> have to be backported to TC6.
>>
>> They all need back-porting. I didn't propose them at the time since the
>> issues had existing for all of the 6.0.x release and no-one had
>> complained. I didn't want to hold up the 6.0.24 release.
>
> Proposed.
>
> Assuming the TCK passes I'll have a fix for 46827 and
> deferredSyntaxAllowedAsLiteral shortly.
>

One error there (for AttributeParser.java of trunk at rev.904949):

In parseEL() the literalQuote variable should be moved outside the
while (i < size && !endEL) loop.  Currently its value is not preserved
among iterations.

E.g.
    <tags:echo echo="${\"foo\"}\\${'bar'}" />
    <tags:echo echo="${'foo'}\\${\"bar\"}" />
should both evaluate to foo\bar,  but now the first one evaluates to foo${'bar'}


The rest is OK. I do not see any other errors in the implementation.

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org