Ticket #2146 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

Geoconcept Open() not careful enough

Reported by: warmerdam Owned by: dgrichard
Priority: normal Milestone: 1.6.0
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: geoconcept
Cc: warmerdam, rouault

Description

It seems that the geoconcept open function is not careful enough when trying to determine if a dataset is in fact a geoconcept file/fileset.

eg. 

warmerda@gdal64[75]% ogrinfo gdal
OGR: OGROpen(gdal/0x607320) succeeded as Geoconcept Export.
INFO: Open of `gdal'
      using driver `Geoconcept Export' successful.
INFO: Internal data source name `gdal/LICENSE.TXT'
      different from user name `gdal'.
OGR: GetLayerCount() = 0

(in this case gdal is my source directory).

Attachments

000_GRD.dbf Download (0.6 KB) - added by dgrichard 5 years ago.
Shape for unit test (DBF)
000_GRD.shp Download (0.8 KB) - added by dgrichard 5 years ago.
Shape for unit test (SHP)
000_GRD.shx Download (140 bytes) - added by dgrichard 5 years ago.
Shape for unit test (SHX)
geoconcept-20081301.patch Download (94.6 KB) - added by dgrichard 5 years ago.
geoconcept-20080120.patch Download (165.6 KB) - added by dgrichard 5 years ago.
patch against trunk with better handling of read and SRS and lco CONFIG optional
expected_tile.mid Download (350 bytes) - added by dgrichard 5 years ago.
Mapinfo for unit test (mid)
expected_tile.mif Download (1.4 KB) - added by dgrichard 5 years ago.
Mapinfo for unit test (mif)
ogrgc.sh Download (6.8 KB) - added by dgrichard 5 years ago.
geoconcept unit tests shell script
geoconcept-20080129.patch Download (75.0 KB) - added by dgrichard 5 years ago.
patch against trunk with various enhancements (bug fix for feature definition, round-tripping of SRS complete, spatial and attribute filters added, better support for update, FID support, etc ...)
geoconcept-20080202.patch Download (1.5 KB) - added by dgrichard 5 years ago.
Correct reading of unknown SRS
geoconcept-20080223.patch Download (89.4 KB) - added by dgrichard 5 years ago.
Upgrade of coordinate systems handler in Geoconcept driver
expected_tile.txt Download (1.0 KB) - added by dgrichard 5 years ago.
Creation test expected result
expected_000_GRD.txt Download (1.0 KB) - added by dgrichard 5 years ago.
Creation test expected result (2)
expected_tile.gxt Download (1.6 KB) - added by dgrichard 5 years ago.
Update unit test expected result
expected_000_GRD.gxt Download (1.6 KB) - added by dgrichard 5 years ago.
Update unit test expected result
expected_tile2.gxt Download (1.8 KB) - added by dgrichard 5 years ago.
Update unit test expected result (2)
geoconcept-20080103.patch Download (9.0 KB) - added by dgrichard 5 years ago.
Pragma DELIMITER enhancement, Pragma VERSION addition. Bug fixes in error messages.

Change History

follow-up: ↓ 5   Changed 5 years ago by warmerdam

  • status changed from new to closed
  • resolution set to fixed

I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?

We really need a test script for this driver! I don't really know if I've completely broken it.

  Changed 5 years ago by warmerdam

  • status changed from closed to reopened
  • resolution fixed deleted

follow-up: ↓ 6   Changed 5 years ago by warmerdam

Still problems with single files:

warmerda@gdal64[110]% ogrinfo Mills.mif 
OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept.
INFO: Open of `Mills.mif'
      using driver `Geoconcept' successful.
OGR: GetLayerCount() = 0

follow-up: ↓ 7   Changed 5 years ago by warmerdam

  • cc warmerdam added; dgrichard removed
  • owner changed from warmerdam to dgrichard
  • status changed from reopened to new

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.

    /* Let CreateLayer do the job ... */
    if( bUpdate )
    {
      _pszName = CPLStrdup( pszName );
      return TRUE;
    }

This logic will need to be updated in some non-obvious way by dgrichard.

in reply to: ↑ 1   Changed 5 years ago by dgrichard

Replying to warmerdam:

I have modified the geoconcept driver such that Open() only succeeds if one or more layers are discovered. I also renamed the driver "Geoconcept" for less messy specification at the commandline and did a few other modifications (r13510). Didier, could you review?

I have modified accordingly and my test script gives me the same results as before.

