LIBKML: report XML id attribute as 'id' field; implement SetFeature()…

… and DeleteFeature() for regular layers

Fixes qgis/QGIS#58780

apps/test_ogrsf.cpp

@@ -2041,7 +2041,8 @@ static int TestOGRLayerRandomWrite(OGRLayer *poLayer)
     CPLString os_Id2;
     CPLString os_Id5;
 
-    const bool bHas_Id = poLayer->GetLayerDefn()->GetFieldIndex("_id") == 0;
+    const bool bHas_Id = poLayer->GetLayerDefn()->GetFieldIndex("_id") == 0 ||
+                         poLayer->GetLayerDefn()->GetFieldIndex("id") == 0;
 
     /* -------------------------------------------------------------------- */
     /*      Fetch five features.                                            */

autotest/ogr/ogr_libkml.py

@@ -29,7 +29,6 @@
 # DEALINGS IN THE SOFTWARE.
 ###############################################################################
 
-
 import gdaltest
 import ogrtest
 import pytest
@@ -289,7 +288,7 @@ def ogr_libkml_write(filename):
         lyr = ds.CreateLayer("test_wgs72", srs=srs)
 
         assert lyr.TestCapability(ogr.OLCSequentialWrite) == 1
-        assert lyr.TestCapability(ogr.OLCRandomWrite) == 0
+        assert lyr.TestCapability(ogr.OLCRandomWrite) == 1
 
         dst_feat = ogr.Feature(lyr.GetLayerDefn())
         dst_feat.SetGeometry(ogr.CreateGeometryFromWkt("POINT (2 49)"))
@@ -530,6 +529,32 @@ def test_ogr_libkml_test_ogrsf():
     )
 
 
+###############################################################################
+# Run test_ogrsf
+
+
+def test_ogr_libkml_test_ogrsf_write(tmp_path):
+
+    test_filename = str(tmp_path / "test.kml")
+    gdal.VectorTranslate(
+        test_filename, "data/poly.shp", options="-s_srs EPSG:32631 -t_srs EPSG:4326"
+    )
+
+    import test_cli_utilities
+
+    if test_cli_utilities.get_test_ogrsf_path() is None:
+        pytest.skip()
+
+    ret = gdaltest.runexternal(
+        test_cli_utilities.get_test_ogrsf_path()
+        + f" --config OGR_SKIP KML {test_filename}"
+    )
+
+    assert "using driver `LIBKML'" in ret
+    assert "INFO" in ret
+    assert "ERROR" not in ret
+
+
 ###############################################################################
 # Test reading KML with only Placemark
 
@@ -2253,3 +2278,64 @@ def test_ogr_libkml_write_geometries(input_wkt, expected_wkt, tmp_vsimem):
             assert f.GetGeometryRef().ExportToIsoWkt() == expected_wkt
         else:
             assert f is None
+
+
+###############################################################################
+# Test update of existing file
+
+
+@pytest.mark.parametrize("custom_id", [False, True])
+def test_ogr_libkml_update_delete_existing_kml(tmp_vsimem, custom_id):
+
+    filename = str(tmp_vsimem / "test.kml")
+    with ogr.GetDriverByName("LIBKML").CreateDataSource(filename) as ds:
+        lyr = ds.CreateLayer("test")
+        lyr.CreateField(ogr.FieldDefn("id"))
+        lyr.CreateField(ogr.FieldDefn("name"))
+        f = ogr.Feature(lyr.GetLayerDefn())
+        if custom_id:
+            f["id"] = "feat1"
+        f["name"] = "name1"
+        f.SetGeometry(ogr.CreateGeometryFromWkt("POINT (1 2)"))
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        if custom_id:
+            f["id"] = "feat2"
+        f["name"] = "name2"
+        f.SetGeometry(ogr.CreateGeometryFromWkt("POINT (3 4)"))
+        lyr.CreateFeature(f)
+
+    with gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) as ds:
+        lyr = ds.GetLayer(0)
+        with pytest.raises(Exception, match="Non existing feature"):
+            lyr.DeleteFeature(0)
+
+    with gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) as ds:
+        lyr = ds.GetLayer(0)
+        with pytest.raises(Exception, match="Non existing feature"):
+            lyr.DeleteFeature(3)
+
+    with gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) as ds:
+        lyr = ds.GetLayer(0)
+        lyr.DeleteFeature(1)
+        assert lyr.GetFeatureCount() == 1
+        lyr.ResetReading()
+        f = lyr.GetNextFeature()
+        assert f.GetFID() == 2
+        if custom_id:
+            assert f["id"] == "feat2"
+        assert f["name"] == "name2"
+        f["name"] = "name2_updated"
+        lyr.SetFeature(f)
+
+    with gdal.OpenEx(filename, gdal.OF_VECTOR | gdal.OF_UPDATE) as ds:
+        lyr = ds.GetLayer(0)
+        f = lyr.GetNextFeature()
+        if custom_id:
+            # FIDs are renumbered after feature update/deletion´if using
+            # custom KML ids
+            assert f.GetFID() == 1
+            assert f["id"] == "feat2"
+        else:
+            assert f.GetFID() == 2
+        assert f["name"] == "name2_updated"

