Ticket #1442 (assigned defect)

Opened 1 year ago

Last modified 7 months ago

Feature.GetFieldAsInteger does not behave correctly on non existant field

Reported by: dpinte@itae.be Assigned to: mloskot (accepted)
Priority: normal Milestone: 1.6.0
Component: OGR_SF Version: 1.3.2
Severity: normal Keywords:
Cc: warmerdam, hobu

Description (Last modified by mloskot)

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

04/04/07 23:26:09 changed by warmerdam

  • priority changed from highest to normal.
  • owner changed from warmerdam to mloskot.
  • description changed.
  • cc set to warmerdam.
  • milestone set to 1.4.2.

Mateusz,

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

04/08/07 17:12:39 changed by mloskot

  • description changed.

04/17/07 22:16:40 changed by mloskot

  • status changed from new to assigned.

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.

04/18/07 09:41:47 changed 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.

04/18/07 09:41:59 changed by warmerdam

  • milestone changed from 1.4.2 to 1.5.0.

04/18/07 11:18:08 changed by 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.

04/18/07 23:14:48 changed by warmerdam

  • cc changed from warmerdam to warmerdam, hobu.

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?

12/18/07 13:42:15 changed by hobu

  • milestone changed from 1.5.0 to 1.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.