We really need a test script for this driver! I don't really know if I've completely broken it.

I have a small (for the moment) shell script for testing. It uses a Shapefile datasets (<1kb) to test basic creation and update as describe in drv_geoconcept.html. I will attach it to this ticket together with test files.

in reply to: ↑ 3   Changed 5 years ago by dgrichard

Replying to warmerdam:

Still problems with single files: {{{ warmerda@gdal64[110]% ogrinfo Mills.mif OGR: OGROpen(Mills.mif/0x607320) succeeded as Geoconcept. INFO: Open of `Mills.mif' using driver `Geoconcept' successful. OGR: GetLayerCount?() = 0 }}}

As for the moment, reading the Geoconcept file is still not implemented, I have just started to implement hight level API before coding the parsing of Geoconcept file at low level.

in reply to: ↑ 4 ; follow-up: ↓ 8   Changed 5 years ago by dgrichard

Replying to warmerdam:

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode. {{{ /* Let CreateLayer? do the job ... */ if( bUpdate ) { _pszName = CPLStrdup( pszName ); return TRUE; } }}} This logic will need to be updated in some non-obvious way by dgrichard.

Ok, I am going to check how to improve this (but, still need to code the parsing, I guess).

Changed 5 years ago by dgrichard

Shape for unit test (DBF)

Changed 5 years ago by dgrichard

Shape for unit test (SHP)

Changed 5 years ago by dgrichard

Shape for unit test (SHX)

in reply to: ↑ 7   Changed 5 years ago by dgrichard

Replying to dgrichard:

Replying to warmerdam:

For the time being I have disabled the RegisterDriver?() call in ogrgeoconceptdriver.cpp (r13512) till this bug is fixed. It appears that OGRGeoconceptDataSource::LoadFile?() has the following logic which will always succeed since the driver can only be invoked in bUpdate=TRUE mode.

I propose a new patch geoconcept-20081301.patch with lot of changes including the logic. The reading support is about to be completed as asked by Even. There are some more improvements to add like on-the-fly Geoconcept schema creation. I have already posted a new shell script for unit tests.

Changed 5 years ago by dgrichard

Changed 5 years ago by dgrichard

patch against trunk with better handling of read and SRS and lco CONFIG optional

follow-up: ↓ 13   Changed 5 years ago by rouault

I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled).

I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.

  Changed 5 years ago by rouault

  • cc rouault added

follow-up: ↓ 14   Changed 5 years ago by rouault

Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference? mainly), and a missing pointer initialization. Could you check that it's OK ?

I've a few observations :

  • I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.
  • You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.
  • Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature? and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature?())
  • For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at  https://svn.osgeo.org/gdal/trunk/autotest.

  Changed 5 years ago by rouault

Didier, sorry for having mixed your first and last name my previous post.

in reply to: ↑ 9   Changed 5 years ago by dgrichard

Replying to rouault:

I've tested geoconcept-20080120.patch. The file opening logic seems to be OK now (directory access seems to have been disabled). I've commited the patch in r13555 with the exception of the ogr/ogrsf_frmts/geoconcept/makefile.vc changes (FrankW reverted the export of internal API after applying the first version of the driver) and the ogr/ogrsf_frmts/mitab/mitab_coordsys.cpp changes.

My mistake for mitab_coordsys.cpp, I am working on it and forgot to remove it from the whole patch. Sorry.

in reply to: ↑ 11   Changed 5 years ago by dgrichard

Replying to rouault:

Richard, I've commited in r13556 a few memory leak fixes (incorrect use of Reference/Dereference? mainly), and a missing pointer initialization. Could you check that it's OK ?

Seems to. As you noticed in the code, I had some question marks about Reference/Dereference?.

I've a few observations : * I've the feeling that each time you read a new feature, you create a new feature definition for it. I don't think this is really a problem, but most OGR driver don't work like this. They create one feature definition that is shared by all features of the same layer. This saves memory (and time). You could probably use the _poFeatureDefn member variable of OGRGeoconceptLayer for that.

The definition is created at low level, I'll changed that.

* You're probably aware of that, but round-tripping of SRS is not complete. For example, EPSG:4326 can be read but not written. This would be great to add it for autotest purposes, as IGNF: namespace needs really recent proj4.

Yeap, I noticed that. I'll improve the process.

