GPKG and SQLite: Fix out of sync (not restored) fields after a ROLLBA…

…CK (#11609)

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)

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)

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)

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<OGRFieldDefn> StealFieldDefn(int iField);
+
+    virtual void AddFieldDefn(std::unique_ptr<OGRFieldDefn> &&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<OGRGeomFieldDefn> StealGeomFieldDefn(int iField);
+
     virtual int GetGeomFieldCount() const;
     virtual OGRGeomFieldDefn *GetGeomFieldDefn(int i);
     virtual const OGRGeomFieldDefn *GetGeomFieldDefn(int i) const;

ogr/ogrfeaturedefn.cpp

@@ -417,6 +417,30 @@ void OGRFeatureDefn::AddFieldDefn(const OGRFieldDefn *poNewDefn)
     apoFieldDefn.emplace_back(std::make_unique<OGRFieldDefn>(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<OGRFieldDefn> &&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<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;
+
+    std::unique_ptr<OGRGeomFieldDefn> poFieldDef =
+        std::move(apoGeomFieldDefn.at(iField));
+    apoGeomFieldDefn.erase(apoGeomFieldDefn.begin() + iField);
+    return poFieldDef;
+}
+
+/************************************************************************/
+/*                          StealFieldDefn()                           */
+/************************************************************************/
+
+std::unique_ptr<OGRFieldDefn> OGRFeatureDefn::StealFieldDefn(int iField)
+{
+    if (iField < 0 || iField >= GetFieldCount())
+        return nullptr;
+
+    std::unique_ptr<OGRFieldDefn> 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)
     {

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()                           */
 /************************************************************************/