doc/source/drivers/vector/libkml.rst

@@ -587,6 +587,13 @@ For example, if you want a field called 'Cities' to map to the
 tag in KML, you can set a configuration option. Note these are independent of layer
 creation and dataset creation options' `<name>`.
 
+-  .. config:: LIBKML_ID_FIELD
+      :default: id
+      :since: 3.10
+
+      Name of the string field that maps to the kml attribute
+      `<id> <https://developers.google.com/kml/documentation/kmlreference#object>`__.
+
 -  .. config:: LIBKML_NAME_FIELD
       :default: name
 

ogr/ogrsf_frmts/libkml/ogr_libkml.h

@@ -33,6 +33,7 @@
 #include "ogrsf_frmts.h"
 
 #include "libkml_headers.h"
+#include "fieldconfig.h"
 
 #include <map>
 
@@ -47,18 +48,20 @@ CPLString OGRLIBKMLGetSanitizedNCName(const char *pszName);
 class OGRLIBKMLLayer final : public OGRLayer,
                              public OGRGetNextFeatureThroughRaw<OGRLIBKMLLayer>
 {
-    int bUpdate;
+    int bUpdate = false;
 
-    int nFeatures;
-    int iFeature;
-    long nFID;
+    int nFeatures = 0;
+    int iFeature = 0;
+    GIntBig nFID = 1;
     const char *m_pszName;
     const char *m_pszFileName;
+    std::string m_osSanitizedNCName;
 
     kmldom::ContainerPtr m_poKmlLayer;
     kmldom::ElementPtr m_poKmlLayerRoot;
     kmldom::UpdatePtr m_poKmlUpdate;
 
+    fieldconfig m_oFieldConfig;
     OGRLIBKMLDataSource *m_poOgrDS;
     OGRFeatureDefn *m_poOgrFeatureDefn;
     kmldom::SchemaPtr m_poKmlSchema;
@@ -84,6 +87,11 @@ class OGRLIBKMLLayer final : public OGRLayer,
 
     bool m_bUpdateIsFolder;
 
+    bool m_bAllReadAtLeastOnce = false;
+    std::map<GIntBig, std::string> m_oMapOGRIdToKmlId{};
+    std::map<std::string, GIntBig> m_oMapKmlIdToOGRId{};
+
+    void ScanAllFeatures();
     OGRFeature *GetNextRawFeature();
 
   public:
@@ -161,6 +169,11 @@ class OGRLIBKMLLayer final : public OGRLayer,
         return m_pszFileName;
     }
 
