Skip to content

Commit

Permalink
Merge pull request OSGeo#9141 from rouault/fix_9135
Browse files Browse the repository at this point in the history 
GPKG: fixes to make most operations compatible with PRAGMA foreign_keys=1
  • Loading branch information
@rouault
rouault authored Jan 29, 2024
2 parents 2e1b7a1 + ddfb901 commit e826cc1
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 30 deletions.
59 changes: 38 additions & 21 deletions autotest/ogr/ogr_gpkg.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1884,11 +1884,11 @@ def test_ogr_gpkg_20a(tmp_vsimem):
ds = None

# Unable to parse srs_id '4326' well-known text 'invalid'
with gdal.quiet_errors():
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"), gdal.quiet_errors():
ds = ogr.Open(fname, update=1)
ds.ExecuteSQL("DELETE FROM gpkg_spatial_ref_sys WHERE srs_id = 4326")
ds = None

ds.ExecuteSQL("DELETE FROM gpkg_spatial_ref_sys WHERE srs_id = 4326")
ds = None
with gdal.config_option(
"OGR_GPKG_FOREIGN_KEY_CHECK", "NO"
), gdaltest.error_handler():
Expand All @@ -1901,24 +1901,25 @@ def test_ogr_gpkg_20b(tmp_vsimem):

fname = tmp_vsimem / "ogr_gpkg_20.gpkg"

ds = gdaltest.gpkg_dr.CreateDataSource(fname)
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
lyr = ds.CreateLayer("foo4326", srs=srs)
assert lyr is not None
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = gdaltest.gpkg_dr.CreateDataSource(fname)
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
lyr = ds.CreateLayer("foo4326", srs=srs)
assert lyr is not None

ds.ExecuteSQL("DROP TABLE gpkg_spatial_ref_sys")
ds.ExecuteSQL(
"CREATE TABLE gpkg_spatial_ref_sys (srs_name TEXT, "
"srs_id INTEGER, organization TEXT, "
"organization_coordsys_id INTEGER, definition TEXT)"
)
ds.ExecuteSQL(
"INSERT INTO gpkg_spatial_ref_sys "
"(srs_name,srs_id,organization,organization_coordsys_id,"
"definition) VALUES (NULL,4326,NULL,NULL,NULL)"
)
ds = None
ds.ExecuteSQL("DROP TABLE gpkg_spatial_ref_sys")
ds.ExecuteSQL(
"CREATE TABLE gpkg_spatial_ref_sys (srs_name TEXT, "
"srs_id INTEGER, organization TEXT, "
"organization_coordsys_id INTEGER, definition TEXT)"
)
ds.ExecuteSQL(
"INSERT INTO gpkg_spatial_ref_sys "
"(srs_name,srs_id,organization,organization_coordsys_id,"
"definition) VALUES (NULL,4326,NULL,NULL,NULL)"
)
ds = None

# Warning 1: null definition for srs_id '4326' in gpkg_spatial_ref_sys
with gdal.config_option(
Expand Down Expand Up @@ -3430,7 +3431,14 @@ def test_ogr_gpkg_35(tmp_vsimem, tmp_path):
):
f.DumpReadable()
pytest.fail()
lyr = None
lyr_nonspatial = None
ds = None

with gdaltest.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = ogr.Open(dbname, update=1)
lyr = ds.GetLayerByName("test")
lyr_nonspatial = ds.GetLayerByName("test_nonspatial")
lyr.StartTransaction()
ret = lyr_nonspatial.DeleteField(1)
lyr.CommitTransaction()
Expand Down Expand Up @@ -5069,7 +5077,8 @@ def test_ogr_gpkg_57(tmp_vsimem):
out_filename = tmp_vsimem / "test_ogr_gpkg_57.gpkg"
ogr.GetDriverByName("GPKG").CreateDataSource(out_filename)

