Re: [Geotools-devel] breaking attribute access behaviour

[Jody G. <jod...@gm...>]

I understand the problem with using null to represent two cases; can you check the filter and xpath specifications to see how they handle this problem? Do they produce an error; or return an empty result? The answer may inform what we do here; perhaps they have thought of a good way to be clear?

I don't want to see a special "mode" added where geotools starts throwing exceptions. Other options include sending something to the log; although is itself frustrating.

I am pretty sure the case justin is worried about is client code that tries several xpath expressions in order to retrieve a value; if we start throwing exception their code would break. Even if we add stuff to the log it will be a bit of a pain as their program would be actings as expected.

I am thinking of something like:

Geometry geom;

geom = ff.propertyName(".").evaualte( feature, Geometry.class ); // try for a default geometry!
if( geom == null ){
   geom = ff.propertyName("the_geom").evaulate( feature, Geometry.class) ; // the_geom is a common geometry name
}
if( geom == null ){
   String name = feature.getSchema().getDefaultGeometry().getLocalName();
   geom = ff.propertyName( name ).evaulate( feature, Geometry.class );
}

I could totally see people writing something like the above as an optimisation; since looking up the correct default geometry name takes time after all. Throwing exceptions would break this style of code; and even logging cases where "the_geom" does not exist as a valid column name would be a problem.

The other place to do this is in the user interface responsible for producing the expression; this is how I handle SLD generation; provide the user help as they create the data structure?

If you have a builder object for your mapping file could you make use of additional information to prevent an invalid mapping file being produced? Allow it to throw exceptions of the filters being passed in do not match the data they are to run on?

Jody


On 21/09/2010, at 12:51 PM, Niels wrote:

> 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
>>> 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
>> 
> 
> 
> -- 
> 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
> Geo...@li...
> https://lists.sourceforge.net/lists/listinfo/geotools-devel