+    const fieldconfig &GetFieldConfig() const
+    {
+        return m_oFieldConfig;
+    }
+
     void SetLookAt(const char *pszLookatLongitude,
                    const char *pszLookatLatitude, const char *pszLookatAltitude,
                    const char *pszLookatHeading, const char *pszLookatTilt,

ogr/ogrsf_frmts/libkml/ogrlibkmlfeature.cpp

@@ -225,9 +225,7 @@ FeaturePtr feat2kml(OGRLIBKMLDataSource *poOgrDS, OGRLIBKMLLayer *poOgrLayer,
                     int bUseSimpleField)
 {
     FeaturePtr poKmlFeature = nullptr;
-
-    struct fieldconfig oFC;
-    get_fieldconfig(&oFC);
+    const auto &oFC = poOgrLayer->GetFieldConfig();
 
     /***** geometry *****/
     OGRGeometry *poOgrGeom = poOgrFeat->GetGeometryRef();
@@ -833,13 +831,13 @@ FeaturePtr feat2kml(OGRLIBKMLDataSource *poOgrDS, OGRLIBKMLLayer *poOgrLayer,
 
     /***** fields *****/
     field2kml(poOgrFeat, poOgrLayer, poKmlFactory, poKmlFeature,
-              bUseSimpleField);
+              bUseSimpleField, oFC);
 
     return poKmlFeature;
 }
 
 OGRFeature *kml2feat(PlacemarkPtr poKmlPlacemark, OGRLIBKMLDataSource *poOgrDS,
-                     OGRLayer *poOgrLayer, OGRFeatureDefn *poOgrFeatDefn,
+                     OGRLIBKMLLayer *poOgrLayer, OGRFeatureDefn *poOgrFeatDefn,
                      OGRSpatialReference *poOgrSRS)
 {
     OGRFeature *poOgrFeat = new OGRFeature(poOgrFeatDefn);
@@ -872,14 +870,15 @@ OGRFeature *kml2feat(PlacemarkPtr poKmlPlacemark, OGRLIBKMLDataSource *poOgrDS,
     }
 
     /***** fields *****/
-    kml2field(poOgrFeat, AsFeature(poKmlPlacemark));
+    kml2field(poOgrFeat, AsFeature(poKmlPlacemark),
+              poOgrLayer->GetFieldConfig());
 
     return poOgrFeat;
 }
 
 OGRFeature *kmlgroundoverlay2feat(GroundOverlayPtr poKmlOverlay,
                                   OGRLIBKMLDataSource * /* poOgrDS */,
-                                  OGRLayer * /* poOgrLayer */,
+                                  OGRLIBKMLLayer *poOgrLayer,
                                   OGRFeatureDefn *poOgrFeatDefn,
                                   OGRSpatialReference *poOgrSRS)
 {
@@ -900,7 +899,7 @@ OGRFeature *kmlgroundoverlay2feat(GroundOverlayPtr poKmlOverlay,
     }
 
     /***** fields *****/
-    kml2field(poOgrFeat, AsFeature(poKmlOverlay));
+    kml2field(poOgrFeat, AsFeature(poKmlOverlay), poOgrLayer->GetFieldConfig());
 
     return poOgrFeat;
 }

ogr/ogrsf_frmts/libkml/ogrlibkmlfeature.h

@@ -45,13 +45,13 @@ kmldom::FeaturePtr feat2kml(OGRLIBKMLDataSource *poOgrDS,
 ******************************************************************************/
 
 OGRFeature *kml2feat(kmldom::PlacemarkPtr poKmlPlacemark,
-                     OGRLIBKMLDataSource *poOgrDS, OGRLayer *poOgrLayer,
+                     OGRLIBKMLDataSource *poOgrDS, OGRLIBKMLLayer *poOgrLayer,
                      OGRFeatureDefn *poOgrFeatDefn,
                      OGRSpatialReference *poOgrSRS);
 
 OGRFeature *kmlgroundoverlay2feat(kmldom::GroundOverlayPtr poKmlOverlay,
                                   OGRLIBKMLDataSource *poOgrDS,
-                                  OGRLayer *poOgrLayer,
+                                  OGRLIBKMLLayer *poOgrLayer,
                                   OGRFeatureDefn *poOgrFeatDefn,
                                   OGRSpatialReference *poOgrSRS);
 

ogr/ogrsf_frmts/libkml/ogrlibkmlfield.cpp

@@ -273,7 +273,7 @@ static char *OGRLIBKMLSanitizeUTF8String(const char *pszString)
 
 void field2kml(OGRFeature *poOgrFeat, OGRLIBKMLLayer *poOgrLayer,
                KmlFactory *poKmlFactory, FeaturePtr poKmlFeature,
-               int bUseSimpleFieldIn)
+               int bUseSimpleFieldIn, const fieldconfig &oFC)
 {
     const bool bUseSimpleField = CPL_TO_BOOL(bUseSimpleFieldIn);
     SchemaDataPtr poKmlSchemaData = nullptr;
@@ -293,10 +293,6 @@ void field2kml(OGRFeature *poOgrFeat, OGRLIBKMLLayer *poOgrLayer,
         }
     }
 
-    /***** Get the field config *****/
-    struct fieldconfig oFC;
-    get_fieldconfig(&oFC);
-
     TimeSpanPtr poKmlTimeSpan = nullptr;
 
     const int nFields = poOgrFeat->GetFieldCount();
@@ -338,6 +334,13 @@ void field2kml(OGRFeature *poOgrFeat, OGRLIBKMLLayer *poOgrLayer,
                     continue;
                 }
 
+                /***** id *****/
+                if (EQUAL(name, oFC.idfield))
+                {
+                    poKmlFeature->set_id(pszUTF8String);
+                    CPLFree(pszUTF8String);
+                    continue;
+                }
                 /***** name *****/
                 if (EQUAL(name, oFC.namefield))
                 {
@@ -1157,13 +1160,19 @@ static void kmldatetime2ogr(OGRFeature *poOgrFeat, const char *pszOGRField,
  function to read kml into ogr fields
 ******************************************************************************/
 
-void kml2field(OGRFeature *poOgrFeat, FeaturePtr poKmlFeature)
+void kml2field(OGRFeature *poOgrFeat, FeaturePtr poKmlFeature,
+               const fieldconfig &oFC)
 {
-    /***** get the field config *****/
+    /***** id *****/
 
-    struct fieldconfig oFC;
-    get_fieldconfig(&oFC);
+    if (poKmlFeature->has_id())
+    {
+        const std::string oKmlId = poKmlFeature->get_id();
+        int iField = poOgrFeat->GetFieldIndex(oFC.idfield);
 
+        if (iField > -1)
+            poOgrFeat->SetField(iField, oKmlId.c_str());
+    }
     /***** name *****/
 
     if (poKmlFeature->has_name())
@@ -1552,15 +1561,13 @@ void kml2field(OGRFeature *poOgrFeat, FeaturePtr poKmlFeature)
 ******************************************************************************/
 
 SimpleFieldPtr FieldDef2kml(const OGRFieldDefn *poOgrFieldDef,
-                            KmlFactory *poKmlFactory, bool bApproxOK)
+                            KmlFactory *poKmlFactory, bool bApproxOK,
+                            const fieldconfig &oFC)
 {
-    /***** Get the field config. *****/
-    struct fieldconfig oFC;
-    get_fieldconfig(&oFC);
-
     const char *pszFieldName = poOgrFieldDef->GetNameRef();
 
-    if (EQUAL(pszFieldName, oFC.namefield) ||
+    if (EQUAL(pszFieldName, oFC.idfield) ||
+        EQUAL(pszFieldName, oFC.namefield) ||
         EQUAL(pszFieldName, oFC.descfield) ||
         EQUAL(pszFieldName, oFC.tsfield) ||
         EQUAL(pszFieldName, oFC.beginfield) ||
@@ -1724,6 +1731,7 @@ void kml2FeatureDef(SchemaPtr poKmlSchema, OGRFeatureDefn *poOgrFeatureDefn)
 
 void get_fieldconfig(struct fieldconfig *oFC)
 {
+    oFC->idfield = CPLGetConfigOption("LIBKML_ID_FIELD", "id");
     oFC->namefield = CPLGetConfigOption("LIBKML_NAME_FIELD", "Name");
     oFC->descfield =
         CPLGetConfigOption("LIBKML_DESCRIPTION_FIELD", "description");