Merge pull request #5223 from boundlessgeo/geom_compatibility_check_r…

…elease-2_18-fix15741

On behalf of Giovanni: Geom compatibility check release 2.18 fixes #15741 #16927

python/core/qgsvectordataprovider.sip

@@ -390,7 +390,17 @@ class QgsVectorDataProvider : QgsDataProvider
 
     void pushError( const QString& msg );
 
-    /** Converts the geometry to the provider type if possible / necessary
-    @return the converted geometry or nullptr if no conversion was necessary or possible*/
+    /** \brief Converts the geometry to the provider type if possible / necessary
+     * this is the list of possible modifications:
+     * - convert compoundcurve to circularstring
+     *   (possible if compoundcurve consists of one circular string)
+     * - convert to multitype if necessary
+     * - convert to curved type if necessary
+     * - convert to linear type from curved type
+     * - Add z/m 0 default values
+     * - Remove z/m
+     * \ref QgsVectorLayerEditBuffer::adaptGeometry()
+     * \param geom Geometry to convert
+     * \returns the converted geometry or nullptr if no conversion was necessary or possible*/
     QgsGeometry* convertToProviderType( const QgsGeometry* geom ) const /Factory/;
 };

python/core/qgsvectorlayereditbuffer.sip

@@ -148,4 +148,17 @@ class QgsVectorLayerEditBuffer : QObject
     void updateAttributeMapIndex( QgsAttributeMap& attrs, int index, int offset ) const;
 
     void updateLayerFields();
+
+    /** \brief Apply geometry modification basing on provider geometry type.
+     * Geometry is modified only if successful conversion is possible.
+     * adaptGeometry calls \ref QgsVectorDataProvider::convertToProviderType()
+     * if necessary and that apply the modifications.
+     * \param geometry pointer to the geometry that should be adapted to provider
+     * \returns bool true if success.
+     *  True: Input geometry is changed because conversion is applied or
+     *  geometry is untouched if geometry conversion is not necessary.
+     *  False: Conversion is not possible and geometry is untouched.
+     * \note added in QGIS 2.18.14
+     */
+    bool adaptGeometry( QgsGeometry* geometry );
 };

src/app/qgisapp.cpp

@@ -7162,7 +7162,7 @@ void QgisApp::mergeSelectedFeatures()
     if ( !isDefaultValue && !vl->fields().at( i ).convertCompatible( val ) )
     {
       messageBar()->pushMessage(
-        tr( "Invalid result" ),
+        tr( "Merge features" ),
         tr( "Could not store value '%1' in field of type %2" ).arg( attrs.at( i ).toString(), vl->fields().at( i ).typeName() ),
         QgsMessageBar::WARNING );
     }
@@ -7176,9 +7176,15 @@ void QgisApp::mergeSelectedFeatures()
     vl->deleteFeature( *feature_it );
   }
 
-  vl->addFeature( newFeature, false );
-
-  vl->endEditCommand();
+  // addFeature can fail if newFeature has no compatibile geometry
+  if ( !vl->addFeature( newFeature, false ) )
+  {
+    vl->destroyEditCommand();
+  }
+  else
+  {
+    vl->endEditCommand();
+  }
 
   if ( mapCanvas() )
   {
@@ -7533,8 +7539,14 @@ void QgisApp::editPaste( QgsMapLayer *destinationLayer )
     ++featureIt;
   }
 
-  pasteVectorLayer->addFeatures( features );
-  pasteVectorLayer->endEditCommand();
+  if ( !pasteVectorLayer->addFeatures( features ) )
+  {
+    pasteVectorLayer->destroyEditCommand();
+  }
+  else
+  {
+    pasteVectorLayer->endEditCommand();
+  }
 
   int nCopiedFeatures = features.count();
   if ( nCopiedFeatures == 0 )
@@ -7721,7 +7733,7 @@ QgsVectorLayer *QgisApp::pasteToNewMemoryVector()
       feature.geometry()->convertToMultiType();
     }
   }
-  if ( ! layer->addFeatures( features, false ) || !layer->commitChanges() )
+  if ( !layer->addFeatures( features, false ) || !layer->commitChanges() )
   {
     QgsDebugMsg( "Cannot add features or commit changes" );
     delete layer;

src/app/qgsfieldcalculator.cpp

@@ -295,7 +295,12 @@ void QgsFieldCalculator::accept()
         if ( value.canConvert< QgsGeometry >() )
         {
           QgsGeometry geom = value.value< QgsGeometry >();
-          mVectorLayer->changeGeometry( feature.id(), &geom );
+          if ( !mVectorLayer->changeGeometry( feature.id(), &geom ) )
+          {
+            calculationSuccess = false;
+            error = tr( "Can not change geometry for feature: %1", "Field calculator" ).arg( feature.id() );
+            break;
+          }
         }
       }
       else