* Spatial filter and attribute filter handling is missing. You can build and run apps/test_ogrsf on a geoconcept export file to fix the behaviour of the driver. Basically, you need to loop over features in GetNextFeature? and discard the ones that do not match the filters. You can see how it is done in the BNA driver for example (see OGRBNALayer::GetNextFeature?())

Ok, it is quite cumbersome !

The great thing would have been to have in common tools : - a index (BTree or like); - a geometric index (Quad or like);

* For autotest, it would be great if you could adapt your shell script into the GDAL/OGR autotest framework which uses the GDAL/OGR python bindings. This should not be too complicated to do. You can try to adapt one existing OGR driver autotest script. A basic idea is to read a small geoconcept export file and check that the feature geometry and attribute are the one that are expect, then export its features to another geoconcept file, and finally read it back and check again the values. See http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr. Autotest suite can be checkout at  https://svn.osgeo.org/gdal/trunk/autotest.

Ok, I'll give it a glance and manage to provide the autotest.

Thanks for all

follow-up: ↓ 17   Changed 5 years ago by rouault

Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.

follow-up: ↓ 18   Changed 5 years ago by warmerdam

Didier,

I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature?() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.

in reply to: ↑ 15   Changed 5 years ago by dgrichard

Replying to rouault:

Didier, I've another thing to mention. The feature ID (FID) is not set and incremented as it should probably be (all OGR drivers I'm aware do that). You should do "poFeature->SetFID( fid )", where fid is incremented even if the feature is filtered out.

I have to check for that. Geoconcept ID when set to -1 means the incrementation when importing into the GIS, otherwise use the ID. So, this should be not to complex to fix when writing. I am wondering what to do when reading (the export could have mixed -1 and IDs).

in reply to: ↑ 16   Changed 5 years ago by dgrichard

Replying to warmerdam:

Didier, I'm willing to work on the python autotest entries if that is a foreign environment for you. But in first trying to do so I discovered that the driver does not properly provide read access to features when the datasource is opened in update mode. For example "ogrinfo -al expected_000_GRD.gxt" does not report the features as it ought to, though if I add -ro (read only) then it does. I'm guessing you access the file in 'append' mode for update access but this is an oversimplification. Really you need to seek to the end to append features when CreateFeature?() is called, but otherwise update mode should provide similar feature reading facilities to read only mode.

Frank, the following seems working : --- geoconcept.c 2008-01-20 22:05:48.000000000 +0100 +++ geoconcept.c 2008-01-20 22:05:46.000000000 +0100 @@ -1864,10 +1864,7 @@

theSubType= NULL;

}

- if( GetGCMode_GCIO(H)==vReadAccess_GCIO ) - { - Rewind_GCIO(H,NULL); - } + Rewind_GCIO(H,NULL);

return Meta;

}/* _completeHeader_GCIO */

I'll send the patch when I am through with other points I need to fix/upgrade.

  Changed 5 years ago by warmerdam

I have added a very minimal read test (r13557) of one of the attached samples. I have so far been unable to successfully create a geoconcept file with ogr2ogr so I haven't adding any write testing.

  Changed 5 years ago by warmerdam

rewind patch applied as r13558. It appears to correct the problem.

Changed 5 years ago by dgrichard

Mapinfo for unit test (mid)

Changed 5 years ago by dgrichard

Mapinfo for unit test (mif)

Changed 5 years ago by dgrichard

geoconcept unit tests shell script

Changed 5 years ago by dgrichard

patch against trunk with various enhancements (bug fix for feature definition, round-tripping of SRS complete, spatial and attribute filters added, better support for update, FID support, etc ...)

  Changed 5 years ago by rouault

Didier, I applied geoconcept-20080129.patch in r13624.

In r13625, I also moved some code in OGRSpatialReference2SysCoord_GCSRS, otherwise I was unable to assign EPSG:4326 (or other geographics SRS) as an output SRS for geoconcept. Does it look ok to you ?

I'm also wondering if there wouldn't be a prettier way to test the SRS in OGRSpatialReference2SysCoord_GCSRS. All those hard-coded parameters in the middle of code look scary to me. Couldn't they be put in a few arrays like it seems to have been done in ogr/ogrsf_frmts/mitab/mitab_spatialref.cpp ?

With SRS round-tripping, we should now be able to add write tests in the GDAL autotest.

follow-up: ↓ 25   Changed 5 years ago by rouault

Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "//$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.

  Changed 5 years ago by rouault

A few memory leaks fixed in r13670.

  Changed 5 years ago by rouault