ds = ogr.Open(out_filename, update=1)
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = ogr.Open(out_filename, update=1)
ds.ExecuteSQL("DROP TABLE gpkg_contents")
ds.ExecuteSQL(
"CREATE TABLE gpkg_contents (table_name,data_type,identifier,description,last_change,min_x, min_y,max_x, max_y,srs_id)"
Expand Down Expand Up @@ -10004,3 +10013,11 @@ def test_ogr_gpkg_extent3d_on_2d_dataset_with_filters(tmp_vsimem):
assert ext3d == (3, 3, 4, 4, float("inf"), float("-inf"))

ds = None


@gdaltest.enable_exceptions()
def test_ogr_gpkg_creation_with_foreign_key_constraint_enabled(tmp_vsimem):

with gdaltest.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=1"):
out_filename = str(tmp_vsimem / "out.gpkg")
gdal.VectorTranslate(out_filename, "data/poly.shp")
41 changes: 41 additions & 0 deletions ogr/ogrsf_frmts/gpkg/ogr_geopackage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ class GDALGeoPackageDataset final : public OGRSQLiteBaseDataSource,
mutable int m_nHasMetadataTables = -1; // -1 = unknown, 0 = false, 1 = true
int m_nCreateMetadataTables = -1; // -1 = on demand, 0 = false, 1 = true

// Set by CreateTileGriddedTable() and used by FinalizeRasterRegistration()
std::string m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary{};

CPLString m_osIdentifier{};
bool m_bIdentifierAsCO = false;
CPLString m_osDescription{};
Expand Down Expand Up @@ -411,6 +414,44 @@ class GDALGeoPackageDataset final : public OGRSQLiteBaseDataSource,

GDALDataset *GetRasterLayerDataset(const char *pszLayerName);

//! Instance of that class temporarily disable foreign key checks
class TemporaryForeignKeyCheckDisabler
{
public:
explicit TemporaryForeignKeyCheckDisabler(GDALGeoPackageDataset *poDS)
: m_poDS(poDS),
m_nPragmaForeignKeysOldValue(SQLGetInteger(
m_poDS->GetDB(), "PRAGMA foreign_keys", nullptr))
{
if (m_nPragmaForeignKeysOldValue)
{
CPL_IGNORE_RET_VAL(
SQLCommand(m_poDS->GetDB(), "PRAGMA foreign_keys = 0"));
}
}

~TemporaryForeignKeyCheckDisabler()
{
if (m_nPragmaForeignKeysOldValue)
{
CPL_IGNORE_RET_VAL(
SQLCommand(m_poDS->GetDB(), "PRAGMA foreign_keys = 1"));
}
}

private:
CPL_DISALLOW_COPY_ASSIGN(TemporaryForeignKeyCheckDisabler)

GDALGeoPackageDataset *m_poDS = nullptr;
int m_nPragmaForeignKeysOldValue = 0;
};

//! Return an object that, while alive, temporarily disables foreign key checks
TemporaryForeignKeyCheckDisabler GetTemporaryForeignKeyCheckDisabler()
{
return TemporaryForeignKeyCheckDisabler(this);
}

protected:
virtual CPLErr IRasterIO(GDALRWFlag, int, int, int, int, void *, int, int,
GDALDataType, int, int *, GSpacing, GSpacing,
Expand Down
41 changes: 39 additions & 2 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ bool GDALGeoPackageDataset::ConvertGpkgSpatialRefSysToExtensionWkt2(
if (!oResultTable)
return false;

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

bool bRet = SoftStartTransaction() == OGRERR_NONE;

if (bRet)
Expand Down Expand Up @@ -3247,7 +3251,8 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
m_adfGeoTransform[0] + nRasterXSize * m_adfGeoTransform[1];
double dfGDALMaxY = m_adfGeoTransform[3];

SoftStartTransaction();
if (SoftStartTransaction() != OGRERR_NONE)
return CE_Failure;

const char *pszCurrentDate =
CPLGetConfigOption("OGR_CURRENT_DATE", nullptr);
Expand All @@ -3270,7 +3275,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

double dfTMSMaxX = m_dfTMSMinX + nTileXCountZoomLevel0 * nTileWidth *
dfPixelXSizeZoomLevel0;
Expand All @@ -3286,7 +3294,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

m_papoOverviewDS = static_cast<GDALGeoPackageDataset **>(
CPLCalloc(sizeof(GDALGeoPackageDataset *), m_nZoomLevel));
Expand Down Expand Up @@ -3323,7 +3334,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

if (i < m_nZoomLevel)
{
Expand All @@ -3339,6 +3353,18 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
}
}

if (!m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.empty())
{
eErr = SQLCommand(
hDB, m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.c_str());
m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.clear();
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}
}

SoftCommitTransaction();

m_nOverviewCount = m_nZoomLevel;
Expand Down Expand Up @@ -5842,7 +5868,7 @@ bool GDALGeoPackageDataset::CreateTileGriddedTable(char **papszOptions)
m_dfOffset, m_dfPrecision, osGridCellEncoding.c_str(),
osUom.empty() ? nullptr : osUom.c_str(), osFieldName.c_str(),
osQuantityDefinition.c_str());
osSQL += pszSQL;
m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary = pszSQL;
sqlite3_free(pszSQL);

