Merge pull request #1846 from rouault/fix_geojson_rewriting_on_windows

GeoJSON driver: use VSIOverwriteFile() to fix update of file on Windows (fixes qgis/QGIS#28580)
OSGeo · Sep 18, 2019 · 27690b4 · 27690b4
2 parents cab1ace + 6fb4c77
commit 27690b4
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 60 deletions.
diff --git a/autotest/ogr/ogr_geojson.py b/autotest/ogr/ogr_geojson.py
@@ -2293,17 +2293,18 @@ def test_ogr_geojson_47():
 
     assert 'something' in data
 
-    # Test appending to feature collection with "bbox"
-    gdal.FileFromMemBuffer('/vsimem/ogr_geojson_47.json', """{ "type": "FeatureCollection", "bbox": [0,0,0,0], "features": [ { "type": "Feature", "geometry": { "type": "Point", "coordinates": [0,0]} } ]}""")
-    ds = ogr.Open('/vsimem/ogr_geojson_47.json', update=1)
-    lyr = ds.GetLayer(0)
-    f = ogr.Feature(lyr.GetLayerDefn())
-    lyr.CreateFeature(f)
-    ds = None
-    ds = ogr.Open('/vsimem/ogr_geojson_47.json')
-    lyr = ds.GetLayer(0)
-    assert lyr.GetFeatureCount() == 2
-    ds = None
+    with gdaltest.config_option('OGR_GEOJSON_REWRITE_IN_PLACE', 'YES'):
+        # Test appending to feature collection with "bbox"
+        gdal.FileFromMemBuffer('/vsimem/ogr_geojson_47.json', """{ "type": "FeatureCollection", "bbox": [0,0,0,0], "features": [ { "type": "Feature", "geometry": { "type": "Point", "coordinates": [0,0]} } ]}""")
+        ds = ogr.Open('/vsimem/ogr_geojson_47.json', update=1)
+        lyr = ds.GetLayer(0)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        lyr.CreateFeature(f)
+        ds = None
+        ds = ogr.Open('/vsimem/ogr_geojson_47.json')
+        lyr = ds.GetLayer(0)
+        assert lyr.GetFeatureCount() == 2
+        ds = None
 
     fp = gdal.VSIFOpenL('/vsimem/ogr_geojson_47.json', 'rb')
     if fp is not None:

diff --git a/gdal/ogr/ogrsf_frmts/geojson/ogrgeojsondatasource.cpp b/gdal/ogr/ogrsf_frmts/geojson/ogrgeojsondatasource.cpp
@@ -1075,22 +1075,65 @@ void OGRGeoJSONDataSource::FlushCache()
                 }
                 if( bOK )
                 {
-                    CPLString osBackup(pszName_);
-                    osBackup += ".bak";
-                    if( VSIRename(pszName_, osBackup) < 0 )
+                    const bool bOverwrite =
+                        CPLTestBool(CPLGetConfigOption("OGR_GEOJSON_REWRITE_IN_PLACE",
+#ifdef WIN32
+                                                       "YES"
+#else
+                                                       "NO"
+#endif
+                                                        ));
+                    if( bOverwrite )
                     {
-                        CPLError(CE_Failure, CPLE_AppDefined,
-                                "Cannot create backup copy");
-                    }
-                    else if( VSIRename(osNewFilename, pszName_) < 0 )
-                    {
-                        CPLError(CE_Failure, CPLE_AppDefined,
-                                "Cannot rename %s to %s",
-                                osNewFilename.c_str(), pszName_);
+                        VSILFILE* fpTarget = nullptr;
+                        for( int attempt = 0; attempt < 10; attempt++ )
+                        {
+                            fpTarget = VSIFOpenL(pszName_, "rb+");
+                            if( fpTarget )
+                                break;
+                            CPLSleep(0.1);
+                        }
+                        if( !fpTarget )
+                        {
+                            CPLError(CE_Failure, CPLE_AppDefined,
+                                     "Cannot rewrite %s", pszName_);
+                        }
+                        else
+                        {
+                            const bool bCopyOK = CPL_TO_BOOL(
+                                VSIOverwriteFile(fpTarget, osNewFilename));
+                            VSIFCloseL(fpTarget);
+                            if( bCopyOK )
+                            {
+                                VSIUnlink(osNewFilename);
+                            }
+                            else
+                            {
+                                CPLError(CE_Failure, CPLE_AppDefined,
+                                         "Cannot rewrite %s with content of %s",
+                                         pszName_, osNewFilename.c_str());
+                            }
+                        }
                     }
                     else
                     {
-                        VSIUnlink(osBackup);
+                        CPLString osBackup(pszName_);
+                        osBackup += ".bak";
+                        if( VSIRename(pszName_, osBackup) < 0 )
+                        {
+                            CPLError(CE_Failure, CPLE_AppDefined,
+                                    "Cannot create backup copy");
+                        }
+                        else if( VSIRename(osNewFilename, pszName_) < 0 )
+                        {
+                            CPLError(CE_Failure, CPLE_AppDefined,
+                                    "Cannot rename %s to %s",
+                                    osNewFilename.c_str(), pszName_);
+                        }
+                        else
+                        {
+                            VSIUnlink(osBackup);
+                        }
                     }
                 }
             }

