On Thu, May 26, 2011 at 7:06 PM, sebb <seb...@gmail.com> wrote:
> On 26 May 2011 14:46, Rahul Akolkar <rahul.akol...@gmail.com> wrote:
>> On Thu, May 26, 2011 at 9:24 AM, sebb <seb...@gmail.com> wrote:
>>> On 23 May 2011 16:22, Rahul Akolkar <rahul.akol...@gmail.com> wrote:
>>>> On Mon, May 23, 2011 at 8:16 AM, sebb <seb...@gmail.com> wrote:
>>>>> SCXML has had a couple of ongoing Gump failures, which I started to
>>>>> look at again.
>>>>>
>>>>
>>>> Thanks, the initial breakage was from EL trunk changes, if you follow
>>>> this trail (if you've begun to look into it, this probably won't be
>>>> new information):
>>>>
>>>> http://markmail.org/message/v2ioxdq2s7c62mtl
>>>>
>>>> Ofcourse, EL isn't active and I haven't looked into it.
>>>
>>> Progress so far:
>>>
>>> The test case breaks because the condition needed to change states is
>>> not set up.
>>>
>>> In particular the following line in jsp/datamodel-01.xml:
>>>
>>> <assign location="${Data(maindata,'root/foo/bar')}" expr="baz" />
>>>
>>> causes the error:
>>>
>>> SEVERE: Attempt to convert String "baz" to type "org.w3c.dom.Node",
>>> but there is no PropertyEditor for that type
>>>
>>> It looks like the handling of assignment statements changes with the
>>> new version of EL.
>>>
>>> I've not yet established what the EL change is that causes the error.
>>> I suppose it's possible that the actual error is in SCXML, and EL code
>>> now reveals it.
>>>
>> <snip/>
>>
>> Right, but certainly doesn't look like EL is a drop-in replacement --
>> I don't know if thats what the EL devs were aiming for :-) We are
>> attempting the conversion in the message above, which is possible when
>> using EL 1.0.
>
> The cause is https://issues.apache.org/jira/browse/EL-17.
>
> Previously, pLogger.isLoggingError() always returned true, whereas
> log.isErrorEnabled() only returns true if error logging is enabled.
> The default error logging is fatal, which prevents the ELException
> from being thrown when doing the conversion.
> If you set the log level to error or info, the tests pass - as can be
> seen in the Gump run:
>
> http://vmgump.apache.org/gump/public/apache-commons/commons-scxml-test/gump_work/build_apache-commons_commons-scxml-test.html
>
> The Gump run works, because I enabled info logging in order to try and
> debug the code ...
>
<snip/>
Thanks for digging into this.
Lets separate the two items clearly:
(a) whether the EL behavior change in r133240 is desired, and
(b) how the piece of SCXML code in question may be improved.
WRT (a) then, as you say in EL-17, the commit message seems to imply
the change is pretty innocuous so it may not be the case that the
change in behavior was actually desired. IMO behavior WRT throwing
exceptions changing based on logging level is not a good pattern. It
seems the relevant portions in EL should be reverted.
> I'm not entirely sure whether there is a bug in SCXML or not.
> Currently SCXML relies on an Exception being thrown for certain
> conversions - e.g. String to Node - when doing an Assign.
> The Assign.execute method has this clause:
>
> } catch (SCXMLExpressionException see) {
> // or something else, stuff toString() into lvalue
> Object valueObject = eval.eval(ctx, expr);
> SCXMLHelper.setNodeValue(oldNode, valueObject.toString());
> }
>
> This is rather fragile, as there are potentially quite a few
> conditions that can cause the exception.
>
> Seems to me it would be better to use some other way of establishing
> whether the assignment is intended to replace the node or set its
> value, but perhaps that would be rather difficult?
>
<snap/>
WRT (b), the WG is aware of the above fragility and has already
addressed this in the latest SCXML WD (Section D.3.5). The
implementation will have to follow the new clearer pattern when it
gets updated.
In summary, I think the EL change in question needs to be revisited,
if anyone is still around to look at EL. If not, EL seems a good
candidate for dormancy and subsequent removal of use of EL trunk from
Gump runs.
-Rahul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
