From ddfb90111071b448e8d7313960286af3003f56c7 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 25 Jan 2024 19:13:30 +0100 Subject: [PATCH] GPKG: fixes to make most operations compatible with PRAGMA foreign_keys=1 (fixes #9135) --- autotest/ogr/ogr_gpkg.py | 59 ++++++++++++------- ogr/ogrsf_frmts/gpkg/ogr_geopackage.h | 41 +++++++++++++ .../gpkg/ogrgeopackagedatasource.cpp | 41 ++++++++++++- .../gpkg/ogrgeopackagetablelayer.cpp | 36 ++++++++--- 4 files changed, 147 insertions(+), 30 deletions(-) diff --git a/autotest/ogr/ogr_gpkg.py b/autotest/ogr/ogr_gpkg.py index 8c119e944955..ca45c745ea7f 100755 --- a/autotest/ogr/ogr_gpkg.py +++ b/autotest/ogr/ogr_gpkg.py @@ -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(): @@ -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( @@ -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() @@ -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)" @@ -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") diff --git a/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h b/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h index c1055f76d9d8..ca8e6ecb245b 100644 --- a/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h +++ b/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h @@ -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{}; @@ -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, diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp index 72730450662c..e0b25f8e18ab 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp @@ -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) @@ -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); @@ -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; @@ -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( CPLCalloc(sizeof(GDALGeoPackageDataset *), m_nZoomLevel)); @@ -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) { @@ -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; @@ -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 @@ -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); @@ -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) @@ -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(); diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp index 05e570ca042e..1c6fc628881d 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp @@ -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); @@ -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) { @@ -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) { @@ -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() @@ -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; @@ -7169,7 +7189,9 @@ OGRErr OGRGeoPackageTableLayer::AlterGeomFieldDefn( } if (m_poDS->SoftCommitTransaction() != OGRERR_NONE) + { return OGRERR_FAILURE; + } m_iSrs = nNewSRID; OGRSpatialReference *poSRS = poNewSRS.release();