From a64554c93397fc130c631c63f585321a2d875683 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Mon, 20 Jan 2025 17:51:27 +0100 Subject: [PATCH] Factor out SQL transaction and remove C api --- autotest/ogr/ogr_gpkg.py | 4 +- ogr/ogr_api.h | 1 - ogr/ogr_feature.h | 2 - ogr/ogrfeature.cpp | 60 -------- .../gpkg/ogrgeopackagedatasource.cpp | 70 +-------- .../gpkg/ogrgeopackagetablelayer.cpp | 2 +- ogr/ogrsf_frmts/pg/ogrpgtablelayer.cpp | 11 +- ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h | 6 + .../sqlite/ogrsqlitedatasource.cpp | 143 ++++++++++-------- swig/include/ogr.i | 6 - 10 files changed, 100 insertions(+), 205 deletions(-) diff --git a/autotest/ogr/ogr_gpkg.py b/autotest/ogr/ogr_gpkg.py index 4dfaab50da1c..92882c408c04 100755 --- a/autotest/ogr/ogr_gpkg.py +++ b/autotest/ogr/ogr_gpkg.py @@ -9193,8 +9193,8 @@ def test_ogr_gpkg_read_generated_column(tmp_vsimem): assert f["strfield"] == "foo" assert f["strfield_generated"] == "foo_generated" assert f["intfield_generated_stored"] == 5 - assert f.IsFieldGenerated(2) - assert f.IsFieldGenerated(3) + assert f.GetFieldDefnRef(2).IsGenerated() + assert f.GetFieldDefnRef(3).IsGenerated() assert lyr.SetFeature(f) == ogr.OGRERR_NONE lyr.ResetReading() diff --git a/ogr/ogr_api.h b/ogr/ogr_api.h index 6f551ec048c9..1b4563562603 100644 --- a/ogr/ogr_api.h +++ b/ogr/ogr_api.h @@ -480,7 +480,6 @@ OGRFieldDefnH CPL_DLL OGR_F_GetFieldDefnRef(OGRFeatureH, int); int CPL_DLL OGR_F_GetFieldIndex(OGRFeatureH, const char *); int CPL_DLL OGR_F_IsFieldSet(OGRFeatureH, int); -int CPL_DLL OGR_F_IsFieldGenerated(OGRFeatureH, int); void CPL_DLL OGR_F_UnsetField(OGRFeatureH, int); int CPL_DLL OGR_F_IsFieldNull(OGRFeatureH, int); diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index 764700079ca4..e223d81b49f4 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -1270,8 +1270,6 @@ class CPL_DLL OGRFeature int IsFieldSet(int iField) const; - bool IsFieldGenerated(int iField) const; - void UnsetField(int iField); bool IsFieldNull(int iField) const; diff --git a/ogr/ogrfeature.cpp b/ogr/ogrfeature.cpp index 170394a9a203..1cc4f594dd44 100644 --- a/ogr/ogrfeature.cpp +++ b/ogr/ogrfeature.cpp @@ -1590,66 +1590,6 @@ int OGRFeature::IsFieldSet(int iField) const } } -/************************************************************************/ -/* IsFieldGenerated() */ -/************************************************************************/ - -/** - * \brief Test if a field is a generated field. - * - * At time of writing, only the GeoPackage and PG drivers fill that information. Consequently, - * only a returned value equal to TRUE can be fully trusted. - * - * This method is the same as the C function OGR_F_IsFieldGenerated(). - * - * @param iField the field to test. - * - * @return TRUE if the field has been generated, otherwise false. - * - * @since GDAL 3.11 - */ - -bool OGRFeature::IsFieldGenerated(int iField) const - -{ - const auto oFieldDefn = poDefn->GetFieldDefn(iField); - return oFieldDefn ? oFieldDefn->IsGenerated() : false; -} - -/************************************************************************/ -/* OGR_F_IsFieldGenerated() */ -/************************************************************************/ - -/** - * \brief Test if a field is a generated field. - * - * At time of writing, only the GeoPackage driver fills that information. Consequently, - * only a returned value equal to TRUE can be fully trusted. - * This function is the same as the C++ method OGRFeature::IsFieldGenerated(). - * - * @param hFeat handle to the feature on which the field is. - * @param iField the field to test. - * - * @return TRUE if the field has been generated, otherwise false. - * - * @since GDAL 3.11 - */ - -int OGR_F_IsFieldGenerated(OGRFeatureH hFeat, int iField) - -{ - VALIDATE_POINTER1(hFeat, "OGR_F_IsFieldGenerated", 0); - - const OGRFeature *poFeature = OGRFeature::FromHandle(hFeat); - if (iField < 0 || iField >= poFeature->GetFieldCount()) - { - CPLError(CE_Failure, CPLE_AppDefined, "Invalid index : %d", iField); - return FALSE; - } - - return poFeature->IsFieldGenerated(iField); -} - /************************************************************************/ /* OGR_F_IsFieldSet() */ /************************************************************************/ diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp index 8beeb6cda387..304455e05361 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp @@ -7703,75 +7703,14 @@ OGRLayer *GDALGeoPackageDataset::ExecuteSQL(const char *pszSQLCommand, CSLDestroy(papszTokens); } - if (EQUAL(osSQLCommand, "VACUUM")) - { - ResetReadingAllLayers(); - } - - if (EQUAL(osSQLCommand, "BEGIN")) - { - SoftStartTransaction(); - return nullptr; - } - else if (EQUAL(osSQLCommand, "COMMIT")) - { - SoftCommitTransaction(); - return nullptr; - } - else if (EQUAL(osSQLCommand, "ROLLBACK")) + if (ProcessTransactionSQL(osSQLCommand)) { - SoftRollbackTransaction(); return nullptr; } - else if (STARTS_WITH_CI(osSQLCommand, "SAVEPOINT")) - { - const CPLStringList aosTokens(SQLTokenize(osSQLCommand)); - if (aosTokens.size() == 2) - { - const char *pszSavepointName = aosTokens[1]; - StartSavepoint(pszSavepointName); - return nullptr; - } - } - else if (STARTS_WITH_CI(osSQLCommand, "RELEASE")) - { - const CPLStringList aosTokens(SQLTokenize(osSQLCommand)); - if (aosTokens.size() == 2) - { - const char *pszSavepointName = aosTokens[1]; - ReleaseSavepoint(pszSavepointName); - return nullptr; - } - else if (aosTokens.size() == 3 && EQUAL(aosTokens[1], "SAVEPOINT")) - { - const char *pszSavepointName = aosTokens[2]; - ReleaseSavepoint(pszSavepointName); - return nullptr; - } - } - else if (STARTS_WITH_CI(osSQLCommand, "ROLLBACK")) + + if (EQUAL(osSQLCommand, "VACUUM")) { - const CPLStringList aosTokens(SQLTokenize(osSQLCommand)); - if (aosTokens.size() == 2) - { - if (EQUAL(aosTokens[1], "TRANSACTION")) - { - SoftRollbackTransaction(); - return nullptr; - } - else - { - const char *pszSavepointName = aosTokens[1]; - RollbackToSavepoint(pszSavepointName); - return nullptr; - } - } - else if (aosTokens.size() > 1) // Savepoint name is last token - { - const char *pszSavepointName = aosTokens[aosTokens.size() - 1]; - RollbackToSavepoint(pszSavepointName); - return nullptr; - } + ResetReadingAllLayers(); } else if (STARTS_WITH_CI(osSQLCommand, "DELETE FROM ")) { @@ -7791,7 +7730,6 @@ OGRLayer *GDALGeoPackageDataset::ExecuteSQL(const char *pszSQLCommand, } } } - else if (pszDialect != nullptr && EQUAL(pszDialect, "INDIRECT_SQLITE")) return GDALDataset::ExecuteSQL(osSQLCommand, poSpatialFilter, "SQLITE"); else if (pszDialect != nullptr && !EQUAL(pszDialect, "") && diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp index 8956a1d74445..05349c3ed1b6 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp @@ -3255,7 +3255,7 @@ std::string OGRGeoPackageTableLayer::FeatureGenerateUpdateSQL( { const int iField = panUpdatedFieldsIdx[i]; if (iField == m_iFIDAsRegularColumnIndex || - poFeature->IsFieldGenerated(iField)) + poFeatureDefn->GetFieldDefn(iField)->IsGenerated()) continue; if (!poFeature->IsFieldSet(iField)) continue; diff --git a/ogr/ogrsf_frmts/pg/ogrpgtablelayer.cpp b/ogr/ogrsf_frmts/pg/ogrpgtablelayer.cpp index c2f51993b8cf..f98e9134069f 100644 --- a/ogr/ogrsf_frmts/pg/ogrpgtablelayer.cpp +++ b/ogr/ogrsf_frmts/pg/ogrpgtablelayer.cpp @@ -1574,7 +1574,7 @@ OGRErr OGRPGTableLayer::IUpdateFeature(OGRFeature *poFeature, continue; if (!poFeature->IsFieldSet(iField)) continue; - if (poFeature->IsFieldGenerated(iField)) + if (poFeature->GetDefnRef()->GetFieldDefn(iField)->IsGenerated()) continue; if (bNeedComma) @@ -1934,7 +1934,7 @@ OGRErr OGRPGTableLayer::CreateFeatureViaInsert(OGRFeature *poFeature) continue; if (!poFeature->IsFieldSet(i)) continue; - if (poFeature->IsFieldGenerated(i)) + if (poFeature->GetDefnRef()->GetFieldDefn(i)->IsGenerated()) continue; if (!bNeedComma) @@ -2058,7 +2058,7 @@ OGRErr OGRPGTableLayer::CreateFeatureViaInsert(OGRFeature *poFeature) continue; if (!poFeature->IsFieldSet(i)) continue; - if (poFeature->IsFieldGenerated(i)) + if (poFeature->GetDefnRef()->GetFieldDefn(i)->IsGenerated()) continue; if (bNeedComma) @@ -2206,8 +2206,9 @@ OGRErr OGRPGTableLayer::CreateFeatureViaCopy(OGRFeature *poFeature) std::vector abFieldsToInclude(poFeature->GetFieldCount(), true); for (size_t i = 0; i < abFieldsToInclude.size(); i++) - abFieldsToInclude[i] = - !poFeature->IsFieldGenerated(static_cast(i)); + abFieldsToInclude[i] = !poFeature->GetDefnRef() + ->GetFieldDefn(static_cast(i)) + ->IsGenerated(); if (bFIDColumnInCopyFields) { diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h index 32f936a0d5c8..bf9642721456 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h @@ -219,6 +219,12 @@ class OGRSQLiteBaseDataSource CPL_NON_FINAL : public GDALPamDataset OGRErr ReleaseSavepoint(const std::string &osName); OGRErr RollbackToSavepoint(const std::string &osName); + /** + * Execute a SQL transaction command (BEGIN, COMMIT, ROLLBACK, SAVEPOINT) + * @return TRUE if the osSQLCommand was recognized as a transaction command + */ + bool ProcessTransactionSQL(const std::string &osSQLCommand); + OGRErr PragmaCheck(const char *pszPragma, const char *pszExpected, int nRowsExpected); diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp index f08b1c3cb921..959d65baad92 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp @@ -3444,71 +3444,10 @@ OGRLayer *OGRSQLiteDataSource::ExecuteSQL(const char *pszSQLCommand, } } } - else if (EQUAL(pszSQLCommand, "BEGIN")) + else if (ProcessTransactionSQL(pszSQLCommand)) { - SoftStartTransaction(); - return nullptr; - } - else if (EQUAL(pszSQLCommand, "COMMIT")) - { - SoftCommitTransaction(); return nullptr; } - else if (EQUAL(pszSQLCommand, "ROLLBACK")) - { - SoftRollbackTransaction(); - return nullptr; - } - else if (STARTS_WITH_CI(pszSQLCommand, "SAVEPOINT")) - { - const CPLStringList aosTokens(SQLTokenize(pszSQLCommand)); - if (aosTokens.size() == 2) - { - const char *pszSavepointName = aosTokens[1]; - StartSavepoint(pszSavepointName); - return nullptr; - } - } - else if (STARTS_WITH_CI(pszSQLCommand, "RELEASE")) - { - const CPLStringList aosTokens(SQLTokenize(pszSQLCommand)); - if (aosTokens.size() == 2) - { - const char *pszSavepointName = aosTokens[1]; - ReleaseSavepoint(pszSavepointName); - return nullptr; - } - else if (aosTokens.size() == 3 && EQUAL(aosTokens[1], "SAVEPOINT")) - { - const char *pszSavepointName = aosTokens[2]; - ReleaseSavepoint(pszSavepointName); - return nullptr; - } - } - else if (STARTS_WITH_CI(pszSQLCommand, "ROLLBACK")) - { - const CPLStringList aosTokens(SQLTokenize(pszSQLCommand)); - if (aosTokens.size() == 2) - { - if (EQUAL(aosTokens[1], "TRANSACTION")) - { - SoftRollbackTransaction(); - return nullptr; - } - else - { - const char *pszSavepointName = aosTokens[1]; - RollbackToSavepoint(pszSavepointName); - return nullptr; - } - } - else if (aosTokens.size() > 1) // Savepoint name is last token - { - const char *pszSavepointName = aosTokens[aosTokens.size() - 1]; - RollbackToSavepoint(pszSavepointName); - return nullptr; - } - } else if (!STARTS_WITH_CI(pszSQLCommand, "SELECT ") && !STARTS_WITH_CI(pszSQLCommand, "CREATE TABLE ") && !STARTS_WITH_CI(pszSQLCommand, "PRAGMA ")) @@ -4412,6 +4351,86 @@ OGRErr OGRSQLiteBaseDataSource::RollbackToSavepoint(const std::string &osName) return eErr; } +/************************************************************************/ +/* ProcessTransactionSQL() */ +/************************************************************************/ +bool OGRSQLiteBaseDataSource::ProcessTransactionSQL( + const std::string &osSQLCommand) +{ + bool retVal = true; + + if (EQUAL(osSQLCommand.c_str(), "BEGIN")) + { + SoftStartTransaction(); + } + else if (EQUAL(osSQLCommand.c_str(), "COMMIT")) + { + SoftCommitTransaction(); + } + else if (EQUAL(osSQLCommand.c_str(), "ROLLBACK")) + { + SoftRollbackTransaction(); + } + else if (STARTS_WITH_CI(osSQLCommand.c_str(), "SAVEPOINT")) + { + const CPLStringList aosTokens(SQLTokenize(osSQLCommand.c_str())); + if (aosTokens.size() == 2) + { + const char *pszSavepointName = aosTokens[1]; + StartSavepoint(pszSavepointName); + } + else + { + retVal = false; + } + } + else if (STARTS_WITH_CI(osSQLCommand.c_str(), "RELEASE")) + { + const CPLStringList aosTokens(SQLTokenize(osSQLCommand.c_str())); + if (aosTokens.size() == 2) + { + const char *pszSavepointName = aosTokens[1]; + ReleaseSavepoint(pszSavepointName); + } + else if (aosTokens.size() == 3 && EQUAL(aosTokens[1], "SAVEPOINT")) + { + const char *pszSavepointName = aosTokens[2]; + ReleaseSavepoint(pszSavepointName); + } + else + { + retVal = false; + } + } + else if (STARTS_WITH_CI(osSQLCommand.c_str(), "ROLLBACK")) + { + const CPLStringList aosTokens(SQLTokenize(osSQLCommand.c_str())); + if (aosTokens.size() == 2) + { + if (EQUAL(aosTokens[1], "TRANSACTION")) + { + SoftRollbackTransaction(); + } + else + { + const char *pszSavepointName = aosTokens[1]; + RollbackToSavepoint(pszSavepointName); + } + } + else if (aosTokens.size() > 1) // Savepoint name is last token + { + const char *pszSavepointName = aosTokens[aosTokens.size() - 1]; + RollbackToSavepoint(pszSavepointName); + } + } + else + { + retVal = false; + } + + return retVal; +} + /************************************************************************/ /* DoTransactionCommand() */ /************************************************************************/ diff --git a/swig/include/ogr.i b/swig/include/ogr.i index 15cbb5b83794..55c06ae1dd11 100644 --- a/swig/include/ogr.i +++ b/swig/include/ogr.i @@ -2280,12 +2280,6 @@ public: return false; } - /* ---- IsFieldGenerated --------------------- */ - - bool IsFieldGenerated(int id) { - return (OGR_F_IsFieldGenerated(self, id) > 0); - } - /* ------------------------------------------- */ /* ---- IsFieldNull --------------------------- */