src/app/qgsmaptooladdpart.cpp

@@ -219,6 +219,10 @@ void QgsMapToolAddPart::cadCanvasReleaseEvent( QgsMapMouseEvent * e )
     case 6:
       errorMessage = tr( "Selected geometry could not be found" );
       break;
+
+    case 7:
+      errorMessage = tr( "Update geometry error" );
+      break;
   }
 
   emit messageEmitted( errorMessage, QgsMessageBar::WARNING );

src/app/qgsmaptooldeletepart.cpp

@@ -105,8 +105,10 @@ void QgsMapToolDeletePart::canvasReleaseEvent( QgsMapMouseEvent* e )
   if ( g->deletePart( mPressedPartNum ) )
   {
     vlayer->beginEditCommand( tr( "Part of multipart feature deleted" ) );
-    vlayer->changeGeometry( f.id(), g );
-    vlayer->endEditCommand();
+    if ( !vlayer->changeGeometry( f.id(), g ) )
+      vlayer->destroyEditCommand();
+    else
+      vlayer->endEditCommand();
     vlayer->triggerRepaint();
   }
   else

src/app/qgsmaptooldeletering.cpp

@@ -113,7 +113,11 @@ void QgsMapToolDeleteRing::canvasReleaseEvent( QgsMapMouseEvent* e )
   if ( g->deleteRing( mPressedRingNum, mPressedPartNum ) )
   {
     vlayer->beginEditCommand( tr( "Ring deleted" ) );
-    vlayer->changeGeometry( mPressedFid, g );
+    if ( !vlayer->changeGeometry( mPressedFid, g ) )
+    {
+      vlayer->destroyEditCommand();
+      return;
+    }
     vlayer->endEditCommand();
     vlayer->triggerRepaint();
   }

src/app/qgsmaptoolsimplify.cpp

@@ -176,17 +176,30 @@ void QgsMapToolSimplify::storeSimplified()
   double layerTolerance = QgsTolerance::toleranceInMapUnits( mTolerance, vlayer, mCanvas->mapSettings(), mToleranceUnits );
 
   vlayer->beginEditCommand( tr( "Geometry simplified" ) );
+  bool success = true;
   Q_FOREACH ( const QgsFeature& feat, mSelectedFeatures )
   {
     if ( QgsGeometry* g = feat.constGeometry()->simplify( layerTolerance ) )
     {
-      vlayer->changeGeometry( feat.id(), g );
+      if ( !vlayer->changeGeometry( feat.id(), g ) )
+      {
+        success = false;
+      }
       delete g;
+
+      if ( !success )
+        break;
     }
   }
-  vlayer->endEditCommand();
-
-  clearSelection();
+  if ( success )
+  {
+    vlayer->endEditCommand();
+    clearSelection();
+  }
+  else
+  {
+    vlayer->destroyEditCommand();
+  }
 
   vlayer->triggerRepaint();
 }

src/core/qgsofflineediting.cpp

@@ -54,7 +54,8 @@ extern "C"
 #define PROJECT_ENTRY_SCOPE_OFFLINE "OfflineEditingPlugin"
 #define PROJECT_ENTRY_KEY_OFFLINE_DB_PATH "/OfflineDbPath"
 
-QgsOfflineEditing::QgsOfflineEditing()
+QgsOfflineEditing::QgsOfflineEditing():
+    syncError( false )
 {
   connect( QgsMapLayerRegistry::instance(), SIGNAL( layerWasAdded( QgsMapLayer* ) ), this, SLOT( layerAdded( QgsMapLayer* ) ) );
 }
@@ -261,6 +262,7 @@ void QgsOfflineEditing::synchronize()
       updateVisibilityPresets( offlineLayer, remoteLayer );
 
       // apply layer edit log
+      syncError = false;
       QString qgisLayerId = layer->id();
       QString sql = QString( "SELECT \"id\" FROM 'log_layer_ids' WHERE \"qgis_id\" = '%1'" ).arg( qgisLayerId );
       int layerId = sqlQueryInt( db, sql, -1 );
