Opened 13 years ago

Closed 11 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 Changed 13 years ago by warmerdam

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 Changed 13 years ago by Mateusz Łoskot

Description: modified (diff)

comment:5 Changed 13 years ago by Mateusz Łoskot

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 Changed 13 years ago by 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.

comment:7 Changed 13 years ago by warmerdam

Milestone: 1.4.21.5.0

comment:8 Changed 13 years ago by Mateusz Łoskot

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 Changed 13 years ago by warmerdam

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 Changed 12 years ago by hobu

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 Changed 11 years ago by Even Rouault

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

comment:12 Changed 11 years ago by Even Rouault

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.