[SQLite] Transaction field handling: intial implementation

Fix out of sync (not restored) fields after a ROLLBACK.

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

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
@@ -643,8 +646,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 iField 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 iField 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 iField
+     *        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 iField 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;

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)
     {

ogr/ogrfielddefn.cpp

@@ -108,6 +108,51 @@ OGRFieldDefn::~OGRFieldDefn()
     CPLFree(pszDefault);
 }
 
+/**
+ * @brief OGRFieldDefn::OGRFieldDefn copy constructor.
+ * @param oOther the object to copy.
+ */
+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)
+{
+}
+
+/**
+ * @brief OGRFieldDefn::operator = assignment operator.
+ * @param oOther the object to copy.
+ * @return the object.
+ */
+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()                           */
 /************************************************************************/