Hi Niels; we really need to keep discussion on the email list (as while my
opinions may be helpful - they may not be correct).
You may indeed have found a hole in the api; performing the work in canHandle
method may still not be correct.
Here is another idea; modify the look up code to go through all the property
accessors; and short list XPathPropertyAccessor and ZPathPropertyAccessor. And
then it can try both of them; and then if one of them returns non null...
Jody
On 07/09/2010, at 8:28 AM, Niels wrote:
> Jody,
>
> I am still wondering about this.
>
> For example, say someone adds another PropertyAccessor, let's call it
> ZPropertyAccessor.
> It might come later in the list than XPathPropertyAccessor.
>
> The user specifies an attribute that can be evaluated by ZPropertyAccessor.
> However, the string can also be interpreted as a valid x-path, but it can't
> be evaluated as a property by the XPathPropertyAccessor.
>
> The XPathPropertyAccessor will say "yes, I can handle this". But throw an
> error as soon as it tries to evaluate it.
>
> ZPropertyAccessor will never be used. This will basically happen to any
> accessor after XPathPropertyAccessor.
>
> That is why I think that, despite the performace issue, XPathPropertyAccessor
> does have to check whether it can actually evaluate the property in the
> canHandle method.
>
> What do you think?
>
> Thanks
> Niels
>
> On 03/09/10 13:46, Niels wrote:
>>
>> Yes, you have a good point there about the performance because otherwise
>> canHandle and evaluate are basically doing two times the same thing (the
>> only way to check if the x-path points to a valid property is by trying to
>> evaluate it).
>>
>> However, considering the API I'm not sure if you're right. The API for
>> canHandle says "Can be used to perform checks against schema to ensure that
>> the propery accessor will actually work with the provided instance. "
>> SimpleFeaturePropertyAccessor does return canHandle=false for non-existing
>> properties.
>>
>> The problem is: What if other property accessors are added in the future?
>> They will never even be used if xpathpropertyaccessor comes before them in
>> the list, returning an exception when maybe this other property accessor
>> could still do the job. I think that is a theoretical possibility?
>>
>> Niels
>>
>> On 03/09/10 13:41, Jody Garnett wrote:
>>>
>>> Sounds good; one clarification for performance considerations.
>>>
>>> I am not sure the API will let you check the x-path against the feature or
>>> feature type; the canHandle method is supposed to be fast; and should just
>>> check that the xpath expression is valid.
>>>
>>> You will need to check that the xpath points to a property when the
>>> expression is evaluated; at that point you could throw an exception (or
>>> otherwise fail).
>>>
>>> Jody
>>>
>>> On 03/09/2010, at 3:11 PM, Niels wrote:
>>>
>>>> Hi,
>>>>
>>>> Thanks for the advice.
>>>>
>>>> So my plan of attack will be:
>>>>
>>>> 1. Implement the 'canHandle' of class 'XPathPropertyAcessor' to make it
>>>> return false when the x-path doesn't resolve to a property
>>>> 2. Throw an exception in AttributeExpressionImpl.evaluate if no property
>>>> accessor can be found.
>>>> 3. Go through all the geotools test cases to be compatible with the
>>>> changes.
>>>>
>>>> regards,
>>>> Niels
>>>>
>>>> On 03/09/10 11:40, Jody Garnett wrote:
>>>>>
>>>>> I expect that JD (ie Justin's) comment was with respect to preserving
>>>>> compatibility with the existing test cases when transitioning to the
>>>>> GeoTools 2.3 Filter implementation (ie with Property Accessors).
>>>>>
>>>>> Now that the transition is mostly completed; throwing a good
>>>>> exception is possible (but some work will be needed to update test
>>>>> cases).
>>>>>
>>>>> This GeoTools 2.3 Filter transition is our oldest "technical debt") in
>>>>> the library; there are a number of test cases that make use of the the
>>>>> FilterImpl method directly (rather than using filter factory).
>>>>>
>>>>> Jody
>>>>>
>>>>> On Fri, Sep 3, 2010 at 1:36 PM, Jody Garnett <[email protected]>
>>>>> wrote:
>>>>>
>>>>>> I think you could throw an exception there; and update the geotools test
>>>>>> cases.
>>>>>> Jody
>>>>>>
>>>>>>
>>>>>> On Thu, Sep 2, 2010 at 4:02 PM, Niels <[email protected]> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I am a new geotools developer who just started working on app-schema for
>>>>>>> CSIRO last week.
>>>>>>> I was asked to look at this problem:
>>>>>>> http://jira.codehaus.org/browse/GEOT-3066 ("No error is returned if I
>>>>>>> gave
>>>>>>> an invalid column name in app-schema mapping file").
>>>>>>>
>>>>>>> I discovered the issue is intertwined with other parts of the geotools
>>>>>>> code.
>>>>>>> The issue is in the evaluation of expressions, more specifically in the
>>>>>>> AttributeExpressionImpl class. The evaluate method will return a null
>>>>>>> value
>>>>>>> when it cannot evaluate the attribute expression. This makes it
>>>>>>> impossible
>>>>>>> for App-schema to distinguish between an actual attribute with a real
>>>>>>> null
>>>>>>> value and a non-existing attribute. I think an evaluation method should
>>>>>>> in
>>>>>>> principle distinguish between an expression that cannot be evaluated
>>>>>>> and an
>>>>>>> expression that evaluates to a null value. However, there are explicit
>>>>>>> comments in the code that say this would cause issues with backward
>>>>>>> compatibility:
>>>>>>>
>>>>>>> public Object evaluate(Object obj, Class target) {
>>>>>>> PropertyAccessor accessor = getPropertyAccessor(obj, target);
>>>>>>>
>>>>>>> if ( accessor == null ) {
>>>>>>> return null; //JD:not throwing exception to remain backwards
>>>>>>> compatabile, just returnign null
>>>>>>> }
>>>>>>> Object value = accessor.get( obj, attPath, target );
>>>>>>>
>>>>>>> if(target == null)
>>>>>>> return value;
>>>>>>>
>>>>>>> return Converters.convert( value, target );
>>>>>>>
>>>>>>> }
>>>>>>> Although I am not sure why, because the FunctionExpressionImpl class
>>>>>>> does
>>>>>>> throw exceptions when it cannot evaluate.
>>>>>>> I considered bypassing the evaluate method altogether from app-schema
>>>>>>> for
>>>>>>> the mapping, but this is not a serious option because expressions can be
>>>>>>> quite complex, with functions etc. and all that functionality cannot be
>>>>>>> lost.
>>>>>>>
>>>>>>> I think the most optimal solution that respects backward compatibility,
>>>>>>> would be to introduce some flag somewhere that can turn error checking
>>>>>>> on
>>>>>>> for the evaluation of attribute expressions (but is off by default). My
>>>>>>> first idea to implement this is : creating an alternative
>>>>>>> implementation of
>>>>>>> AttributeExpression, and then passing an optional boolean value to the
>>>>>>> CQL
>>>>>>> compiler that specifies which attribute expression implementation to
>>>>>>> use.
>>>>>>> Ben Caradoc-Davies however suggested the use of the 'Hints', but I am
>>>>>>> not
>>>>>>> familiar yet with how this exactly works.
>>>>>>>
>>>>>>> What are your thoughts?
>>>>>>>
>>>>>>> One more thing, another issue that would need to be resolved is in the
>>>>>>> XPathPropertyAccessor class in the xml package.
>>>>>>> When evaluating an attribute expression, an iteration through all
>>>>>>> property
>>>>>>> accessors determines which one to use.
>>>>>>> When an invalid column name is specified, he will end up with
>>>>>>> XPathPropertyAccessor as last choice. The problem is this code:
>>>>>>> JXPathContext context =
>>>>>>> JXPathContextFactory.newInstance().newContext(null, object);
>>>>>>> context.setLenient(true);
>>>>>>>
>>>>>>> The Lenient property will make sure that no exception is thrown if the
>>>>>>> xpath
>>>>>>> cannot be resolved and a null is returned instead, again making it
>>>>>>> impossible to distinguish between an error and a real null value. I
>>>>>>> don't
>>>>>>> know why it was chosen to do it this way?
>>>>>>> That too would need to change.
>>>>>>>
>>>>>>>
>>>>>>> With Regards,
>>>>>>> Niels
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> 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
>>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>> This SF.net Dev2Dev email is sponsored by:
>>>>>>>
>>>>>>> Show off your parallel programming skills.
>>>>>>> Enter the Intel(R) Threading Challenge 2010.
>>>>>>> http://p.sf.net/sfu/intel-thread-sfd
>>>>>>> _______________________________________________
>>>>>>> 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
>>>> ------------------------------------------------------------------------------
>>>> This SF.net Dev2Dev email is sponsored by:
>>>>
>>>> Show off your parallel programming skills.
>>>> Enter the Intel(R) Threading Challenge 2010.
>>>> http://p.sf.net/sfu/intel-thread-sfd_______________________________________________
>>>> 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
>
>
> --
> 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
------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:
Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd
_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