// Requirement 3 /gpkg-spatial-ref-sys-row
Expand Down Expand Up @@ -6729,6 +6755,10 @@ int GDALGeoPackageDataset::FindLayerIndex(const char *pszLayerName)

OGRErr GDALGeoPackageDataset::DeleteLayerCommon(const char *pszLayerName)
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

char *pszSQL = sqlite3_mprintf(
"DELETE FROM gpkg_contents WHERE lower(table_name) = lower('%q')",
pszLayerName);
Expand Down Expand Up @@ -6858,6 +6888,10 @@ OGRErr GDALGeoPackageDataset::DeleteLayer(int iLayer)

CPLDebug("GPKG", "DeleteLayer(%s)", osLayerName.c_str());

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

OGRErr eErr = SoftStartTransaction();

if (eErr == OGRERR_NONE)
Expand Down Expand Up @@ -6924,6 +6958,9 @@ OGRErr GDALGeoPackageDataset::DeleteLayer(int iLayer)

OGRErr GDALGeoPackageDataset::DeleteRasterLayer(const char *pszLayerName)
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

OGRErr eErr = SoftStartTransaction();

Expand Down
36 changes: 29 additions & 7 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5142,8 +5142,14 @@ OGRErr OGRGeoPackageTableLayer::Rename(const char *pszDstTableName)
return OGRERR_FAILURE;
}

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

#ifdef ENABLE_GPKG_OGR_CONTENTS
DisableFeatureCountTriggers(false);
Expand Down Expand Up @@ -5736,11 +5742,6 @@ OGRErr OGRGeoPackageTableLayer::RunDeferredCreationIfNecessary()
/* Update gpkg_contents with the table info */
const OGRwkbGeometryType eGType = GetGeomType();
const bool bIsSpatial = (eGType != wkbNone);
if (bIsSpatial)
err = RegisterGeometryColumn();

if (err != OGRERR_NONE)
return OGRERR_FAILURE;

if (bIsSpatial || m_eASpatialVariant == GPKG_ATTRIBUTES)
{
Expand All @@ -5766,6 +5767,15 @@ OGRErr OGRGeoPackageTableLayer::RunDeferredCreationIfNecessary()
return OGRERR_FAILURE;
}

if (bIsSpatial)
{
// Insert into gpkg_geometry_columns after gpkg_contents because of
// foreign key constraints
err = RegisterGeometryColumn();
if (err != OGRERR_NONE)
return OGRERR_FAILURE;
}

#ifdef ENABLE_GPKG_OGR_CONTENTS
if (m_poDS->m_bHasGPKGOGRContents)
{
Expand Down Expand Up @@ -6172,11 +6182,17 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete)
/* -------------------------------------------------------------------- */
m_poDS->ResetReadingAllLayers();

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

// ALTER TABLE ... DROP COLUMN ... was first implemented in 3.35.0 but
// there was bug fixes related to it until 3.35.5
// ALTER TABLE ... DROP COLUMN ... was first implemented in 3.35.0 but
// there was bug fixes related to it until 3.35.5
#if SQLITE_VERSION_NUMBER >= 3035005L
OGRErr eErr = SQLCommand(
m_poDS->GetDB(), CPLString()
Expand Down Expand Up @@ -7098,6 +7114,10 @@ OGRErr OGRGeoPackageTableLayer::AlterGeomFieldDefn(
(poOldSRS != nullptr && poNewSRS != nullptr &&
!poOldSRS->IsSame(poNewSRS.get(), apszOptions)))
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
return OGRERR_FAILURE;

Expand Down Expand Up @@ -7169,7 +7189,9 @@ OGRErr OGRGeoPackageTableLayer::AlterGeomFieldDefn(
}

if (m_poDS->SoftCommitTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

m_iSrs = nNewSRID;
OGRSpatialReference *poSRS = poNewSRS.release();
Expand Down

0 comments on commit e826cc1

Please sign in to comment.