Correctly handle sort field as field in atlas sorting

Fixes #40332
qgis · Feb 21, 2022 · 837b775 · 837b775
1 parent aca7af4
commit 837b775
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 1 deletion.
diff --git a/python/core/auto_generated/expression/qgsexpression.sip.in b/python/core/auto_generated/expression/qgsexpression.sip.in
@@ -304,6 +304,21 @@ method will return the corresponding field index.
 .. seealso:: :py:func:`isField`
 
 .. versionadded:: 3.22
+%End
+
+    static QString quoteFieldExpression( const QString &expression, const QgsVectorLayer *layer );
+%Docstring
+Validate if the expression is a field in the ``layer`` and ensure it is quoted.
+
+Given a string which may either directly match a field name from a layer, OR may
+be an expression which consists only of a single field reference for that layer, this
+method will return the quoted field.
+
+:return: the ``expression`` if not a field or quotes are not required, otherwise requite a quoted field.
+
+.. seealso:: :py:func:`expressionToLayerFieldIndex`
+
+.. versionadded:: 3.24
 %End
 
     static bool checkExpression( const QString &text, const QgsExpressionContext *context, QString &errorMessage /Out/ );

diff --git a/src/core/expression/qgsexpression.cpp b/src/core/expression/qgsexpression.cpp
@@ -1374,6 +1374,13 @@ int QgsExpression::expressionToLayerFieldIndex( const QString &expression, const
   return -1;
 }
 
+QString QgsExpression::quoteFieldExpression( const QString &expression, const QgsVectorLayer *layer )
+{
+  if ( !expression.contains( '\"' ) && QgsExpression::expressionToLayerFieldIndex( expression, layer ) != -1 )
+    return QgsExpression::quotedColumnRef( expression );
+  return expression;
+}
+
 QList<const QgsExpressionNode *> QgsExpression::nodes() const
 {
   if ( !d->mRootNode )

diff --git a/src/core/expression/qgsexpression.h b/src/core/expression/qgsexpression.h
@@ -369,6 +369,20 @@ class CORE_EXPORT QgsExpression
      */
     static int expressionToLayerFieldIndex( const QString &expression, const QgsVectorLayer *layer );
 
+    /**
+     * Validate if the expression is a field in the \a layer and ensure it is quoted.
+     *
+     * Given a string which may either directly match a field name from a layer, OR may
+     * be an expression which consists only of a single field reference for that layer, this
+     * method will return the quoted field.
+     *
+     * \returns the \a expression if not a field or quotes are not required, otherwise requite a quoted field.
+     *
+     * \see expressionToLayerFieldIndex()
+     * \since QGIS 3.24
+     */
+    static QString quoteFieldExpression( const QString &expression, const QgsVectorLayer *layer );
+
     /**
      * Tests whether a string is a valid expression.
      * \param text string to test

diff --git a/src/gui/layout/qgslayoutatlaswidget.cpp b/src/gui/layout/qgslayoutatlaswidget.cpp
@@ -240,7 +240,8 @@ void QgsLayoutAtlasWidget::changesSortFeatureExpression( const QString &expressi
 
   mBlockUpdates = true;
   mLayout->undoStack()->beginCommand( mAtlas, tr( "Change Atlas Sort" ) );
-  mAtlas->setSortExpression( expression );
+  QgsVectorLayer *vlayer = qobject_cast<QgsVectorLayer *>( mAtlasCoverageLayerComboBox->currentLayer() );
+  mAtlas->setSortExpression( QgsExpression::quoteFieldExpression( expression, vlayer ) );
   mLayout->undoStack()->endCommand();
   mBlockUpdates = false;
   updateAtlasFeatures();

diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -3906,6 +3906,32 @@ class TestQgsExpression: public QObject
       QCOMPARE( QgsExpression::expressionToLayerFieldIndex( "  (  \"ANOTHER FIELD\"   )   ", layer.get() ), 1 );
     }
 
+    void test_quoteFieldExpression()
+    {
+      std::unique_ptr layer = std::make_unique< QgsVectorLayer >( QStringLiteral( "Point" ), QStringLiteral( "test" ), QStringLiteral( "memory" ) );
+      layer->dataProvider()->addAttributes( { QgsField( QStringLiteral( "field1" ), QVariant::String ),
+                                              QgsField( QStringLiteral( "another FIELD" ), QVariant::String ) } );
+      layer->updateFields();
+
+      QCOMPARE( QgsExpression::quoteFieldExpression( "", layer.get() ), "" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "42", layer.get() ), "42" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "foo", layer.get() ), "foo" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "\"foo bar\"", layer.get() ), "\"foo bar\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "sqrt(foo)", layer.get() ), "sqrt(foo)" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "foo + bar", layer.get() ), "foo + bar" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "field1", layer.get() ), "\"field1\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "FIELD1", layer.get() ), "\"FIELD1\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "\"field1\"", layer.get() ), "\"field1\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "\"FIELD1\"", layer.get() ), "\"FIELD1\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "  (  \"field1\"   )   ", layer.get() ), "  (  \"field1\"   )   " );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "another FIELD", layer.get() ), "\"another FIELD\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "ANOTHER field", layer.get() ), "\"ANOTHER field\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "  ANOTHER field  ", layer.get() ), "\"  ANOTHER field  \"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "\"another field\"", layer.get() ), "\"another field\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "\"ANOTHER FIELD\"", layer.get() ), "\"ANOTHER FIELD\"" );
+      QCOMPARE( QgsExpression::quoteFieldExpression( "  (  \"ANOTHER FIELD\"   )   ", layer.get() ), "  (  \"ANOTHER FIELD\"   )   " );
+    }
+
     void test_implicitSharing()
     {
       QgsExpression *exp = new QgsExpression( QStringLiteral( "Pilots > 2" ) );