From: Jody G. <jod...@gm...> - 2010-09-21 01:53:09
|
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 > 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 > Geo...@li... > https://lists.sourceforge.net/lists/listinfo/geotools-devel |