diff --git a/gdal/ogr/ogrsf_frmts/shape/ogrshapedatasource.cpp b/gdal/ogr/ogrsf_frmts/shape/ogrshapedatasource.cpp
@@ -1782,41 +1782,5 @@ bool OGRShapeDataSource::RecompressIfNeeded(const std::vector<CPLString>& layerN
 bool OGRShapeDataSource::CopyInPlace( VSILFILE* fpTarget,
                                       const CPLString& osSourceFilename )
 {
-    VSILFILE* fpSource = VSIFOpenL(osSourceFilename, "rb");
-    if( fpSource == nullptr )
-    {
-        CPLError(CE_Failure, CPLE_FileIO,
-                 "Cannot open %s", osSourceFilename.c_str());
-        return false;
-    }
-
-    const size_t nBufferSize = 4096;
-    void* pBuffer = CPLMalloc(nBufferSize);
-    VSIFSeekL( fpTarget, 0, SEEK_SET );
-    bool bRet = true;
-    while( true )
-    {
-        size_t nRead = VSIFReadL( pBuffer, 1, nBufferSize, fpSource );
-        size_t nWritten = VSIFWriteL( pBuffer, 1, nRead, fpTarget );
-        if( nWritten != nRead )
-        {
-            bRet = false;
-            break;
-        }
-        if( nRead < nBufferSize )
-            break;
-    }
-
-    if( bRet )
-    {
-        bRet = VSIFTruncateL( fpTarget, VSIFTellL(fpTarget) ) == 0;
-        if( !bRet )
-        {
-            CPLError(CE_Failure, CPLE_FileIO, "Truncation failed");
-        }
-    }
-
-    CPLFree(pBuffer);
-    VSIFCloseL(fpSource);
-    return bRet;
+    return CPL_TO_BOOL(VSIOverwriteFile(fpTarget, osSourceFilename.c_str()));
 }
diff --git a/gdal/port/cpl_vsi.h b/gdal/port/cpl_vsi.h
@@ -187,6 +187,8 @@ int CPL_DLL     VSIIngestFile( VSILFILE* fp,
                                vsi_l_offset* pnSize,
                                GIntBig nMaxSize ) CPL_WARN_UNUSED_RESULT;
 
+int CPL_DLL     VSIOverwriteFile( VSILFILE* fpTarget, const char* pszSourceFilename ) CPL_WARN_UNUSED_RESULT;
+
 #if defined(VSI_STAT64_T)
 /** Type for VSIStatL() */
 typedef struct VSI_STAT64_T VSIStatBufL;

diff --git a/gdal/port/cpl_vsil.cpp b/gdal/port/cpl_vsil.cpp
@@ -2211,6 +2211,63 @@ int VSIIngestFile( VSILFILE* fp,
     return TRUE;
 }
 
+
+/************************************************************************/
+/*                         VSIOverwriteFile()                           */
+/************************************************************************/
+
+/**
+ * \brief Overwrite an existing file with content from another one
+ *
+ * @param fpTarget file handle opened with VSIFOpenL() with "rb+" flag.
+ * @param pszSourceFilename source filename
+ *
+ * @return TRUE in case of success.
+ *
+ * @since GDAL 3.1
+ */
+
+int VSIOverwriteFile( VSILFILE* fpTarget, const char* pszSourceFilename )
+{
+    VSILFILE* fpSource = VSIFOpenL(pszSourceFilename, "rb");
+    if( fpSource == nullptr )
+    {
+        CPLError(CE_Failure, CPLE_FileIO,
+                 "Cannot open %s", pszSourceFilename);
+        return false;
+    }
+
+    const size_t nBufferSize = 4096;
+    void* pBuffer = CPLMalloc(nBufferSize);
+    VSIFSeekL( fpTarget, 0, SEEK_SET );
+    bool bRet = true;
+    while( true )
+    {
+        size_t nRead = VSIFReadL( pBuffer, 1, nBufferSize, fpSource );
+        size_t nWritten = VSIFWriteL( pBuffer, 1, nRead, fpTarget );
+        if( nWritten != nRead )
+        {
+            bRet = false;
+            break;
+        }
+        if( nRead < nBufferSize )
+            break;
+    }
+
+    if( bRet )
+    {
+        bRet = VSIFTruncateL( fpTarget, VSIFTellL(fpTarget) ) == 0;
+        if( !bRet )
+        {
+            CPLError(CE_Failure, CPLE_FileIO, "Truncation failed");
+        }
+    }
+
+    CPLFree(pBuffer);
+    VSIFCloseL(fpSource);
+    return bRet;
+}
+
 /************************************************************************/
 /*                        VSIFGetNativeFileDescriptorL()                */
 /************************************************************************/