From 4cc3cd9eddf4d7d3e682491f06cdb77026f593d1 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 10 Jan 2025 15:28:57 +0100 Subject: [PATCH] GPKG and SQLite: Fix out of sync (not restored) fields after a ROLLBACK (#11609) --- autotest/ogr/ogr_gpkg.py | 11 ++ autotest/ogr/ogr_sqlite.py | 14 ++ autotest/pymod/ogrtest.py | 138 +++++++++++++++ ogr/ogr_feature.h | 41 ++++- ogr/ogrfeaturedefn.cpp | 61 ++++++- ogr/ogrfielddefn.cpp | 58 +++++++ ogr/ogrgeomfielddefn.cpp | 54 ++++++ ogr/ogrsf_frmts/generic/ogrlayer.cpp | 159 ++++++++++++++++++ .../gpkg/ogrgeopackagetablelayer.cpp | 48 +++++- ogr/ogrsf_frmts/ogrsf_frmts.dox | 25 +++ ogr/ogrsf_frmts/ogrsf_frmts.h | 33 ++++ ogr/ogrsf_frmts/sqlite/ogr_sqlite.h | 1 + .../sqlite/ogrsqlitedatasource.cpp | 15 ++ ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp | 3 - .../sqlite/ogrsqlitetablelayer.cpp | 46 ++++- 15 files changed, 693 insertions(+), 14 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 a7094a1658c0..898dd65168cd 100755 --- a/autotest/ogr/ogr_sqlite.py +++ b/autotest/ogr/ogr_sqlite.py @@ -4536,3 +4536,17 @@ 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): + + filename = str(tmp_path / "test_field_rollback.db") + + # Create a new database. + 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 b8004bc2ccec..b6e1bd181ee9 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -94,6 +94,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 @@ -254,9 +260,6 @@ class CPL_DLL OGRFieldDefn /*! @endcond */ TemporaryUnsealer GetTemporaryUnsealer(); - - private: - CPL_DISALLOW_COPY_ASSIGN(OGRFieldDefn) }; #ifdef GDAL_COMPILATION @@ -324,6 +327,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 @@ -417,9 +426,6 @@ class CPL_DLL OGRGeomFieldDefn /*! @endcond */ TemporaryUnsealer GetTemporaryUnsealer(); - - private: - CPL_DISALLOW_COPY_ASSIGN(OGRGeomFieldDefn) }; #ifdef GDAL_COMPILATION @@ -618,8 +624,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 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. + * @since GDAL 3.11 + */ + virtual std::unique_ptr StealFieldDefn(int iField); + + virtual void AddFieldDefn(std::unique_ptr &&poFieldDefn); + 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 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. + * @since GDAL 3.11 + */ + virtual std::unique_ptr 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..54437fbe189d 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 &&poNewDefn) +{ + if (m_bSealed) + { + CPLError( + CE_Failure, CPLE_AppDefined, + "OGRFeatureDefn::AddFieldDefn() not allowed on a sealed object"); + return; + } + apoFieldDefn.push_back(std::move(poNewDefn)); +} + /************************************************************************/ /* OGR_FD_AddFieldDefn() */ /************************************************************************/ @@ -482,6 +506,42 @@ OGRErr OGRFeatureDefn::DeleteFieldDefn(int iField) return OGRERR_NONE; } +/************************************************************************/ +/* StealGeomFieldDefn() */ +/************************************************************************/ + +std::unique_ptr 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; + + std::unique_ptr poFieldDef = + std::move(apoGeomFieldDefn.at(iField)); + apoGeomFieldDefn.erase(apoGeomFieldDefn.begin() + iField); + return poFieldDef; +} + +/************************************************************************/ +/* StealFieldDefn() */ +/************************************************************************/ + +std::unique_ptr OGRFeatureDefn::StealFieldDefn(int iField) +{ + if (iField < 0 || iField >= GetFieldCount()) + return nullptr; + + std::unique_ptr poFDef = std::move(apoFieldDefn.at(iField)); + apoFieldDefn.erase(apoFieldDefn.begin() + iField); + return poFDef; +} + /************************************************************************/ /* OGR_FD_DeleteFieldDefn() */ /************************************************************************/ @@ -533,7 +593,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..bb9c234fc580 100644 --- a/ogr/ogrfielddefn.cpp +++ b/ogr/ogrfielddefn.cpp @@ -108,6 +108,64 @@ 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(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), + m_nTZFlag(oOther.m_nTZFlag), m_bSealed(oOther.m_bSealed) +{ +} + +/************************************************************************/ +/* 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 = 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; + m_nTZFlag = oOther.m_nTZFlag; + m_bSealed = oOther.m_bSealed; + } + return *this; +} + /************************************************************************/ /* OGR_Fld_Destroy() */ /************************************************************************/ diff --git a/ogr/ogrgeomfielddefn.cpp b/ogr/ogrgeomfielddefn.cpp index 4760e0c1c777..9942cf2c8ec1 100644 --- a/ogr/ogrgeomfielddefn.cpp +++ b/ogr/ogrgeomfielddefn.cpp @@ -123,6 +123,51 @@ 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), bIgnore(oOther.bIgnore), bNullable(oOther.bNullable), + m_bSealed(oOther.m_bSealed), 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; + m_bSealed = oOther.m_bSealed; + bIgnore = oOther.bIgnore; + } + return *this; +} + /************************************************************************/ /* OGR_GFld_Destroy() */ /************************************************************************/ @@ -520,6 +565,7 @@ OGRSpatialReferenceH OGR_GFld_GetSpatialRef(OGRGeomFieldDefnH hDefn) */ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) { + if (m_bSealed) { CPLError( @@ -527,9 +573,17 @@ void OGRGeomFieldDefn::SetSpatialRef(const OGRSpatialReference *poSRSIn) "OGRGeomFieldDefn::SetSpatialRef() not allowed on a sealed object"); return; } + + if (poSRS == poSRSIn) + { + return; + } + if (poSRS != nullptr) const_cast(poSRS)->Release(); + poSRS = poSRSIn; + if (poSRS != nullptr) const_cast(poSRS)->Reference(); } diff --git a/ogr/ogrsf_frmts/generic/ogrlayer.cpp b/ogr/ogrsf_frmts/generic/ogrlayer.cpp index 1047d3c74ff3..3870e7d358f5 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_FIELD: + { + // 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_FIELD: + { + 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_FIELD: + { + 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_FIELD: + case FieldChangeType::ALTER_FIELD: + { + // Currently not handled by OGR for geometry fields + break; + } + case FieldChangeType::ADD_FIELD: + { + 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..5f289d8c6da5 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_FIELD); + } + 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_FIELD); + } + 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_FIELD); + } + 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_FIELD); + } + auto oTemporaryUnsealer(poFieldDefnToAlter->GetTemporaryUnsealer()); bool bNeedsEntryInGpkgDataColumns = false; diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.dox b/ogr/ogrsf_frmts/ogrsf_frmts.dox index 4c8577921e7c..d39476a4e545 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; + \endcode + @return OGRERR_NONE on success. */ diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.h b/ogr/ogrsf_frmts/ogrsf_frmts.h index ea78856d03be..c9cefb6955f3 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_FIELD, + ALTER_FIELD, + DELETE_FIELD + }; + + // 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 d4621fbe7994..8f70475b7c63 100644 --- a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h +++ b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h @@ -777,6 +777,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..b133594200d2 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); @@ -4100,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(); } @@ -4128,6 +4138,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..839b273e9790 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,7 +3553,6 @@ OGRErr OGRSQLiteLayer::CommitTransaction() /************************************************************************/ OGRErr OGRSQLiteLayer::RollbackTransaction() - { return m_poDS->RollbackTransaction(); } diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp index 0878bedc3653..80a2fe120349 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_FIELD); + } + if (m_pszFIDColumn != nullptr && EQUAL(oField.GetNameRef(), m_pszFIDColumn)) { m_iFIDAsRegularColumnIndex = m_poFeatureDefn->GetFieldCount() - 1; @@ -1710,6 +1717,15 @@ 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( + std::make_unique(*poGeomField), + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD); + } + m_poFeatureDefn->AddGeomFieldDefn(std::move(poGeomField)); if (!m_bDeferredCreation) @@ -2150,7 +2166,28 @@ OGRErr OGRSQLiteTableLayer::DeleteField(int iFieldToDelete) eErr = m_poDS->SoftCommitTransaction(); if (eErr == OGRERR_NONE) { - eErr = m_poFeatureDefn->DeleteFieldDefn(iFieldToDelete); + + // 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 = m_poFeatureDefn->StealFieldDefn(iFieldToDelete); + if (poFieldDefn) + { + m_apoFieldDefnChanges.emplace_back( + std::move(poFieldDefn), iFieldToDelete, + FieldChangeType::DELETE_FIELD); + } + else + { + eErr = OGRERR_FAILURE; + } + } + else + { + eErr = m_poFeatureDefn->DeleteFieldDefn(iFieldToDelete); + } RecomputeOrdinals(); @@ -2373,6 +2410,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_FIELD); + } + if (nActualFlags & ALTER_TYPE_FLAG) { int iIdx = 0;