Replace spaces by underscores in field names, when creating fields in r13672.

Add a test for writing Geoconcept file in r13673.

Changed 5 years ago by dgrichard

Correct reading of unknown SRS

in reply to: ↑ 22   Changed 5 years ago by dgrichard

Replying to rouault:

Another note : currently, if I specify an output SRS not handled by OGRSpatialReference2SysCoord_GCSRS, "//$SYSCOORD {Type: -1}" is written in the output file, but it makes it impossible to read back with the driver. I think it would be good if the driver could handle that case and read the file. It could report as a warning that the SRS is invalid, but it's not compulsory that a OGR driver specify a SRS when reading features. For example there's no notion of SRS in the BNA driver.

Patch for not handled SRS sent. I am currently working on re-organising the SRS comparizon in geoconcept_syscoord.c

  Changed 5 years ago by rouault

geoconcept-20080202.patch applied in r13676

Changed 5 years ago by dgrichard

Upgrade of coordinate systems handler in Geoconcept driver

follow-up: ↓ 28   Changed 5 years ago by rouault

Didier,

I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test.

In http://trac.osgeo.org/gdal/browser/trunk/autotest/ogr/ogr_geoconcept.py, ogr_gxt_2 reads the following file and translates it to Geoconcept, and read it back. The round-trip of the SRS is KO.

Input file :

//$DELIMITER "  "
//$QUOTED-TEXT "no"
//$CHARSET ANSI
//$UNIT Distance:m
//$FORMAT 2
//$SYSCOORD {Type: 101}
//$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class   Private#Subclass        Private#NbFields        Primary_ID      Secondary_ID   Third_ID Private#X       Private#Y
-1      points  points  3       PID1    SID1    TID1    0       1
-1      points  points  3       PID2    SID2            2       3

Output file :

//$DELIMITER "  "
//$QUOTED-TEXT "no"
//$CHARSET ANSI
//$UNIT Distance:m
//$FORMAT 2
//$SYSCOORD {Type: 113020768};{TimeZone: 113020824}
//$FIELDS Class=points;Subclass=points;Kind=1;Fields=Private#Identifier Private#Class   Private#Subclass        Private#NbFields        Primary_ID      Secondary_ID   Third_ID Private#X       Private#Y
-1      points  points  3       PID1    SID1    TID1    0       1
-1      points  points  3       PID2    SID2            2       3

There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian?_GCSRS(theSysCoord, 0);

Could you look at this ?

in reply to: ↑ 27   Changed 5 years ago by dgrichard

Replying to rouault:

Didier, I've applied the series of patch of #2134 and geoconcept-20080223.patch in my tree, but didn't commit in trunk since it broke the unit test. There's also a missing field initialization in _InitSysCoord_GCSRS: SetSysCoordPrimeMeridian?_GCSRS(theSysCoord, 0);

Added

Could you look at this ?

Allright, I'll have a look. Nevertheless, my unit tests were OK with ogrgc.sh; I had the same problem as you mentioned, but after a full compilation it was running ... I have also tested the SRS round-trip and EPSG:4326 (Type:101) was OK.

  Changed 5 years ago by rouault

r13869 : "Apply geoconcept-20080223.patch (except for the makefile)"

  Changed 5 years ago by rouault

r13870 : "A few changes for WGS 84 handling so that ogr_gxt_2 works"

Changed 5 years ago by dgrichard

Creation test expected result

Changed 5 years ago by dgrichard

Creation test expected result (2)

Changed 5 years ago by dgrichard

Update unit test expected result

Changed 5 years ago by dgrichard

Update unit test expected result

Changed 5 years ago by dgrichard

Update unit test expected result (2)

Changed 5 years ago by dgrichard

Pragma DELIMITER enhancement, Pragma VERSION addition. Bug fixes in error messages.

  Changed 5 years ago by dgrichard

Frank, Even,

I made few changes and fixes to the Geoconcept driver and change test data accordingly. I wonder whether I may open an enhancement request against Geoconcept for transferring changes/fixes next time or I still use this bug folder ?

  Changed 5 years ago by rouault

r13907 : apply geoconcept-20080103.patch

r13908 : Enhance geoconcept test with data file with TAB delimitor

  Changed 5 years ago by warmerdam

Are we at the point where this ticket can be closed?

  Changed 5 years ago by rouault

  • status changed from new to closed
  • resolution set to fixed

Yes, closing

Note: See TracTickets for help on using tickets.