Opened 17 years ago

Closed 15 years ago

#1442 closed defect (fixed)

Feature.GetFieldAsInteger does not behave correctly on non existant field

Reported by: dpinte@… Owned by: Even Rouault
Priority: normal Milestone: 1.7.0
Component: OGR_SF Version: 1.3.2
Severity: normal Keywords:
Cc: warmerdam, hobu

Description (last modified by Mateusz Łoskot)

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().

Change History (10)

comment:3 by warmerdam, 17 years ago

Cc: warmerdam added
Description: modified (diff)
Milestone: 1.4.2
Owner: changed from warmerdam to Mateusz Łoskot
Priority: highestnormal

Mateusz,

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

comment:4 by Mateusz Łoskot, 17 years ago

Description: modified (diff)

comment:5 by Mateusz Łoskot, 17 years ago

Status: newassigned

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.

comment:6 by warmerdam, 17 years ago

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.

comment:7 by warmerdam, 17 years ago

Milestone: 1.4.21.5.0

comment:8 by Mateusz Łoskot, 17 years ago

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.

comment:9 by warmerdam, 17 years ago

Cc: hobu added

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?

comment:10 by hobu, 16 years ago

Milestone: 1.5.01.6.0

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.

comment:11 by Even Rouault, 15 years ago

Owner: changed from Mateusz Łoskot to Even Rouault
Status: assignednew

comment:12 by Even Rouault, 15 years ago

Milestone: 1.6.11.7.0
Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.