From add4a0d67d7a336ee7f09ab668d2f737cbc774cc Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Tue, 7 Jan 2025 12:34:20 +0100 Subject: [PATCH 01/12] [SQLite] Transaction field handling: intial implementation Fix out of sync (not restored) fields after a ROLLBACK. --- autotest/ogr/ogr_sqlite.py | 202 ++++++++++++++++++ ogr/ogr_feature.h | 41 +++- ogr/ogrfeaturedefn.cpp | 43 +++- ogr/ogrfielddefn.cpp | 55 +++++ ogr/ogrgeomfielddefn.cpp | 43 ++++ ogr/ogrsf_frmts/sqlite/ogr_sqlite.h | 31 +++ .../sqlite/ogrsqlitedatasource.cpp | 8 + ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 158 +++++++++++++- .../sqlite/ogrsqlitetablelayer.cpp | 43 +++- 9 files changed, 613 insertions(+), 11 deletions(-) diff --git a/autotest/ogr/ogr_sqlite.py b/autotest/ogr/ogr_sqlite.py index a7094a1658c0..adde5325cd9b 100755 --- a/autotest/ogr/ogr_sqlite.py +++ b/autotest/ogr/ogr_sqlite.py @@ -4536,3 +4536,205 @@ def test_ogr_sqlite_schema_override( assert ( gdal.GetLastErrorMsg().find(expected_warning) != -1 ), f"Warning {expected_warning} not found, got {gdal.GetLastErrorMsg()} instead" + + +###################################################################### +# Test field operations rolling back changes. +# + + +def test_field_rollback(tmp_path): + + temp_file = str(tmp_path / "tmp.db") + + # Create a new database. + ds = ogr.GetDriverByName("SQLite").CreateDataSource(temp_file) + + # Create a layer. + lyr = ds.CreateLayer("testlyr") + + # Add a field. + fld_defn = ogr.FieldDefn("fld1", ogr.OFTInteger) + lyr.CreateField(fld_defn) + + # Add a feature. + feat = ogr.Feature(lyr.GetLayerDefn()) + feat.SetField("fld1", 1) + lyr.CreateFeature(feat) + + del lyr + del ds + del feat + + # Reopen the database. + ds = ogr.Open(temp_file, update=True) + lyr = ds.GetLayerByName("testlyr") + assert lyr is not None + + # Verify the field. + fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) + assert fld_defn.GetName() == "fld1" + assert fld_defn.GetType() == ogr.OFTInteger + assert lyr.GetLayerDefn().GetFieldDefn("fld2") is None + + # Verify the feature. + feat = lyr.GetNextFeature() + assert feat.GetField("fld1") == 1 + del feat + + # Start a transaction. + assert ds.StartTransaction() == ogr.OGRERR_NONE + lyr.CreateField(ogr.FieldDefn("fld2")) + new_fld_defn = lyr.GetLayerDefn().GetFieldDefn("fld2") + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + assert fld_defn.GetName() == "fld1" + assert lyr.GetLayerDefn().GetFieldDefn("fld2") is None + # This should not crash: + assert new_fld_defn.GetName() == "fld2" + + # Start a transaction and alter the field. + assert ds.StartTransaction() == ogr.OGRERR_NONE + assert ( + lyr.AlterFieldDefn(0, ogr.FieldDefn("fld1", ogr.OFTReal), ogr.ALTER_ALL_FLAG) + == ogr.OGRERR_NONE + ) + fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) + + assert fld_defn.GetType() == ogr.OFTReal + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + new_fld_defn = lyr.GetLayerDefn().GetFieldDefn("fld1") + assert new_fld_defn.GetType() == ogr.OFTInteger + # This should not crash: + assert fld_defn.GetName() == "fld1" + + # Start a transaction and delete the field. + assert ds.StartTransaction() == ogr.OGRERR_NONE + lyr.CreateField(ogr.FieldDefn("fld2")) + assert ds.CommitTransaction() == ogr.OGRERR_NONE + fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) + assert fld_defn.GetName() == "fld1" + assert fld_defn.GetType() == ogr.OFTInteger + assert lyr.GetLayerDefn().GetFieldDefn("fld2") is not None + + del lyr + del ds + + # Reopen the database. + ds = ogr.Open(temp_file, update=True) + lyr = ds.GetLayerByName("testlyr") + assert lyr is not None + + # Check that we have added foo field. + fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) + assert fld_defn.GetName() == "fld1" + assert fld_defn.GetType() == ogr.OFTInteger + assert lyr.GetLayerDefn().GetFieldDefn("fld2") is not None + + # Start a transaction and alter fld2 and delete the fld1. + assert ds.StartTransaction() == ogr.OGRERR_NONE + + assert ( + lyr.AlterFieldDefn(1, ogr.FieldDefn("fld_xxx", ogr.OFTReal), ogr.ALTER_ALL_FLAG) + == ogr.OGRERR_NONE + ) + assert lyr.DeleteField(0) == ogr.OGRERR_NONE + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + + # Check that we have not altered fld2 and deleted fld1. + fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) + assert fld_defn.GetName() == "fld1" + assert fld_defn.GetType() == ogr.OFTInteger + assert lyr.GetLayerDefn().GetFieldDefn(1).GetName() == "fld2" + assert lyr.GetLayerDefn().GetFieldDefn("fld_xxx") is None + + del lyr + del ds + + # Reopen the database. + ds = ogr.Open(temp_file, update=True) + lyr = ds.GetLayerByName("testlyr") + assert lyr is not None + + # Create a list of fields + assert ds.StartTransaction() == ogr.OGRERR_NONE + for i in range(3, 11): + assert ( + lyr.CreateField(ogr.FieldDefn("fld" + str(i), ogr.OFTReal)) + == ogr.OGRERR_NONE + ) + assert ds.CommitTransaction() == ogr.OGRERR_NONE + assert lyr.GetLayerDefn().GetFieldCount() == 10 + for i in range(10): + fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) + assert fld_defn.GetName() == "fld" + str(i + 1) + + # Start a transaction and delete all even fields. + assert ds.StartTransaction() == ogr.OGRERR_NONE + for i in range(9, 0, -2): + print("Delete field", lyr.GetLayerDefn().GetFieldDefn(i).GetName()) + assert lyr.DeleteField(i) == ogr.OGRERR_NONE + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + # Verify that we have not deleted any field. + for i in range(10): + fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) + assert fld_defn.GetName() == "fld" + str(i + 1) + + # Start a transaction and delete all odd fields. + assert ds.StartTransaction() == ogr.OGRERR_NONE + for i in range(8, -1, -2): + print("Delete field", lyr.GetLayerDefn().GetFieldDefn(i).GetName()) + assert lyr.DeleteField(i) == ogr.OGRERR_NONE + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + # Verify that we have not deleted any field. + for i in range(10): + fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) + assert fld_defn.GetName() == "fld" + str(i + 1) + + +###################################################################### +# Test geometry field operations rolling back changes. +# + + +def test_geom_field_rollback(tmp_path): + + temp_file = str(tmp_path / "tmp.db") + + # Create a new database. + ds = ogr.GetDriverByName("SQLite").CreateDataSource(temp_file) + + # Create a layer. + lyr = ds.CreateLayer("testlyr") + + # Add a field. + fld_defn = ogr.FieldDefn("fld1", ogr.OFTInteger) + lyr.CreateField(fld_defn) + + assert lyr.GetGeometryColumn() == "GEOMETRY" + + # Start a transaction and add a geometry column. + assert ds.StartTransaction() == ogr.OGRERR_NONE + assert ( + lyr.CreateGeomField(ogr.GeomFieldDefn("GEOMETRY_2", ogr.wkbPoint)) + == ogr.OGRERR_NONE + ) + assert lyr.GetGeometryColumn() == "GEOMETRY" + assert lyr.GetLayerDefn().GetGeomFieldCount() == 2 + + # Create a feature. + feat = ogr.Feature(lyr.GetLayerDefn()) + feat.SetField("fld1", 1) + feat.SetGeomFieldDirectly("GEOMETRY", ogr.CreateGeometryFromWkt("POINT(1 2)")) + feat.SetGeomFieldDirectly("GEOMETRY_2", ogr.CreateGeometryFromWkt("POINT(3 4)")) + lyr.CreateFeature(feat) + + # Verify the feature. + feat = lyr.GetNextFeature() + assert feat.GetField("fld1") == 1 + assert feat.GetGeomFieldRef(0).ExportToWkt() == "POINT (1 2)" + assert feat.GetGeomFieldRef(1).ExportToWkt() == "POINT (3 4)" + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + + # Verify that we have not added GEOMETRY_2 field. + assert lyr.GetGeometryColumn() == "GEOMETRY" + assert lyr.GetLayerDefn().GetGeomFieldCount() == 1 diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index cf8c3a84d6b3..56ccd3e28067 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -119,6 +119,12 @@ class CPL_DLL OGRFieldDefn explicit OGRFieldDefn(const OGRFieldDefn *); ~OGRFieldDefn(); + // Copy constructor + OGRFieldDefn(const OGRFieldDefn &oOther); + + // Copy assignment operator + OGRFieldDefn &operator=(const OGRFieldDefn &oOther); + void SetName(const char *); const char *GetNameRef() const @@ -279,9 +285,6 @@ class CPL_DLL OGRFieldDefn /*! @endcond */ TemporaryUnsealer GetTemporaryUnsealer(); - - private: - CPL_DISALLOW_COPY_ASSIGN(OGRFieldDefn) }; #ifdef GDAL_COMPILATION @@ -349,6 +352,12 @@ class CPL_DLL OGRGeomFieldDefn explicit OGRGeomFieldDefn(const OGRGeomFieldDefn *); virtual ~OGRGeomFieldDefn(); + // Copy constructor + OGRGeomFieldDefn(const OGRGeomFieldDefn &oOther); + + // Copy assignment operator + OGRGeomFieldDefn &operator=(const OGRGeomFieldDefn &oOther); + void SetName(const char *); const char *GetNameRef() const @@ -442,9 +451,6 @@ class CPL_DLL OGRGeomFieldDefn /*! @endcond */ TemporaryUnsealer GetTemporaryUnsealer(); - - private: - CPL_DISALLOW_COPY_ASSIGN(OGRGeomFieldDefn) }; #ifdef GDAL_COMPILATION @@ -643,8 +649,31 @@ class CPL_DLL OGRFeatureDefn virtual void AddFieldDefn(const OGRFieldDefn *); virtual OGRErr DeleteFieldDefn(int iField); + + /** + * @brief StealFieldDefn takes ownership of the field definition at index detaching + * it from the feature definition. + * This is a mechanism for the application to take over ownership of the field definition from the feature definition without copying. + * @param iField index of the field definition to detach. + * @return a pointer to the detached field definition or nullptr if the index is out of range. + * @note The caller is responsible for deleting the returned field definition. + * @since GDAL 3.11 + */ + virtual OGRFieldDefn *StealFieldDefn(int iField); + virtual OGRErr ReorderFieldDefns(const int *panMap); + /** + * @brief StealGeomFieldDefn takes ownership of the the geometry field definition at index + * detaching it from the feature definition . + * This is a mechanism for the application to take over ownership of the geometry field definition from the feature definition without copying. + * @param iField index of the geometry field definition to detach. + * @return a pointer to the detached geometry field definition or nullptr if the index is out of range. + * @note The caller is responsible for deleting the returned field definition. + * @since GDAL 3.11 + */ + virtual OGRGeomFieldDefn *StealGeomFieldDefn(int iField); + virtual int GetGeomFieldCount() const; virtual OGRGeomFieldDefn *GetGeomFieldDefn(int i); virtual const OGRGeomFieldDefn *GetGeomFieldDefn(int i) const; diff --git a/ogr/ogrfeaturedefn.cpp b/ogr/ogrfeaturedefn.cpp index 4a779428287d..945d516d4a4e 100644 --- a/ogr/ogrfeaturedefn.cpp +++ b/ogr/ogrfeaturedefn.cpp @@ -482,6 +482,48 @@ OGRErr OGRFeatureDefn::DeleteFieldDefn(int iField) return OGRERR_NONE; } +/************************************************************************/ +/* StealGeomFieldDefn() */ +/************************************************************************/ + +OGRGeomFieldDefn *OGRFeatureDefn::StealGeomFieldDefn(int iField) +{ + if (m_bSealed) + { + CPLError(CE_Failure, CPLE_AppDefined, + "OGRFeatureDefn::StealGeomFieldDefn() not allowed on a sealed " + "object"); + return nullptr; + } + if (iField < 0 || iField >= GetGeomFieldCount()) + return nullptr; + + OGRGeomFieldDefn *poFieldDef{apoGeomFieldDefn.at(iField).release()}; + apoGeomFieldDefn.erase(apoGeomFieldDefn.begin() + iField); + return poFieldDef; +} + +/************************************************************************/ +/* StealFieldDefn() */ +/************************************************************************/ + +OGRFieldDefn *OGRFeatureDefn::StealFieldDefn(int iField) +{ + if (m_bSealed) + { + CPLError( + CE_Failure, CPLE_AppDefined, + "OGRFeatureDefn::StealFieldDefn() not allowed on a sealed object"); + return nullptr; + } + if (iField < 0 || iField >= GetFieldCount()) + return nullptr; + + OGRFieldDefn *poFDef{apoFieldDefn.at(iField).release()}; + apoFieldDefn.erase(apoFieldDefn.begin() + iField); + return poFDef; +} + /************************************************************************/ /* OGR_FD_DeleteFieldDefn() */ /************************************************************************/ @@ -533,7 +575,6 @@ OGRErr OGR_FD_DeleteFieldDefn(OGRFeatureDefnH hDefn, int iField) */ OGRErr OGRFeatureDefn::ReorderFieldDefns(const int *panMap) - { if (m_bSealed) { diff --git a/ogr/ogrfielddefn.cpp b/ogr/ogrfielddefn.cpp index 35482ce71211..5eec10518f4b 100644 --- a/ogr/ogrfielddefn.cpp +++ b/ogr/ogrfielddefn.cpp @@ -108,6 +108,61 @@ OGRFieldDefn::~OGRFieldDefn() CPLFree(pszDefault); } +/************************************************************************/ +/* OGRFieldDefn::OGRFieldDefn() */ +/************************************************************************/ + +/** + * @brief OGRFieldDefn::OGRFieldDefn copy constructor. + * @param oOther the object to copy. + * @since GDAL 3.11 + */ +OGRFieldDefn::OGRFieldDefn(const OGRFieldDefn &oOther) + : pszName(CPLStrdup(oOther.pszName)), + pszAlternativeName(CPLStrdup(oOther.pszAlternativeName)), + eType(oOther.eType), eJustify(oOther.eJustify), nWidth(oOther.nWidth), + nPrecision(oOther.nPrecision), pszDefault(CPLStrdup(oOther.pszDefault)), + bIgnore(oOther.bIgnore), eSubType(oOther.eSubType), + bNullable(oOther.bNullable), bUnique(oOther.bUnique), + m_nTZFlag(oOther.m_nTZFlag) +{ +} + +/************************************************************************/ +/* OGRFieldDefn::operator=() */ +/************************************************************************/ + +/** + * @brief OGRFieldDefn::operator = assignment operator. + * @param oOther the object to copy. + * @return the current object. + * @since GDAL 3.11 + */ +OGRFieldDefn &OGRFieldDefn::operator=(const OGRFieldDefn &oOther) +{ + if (&oOther != this) + { + CPLFree(pszName); + pszName = CPLStrdup(oOther.pszName); + CPLFree(pszAlternativeName); + pszAlternativeName = CPLStrdup(oOther.pszAlternativeName); + eType = oOther.eType; + eJustify = oOther.eJustify; + nWidth = oOther.nWidth; + nPrecision = oOther.nPrecision; + CPLFree(pszDefault); + pszDefault = CPLStrdup(oOther.pszDefault); + bIgnore = oOther.bIgnore; + eSubType = oOther.eSubType; + bNullable = oOther.bNullable; + bUnique = oOther.bUnique; + m_osDomainName = oOther.m_osDomainName; + m_osComment = oOther.m_osComment; + m_nTZFlag = oOther.m_nTZFlag; + } + return *this; +} + /************************************************************************/ /* OGR_Fld_Destroy() */ /************************************************************************/ diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index 4760e0c1c777..7f2f34b1ffab 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -123,6 +123,49 @@ OGRGeomFieldDefn::~OGRGeomFieldDefn() const_cast(poSRS)->Release(); } +/************************************************************************/ +/* OGRGeomFieldDefn::OGRGeomFieldDefn() */ +/************************************************************************/ + +/** + * @brief OGRGeomFieldDefn::OGRGeomFieldDefn Copy constructor + * @param oOther the OGRGeomFieldDefn to copy. + * @since GDAL 3.11 + */ +OGRGeomFieldDefn::OGRGeomFieldDefn(const OGRGeomFieldDefn &oOther) + : pszName(CPLStrdup(oOther.pszName)), eGeomType(oOther.eGeomType), + poSRS(nullptr), bNullable(oOther.bNullable), m_bSealed(false), + m_oCoordPrecision(oOther.m_oCoordPrecision) +{ + if (oOther.poSRS) + { + poSRS = oOther.poSRS->Clone(); + } +} + +/************************************************************************/ +/* OGRGeomFieldDefn::operator=() */ +/************************************************************************/ + +/** + * Copy assignment operator + * @param oOther the OGRGeomFieldDefn to copy. + * @return a reference to the current object. + * @since GDAL 3.11 + */ +OGRGeomFieldDefn &OGRGeomFieldDefn::operator=(const OGRGeomFieldDefn &oOther) +{ + if (&oOther != this) + { + SetName(oOther.pszName); + SetType(oOther.eGeomType); + SetSpatialRef(oOther.poSRS); + SetNullable(oOther.bNullable); + m_oCoordPrecision = oOther.m_oCoordPrecision; + } + return *this; +} + /************************************************************************/ /* OGR_GFld_Destroy() */ /************************************************************************/ diff --git a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h index d4621fbe7994..6895add8a5f9 100644 --- a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h +++ b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h @@ -191,6 +191,32 @@ class OGRSQLiteLayer CPL_NON_FINAL : public OGRLayer, bool m_bAllowMultipleGeomFields = false; + enum class FieldChangeType : char + { + ADD, + ALTER, + DELETE + }; + + // Store changes to the fields that happened inside a transaction + template struct FieldDefnChange + { + + FieldDefnChange(std::unique_ptr &&poFieldDefnIn, int iFieldIn, + FieldChangeType eChangeTypeIn) + : poFieldDefn(std::move(poFieldDefnIn)), iField(iFieldIn), + eChangeType(eChangeTypeIn) + { + } + + std::unique_ptr poFieldDefn; + int iField; + FieldChangeType eChangeType; + }; + + std::vector> m_apoFieldDefnChanges{}; + std::vector> m_apoGeomFieldDefnChanges{}; + static CPLString FormatSpatialFilterFromRTree( OGRGeometry *poFilterGeom, const char *pszRowIDName, const char *pszEscapedTable, const char *pszEscapedGeomCol); @@ -232,6 +258,10 @@ class OGRSQLiteLayer CPL_NON_FINAL : public OGRLayer, virtual OGRErr CommitTransaction() override; virtual OGRErr RollbackTransaction() override; + // Keep field definitions in sync with transactions + void PrepareStartTransaction(); + void FinishRollbackTransaction(); + virtual void InvalidateCachedFeatureCountAndExtent() { } @@ -777,6 +807,7 @@ class OGRSQLiteDataSource final : public OGRSQLiteBaseDataSource virtual OGRErr StartTransaction(int bForce = FALSE) override; virtual OGRErr CommitTransaction() override; virtual OGRErr RollbackTransaction() override; + bool IsInTransaction() const; virtual char **GetMetadata(const char *pszDomain = "") override; diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp index 48f0d6bf1c5a..920a4bc886de 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp @@ -4041,6 +4041,8 @@ OGRErr OGRSQLiteDataSource::StartTransaction(int bForce) cpl::down_cast(poLayer.get()); poTableLayer->RunDeferredCreationIfNecessary(); } + + poLayer->PrepareStartTransaction(); } return OGRSQLiteBaseDataSource::StartTransaction(bForce); @@ -4120,6 +4122,7 @@ OGRErr OGRSQLiteDataSource::RollbackTransaction() for (auto &poLayer : m_apoLayers) { + poLayer->FinishRollbackTransaction(); poLayer->InvalidateCachedFeatureCountAndExtent(); poLayer->ResetReading(); } @@ -4128,6 +4131,11 @@ OGRErr OGRSQLiteDataSource::RollbackTransaction() return OGRSQLiteBaseDataSource::RollbackTransaction(); } +bool OGRSQLiteDataSource::IsInTransaction() const +{ + return nSoftTransactionLevel > 0; +} + /************************************************************************/ /* SoftStartTransaction() */ /* */ diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp index a5ceb248b660..998232beee49 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp @@ -3535,7 +3535,6 @@ int OGRSQLiteLayer::TestCapability(const char *pszCap) /************************************************************************/ OGRErr OGRSQLiteLayer::StartTransaction() - { return m_poDS->StartTransaction(); } @@ -3545,7 +3544,6 @@ OGRErr OGRSQLiteLayer::StartTransaction() /************************************************************************/ OGRErr OGRSQLiteLayer::CommitTransaction() - { return m_poDS->CommitTransaction(); } @@ -3555,11 +3553,165 @@ OGRErr OGRSQLiteLayer::CommitTransaction() /************************************************************************/ OGRErr OGRSQLiteLayer::RollbackTransaction() - { return m_poDS->RollbackTransaction(); } +/************************************************************************/ +/* PrepareStartTransaction() */ +/************************************************************************/ +void OGRSQLiteLayer::PrepareStartTransaction() +{ + m_apoFieldDefnChanges.clear(); + m_apoGeomFieldDefnChanges.clear(); +} + +/************************************************************************/ +/* FinishRollbackTransaction() */ +/************************************************************************/ +void OGRSQLiteLayer::FinishRollbackTransaction() +{ + + // Deleted fields can be safely removed from the storage after being restored. + std::vector toBeRemoved; + int idx{0}; + + // Loop through all changed fields and reset them to their previous state. + for (int i = static_cast(m_apoFieldDefnChanges.size()) - 1; i >= 0; + i--) + { + auto &oFieldChange = m_apoFieldDefnChanges[i]; + const char *pszName = oFieldChange.poFieldDefn->GetNameRef(); + const int iField = oFieldChange.iField; + if (iField >= 0) + { + switch (oFieldChange.eChangeType) + { + case FieldChangeType::DELETE: + { + GetLayerDefn()->AddFieldDefn( + oFieldChange.poFieldDefn.get()); + + // Now move the field to the right place + // from the last position to its original position + const int iFieldCount = GetLayerDefn()->GetFieldCount(); + int *panOrder = + static_cast(CPLCalloc(iFieldCount, sizeof(int))); + for (int j = 0; j < oFieldChange.iField; j++) + { + panOrder[j] = j; + } + for (int j = oFieldChange.iField + 1; j < iFieldCount; j++) + { + panOrder[j] = j - 1; + } + panOrder[oFieldChange.iField] = iFieldCount - 1; + if (OGRERR_NONE == + GetLayerDefn()->ReorderFieldDefns(panOrder)) + { + toBeRemoved.push_back(idx); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore deleted field %s", pszName); + } + CPLFree(panOrder); + break; + } + case FieldChangeType::ALTER: + { + OGRFieldDefn *poFieldDefn = + GetLayerDefn()->GetFieldDefn(iField); + if (poFieldDefn) + { + *poFieldDefn = *oFieldChange.poFieldDefn; + toBeRemoved.push_back(idx); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore altered field %s", pszName); + } + break; + } + case FieldChangeType::ADD: + { + if (OGRFieldDefn *poFieldDef = + GetLayerDefn()->StealFieldDefn(oFieldChange.iField)) + { + oFieldChange.poFieldDefn.reset(poFieldDef); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to delete added field %s", pszName); + } + break; + } + } + ++idx; + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore field %s (field not found at index %d)", + pszName, oFieldChange.iField); + } + } + + // Remove from the storage the deleted fields that have been restored + for (const auto &i : toBeRemoved) + { + m_apoFieldDefnChanges.erase(m_apoFieldDefnChanges.begin() + i); + } + + // Loop through all changed geometry fields and reset them to their previous state. + for (int i = static_cast(m_apoGeomFieldDefnChanges.size()) - 1; i >= 0; + i--) + { + auto &oGeomFieldChange = m_apoGeomFieldDefnChanges[i]; + const char *pszName = oGeomFieldChange.poFieldDefn->GetNameRef(); + const int iGeomField = oGeomFieldChange.iField; + if (iGeomField >= 0) + { + switch (oGeomFieldChange.eChangeType) + { + case FieldChangeType::DELETE: + case FieldChangeType::ALTER: + { + // Currently not handled by OGR for geometry fields + break; + } + case FieldChangeType::ADD: + { + if (OGRGeomFieldDefn *poGeomFieldDef = + GetLayerDefn()->StealGeomFieldDefn( + oGeomFieldChange.iField)) + { + oGeomFieldChange.poFieldDefn.reset(poGeomFieldDef); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to delete added geometry field %s", + pszName); + } + break; + } + } + ++idx; + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore geometry field %s (field not found at " + "index %d)", + pszName, oGeomFieldChange.iField); + } + } +} + /************************************************************************/ /* ClearStatement() */ /************************************************************************/ diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp index 0878bedc3653..466ecbd5a674 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp @@ -1613,6 +1613,13 @@ OGRErr OGRSQLiteTableLayer::CreateField(const OGRFieldDefn *poFieldIn, /* -------------------------------------------------------------------- */ m_poFeatureDefn->AddFieldDefn(&oField); + if (m_poDS->IsInTransaction()) + { + m_apoFieldDefnChanges.emplace_back( + std::make_unique(oField), + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD); + } + if (m_pszFIDColumn != nullptr && EQUAL(oField.GetNameRef(), m_pszFIDColumn)) { m_iFIDAsRegularColumnIndex = m_poFeatureDefn->GetFieldCount() - 1; @@ -1710,6 +1717,13 @@ OGRSQLiteTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, } } + if (m_poDS->IsInTransaction()) + { + m_apoGeomFieldDefnChanges.emplace_back( + std::make_unique(*poGeomField), + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD); + } + m_poFeatureDefn->AddGeomFieldDefn(std::move(poGeomField)); if (!m_bDeferredCreation) @@ -2150,7 +2164,27 @@ OGRErr OGRSQLiteTableLayer::DeleteField(int iFieldToDelete) eErr = m_poDS->SoftCommitTransaction(); if (eErr == OGRERR_NONE) { - eErr = m_poFeatureDefn->DeleteFieldDefn(iFieldToDelete); + + if (m_poDS->IsInTransaction()) + { + std::unique_ptr poFieldDefn; + poFieldDefn.reset( + m_poFeatureDefn->StealFieldDefn(iFieldToDelete)); + if (poFieldDefn != nullptr) + { + m_apoFieldDefnChanges.emplace_back(std::move(poFieldDefn), + iFieldToDelete, + FieldChangeType::DELETE); + } + else + { + eErr = OGRERR_FAILURE; + } + } + else + { + eErr = m_poFeatureDefn->DeleteFieldDefn(iFieldToDelete); + } RecomputeOrdinals(); @@ -2373,6 +2407,13 @@ OGRErr OGRSQLiteTableLayer::AlterFieldDefn(int iFieldToAlter, OGRFieldDefn *poFieldDefn = m_poFeatureDefn->GetFieldDefn(iFieldToAlter); + if (m_poDS->IsInTransaction()) + { + m_apoFieldDefnChanges.emplace_back( + std::make_unique(poFieldDefn), iFieldToAlter, + FieldChangeType::ALTER); + } + if (nActualFlags & ALTER_TYPE_FLAG) { int iIdx = 0; From 01189c0573c0f593ecb7ba79d7c272c53f2f3ecf Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 8 Jan 2025 16:55:32 +0100 Subject: [PATCH 02/12] Address PR comments --- ogr/ogr_feature.h | 8 ++--- ogr/ogrfeaturedefn.cpp | 9 +++--- ogr/ogrgeomfielddefn.cpp | 8 +++++ ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 29 ++++++++++--------- .../sqlite/ogrsqlitetablelayer.cpp | 9 ++++-- 5 files changed, 38 insertions(+), 25 deletions(-) diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index 56ccd3e28067..7da2e57948c8 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -655,11 +655,11 @@ class CPL_DLL OGRFeatureDefn * it from the feature definition. * This is a mechanism for the application to take over ownership of the field definition from the feature definition without copying. * @param iField index of the field definition to detach. - * @return a pointer to the detached field definition or nullptr if the index is out of range. + * @return a unique pointer to the detached field definition or nullptr if the index is out of range. * @note The caller is responsible for deleting the returned field definition. * @since GDAL 3.11 */ - virtual OGRFieldDefn *StealFieldDefn(int iField); + virtual std::unique_ptr StealFieldDefn(int iField); virtual OGRErr ReorderFieldDefns(const int *panMap); @@ -668,11 +668,11 @@ class CPL_DLL OGRFeatureDefn * detaching it from the feature definition . * This is a mechanism for the application to take over ownership of the geometry field definition from the feature definition without copying. * @param iField index of the geometry field definition to detach. - * @return a pointer to the detached geometry field definition or nullptr if the index is out of range. + * @return a unique pointer to the detached geometry field definition or nullptr if the index is out of range. * @note The caller is responsible for deleting the returned field definition. * @since GDAL 3.11 */ - virtual OGRGeomFieldDefn *StealGeomFieldDefn(int iField); + virtual std::unique_ptr StealGeomFieldDefn(int iField); virtual int GetGeomFieldCount() const; virtual OGRGeomFieldDefn *GetGeomFieldDefn(int i); diff --git a/ogr/ogrfeaturedefn.cpp b/ogr/ogrfeaturedefn.cpp index 945d516d4a4e..d9cd37ee2703 100644 --- a/ogr/ogrfeaturedefn.cpp +++ b/ogr/ogrfeaturedefn.cpp @@ -486,7 +486,7 @@ OGRErr OGRFeatureDefn::DeleteFieldDefn(int iField) /* StealGeomFieldDefn() */ /************************************************************************/ -OGRGeomFieldDefn *OGRFeatureDefn::StealGeomFieldDefn(int iField) +std::unique_ptr OGRFeatureDefn::StealGeomFieldDefn(int iField) { if (m_bSealed) { @@ -498,7 +498,8 @@ OGRGeomFieldDefn *OGRFeatureDefn::StealGeomFieldDefn(int iField) if (iField < 0 || iField >= GetGeomFieldCount()) return nullptr; - OGRGeomFieldDefn *poFieldDef{apoGeomFieldDefn.at(iField).release()}; + std::unique_ptr poFieldDef = + std::move(apoGeomFieldDefn.at(iField)); apoGeomFieldDefn.erase(apoGeomFieldDefn.begin() + iField); return poFieldDef; } @@ -507,7 +508,7 @@ OGRGeomFieldDefn *OGRFeatureDefn::StealGeomFieldDefn(int iField) /* StealFieldDefn() */ /************************************************************************/ -OGRFieldDefn *OGRFeatureDefn::StealFieldDefn(int iField) +std::unique_ptr OGRFeatureDefn::StealFieldDefn(int iField) { if (m_bSealed) { @@ -519,7 +520,7 @@ OGRFieldDefn *OGRFeatureDefn::StealFieldDefn(int iField) if (iField < 0 || iField >= GetFieldCount()) return nullptr; - OGRFieldDefn *poFDef{apoFieldDefn.at(iField).release()}; + std::unique_ptr poFDef = std::move(apoFieldDefn.at(iField)); apoFieldDefn.erase(apoFieldDefn.begin() + iField); return poFDef; } diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index 7f2f34b1ffab..42a429da62dc 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -563,6 +563,11 @@ OGRSpatialReferenceH OGR_GFld_GetSpatialRef(OGRGeomFieldDefnH hDefn) */ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) { + if (poSRS == poSRSIn) + { + return; + } + if (m_bSealed) { CPLError( @@ -570,9 +575,12 @@ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) "OGRGeomFieldDefn::SetSpatialRef() not allowed on a sealed object"); return; } + if (poSRS != nullptr) const_cast(poSRS)->Release(); + poSRS = poSRSIn; + if (poSRS != nullptr) const_cast(poSRS)->Reference(); } diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp index 998232beee49..9ad641918925 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp @@ -3595,19 +3595,18 @@ void OGRSQLiteLayer::FinishRollbackTransaction() // Now move the field to the right place // from the last position to its original position const int iFieldCount = GetLayerDefn()->GetFieldCount(); - int *panOrder = - static_cast(CPLCalloc(iFieldCount, sizeof(int))); + std::vector anOrder(iFieldCount); for (int j = 0; j < oFieldChange.iField; j++) { - panOrder[j] = j; + anOrder[j] = j; } for (int j = oFieldChange.iField + 1; j < iFieldCount; j++) { - panOrder[j] = j - 1; + anOrder[j] = j - 1; } - panOrder[oFieldChange.iField] = iFieldCount - 1; + anOrder[oFieldChange.iField] = iFieldCount - 1; if (OGRERR_NONE == - GetLayerDefn()->ReorderFieldDefns(panOrder)) + GetLayerDefn()->ReorderFieldDefns(anOrder.data())) { toBeRemoved.push_back(idx); } @@ -3616,7 +3615,6 @@ void OGRSQLiteLayer::FinishRollbackTransaction() CPLError(CE_Failure, CPLE_AppDefined, "Failed to restore deleted field %s", pszName); } - CPLFree(panOrder); break; } case FieldChangeType::ALTER: @@ -3637,10 +3635,11 @@ void OGRSQLiteLayer::FinishRollbackTransaction() } case FieldChangeType::ADD: { - if (OGRFieldDefn *poFieldDef = - GetLayerDefn()->StealFieldDefn(oFieldChange.iField)) + std::unique_ptr poFieldDef = + GetLayerDefn()->StealFieldDefn(oFieldChange.iField); + if (poFieldDef) { - oFieldChange.poFieldDefn.reset(poFieldDef); + oFieldChange.poFieldDefn = std::move(poFieldDef); } else { @@ -3685,11 +3684,13 @@ void OGRSQLiteLayer::FinishRollbackTransaction() } case FieldChangeType::ADD: { - if (OGRGeomFieldDefn *poGeomFieldDef = - GetLayerDefn()->StealGeomFieldDefn( - oGeomFieldChange.iField)) + std::unique_ptr poGeomFieldDef = + GetLayerDefn()->StealGeomFieldDefn( + oGeomFieldChange.iField); + if (poGeomFieldDef) { - oGeomFieldChange.poFieldDefn.reset(poGeomFieldDef); + oGeomFieldChange.poFieldDefn = + std::move(poGeomFieldDef); } else { diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp index 466ecbd5a674..6f84866e7b8a 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp @@ -1717,6 +1717,8 @@ OGRSQLiteTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, } } + // Add to the list of changes BEFORE adding it to the feature definition + // because poGeomField is a unique ptr. if (m_poDS->IsInTransaction()) { m_apoGeomFieldDefnChanges.emplace_back( @@ -2165,12 +2167,13 @@ OGRErr OGRSQLiteTableLayer::DeleteField(int iFieldToDelete) if (eErr == OGRERR_NONE) { + // Keep the field definition alive until a new transaction is started + // or the layer is destroyed. if (m_poDS->IsInTransaction()) { std::unique_ptr poFieldDefn; - poFieldDefn.reset( - m_poFeatureDefn->StealFieldDefn(iFieldToDelete)); - if (poFieldDefn != nullptr) + poFieldDefn = m_poFeatureDefn->StealFieldDefn(iFieldToDelete); + if (poFieldDefn) { m_apoFieldDefnChanges.emplace_back(std::move(poFieldDefn), iFieldToDelete, From b4bc8cd8316ab271ddb4a21006d40a793c3cd0ec Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 8 Jan 2025 17:35:34 +0100 Subject: [PATCH 03/12] Fix order after failing test test_ogr.OGRGeomFieldDefn_sealing --- autotest/ogr/ogr_sqlite.py | 3 +++ ogr/ogrfielddefn.cpp | 4 +++- ogr/ogrgeomfielddefn.cpp | 16 +++++++++------- ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 1 + 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/autotest/ogr/ogr_sqlite.py b/autotest/ogr/ogr_sqlite.py index adde5325cd9b..6daafec6f95e 100755 --- a/autotest/ogr/ogr_sqlite.py +++ b/autotest/ogr/ogr_sqlite.py @@ -4619,6 +4619,9 @@ def test_field_rollback(tmp_path): del lyr del ds + # Bisect the test to avoid a crash on Windows/MacOS + return + # Reopen the database. ds = ogr.Open(temp_file, update=True) lyr = ds.GetLayerByName("testlyr") diff --git a/ogr/ogrfielddefn.cpp b/ogr/ogrfielddefn.cpp index 5eec10518f4b..917580faccf2 100644 --- a/ogr/ogrfielddefn.cpp +++ b/ogr/ogrfielddefn.cpp @@ -124,7 +124,8 @@ OGRFieldDefn::OGRFieldDefn(const OGRFieldDefn &oOther) nPrecision(oOther.nPrecision), pszDefault(CPLStrdup(oOther.pszDefault)), bIgnore(oOther.bIgnore), eSubType(oOther.eSubType), bNullable(oOther.bNullable), bUnique(oOther.bUnique), - m_nTZFlag(oOther.m_nTZFlag) + m_osDomainName(oOther.m_osDomainName), m_osComment(oOther.m_osComment), + m_nTZFlag(oOther.m_nTZFlag), m_bSealed(oOther.m_bSealed) { } @@ -159,6 +160,7 @@ OGRFieldDefn &OGRFieldDefn::operator=(const OGRFieldDefn &oOther) m_osDomainName = oOther.m_osDomainName; m_osComment = oOther.m_osComment; m_nTZFlag = oOther.m_nTZFlag; + m_bSealed = oOther.m_bSealed; } return *this; } diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index 42a429da62dc..1295af196802 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -134,8 +134,8 @@ OGRGeomFieldDefn::~OGRGeomFieldDefn() */ OGRGeomFieldDefn::OGRGeomFieldDefn(const OGRGeomFieldDefn &oOther) : pszName(CPLStrdup(oOther.pszName)), eGeomType(oOther.eGeomType), - poSRS(nullptr), bNullable(oOther.bNullable), m_bSealed(false), - m_oCoordPrecision(oOther.m_oCoordPrecision) + poSRS(nullptr), bIgnore(oOther.bIgnore), bNullable(oOther.bNullable), + m_bSealed(oOther.m_bSealed), m_oCoordPrecision(oOther.m_oCoordPrecision) { if (oOther.poSRS) { @@ -162,6 +162,8 @@ OGRGeomFieldDefn &OGRGeomFieldDefn::operator=(const OGRGeomFieldDefn &oOther) SetSpatialRef(oOther.poSRS); SetNullable(oOther.bNullable); m_oCoordPrecision = oOther.m_oCoordPrecision; + m_bSealed = oOther.m_bSealed; + bIgnore = oOther.bIgnore; } return *this; } @@ -563,11 +565,6 @@ OGRSpatialReferenceH OGR_GFld_GetSpatialRef(OGRGeomFieldDefnH hDefn) */ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) { - if (poSRS == poSRSIn) - { - return; - } - if (m_bSealed) { CPLError( @@ -576,6 +573,11 @@ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) return; } + if (poSRS == poSRSIn) + { + return; + } + if (poSRS != nullptr) const_cast(poSRS)->Release(); diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp index 9ad641918925..bf2f71f3c39b 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp @@ -3581,6 +3581,7 @@ void OGRSQLiteLayer::FinishRollbackTransaction() i--) { auto &oFieldChange = m_apoFieldDefnChanges[i]; + CPLAssert(oFieldChange.poFieldDefn); const char *pszName = oFieldChange.poFieldDefn->GetNameRef(); const int iField = oFieldChange.iField; if (iField >= 0) From 90f441a13df3360ee55995eeeeef8ad3e7efe5cd Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 10:04:33 +0100 Subject: [PATCH 04/12] Fix erase index --- autotest/ogr/ogr_sqlite.py | 3 --- ogr/ogr_feature.h | 8 +++----- ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 19 +++++++++---------- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/autotest/ogr/ogr_sqlite.py b/autotest/ogr/ogr_sqlite.py index 6daafec6f95e..adde5325cd9b 100755 --- a/autotest/ogr/ogr_sqlite.py +++ b/autotest/ogr/ogr_sqlite.py @@ -4619,9 +4619,6 @@ def test_field_rollback(tmp_path): del lyr del ds - # Bisect the test to avoid a crash on Windows/MacOS - return - # Reopen the database. ds = ogr.Open(temp_file, update=True) lyr = ds.GetLayerByName("testlyr") diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index 7da2e57948c8..6a2aec4cf101 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -653,10 +653,9 @@ class CPL_DLL OGRFeatureDefn /** * @brief StealFieldDefn takes ownership of the field definition at index detaching * it from the feature definition. - * This is a mechanism for the application to take over ownership of the field definition from the feature definition without copying. + * This is an advanced method designed to be only used for driver implementations. * @param iField index of the field definition to detach. * @return a unique pointer to the detached field definition or nullptr if the index is out of range. - * @note The caller is responsible for deleting the returned field definition. * @since GDAL 3.11 */ virtual std::unique_ptr StealFieldDefn(int iField); @@ -665,11 +664,10 @@ class CPL_DLL OGRFeatureDefn /** * @brief StealGeomFieldDefn takes ownership of the the geometry field definition at index - * detaching it from the feature definition . - * This is a mechanism for the application to take over ownership of the geometry field definition from the feature definition without copying. + * detaching it from the feature definition. + * This is an advanced method designed to be only used for driver implementations. * @param iField index of the geometry field definition to detach. * @return a unique pointer to the detached geometry field definition or nullptr if the index is out of range. - * @note The caller is responsible for deleting the returned field definition. * @since GDAL 3.11 */ virtual std::unique_ptr StealGeomFieldDefn(int iField); diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp index bf2f71f3c39b..b5b3781eb90d 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp @@ -3574,7 +3574,6 @@ void OGRSQLiteLayer::FinishRollbackTransaction() // Deleted fields can be safely removed from the storage after being restored. std::vector toBeRemoved; - int idx{0}; // Loop through all changed fields and reset them to their previous state. for (int i = static_cast(m_apoFieldDefnChanges.size()) - 1; i >= 0; @@ -3596,20 +3595,22 @@ void OGRSQLiteLayer::FinishRollbackTransaction() // Now move the field to the right place // from the last position to its original position const int iFieldCount = GetLayerDefn()->GetFieldCount(); + CPLAssert(iFieldCount > 0); + CPLAssert(iFieldCount > iField); std::vector anOrder(iFieldCount); - for (int j = 0; j < oFieldChange.iField; j++) + for (int j = 0; j < iField; j++) { anOrder[j] = j; } - for (int j = oFieldChange.iField + 1; j < iFieldCount; j++) + for (int j = iField + 1; j < iFieldCount; j++) { anOrder[j] = j - 1; } - anOrder[oFieldChange.iField] = iFieldCount - 1; + anOrder[iField] = iFieldCount - 1; if (OGRERR_NONE == GetLayerDefn()->ReorderFieldDefns(anOrder.data())) { - toBeRemoved.push_back(idx); + toBeRemoved.push_back(i); } else { @@ -3625,7 +3626,7 @@ void OGRSQLiteLayer::FinishRollbackTransaction() if (poFieldDefn) { *poFieldDefn = *oFieldChange.poFieldDefn; - toBeRemoved.push_back(idx); + toBeRemoved.push_back(i); } else { @@ -3637,7 +3638,7 @@ void OGRSQLiteLayer::FinishRollbackTransaction() case FieldChangeType::ADD: { std::unique_ptr poFieldDef = - GetLayerDefn()->StealFieldDefn(oFieldChange.iField); + GetLayerDefn()->StealFieldDefn(iField); if (poFieldDef) { oFieldChange.poFieldDefn = std::move(poFieldDef); @@ -3650,13 +3651,12 @@ void OGRSQLiteLayer::FinishRollbackTransaction() break; } } - ++idx; } else { CPLError(CE_Failure, CPLE_AppDefined, "Failed to restore field %s (field not found at index %d)", - pszName, oFieldChange.iField); + pszName, iField); } } @@ -3702,7 +3702,6 @@ void OGRSQLiteLayer::FinishRollbackTransaction() break; } } - ++idx; } else { From 9ceb401ca77991ad5cb58377e7677a487e81f940 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 15:36:44 +0100 Subject: [PATCH 05/12] Refactor rollback and implement for GPKG --- autotest/ogr/ogr_gpkg.py | 11 + autotest/ogr/ogr_sqlite.py | 194 +----------------- autotest/pymod/ogrtest.py | 138 +++++++++++++ ogr/ogr_feature.h | 2 + ogr/ogrfeaturedefn.cpp | 31 ++- ogr/ogrgeomfielddefn.cpp | 8 - ogr/ogrsf_frmts/generic/ogrlayer.cpp | 159 ++++++++++++++ .../gpkg/ogrgeopackagetablelayer.cpp | 48 ++++- ogr/ogrsf_frmts/ogrsf_frmts.h | 33 +++ ogr/ogrsf_frmts/sqlite/ogr_sqlite.h | 30 --- .../sqlite/ogrsqlitedatasource.cpp | 9 +- ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 156 -------------- 12 files changed, 423 insertions(+), 396 deletions(-) diff --git a/autotest/ogr/ogr_gpkg.py b/autotest/ogr/ogr_gpkg.py index c7b3dc13dda5..ad353c841165 100755 --- a/autotest/ogr/ogr_gpkg.py +++ b/autotest/ogr/ogr_gpkg.py @@ -10908,3 +10908,14 @@ def test_ogr_gpkg_arrow_stream_numpy_datetime_as_string(tmp_vsimem): assert batch["datetime"][1] == b"2022-05-31T12:34:56.789Z" assert batch["datetime"][2] == b"2022-05-31T12:34:56.000" assert batch["datetime"][3] == b"2022-05-31T12:34:56.000+12:30" + + +############################################################################### +# Test field operations rolling back changes. + + +def test_ogr_gpkg_field_operations_rollback(tmp_vsimem): + + filename = str(tmp_vsimem / "test.gpkg") + with ogr.GetDriverByName("GPKG").CreateDataSource(filename) as ds: + ogrtest.check_transaction_rollback(ds, test_geometry=False) diff --git a/autotest/ogr/ogr_sqlite.py b/autotest/ogr/ogr_sqlite.py index adde5325cd9b..898dd65168cd 100755 --- a/autotest/ogr/ogr_sqlite.py +++ b/autotest/ogr/ogr_sqlite.py @@ -4545,196 +4545,8 @@ def test_ogr_sqlite_schema_override( def test_field_rollback(tmp_path): - temp_file = str(tmp_path / "tmp.db") + filename = str(tmp_path / "test_field_rollback.db") # Create a new database. - ds = ogr.GetDriverByName("SQLite").CreateDataSource(temp_file) - - # Create a layer. - lyr = ds.CreateLayer("testlyr") - - # Add a field. - fld_defn = ogr.FieldDefn("fld1", ogr.OFTInteger) - lyr.CreateField(fld_defn) - - # Add a feature. - feat = ogr.Feature(lyr.GetLayerDefn()) - feat.SetField("fld1", 1) - lyr.CreateFeature(feat) - - del lyr - del ds - del feat - - # Reopen the database. - ds = ogr.Open(temp_file, update=True) - lyr = ds.GetLayerByName("testlyr") - assert lyr is not None - - # Verify the field. - fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) - assert fld_defn.GetName() == "fld1" - assert fld_defn.GetType() == ogr.OFTInteger - assert lyr.GetLayerDefn().GetFieldDefn("fld2") is None - - # Verify the feature. - feat = lyr.GetNextFeature() - assert feat.GetField("fld1") == 1 - del feat - - # Start a transaction. - assert ds.StartTransaction() == ogr.OGRERR_NONE - lyr.CreateField(ogr.FieldDefn("fld2")) - new_fld_defn = lyr.GetLayerDefn().GetFieldDefn("fld2") - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - assert fld_defn.GetName() == "fld1" - assert lyr.GetLayerDefn().GetFieldDefn("fld2") is None - # This should not crash: - assert new_fld_defn.GetName() == "fld2" - - # Start a transaction and alter the field. - assert ds.StartTransaction() == ogr.OGRERR_NONE - assert ( - lyr.AlterFieldDefn(0, ogr.FieldDefn("fld1", ogr.OFTReal), ogr.ALTER_ALL_FLAG) - == ogr.OGRERR_NONE - ) - fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) - - assert fld_defn.GetType() == ogr.OFTReal - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - new_fld_defn = lyr.GetLayerDefn().GetFieldDefn("fld1") - assert new_fld_defn.GetType() == ogr.OFTInteger - # This should not crash: - assert fld_defn.GetName() == "fld1" - - # Start a transaction and delete the field. - assert ds.StartTransaction() == ogr.OGRERR_NONE - lyr.CreateField(ogr.FieldDefn("fld2")) - assert ds.CommitTransaction() == ogr.OGRERR_NONE - fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) - assert fld_defn.GetName() == "fld1" - assert fld_defn.GetType() == ogr.OFTInteger - assert lyr.GetLayerDefn().GetFieldDefn("fld2") is not None - - del lyr - del ds - - # Reopen the database. - ds = ogr.Open(temp_file, update=True) - lyr = ds.GetLayerByName("testlyr") - assert lyr is not None - - # Check that we have added foo field. - fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) - assert fld_defn.GetName() == "fld1" - assert fld_defn.GetType() == ogr.OFTInteger - assert lyr.GetLayerDefn().GetFieldDefn("fld2") is not None - - # Start a transaction and alter fld2 and delete the fld1. - assert ds.StartTransaction() == ogr.OGRERR_NONE - - assert ( - lyr.AlterFieldDefn(1, ogr.FieldDefn("fld_xxx", ogr.OFTReal), ogr.ALTER_ALL_FLAG) - == ogr.OGRERR_NONE - ) - assert lyr.DeleteField(0) == ogr.OGRERR_NONE - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - - # Check that we have not altered fld2 and deleted fld1. - fld_defn = lyr.GetLayerDefn().GetFieldDefn(0) - assert fld_defn.GetName() == "fld1" - assert fld_defn.GetType() == ogr.OFTInteger - assert lyr.GetLayerDefn().GetFieldDefn(1).GetName() == "fld2" - assert lyr.GetLayerDefn().GetFieldDefn("fld_xxx") is None - - del lyr - del ds - - # Reopen the database. - ds = ogr.Open(temp_file, update=True) - lyr = ds.GetLayerByName("testlyr") - assert lyr is not None - - # Create a list of fields - assert ds.StartTransaction() == ogr.OGRERR_NONE - for i in range(3, 11): - assert ( - lyr.CreateField(ogr.FieldDefn("fld" + str(i), ogr.OFTReal)) - == ogr.OGRERR_NONE - ) - assert ds.CommitTransaction() == ogr.OGRERR_NONE - assert lyr.GetLayerDefn().GetFieldCount() == 10 - for i in range(10): - fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) - assert fld_defn.GetName() == "fld" + str(i + 1) - - # Start a transaction and delete all even fields. - assert ds.StartTransaction() == ogr.OGRERR_NONE - for i in range(9, 0, -2): - print("Delete field", lyr.GetLayerDefn().GetFieldDefn(i).GetName()) - assert lyr.DeleteField(i) == ogr.OGRERR_NONE - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - # Verify that we have not deleted any field. - for i in range(10): - fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) - assert fld_defn.GetName() == "fld" + str(i + 1) - - # Start a transaction and delete all odd fields. - assert ds.StartTransaction() == ogr.OGRERR_NONE - for i in range(8, -1, -2): - print("Delete field", lyr.GetLayerDefn().GetFieldDefn(i).GetName()) - assert lyr.DeleteField(i) == ogr.OGRERR_NONE - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - # Verify that we have not deleted any field. - for i in range(10): - fld_defn = lyr.GetLayerDefn().GetFieldDefn(i) - assert fld_defn.GetName() == "fld" + str(i + 1) - - -###################################################################### -# Test geometry field operations rolling back changes. -# - - -def test_geom_field_rollback(tmp_path): - - temp_file = str(tmp_path / "tmp.db") - - # Create a new database. - ds = ogr.GetDriverByName("SQLite").CreateDataSource(temp_file) - - # Create a layer. - lyr = ds.CreateLayer("testlyr") - - # Add a field. - fld_defn = ogr.FieldDefn("fld1", ogr.OFTInteger) - lyr.CreateField(fld_defn) - - assert lyr.GetGeometryColumn() == "GEOMETRY" - - # Start a transaction and add a geometry column. - assert ds.StartTransaction() == ogr.OGRERR_NONE - assert ( - lyr.CreateGeomField(ogr.GeomFieldDefn("GEOMETRY_2", ogr.wkbPoint)) - == ogr.OGRERR_NONE - ) - assert lyr.GetGeometryColumn() == "GEOMETRY" - assert lyr.GetLayerDefn().GetGeomFieldCount() == 2 - - # Create a feature. - feat = ogr.Feature(lyr.GetLayerDefn()) - feat.SetField("fld1", 1) - feat.SetGeomFieldDirectly("GEOMETRY", ogr.CreateGeometryFromWkt("POINT(1 2)")) - feat.SetGeomFieldDirectly("GEOMETRY_2", ogr.CreateGeometryFromWkt("POINT(3 4)")) - lyr.CreateFeature(feat) - - # Verify the feature. - feat = lyr.GetNextFeature() - assert feat.GetField("fld1") == 1 - assert feat.GetGeomFieldRef(0).ExportToWkt() == "POINT (1 2)" - assert feat.GetGeomFieldRef(1).ExportToWkt() == "POINT (3 4)" - assert ds.RollbackTransaction() == ogr.OGRERR_NONE - - # Verify that we have not added GEOMETRY_2 field. - assert lyr.GetGeometryColumn() == "GEOMETRY" - assert lyr.GetLayerDefn().GetGeomFieldCount() == 1 + with ogr.GetDriverByName("SQLite").CreateDataSource(filename) as ds: + ogrtest.check_transaction_rollback(ds, test_geometry=True) diff --git a/autotest/pymod/ogrtest.py b/autotest/pymod/ogrtest.py index 904d274143a8..956764c01a37 100755 --- a/autotest/pymod/ogrtest.py +++ b/autotest/pymod/ogrtest.py @@ -405,3 +405,141 @@ def spatial_filter(lyr, *args): yield finally: lyr.SetSpatialFilter(None) + + +############################################################################### +# Check transactions rollback, to be called with a freshly created datasource + + +def check_transaction_rollback(ds, test_geometry=False): + + lyr = ds.CreateLayer("test", options=["GEOMETRY_NAME=geom"]) + lyr.CreateField(ogr.FieldDefn("fld1", ogr.OFTString)) + lyr.CreateField(ogr.FieldDefn("fld2", ogr.OFTString)) + + # Insert a feature + f = ogr.Feature(lyr.GetLayerDefn()) + f.SetField("fld1", "value1") + f.SetField("fld2", "value2") + assert lyr.CreateFeature(f) == ogr.OGRERR_NONE + + fld1 = lyr.GetLayerDefn().GetFieldDefn(0) + fld2 = lyr.GetLayerDefn().GetFieldDefn(1) + + def verify(lyr, fld1, fld2): + assert lyr.GetGeometryColumn() == "geom" + assert lyr.GetLayerDefn().GetGeomFieldCount() == 1 + assert lyr.GetLayerDefn().GetFieldCount() == 2 + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld1" + assert lyr.GetLayerDefn().GetFieldDefn(0).GetType() == ogr.OFTString + assert lyr.GetLayerDefn().GetFieldDefn(1).GetName() == "fld2" + assert lyr.GetLayerDefn().GetFieldDefn(1).GetType() == ogr.OFTString + # Test do not crash + assert fld1.GetName() == "fld1" + assert fld2.GetName() == "fld2" + + # Test deleting a field + ds.StartTransaction() + lyr.DeleteField(0) + # Test do not crash + assert fld1.GetName() == "fld1" + assert lyr.GetLayerDefn().GetFieldCount() == 1 + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld2" + ds.RollbackTransaction() + verify(lyr, fld1, fld2) + + # Test deleting the second field + ds.StartTransaction() + lyr.DeleteField(1) + assert lyr.GetLayerDefn().GetFieldCount() == 1 + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld1" + ds.RollbackTransaction() + verify(lyr, fld1, fld2) + + # Test renaming and changing the type of a field + fld1 = lyr.GetLayerDefn().GetFieldDefn(0) + assert fld1.GetName() == "fld1" + ds.StartTransaction() + assert ( + lyr.AlterFieldDefn( + 0, ogr.FieldDefn("fld1_renamed", ogr.OFTInteger), ogr.ALTER_ALL_FLAG + ) + == ogr.OGRERR_NONE + ) + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld1_renamed" + assert lyr.GetLayerDefn().GetFieldDefn(0).GetType() == ogr.OFTInteger + assert fld1.GetName() == "fld1_renamed" + ds.RollbackTransaction() + verify(lyr, fld1, fld2) + + # Test adding a field + assert lyr.GetLayerDefn().GetFieldCount() == 2 + ds.StartTransaction() + fld = ogr.FieldDefn("fld3", ogr.OFTInteger) + assert lyr.CreateField(fld) == ogr.OGRERR_NONE + assert lyr.GetLayerDefn().GetFieldCount() == 3 + fld3 = lyr.GetLayerDefn().GetFieldDefn(2) + assert fld3.GetName() == "fld3" + ds.RollbackTransaction() + verify(lyr, fld1, fld2) + # Test fld3 does not crash + assert fld3.GetName() == "fld3" + + # Test multiple operations + ds.StartTransaction() + lyr.DeleteField(0) + assert lyr.GetLayerDefn().GetFieldCount() == 1 + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld2" + # Add a field + fld = ogr.FieldDefn("fld3", ogr.OFTInteger) + assert lyr.CreateField(fld) == ogr.OGRERR_NONE + assert lyr.GetLayerDefn().GetFieldCount() == 2 + assert lyr.GetLayerDefn().GetFieldDefn(1).GetName() == "fld3" + # Rename fld2 + assert ( + lyr.AlterFieldDefn( + 0, ogr.FieldDefn("fld2_renamed", ogr.OFTInteger), ogr.ALTER_ALL_FLAG + ) + == ogr.OGRERR_NONE + ) + assert lyr.GetLayerDefn().GetFieldDefn(0).GetName() == "fld2_renamed" + assert lyr.GetLayerDefn().GetFieldDefn(0).GetType() == ogr.OFTInteger + assert lyr.GetLayerDefn().GetFieldDefn(1).GetName() == "fld3" + assert lyr.GetLayerDefn().GetFieldDefn(1).GetType() == ogr.OFTInteger + ds.RollbackTransaction() + verify(lyr, fld1, fld2) + + if not test_geometry: + return + + # Start a transaction and add a geometry column. + assert ds.StartTransaction() == ogr.OGRERR_NONE + assert ( + lyr.CreateGeomField(ogr.GeomFieldDefn("GEOMETRY_2", ogr.wkbPoint)) + == ogr.OGRERR_NONE + ) + assert lyr.GetGeometryColumn() == "geom" + assert lyr.GetLayerDefn().GetGeomFieldCount() == 2 + + # Create a feature. + feat = ogr.Feature(lyr.GetLayerDefn()) + feat.SetField("fld1", "value1-2") + feat.SetField("fld2", "value2-2") + feat.SetGeomFieldDirectly("geom", ogr.CreateGeometryFromWkt("POINT(1 2)")) + feat.SetGeomFieldDirectly("GEOMETRY_2", ogr.CreateGeometryFromWkt("POINT(3 4)")) + lyr.CreateFeature(feat) + + # Verify the feature. + feat = lyr.GetNextFeature() + feat = lyr.GetNextFeature() + assert feat.GetField("fld1") == "value1-2" + assert feat.GetField("fld2") == "value2-2" + assert feat.GetGeomFieldRef(0).ExportToWkt() == "POINT (1 2)" + assert feat.GetGeomFieldRef(1).ExportToWkt() == "POINT (3 4)" + assert ds.RollbackTransaction() == ogr.OGRERR_NONE + + # Verify that we have not added GEOMETRY_2 field. + assert lyr.GetGeometryColumn() == "geom" + assert lyr.GetLayerDefn().GetGeomFieldCount() == 1 + + verify(lyr, fld1, fld2) diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index 6a2aec4cf101..861cf3dd22c1 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -660,6 +660,8 @@ class CPL_DLL OGRFeatureDefn */ virtual std::unique_ptr StealFieldDefn(int iField); + virtual void AddFieldDefn(std::unique_ptr &&poFieldDefn); + virtual OGRErr ReorderFieldDefns(const int *panMap); /** diff --git a/ogr/ogrfeaturedefn.cpp b/ogr/ogrfeaturedefn.cpp index d9cd37ee2703..14faa367bf24 100644 --- a/ogr/ogrfeaturedefn.cpp +++ b/ogr/ogrfeaturedefn.cpp @@ -417,6 +417,30 @@ void OGRFeatureDefn::AddFieldDefn(const OGRFieldDefn *poNewDefn) apoFieldDefn.emplace_back(std::make_unique(poNewDefn)); } +/** + * \brief Add a new field definition taking ownership of the passed field. + * + * To add a new field definition to a layer definition, do not use this + * function directly, but use OGRLayer::CreateField() instead. + * + * This method should only be called while there are no OGRFeature + * objects in existence based on this OGRFeatureDefn. + * + * @param poNewDefn the definition of the new field. + */ + +void OGRFeatureDefn::AddFieldDefn(std::unique_ptr &&poFieldDefn) +{ + if (m_bSealed) + { + CPLError( + CE_Failure, CPLE_AppDefined, + "OGRFeatureDefn::AddFieldDefn() not allowed on a sealed object"); + return; + } + apoFieldDefn.push_back(std::move(poFieldDefn)); +} + /************************************************************************/ /* OGR_FD_AddFieldDefn() */ /************************************************************************/ @@ -510,13 +534,6 @@ std::unique_ptr OGRFeatureDefn::StealGeomFieldDefn(int iField) std::unique_ptr OGRFeatureDefn::StealFieldDefn(int iField) { - if (m_bSealed) - { - CPLError( - CE_Failure, CPLE_AppDefined, - "OGRFeatureDefn::StealFieldDefn() not allowed on a sealed object"); - return nullptr; - } if (iField < 0 || iField >= GetFieldCount()) return nullptr; diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index 1295af196802..aacece2482b2 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -565,14 +565,6 @@ OGRSpatialReferenceH OGR_GFld_GetSpatialRef(OGRGeomFieldDefnH hDefn) */ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) { - if (m_bSealed) - { - CPLError( - CE_Failure, CPLE_AppDefined, - "OGRGeomFieldDefn::SetSpatialRef() not allowed on a sealed object"); - return; - } - if (poSRS == poSRSIn) { return; diff --git a/ogr/ogrsf_frmts/generic/ogrlayer.cpp b/ogr/ogrsf_frmts/generic/ogrlayer.cpp index 1047d3c74ff3..9e141468e802 100644 --- a/ogr/ogrsf_frmts/generic/ogrlayer.cpp +++ b/ogr/ogrsf_frmts/generic/ogrlayer.cpp @@ -1899,6 +1899,165 @@ bool OGRLayer::FilterWKBGeometry(const GByte *pabyWKB, size_t nWKBSize, return false; } +/************************************************************************/ +/* PrepareStartTransaction() */ +/************************************************************************/ + +void OGRLayer::PrepareStartTransaction() +{ + m_apoFieldDefnChanges.clear(); + m_apoGeomFieldDefnChanges.clear(); +} + +/************************************************************************/ +/* FinishRollbackTransaction() */ +/************************************************************************/ + +void OGRLayer::FinishRollbackTransaction() +{ + + // Deleted fields can be safely removed from the storage after being restored. + std::vector toBeRemoved; + + // Loop through all changed fields and reset them to their previous state. + for (int i = static_cast(m_apoFieldDefnChanges.size()) - 1; i >= 0; + i--) + { + auto &oFieldChange = m_apoFieldDefnChanges[i]; + CPLAssert(oFieldChange.poFieldDefn); + const char *pszName = oFieldChange.poFieldDefn->GetNameRef(); + const int iField = oFieldChange.iField; + if (iField >= 0) + { + switch (oFieldChange.eChangeType) + { + case FieldChangeType::DELETE: + { + // Transfer ownership of the field to the layer + whileUnsealing(GetLayerDefn()) + ->AddFieldDefn(std::move(oFieldChange.poFieldDefn)); + + // Now move the field to the right place + // from the last position to its original position + const int iFieldCount = GetLayerDefn()->GetFieldCount(); + CPLAssert(iFieldCount > 0); + CPLAssert(iFieldCount > iField); + std::vector anOrder(iFieldCount); + for (int j = 0; j < iField; j++) + { + anOrder[j] = j; + } + for (int j = iField + 1; j < iFieldCount; j++) + { + anOrder[j] = j - 1; + } + anOrder[iField] = iFieldCount - 1; + if (OGRERR_NONE == whileUnsealing(GetLayerDefn()) + ->ReorderFieldDefns(anOrder.data())) + { + toBeRemoved.push_back(i); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore deleted field %s", pszName); + } + break; + } + case FieldChangeType::ALTER: + { + OGRFieldDefn *poFieldDefn = + GetLayerDefn()->GetFieldDefn(iField); + if (poFieldDefn) + { + *poFieldDefn = *oFieldChange.poFieldDefn; + toBeRemoved.push_back(i); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore altered field %s", pszName); + } + break; + } + case FieldChangeType::ADD: + { + std::unique_ptr poFieldDef = + GetLayerDefn()->StealFieldDefn(iField); + if (poFieldDef) + { + oFieldChange.poFieldDefn = std::move(poFieldDef); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to delete added field %s", pszName); + } + break; + } + } + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore field %s (field not found at index %d)", + pszName, iField); + } + } + + // Remove from the storage the deleted fields that have been restored + for (const auto &i : toBeRemoved) + { + m_apoFieldDefnChanges.erase(m_apoFieldDefnChanges.begin() + i); + } + + // Loop through all changed geometry fields and reset them to their previous state. + for (int i = static_cast(m_apoGeomFieldDefnChanges.size()) - 1; i >= 0; + i--) + { + auto &oGeomFieldChange = m_apoGeomFieldDefnChanges[i]; + const char *pszName = oGeomFieldChange.poFieldDefn->GetNameRef(); + const int iGeomField = oGeomFieldChange.iField; + if (iGeomField >= 0) + { + switch (oGeomFieldChange.eChangeType) + { + case FieldChangeType::DELETE: + case FieldChangeType::ALTER: + { + // Currently not handled by OGR for geometry fields + break; + } + case FieldChangeType::ADD: + { + std::unique_ptr poGeomFieldDef = + GetLayerDefn()->StealGeomFieldDefn( + oGeomFieldChange.iField); + if (poGeomFieldDef) + { + oGeomFieldChange.poFieldDefn = + std::move(poGeomFieldDef); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to delete added geometry field %s", + pszName); + } + break; + } + } + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Failed to restore geometry field %s (field not found at " + "index %d)", + pszName, oGeomFieldChange.iField); + } + } +} + //! @endcond /************************************************************************/ diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp index 386f77ca5582..b40a135a6c98 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp @@ -1845,6 +1845,13 @@ OGRErr OGRGeoPackageTableLayer::CreateField(const OGRFieldDefn *poField, whileUnsealing(m_poFeatureDefn)->AddFieldDefn(&oFieldDefn); + if (m_poDS->IsInTransaction()) + { + m_apoFieldDefnChanges.emplace_back( + std::make_unique(oFieldDefn), + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD); + } + m_abGeneratedColumns.resize(m_poFeatureDefn->GetFieldCount()); if (m_pszFidColumn != nullptr && @@ -1967,7 +1974,7 @@ OGRGeoPackageTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, if (m_poFeatureDefn->GetGeomFieldCount() == 1) { CPLError(CE_Failure, CPLE_AppDefined, - "Cannot create more than on geometry field in GeoPackage"); + "Cannot create more than one geometry field in GeoPackage"); return OGRERR_FAILURE; } @@ -2018,6 +2025,13 @@ OGRGeoPackageTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, return err; } + if (m_poDS->IsInTransaction()) + { + m_apoGeomFieldDefnChanges.emplace_back( + std::make_unique(oGeomField), + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD); + } + whileUnsealing(m_poFeatureDefn)->AddGeomFieldDefn(&oGeomField); if (!m_bDeferredCreation) @@ -6567,8 +6581,27 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete) eErr = m_poDS->SoftCommitTransaction(); if (eErr == OGRERR_NONE) { - eErr = whileUnsealing(m_poFeatureDefn) - ->DeleteFieldDefn(iFieldToDelete); + + if (m_poDS->IsInTransaction()) + { + auto poFieldDefn{whileUnsealing(m_poFeatureDefn) + ->StealFieldDefn(iFieldToDelete)}; + if (poFieldDefn) + { + m_apoFieldDefnChanges.emplace_back(std::move(poFieldDefn), + iFieldToDelete, + FieldChangeType::DELETE); + } + else + { + eErr = OGRERR_FAILURE; + } + } + else + { + eErr = whileUnsealing(m_poFeatureDefn) + ->DeleteFieldDefn(iFieldToDelete); + } if (eErr == OGRERR_NONE) { @@ -7014,6 +7047,7 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter, /* -------------------------------------------------------------------- */ if (eErr == OGRERR_NONE) { + eErr = m_poDS->SoftCommitTransaction(); // We need to force database reopening due to schema change @@ -7059,6 +7093,14 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter, if (eErr == OGRERR_NONE) { + + if (m_poDS->IsInTransaction()) + { + m_apoFieldDefnChanges.emplace_back( + std::make_unique(poFieldDefnToAlter), + iFieldToAlter, FieldChangeType::ALTER); + } + auto oTemporaryUnsealer(poFieldDefnToAlter->GetTemporaryUnsealer()); bool bNeedsEntryInGpkgDataColumns = false; diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.h b/ogr/ogrsf_frmts/ogrsf_frmts.h index ea78856d03be..88f490bf29d4 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.h +++ b/ogr/ogrsf_frmts/ogrsf_frmts.h @@ -283,6 +283,12 @@ class CPL_DLL OGRLayer : public GDALMajorObject virtual OGRErr CommitTransaction() CPL_WARN_UNUSED_RESULT; virtual OGRErr RollbackTransaction(); + //! @cond Doxygen_Suppress + // Keep field definitions in sync with transactions + void PrepareStartTransaction(); + void FinishRollbackTransaction(); + //! @endcond + virtual const char *GetFIDColumn(); virtual const char *GetGeometryColumn(); @@ -395,6 +401,33 @@ class CPL_DLL OGRLayer : public GDALMajorObject protected: //! @cond Doxygen_Suppress + + enum class FieldChangeType : char + { + ADD, + ALTER, + DELETE + }; + + // Store changes to the fields that happened inside a transaction + template struct FieldDefnChange + { + + FieldDefnChange(std::unique_ptr &&poFieldDefnIn, int iFieldIn, + FieldChangeType eChangeTypeIn) + : poFieldDefn(std::move(poFieldDefnIn)), iField(iFieldIn), + eChangeType(eChangeTypeIn) + { + } + + std::unique_ptr poFieldDefn; + int iField; + FieldChangeType eChangeType; + }; + + std::vector> m_apoFieldDefnChanges{}; + std::vector> m_apoGeomFieldDefnChanges{}; + OGRStyleTable *m_poStyleTable; OGRFeatureQuery *m_poAttrQuery; char *m_pszAttrQueryString; diff --git a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h index 6895add8a5f9..8f70475b7c63 100644 --- a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h +++ b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h @@ -191,32 +191,6 @@ class OGRSQLiteLayer CPL_NON_FINAL : public OGRLayer, bool m_bAllowMultipleGeomFields = false; - enum class FieldChangeType : char - { - ADD, - ALTER, - DELETE - }; - - // Store changes to the fields that happened inside a transaction - template struct FieldDefnChange - { - - FieldDefnChange(std::unique_ptr &&poFieldDefnIn, int iFieldIn, - FieldChangeType eChangeTypeIn) - : poFieldDefn(std::move(poFieldDefnIn)), iField(iFieldIn), - eChangeType(eChangeTypeIn) - { - } - - std::unique_ptr poFieldDefn; - int iField; - FieldChangeType eChangeType; - }; - - std::vector> m_apoFieldDefnChanges{}; - std::vector> m_apoGeomFieldDefnChanges{}; - static CPLString FormatSpatialFilterFromRTree( OGRGeometry *poFilterGeom, const char *pszRowIDName, const char *pszEscapedTable, const char *pszEscapedGeomCol); @@ -258,10 +232,6 @@ class OGRSQLiteLayer CPL_NON_FINAL : public OGRLayer, virtual OGRErr CommitTransaction() override; virtual OGRErr RollbackTransaction() override; - // Keep field definitions in sync with transactions - void PrepareStartTransaction(); - void FinishRollbackTransaction(); - virtual void InvalidateCachedFeatureCountAndExtent() { } diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp index 920a4bc886de..b133594200d2 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp @@ -4102,6 +4102,14 @@ OGRErr OGRSQLiteBaseDataSource::RollbackTransaction() bUserTransactionActive = false; CPLAssert(nSoftTransactionLevel == 1); + + // Loop through all layers and finish transaction + for (int i = 0; i < GetLayerCount(); i++) + { + OGRLayer *poLayer = GetLayer(i); + poLayer->FinishRollbackTransaction(); + } + return SoftRollbackTransaction(); } @@ -4122,7 +4130,6 @@ OGRErr OGRSQLiteDataSource::RollbackTransaction() for (auto &poLayer : m_apoLayers) { - poLayer->FinishRollbackTransaction(); poLayer->InvalidateCachedFeatureCountAndExtent(); poLayer->ResetReading(); } diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp index b5b3781eb90d..839b273e9790 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp @@ -3557,162 +3557,6 @@ OGRErr OGRSQLiteLayer::RollbackTransaction() return m_poDS->RollbackTransaction(); } -/************************************************************************/ -/* PrepareStartTransaction() */ -/************************************************************************/ -void OGRSQLiteLayer::PrepareStartTransaction() -{ - m_apoFieldDefnChanges.clear(); - m_apoGeomFieldDefnChanges.clear(); -} - -/************************************************************************/ -/* FinishRollbackTransaction() */ -/************************************************************************/ -void OGRSQLiteLayer::FinishRollbackTransaction() -{ - - // Deleted fields can be safely removed from the storage after being restored. - std::vector toBeRemoved; - - // Loop through all changed fields and reset them to their previous state. - for (int i = static_cast(m_apoFieldDefnChanges.size()) - 1; i >= 0; - i--) - { - auto &oFieldChange = m_apoFieldDefnChanges[i]; - CPLAssert(oFieldChange.poFieldDefn); - const char *pszName = oFieldChange.poFieldDefn->GetNameRef(); - const int iField = oFieldChange.iField; - if (iField >= 0) - { - switch (oFieldChange.eChangeType) - { - case FieldChangeType::DELETE: - { - GetLayerDefn()->AddFieldDefn( - oFieldChange.poFieldDefn.get()); - - // Now move the field to the right place - // from the last position to its original position - const int iFieldCount = GetLayerDefn()->GetFieldCount(); - CPLAssert(iFieldCount > 0); - CPLAssert(iFieldCount > iField); - std::vector anOrder(iFieldCount); - for (int j = 0; j < iField; j++) - { - anOrder[j] = j; - } - for (int j = iField + 1; j < iFieldCount; j++) - { - anOrder[j] = j - 1; - } - anOrder[iField] = iFieldCount - 1; - if (OGRERR_NONE == - GetLayerDefn()->ReorderFieldDefns(anOrder.data())) - { - toBeRemoved.push_back(i); - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to restore deleted field %s", pszName); - } - break; - } - case FieldChangeType::ALTER: - { - OGRFieldDefn *poFieldDefn = - GetLayerDefn()->GetFieldDefn(iField); - if (poFieldDefn) - { - *poFieldDefn = *oFieldChange.poFieldDefn; - toBeRemoved.push_back(i); - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to restore altered field %s", pszName); - } - break; - } - case FieldChangeType::ADD: - { - std::unique_ptr poFieldDef = - GetLayerDefn()->StealFieldDefn(iField); - if (poFieldDef) - { - oFieldChange.poFieldDefn = std::move(poFieldDef); - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to delete added field %s", pszName); - } - break; - } - } - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to restore field %s (field not found at index %d)", - pszName, iField); - } - } - - // Remove from the storage the deleted fields that have been restored - for (const auto &i : toBeRemoved) - { - m_apoFieldDefnChanges.erase(m_apoFieldDefnChanges.begin() + i); - } - - // Loop through all changed geometry fields and reset them to their previous state. - for (int i = static_cast(m_apoGeomFieldDefnChanges.size()) - 1; i >= 0; - i--) - { - auto &oGeomFieldChange = m_apoGeomFieldDefnChanges[i]; - const char *pszName = oGeomFieldChange.poFieldDefn->GetNameRef(); - const int iGeomField = oGeomFieldChange.iField; - if (iGeomField >= 0) - { - switch (oGeomFieldChange.eChangeType) - { - case FieldChangeType::DELETE: - case FieldChangeType::ALTER: - { - // Currently not handled by OGR for geometry fields - break; - } - case FieldChangeType::ADD: - { - std::unique_ptr poGeomFieldDef = - GetLayerDefn()->StealGeomFieldDefn( - oGeomFieldChange.iField); - if (poGeomFieldDef) - { - oGeomFieldChange.poFieldDefn = - std::move(poGeomFieldDef); - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to delete added geometry field %s", - pszName); - } - break; - } - } - } - else - { - CPLError(CE_Failure, CPLE_AppDefined, - "Failed to restore geometry field %s (field not found at " - "index %d)", - pszName, oGeomFieldChange.iField); - } - } -} - /************************************************************************/ /* ClearStatement() */ /************************************************************************/ From ad0dc2890dab895a81513d5d7283bb1c5aa72058 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 15:42:19 +0100 Subject: [PATCH 06/12] doxy --- ogr/ogrfeaturedefn.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ogr/ogrfeaturedefn.cpp b/ogr/ogrfeaturedefn.cpp index 14faa367bf24..54437fbe189d 100644 --- a/ogr/ogrfeaturedefn.cpp +++ b/ogr/ogrfeaturedefn.cpp @@ -429,7 +429,7 @@ void OGRFeatureDefn::AddFieldDefn(const OGRFieldDefn *poNewDefn) * @param poNewDefn the definition of the new field. */ -void OGRFeatureDefn::AddFieldDefn(std::unique_ptr &&poFieldDefn) +void OGRFeatureDefn::AddFieldDefn(std::unique_ptr &&poNewDefn) { if (m_bSealed) { @@ -438,7 +438,7 @@ void OGRFeatureDefn::AddFieldDefn(std::unique_ptr &&poFieldDefn) "OGRFeatureDefn::AddFieldDefn() not allowed on a sealed object"); return; } - apoFieldDefn.push_back(std::move(poFieldDefn)); + apoFieldDefn.push_back(std::move(poNewDefn)); } /************************************************************************/ From c7681650a7a217d456fa6d0080398a403cf8ebaf Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 16:20:49 +0100 Subject: [PATCH 07/12] return geom field seal error --- ogr/ogrgeomfielddefn.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index aacece2482b2..9942cf2c8ec1 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -565,6 +565,15 @@ OGRSpatialReferenceH OGR_GFld_GetSpatialRef(OGRGeomFieldDefnH hDefn) */ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) { + + if (m_bSealed) + { + CPLError( + CE_Failure, CPLE_AppDefined, + "OGRGeomFieldDefn::SetSpatialRef() not allowed on a sealed object"); + return; + } + if (poSRS == poSRSIn) { return; From 3cb9d2639cd2893652272aed8e9af3fe37b79512 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 16:35:21 +0100 Subject: [PATCH 08/12] Update ogr/ogrfielddefn.cpp Co-authored-by: Even Rouault --- ogr/ogrfielddefn.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ogr/ogrfielddefn.cpp b/ogr/ogrfielddefn.cpp index 917580faccf2..c32b18491eaf 100644 --- a/ogr/ogrfielddefn.cpp +++ b/ogr/ogrfielddefn.cpp @@ -121,7 +121,8 @@ OGRFieldDefn::OGRFieldDefn(const OGRFieldDefn &oOther) : pszName(CPLStrdup(oOther.pszName)), pszAlternativeName(CPLStrdup(oOther.pszAlternativeName)), eType(oOther.eType), eJustify(oOther.eJustify), nWidth(oOther.nWidth), - nPrecision(oOther.nPrecision), pszDefault(CPLStrdup(oOther.pszDefault)), + nPrecision(oOther.nPrecision), + pszDefault(oOther.pszDefault ? CPLStrdup(oOther.pszDefault) : nullptr), bIgnore(oOther.bIgnore), eSubType(oOther.eSubType), bNullable(oOther.bNullable), bUnique(oOther.bUnique), m_osDomainName(oOther.m_osDomainName), m_osComment(oOther.m_osComment), From 4759e694d08da562f3492fd240e283ec987c3d7b Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 16:35:32 +0100 Subject: [PATCH 09/12] Dox --- ogr/ogrsf_frmts/ogrsf_frmts.dox | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.dox b/ogr/ogrsf_frmts/ogrsf_frmts.dox index 4c8577921e7c..896f8fb410d3 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.dox +++ b/ogr/ogrsf_frmts/ogrsf_frmts.dox @@ -3049,6 +3049,31 @@ form depending on the limitations of the format driver. This function is the same as the C function OGR_L_RollbackTransaction(). + + OGRFeature* instances acquired or created between the StartTransaction() and RollbackTransaction() should + be destroyed before RollbackTransaction() if the field structure has been modified during the transaction. + + In particular, the following is invalid: + + \code + lyr->StartTransaction(); + lyr->DeleteField(...); + f = new OGRFeature(lyr->GetLayerDefn()); + lyr->RollbackTransaction(); + // f is in a inconsistent state at this point, given its array of fields doesn't match + // the updated layer definition, and thus it cannot even be safely deleted ! + \endcode + + Instead, the feature should be destroyed before the rollback: + + \code + lyr->StartTransaction(); + lyr->DeleteField(...); + f = new OGRFeature(lyr->GetLayerDefn()); + ... + delete f; + \encode + @return OGRERR_NONE on success. */ From bd3df401b154d21437b035b3a2be9f0e6153c107 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 16:42:44 +0100 Subject: [PATCH 10/12] initialize pszDefault --- ogr/ogrfielddefn.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ogr/ogrfielddefn.cpp b/ogr/ogrfielddefn.cpp index c32b18491eaf..bb9c234fc580 100644 --- a/ogr/ogrfielddefn.cpp +++ b/ogr/ogrfielddefn.cpp @@ -153,7 +153,7 @@ OGRFieldDefn &OGRFieldDefn::operator=(const OGRFieldDefn &oOther) nWidth = oOther.nWidth; nPrecision = oOther.nPrecision; CPLFree(pszDefault); - pszDefault = CPLStrdup(oOther.pszDefault); + pszDefault = oOther.pszDefault ? CPLStrdup(oOther.pszDefault) : nullptr; bIgnore = oOther.bIgnore; eSubType = oOther.eSubType; bNullable = oOther.bNullable; From 029d627f32fbb6b1c2e3f0c994a82b7e4c693ab6 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 9 Jan 2025 17:08:06 +0100 Subject: [PATCH 11/12] Add _FIELD suffix to fix windows build errors --- ogr/ogrsf_frmts/generic/ogrlayer.cpp | 12 ++++++------ ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp | 12 ++++++------ ogr/ogrsf_frmts/ogrsf_frmts.h | 6 +++--- ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp | 12 ++++++------ 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/ogr/ogrsf_frmts/generic/ogrlayer.cpp b/ogr/ogrsf_frmts/generic/ogrlayer.cpp index 9e141468e802..3870e7d358f5 100644 --- a/ogr/ogrsf_frmts/generic/ogrlayer.cpp +++ b/ogr/ogrsf_frmts/generic/ogrlayer.cpp @@ -1931,7 +1931,7 @@ void OGRLayer::FinishRollbackTransaction() { switch (oFieldChange.eChangeType) { - case FieldChangeType::DELETE: + case FieldChangeType::DELETE_FIELD: { // Transfer ownership of the field to the layer whileUnsealing(GetLayerDefn()) @@ -1964,7 +1964,7 @@ void OGRLayer::FinishRollbackTransaction() } break; } - case FieldChangeType::ALTER: + case FieldChangeType::ALTER_FIELD: { OGRFieldDefn *poFieldDefn = GetLayerDefn()->GetFieldDefn(iField); @@ -1980,7 +1980,7 @@ void OGRLayer::FinishRollbackTransaction() } break; } - case FieldChangeType::ADD: + case FieldChangeType::ADD_FIELD: { std::unique_ptr poFieldDef = GetLayerDefn()->StealFieldDefn(iField); @@ -2022,13 +2022,13 @@ void OGRLayer::FinishRollbackTransaction() { switch (oGeomFieldChange.eChangeType) { - case FieldChangeType::DELETE: - case FieldChangeType::ALTER: + case FieldChangeType::DELETE_FIELD: + case FieldChangeType::ALTER_FIELD: { // Currently not handled by OGR for geometry fields break; } - case FieldChangeType::ADD: + case FieldChangeType::ADD_FIELD: { std::unique_ptr poGeomFieldDef = GetLayerDefn()->StealGeomFieldDefn( diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp index b40a135a6c98..5f289d8c6da5 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp @@ -1849,7 +1849,7 @@ OGRErr OGRGeoPackageTableLayer::CreateField(const OGRFieldDefn *poField, { m_apoFieldDefnChanges.emplace_back( std::make_unique(oFieldDefn), - m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD); + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD); } m_abGeneratedColumns.resize(m_poFeatureDefn->GetFieldCount()); @@ -2029,7 +2029,7 @@ OGRGeoPackageTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, { m_apoGeomFieldDefnChanges.emplace_back( std::make_unique(oGeomField), - m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD); + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD); } whileUnsealing(m_poFeatureDefn)->AddGeomFieldDefn(&oGeomField); @@ -6588,9 +6588,9 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete) ->StealFieldDefn(iFieldToDelete)}; if (poFieldDefn) { - m_apoFieldDefnChanges.emplace_back(std::move(poFieldDefn), - iFieldToDelete, - FieldChangeType::DELETE); + m_apoFieldDefnChanges.emplace_back( + std::move(poFieldDefn), iFieldToDelete, + FieldChangeType::DELETE_FIELD); } else { @@ -7098,7 +7098,7 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter, { m_apoFieldDefnChanges.emplace_back( std::make_unique(poFieldDefnToAlter), - iFieldToAlter, FieldChangeType::ALTER); + iFieldToAlter, FieldChangeType::ALTER_FIELD); } auto oTemporaryUnsealer(poFieldDefnToAlter->GetTemporaryUnsealer()); diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.h b/ogr/ogrsf_frmts/ogrsf_frmts.h index 88f490bf29d4..c9cefb6955f3 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.h +++ b/ogr/ogrsf_frmts/ogrsf_frmts.h @@ -404,9 +404,9 @@ class CPL_DLL OGRLayer : public GDALMajorObject enum class FieldChangeType : char { - ADD, - ALTER, - DELETE + ADD_FIELD, + ALTER_FIELD, + DELETE_FIELD }; // Store changes to the fields that happened inside a transaction diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp index 6f84866e7b8a..80a2fe120349 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp @@ -1617,7 +1617,7 @@ OGRErr OGRSQLiteTableLayer::CreateField(const OGRFieldDefn *poFieldIn, { m_apoFieldDefnChanges.emplace_back( std::make_unique(oField), - m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD); + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD); } if (m_pszFIDColumn != nullptr && EQUAL(oField.GetNameRef(), m_pszFIDColumn)) @@ -1723,7 +1723,7 @@ OGRSQLiteTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, { m_apoGeomFieldDefnChanges.emplace_back( std::make_unique(*poGeomField), - m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD); + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD); } m_poFeatureDefn->AddGeomFieldDefn(std::move(poGeomField)); @@ -2175,9 +2175,9 @@ OGRErr OGRSQLiteTableLayer::DeleteField(int iFieldToDelete) poFieldDefn = m_poFeatureDefn->StealFieldDefn(iFieldToDelete); if (poFieldDefn) { - m_apoFieldDefnChanges.emplace_back(std::move(poFieldDefn), - iFieldToDelete, - FieldChangeType::DELETE); + m_apoFieldDefnChanges.emplace_back( + std::move(poFieldDefn), iFieldToDelete, + FieldChangeType::DELETE_FIELD); } else { @@ -2414,7 +2414,7 @@ OGRErr OGRSQLiteTableLayer::AlterFieldDefn(int iFieldToAlter, { m_apoFieldDefnChanges.emplace_back( std::make_unique(poFieldDefn), iFieldToAlter, - FieldChangeType::ALTER); + FieldChangeType::ALTER_FIELD); } if (nActualFlags & ALTER_TYPE_FLAG) From 5ab85431a94874a26fc63c0c8c5bfb9afa71986e Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 9 Jan 2025 19:07:15 +0100 Subject: [PATCH 12/12] Update ogr/ogrsf_frmts/ogrsf_frmts.dox --- ogr/ogrsf_frmts/ogrsf_frmts.dox | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.dox b/ogr/ogrsf_frmts/ogrsf_frmts.dox index 896f8fb410d3..d39476a4e545 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.dox +++ b/ogr/ogrsf_frmts/ogrsf_frmts.dox @@ -3072,7 +3072,7 @@ form depending on the limitations of the format driver. f = new OGRFeature(lyr->GetLayerDefn()); ... delete f; - \encode + \endcode @return OGRERR_NONE on success.