The problem with separating 'canExecute' from 'execute' is that it
wouldn't solve the performance issue. To test whether a x-path can be
evaluated, the only way is often to actually try to evaluate it, which
would mean you would be doing the same thing twice if they are in
separate methods. The way I solved it is to catch any exception that is
thrown by 'execute' and keep trying until no exception is thrown. This
way nothing is done twice, and only what is necessary is done.
The problem with returning null for a non-existing attribute is that it
is still perfectly possible that an attribute does exist but it has no
value. So null can mean two things:
- the attribute doesn't exist
- the attribute does exist but doesn't have a value
These two cases should be distinguishable. Otherwise, app-schema can
never provide proper error reporting on invalid attributes in the
mapping file (it cannot assume that because the attribute has no value -
the null case - that it doesn't exist!).
So if people make a small typo they might get really confused on why it
is not working - unless geotools tell them their attribute does not exist.
It would be a good idea to at least give the option of distinguishing
between these two cases, while remaining backward compatibility for
those who don't want it.
With Regards,
Niels
On 21/09/10 09:52, Jody Garnett wrote:
Thanks for taking the discussion to the email list Justin; I did warn
the patch author that discussion needed to be public (and would have
to wait until after foss4g).
The one thing I like about the suggestion is not hard coding the
jxpath case as a fall back position.
The concerns I had were:
- performance implications (not sure if that has been measured)
- doing too much work in the canHandle method (this may be the same
performance consideration). I like the idea of canHandle looking at
the expression and short listing ... I don't want it to try and do all
the work each time
- use of exception for flow control. Returning null is not really
unambiguous either; perhaps introducing another method would work?
Am I correct in thinking the current implementation is broken with
respect to the bug report? Or is the bug report itself wrong? Should
we expect an error to be returned when an invalid column name is
provided? Or should we just return null to indicate that there was
nothing available for the provided xpath expression?
Currently we have:
canHandle( String xpath ) - used to shortlist
execute( String xpath ) - access the content; may return null if
content is not available
Adding an extra method would look like:
canHandle( String xpath ) - used to short list
canExecute( String xpath ) - used to test if the content is there
execute( String xpath ) - used to access the content; may return null
if the content is null (or is not available)
For reference the definition of PropertyIsNull is
The *<PropertyIsNull> *element encodes an operator that checks to see
if the value of its content is NULL. A NULL is equivalent to no value
present. The value 0 is a valid value and is not considered NULL.
Section 6.3 has a whole bunch of stuff on the xpath expressions
suitable for use with PropertyName; however they are all positive
examples showing how to query existing content; what we need is an
example showing how to fail. I think the assumption of returning
"null" when the content is not available at the requested xpath
expression is a good one (it does represent that no information is
present after all; and the PropertyIsNull filter is available to make
use of this information).
Jody
On 21/09/2010, at 11:18 AM, Justin Deoliveira wrote:
Hi all,
Recently this patch has been proposed:
http://jira.codehaus.org/browse/GEOT-3066
And this issue was discussed in detail on the mailing list some time
ago in this thread:
http://osgeo-org.1803224.n2.nabble.com/evaluating-invalid-attribute-names-td5489979.html
Apologies for not weighing in then. But I think this has some pretty
serious backward compatibility issues. The issue/proposal is to throw
an exception vs return null when a type has no such attribute
described by an attribute name or xpath, rather than return null. The
thread above proposes to just change the behaviour and update any
test cases that rely on it. I could not disagree more with his
approach. This breaks backward compatibility and i think requires
more discussion. There is much more use of geotools beyond test cases
and even beyond geoserver and udig. If there is one thing i learned
at foss4g it is that there are lots of people using geotools, even
though they don't take parts in discussions on the developer list.
So i think this change requires more discussion and probably a
proposal. And it certainly can't take place on stable branch imo.
Is there any way we can have this behaviour engage only when the user
explicitly requests it? That has been a pattern that has worked very
well in the past for such changes.
-Justin
--
Justin Deoliveira
OpenGeo - http://opengeo.org <http://opengeo.org/>
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
--
*Niels Charlier*
Software Engineer
CSIRO Earth Science and Resource Engineering
Phone: +61 8 6436 8914
Australian Resources Research Centre
26 Dick Perry Avenue, Kensington WA 6151
------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
