From add4a0d67d7a336ee7f09ab668d2f737cbc774cc Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Tue, 7 Jan 2025 12:34:20 +0100 Subject: [PATCH] [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;