#1223 closed defect (fixed)
OGRSFDriver::CreateDataSource creates data source with NULL driver
Reported by: | Owned by: | Mateusz Łoskot | |
---|---|---|---|
Priority: | normal | Milestone: | 1.4.2 |
Component: | OGR_SRS | Version: | unspecified |
Severity: | normal | Keywords: | |
Cc: | warmerdam |
Description (last modified by )
I use CreateDataSource to create several shape files layers in a folder, or to create tables in PGSQL.
I get an instance of the SHAPE driver, and then I specify in the name paramater the relative path of the folder, then I issue CreateLayer to create the shape files.
- First part
When I use OGRSFDriver::CreateDataSource the driver (fetched with GetDriver()) is NULL.
- Second part
When I use OGRSFDriverRegistrar::Open on the same directory with shape files the driver pointer is attached.
What's more interesting is that it still crates the layers properly even though the driver is NULL :)
Ariel
Attachments (4)
Change History (16)
comment:1 by , 17 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
Milestone: | → 1.4.2 |
Owner: | changed from | to
Priority: | high → normal |
Severity: | major → normal |
comment:2 by , 17 years ago
Description: | modified (diff) |
---|---|
Status: | new → assigned |
comment:3 by , 17 years ago
Description: | modified (diff) |
---|
Ariel,
I attached two small tests based on your explanation of the problem.
I think I understand the problem correctly, but please could you verify my understanding and tests I prepared?
Here is my run of these tests:
mloskot:~/dev/gdal/bugs/1223$ ./create Test: create NOT OK! Driver is NULL! mloskot:~/dev/gdal/bugs/1223$ ./open Test: open OK! Driver attached!
comment:4 by , 17 years ago
Frank,
I found where is the problem.
If a datasource is opened using OGRSFDriverRegistrar::Open() then this function assigns pointer to activated driver to the returned datastore. This works well.
When a datastore is created using OGRSFDriver::CreateDataStore() interface, what results in call fowarded to <specialized driver>::CreateDataStore() (ie. OGRShapeDriver::Open() function), then the driver is not assigned to datasource object. This is the problem.
Potentially, the fix is trivial: assign pointer to used driver inside CreateDataStore call, but this is not possible with current interface of OGRDataSource.
The problem here is that the OGRDataSource::m_poDriver is protected and there is no way to access it inside function like OGRShapeDriver::Open().
In the OGRSFDriverRegistrar::Open() function the situation is different, because the OGRSFDriverRegistrar is a friend of OGRDataSource, so it's possible to access the m_poDriver member directly.
In order to solve this issue, we need to find a way how to set new value to OGRDataSource::m_poDriver from subclasses of OGRSFDriver.
I see following possibilities:
- make the OGRSFDriver a friend of OGRDataSource (simpler, but less elegant)
- add new public function OGRDataSource::SetDriver(OGRSFDriver* poDriver) what will change the interface (harder, but better designed)
What do you think?
comment:5 by , 17 years ago
Mateusz,
I could live with the second solution, but the problem with it (in my mind) is that it means that ever driver has to be updated to set this. The nice thing about centralizing the driver setting in OGROpen() is that the drivers don't even have to be aware about it.
Garr.
This is one of the things I hate about the approach of directly overriding methods - it makes it very hard to provide generic level checking and actions before the format specific method gets called.
I'm not sure that I want to have to touch all the drivers. Let me think on this for a bit, unless you have a solution to my dilemma.
follow-up: 7 comment:6 by , 17 years ago
Frank,
There is also a modification of 2nd solution possible
The idea is to add new constructor to OGRDataSource class:
OGRDataSource::OGRDataSource(OGRSFDriver* poDriver);
and next it used as follows:
OGRDataSource *OGRShapeDriver::Open( const char * pszFilename, int bUpdate ) { OGRShapeDataSource *poDS; poDS = new OGRShapeDataSource( this ); // HERE
- This version makes the contract stronger that a DS can not be instantiated without assigned driver.
- Also, explicit assignment of driver in OGRSFDriverRegistrar::Open can be removed, because this responsibility could move to the specialized data source class, similarly to the code above:
OGRDataSource *OGRShapeDriver::Open( const char * pszFilename, int bUpdate ) { OGRShapeDataSource *poDS; poDS = new OGRShapeDataSource( this );
I understand this solution may not be as less intrusive as you like, but let's have it discussed and archived.
comment:7 by , 17 years ago
Replying to mloskot:
The second code snipped is incorrect, certainly.
Here I wanted to update OGRShapeDriver::CreateDataSource, as follows:
OGRDataSource *OGRShapeDriver::CreateDataSource( const char * pszName, char **papszOptions ) { // ... OGRShapeDataSource *poDS = NULL; poDS = new OGRShapeDataSource( this ); // HERE IS THE FIX if( !poDS->Open( pszName, TRUE, FALSE, bSingleNewFile ) )
comment:8 by , 17 years ago
Frank,
There is yet another solution, a dirty hack but I think it's as less intrusive for public API as possible. Consider following change in a specialized class of OGRDataSource:
OGRShapeDataSource::OGRShapeDataSource() { pszName = NULL; papoLayers = NULL; nLayers = 0; bSingleNewFile = FALSE; m_poDriver = OGRSFDriverRegistrar::GetRegistrar()->GetDriverByName("ESRI Shapefile"); CPLAssert( NULL != m_poDriver ); }
What do you think?
comment:9 by , 17 years ago
Here I paste discussion Frank and I had on the IRC:
<FrankW> We could modify the OGR_Dr_CreateDataSource() C API function to set the driver, and just document the C method as not setting the driver, so people should be wary. <FrankW> ...just document the *C++* method... <mloskot> We don not have access to OGRDataSource::m_poDriver from public space <FrankW> We can make OGR_Dr_CreateDataSource() a friend of the class for this purpose. <FrankW> Of, if you prefer add a setter method. mloskot> both are possible, though I'd prefer the latter ;-) <FrankW> ok, fine with me
comment:10 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
The discussed fix was applied in r11378.
by , 17 years ago
Test presenting the second part of explanation in the ticket report
by , 17 years ago
Attachment: | create.cpp added |
---|
Test presenting the first part of explanation in the ticket report
by , 17 years ago
Attachment: | create_c.cpp added |
---|
This program presents the idea behing the half-fix of this issue.
Mateusz,
Could you look into this? This may be a general problem with many drivers with regard to setting the driver information when doing a CreateDataSource(). Depending on the complexity of changes we may put into 1.4 or just in trunk.