@@ -276,37 +278,49 @@ void QgsOfflineEditing::synchronize()
           QgsDebugMsgLevel( "Apply commits chronologically", 4 );
           // apply commits chronologically
           applyAttributesAdded( remoteLayer, db, layerId, i );
-          applyAttributeValueChanges( offlineLayer, remoteLayer, db, layerId, i );
-          applyGeometryChanges( remoteLayer, db, layerId, i );
+          if ( !syncError )
+            applyAttributeValueChanges( offlineLayer, remoteLayer, db, layerId, i );
+          if ( !syncError )
+            applyGeometryChanges( remoteLayer, db, layerId, i );
         }
 
-        applyFeaturesAdded( offlineLayer, remoteLayer, db, layerId );
-        applyFeaturesRemoved( remoteLayer, db, layerId );
+        if ( !syncError )
+          applyFeaturesAdded( offlineLayer, remoteLayer, db, layerId );
+        if ( !syncError )
+          applyFeaturesRemoved( remoteLayer, db, layerId );
 
-        if ( remoteLayer->commitChanges() )
+        if ( !syncError )
         {
-          // update fid lookup
-          updateFidLookup( remoteLayer, db, layerId );
-
-          // clear edit log for this layer
-          sql = QString( "DELETE FROM 'log_added_attrs' WHERE \"layer_id\" = %1" ).arg( layerId );
-          sqlExec( db, sql );
-          sql = QString( "DELETE FROM 'log_added_features' WHERE \"layer_id\" = %1" ).arg( layerId );
-          sqlExec( db, sql );
-          sql = QString( "DELETE FROM 'log_removed_features' WHERE \"layer_id\" = %1" ).arg( layerId );
-          sqlExec( db, sql );
-          sql = QString( "DELETE FROM 'log_feature_updates' WHERE \"layer_id\" = %1" ).arg( layerId );
-          sqlExec( db, sql );
-          sql = QString( "DELETE FROM 'log_geometry_updates' WHERE \"layer_id\" = %1" ).arg( layerId );
-          sqlExec( db, sql );
-
-          // reset commitNo
-          QString sql = QString( "UPDATE 'log_indices' SET 'last_index' = 0 WHERE \"name\" = 'commit_no'" );
-          sqlExec( db, sql );
+          if ( remoteLayer->commitChanges() )
+          {
+            // update fid lookup
+            updateFidLookup( remoteLayer, db, layerId );
+
+            // clear edit log for this layer
+            sql = QString( "DELETE FROM 'log_added_attrs' WHERE \"layer_id\" = %1" ).arg( layerId );
+            sqlExec( db, sql );
+            sql = QString( "DELETE FROM 'log_added_features' WHERE \"layer_id\" = %1" ).arg( layerId );
+            sqlExec( db, sql );
+            sql = QString( "DELETE FROM 'log_removed_features' WHERE \"layer_id\" = %1" ).arg( layerId );
+            sqlExec( db, sql );
+            sql = QString( "DELETE FROM 'log_feature_updates' WHERE \"layer_id\" = %1" ).arg( layerId );
+            sqlExec( db, sql );
+            sql = QString( "DELETE FROM 'log_geometry_updates' WHERE \"layer_id\" = %1" ).arg( layerId );
+            sqlExec( db, sql );
+
+            // reset commitNo
+            QString sql = QString( "UPDATE 'log_indices' SET 'last_index' = 0 WHERE \"name\" = 'commit_no'" );
+            sqlExec( db, sql );
+          }
+          else
+          {
+            showWarning( remoteLayer->commitErrors().join( "\n" ) );
+          }
         }
         else
         {
-          showWarning( remoteLayer->commitErrors().join( "\n" ) );
+          remoteLayer->rollBack();
+          showWarning( tr( "Syncronization failed" ) );
         }
       }
       else
@@ -688,7 +702,8 @@ QgsVectorLayer* QgsOfflineEditing::copyVectorLayer( QgsVectorLayer* layer, sqlit
           f.geometry()->setGeometry( geom );
         }
 
-        newLayer->addFeature( f, false );
+        if ( !newLayer->addFeature( f, false ) )
+          return nullptr;
 
         emit progressUpdated( featureCount++ );
       }
