Feature.GetFieldAsInteger does not behave correctly on non existant field

[dpinte@…]

Using Feature.GetFieldAsInteger? on a inexistant field in the feature, answer is always 0. I guess it should report a ValueError? like Feature.GetField? (or even an AttributeError?).

Demonstration :

did@geru-itae:~/Documents/ucl/alert/python$ python
Python 2.4.4 (#2, Jan 13 2007, 17:50:26)
[GCC 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ogr
>>> dataset = ogr.Open('alert/test/scenario1/polygones.shp')
>>> layer = dataset.GetLayer()
>>> feature = layer.GetNextFeature()
>>> print "Test with an existing field"
Test with an existing field
>>> feature.GetFieldIndex('cat')
0
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('cat'))
1
>>> print "Test with a non existant field"
Test with a non existant field
>>> feature.GetFieldIndex('id')
-1
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('id'))
0
>>> feature.GetField(feature.GetFieldIndex('id'))
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/usr/lib/python2.4/site-packages/ogr.py", line 854, in GetField
    return _gdal.OGR_F_GetField( self._o, fld_index )
ValueError: Illegal field requested in GetField().

[warmerdam]

Mateusz,

Chat with me about what you think the solution should be before implementing it for this.

[mloskot]

First, I confirm the problem still exists (see attached script, easier to test).

None of GetFieldAs?* functions from the Feature class include testing if field index value is valid.

Currently, user has no chance to find out there is something wrong because everything behaves silently. I think users should be notified that requested field is not available in a feature attributes.

Summarizing, I think it's a good idea to throw ValueError? if requested field does not exist.

[warmerdam]

Mateusz,

Did you have a plan how to address this? I'm inclined to modify the c++ methods on OGFeature to issue a CPLError() if the field is out of range. I believe that the swig bindings will automatically turn this into an exception depending on the rules for the language. In python I believe CPLErrors are not turned into exceptions by default, but things can be put into that mode if desired.

I do not think this changes should go into 1.4 branch.

[mloskot]

Frank,

My first thought was related to the Traditional Python and here I've been thinking about manual hacking in gdal_wrap.c:

if( iField < 0 || iField >= OGR_F_GetFieldCount( hFeat ) )
{
   PyErr_SetString(PyExc_ValueError,
                   "Illegal field requested in GetField()." );
   return NULL;	
}

For the NG Python, the idea of automatic issue of Python exception on CPLError() sounds good to me. Also, I think it's a good idea to consult it with Hobu.

[warmerdam]

Mateusz,

I do not think that we want to address this in the bindings code, and we certainly don't want to bother with updates to the old generation python bindings.

Howard, can you verify that the ng bindings can turn a CPLError into an exception if we issue a CPLError down in the C code?

[hobu]

I have confirmed that the ng bindings behave similarly:

>>> from osgeo import gdal
>>> from osgeo import ogr
>>> ds = ogr.Open('/Users/hobu/svn/gdalautotest/ogr/data/asm.shp')
>>> layer = ds.GetLayer(0)
>>> feature = layer.GetNextFeature()
>>> feature
<osgeo.ogr.Feature; proxy of <Swig Object of type 'OGRFeatureShadow *' at 0x34b2f0> >
>>> feature.GetFieldIndex('GEO_ID')
5
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('GEO_ID'))
16
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('id'))
0
>>> ogr.UseExceptions()
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('id'))
0
>>> feature.GetField(feature.GetFieldIndex('junk'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "build/bdist.macosx-10.5-i386/egg/osgeo/ogr.py", line 1835, in GetField
ValueError: Illegal field requested in GetField()
>>> feature.GetFieldAsInteger(feature.GetFieldIndex('junk'))
0

I think this stuff should be handled at the C/C++ level so that bindings can decide what to do with the error. I'm not sure how to go about it though. I am deferring this bug until 1.6.

[rouault]

r16845: Issue a CPLError if OGRFeatureDefn::GetFieldDefn?() is called with an invalid index

At the beginning I had thought of adding the error in OGRFeatureDefn::GetFieldIndex?( const char * pszFieldName ), but many OGR drivers use this method to detect if a field exists or not, so it was not appropriate to put it there.