@@ -762,7 +777,11 @@ void QgsOfflineEditing::applyAttributesAdded( QgsVectorLayer* remoteLayer, sqlit
     {
       QString typeName = typeNameLookup[ field.type()];
       field.setTypeName( typeName );
-      remoteLayer->addAttribute( field );
+      if ( !remoteLayer->addAttribute( field ) )
+      {
+        syncError = true;
+        return;
+      }
     }
     else
     {
@@ -830,7 +849,11 @@ void QgsOfflineEditing::applyFeaturesAdded( QgsVectorLayer* offlineLayer, QgsVec
 
     f.setAttributes( newAttrs );
 
-    remoteLayer->addFeature( f, false );
+    if ( !remoteLayer->addFeature( f, false ) )
+    {
+      syncError = true;
+      return;
+    }
 
     emit progressUpdated( i++ );
   }
@@ -847,7 +870,11 @@ void QgsOfflineEditing::applyFeaturesRemoved( QgsVectorLayer* remoteLayer, sqlit
   for ( QgsFeatureIds::const_iterator it = values.begin(); it != values.end(); ++it )
   {
     QgsFeatureId fid = remoteFid( db, layerId, *it );
-    remoteLayer->deleteFeature( fid );
+    if ( !remoteLayer->deleteFeature( fid ) )
+    {
+      syncError = true;
+      return;
+    }
 
     emit progressUpdated( i++ );
   }
@@ -866,7 +893,11 @@ void QgsOfflineEditing::applyAttributeValueChanges( QgsVectorLayer* offlineLayer
   {
     QgsFeatureId fid = remoteFid( db, layerId, values.at( i ).fid );
     QgsDebugMsgLevel( QString( "Offline changeAttributeValue %1 = %2" ).arg( QString( attrLookup[ values.at( i ).attr ] ), values.at( i ).value ), 4 );
-    remoteLayer->changeAttributeValue( fid, attrLookup[ values.at( i ).attr ], values.at( i ).value );
+    if ( !remoteLayer->changeAttributeValue( fid, attrLookup[ values.at( i ).attr ], values.at( i ).value ) )
+    {
+      syncError = true;
+      return;
+    }
 
     emit progressUpdated( i + 1 );
   }
@@ -882,7 +913,11 @@ void QgsOfflineEditing::applyGeometryChanges( QgsVectorLayer* remoteLayer, sqlit
   for ( int i = 0; i < values.size(); i++ )
   {
     QgsFeatureId fid = remoteFid( db, layerId, values.at( i ).fid );
-    remoteLayer->changeGeometry( fid, QgsGeometry::fromWkt( values.at( i ).geom_wkt ) );
+    if ( !remoteLayer->changeGeometry( fid, QgsGeometry::fromWkt( values.at( i ).geom_wkt ) ) )
+    {
+      syncError = true;
+      return;
+    }
 
     emit progressUpdated( i + 1 );
   }

src/core/qgsofflineediting.h

@@ -153,6 +153,11 @@ class CORE_EXPORT QgsOfflineEditing : public QObject
     };
     typedef QList<GeometryChange> GeometryChanges;
     GeometryChanges sqlQueryGeometryChanges( sqlite3* db, const QString& sql );
+    /**
+     * in case of sync error the flag is set to true
+     */
+    bool syncError;
+
 
   private slots:
     void layerAdded( QgsMapLayer* layer );

src/core/qgsvectordataprovider.cpp

@@ -714,10 +714,37 @@ QgsGeometry* QgsVectorDataProvider::convertToProviderType( const QgsGeometry* ge
     outputGeom->addMValue();
   }
 
+  // remove Z if provider does not have
+  // control added to fix https://issues.qgis.org/issues/16927
+  if ( !QgsWKBTypes::hasZ( providerGeomType ) && QgsWKBTypes::hasZ( geometry->wkbType() ) )
+  {
+    if ( !outputGeom )
+    {
+      outputGeom = geometry->clone();
+    }
+    outputGeom->dropZValue();
+  }
+
+  // remove M if provider does not have
+  // control added as follow-up of https://issues.qgis.org/issues/16927
+  if ( !QgsWKBTypes::hasM( providerGeomType ) && QgsWKBTypes::hasM( geometry->wkbType() ) )
+  {
+    if ( !outputGeom )
+    {
+      outputGeom = geometry->clone();
+    }
+    outputGeom->dropMValue();
+  }
+
   if ( outputGeom )
   {
     return new QgsGeometry( outputGeom );
   }
+
+  QString msg = tr( "Geometry type %1 not compatible with provider type %2.", "not compatible geometry" )
+                .arg( QgsWKBTypes::displayString( geometry->wkbType() ) )
+                .arg( QgsWKBTypes::displayString( providerGeomType ) );
+  const_cast<QgsVectorDataProvider*>( this )->pushError( msg );
   return nullptr;
 }