Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(regexp_replace): handle null value #59186

Closed
wants to merge 1 commit into from
Closed

fix(regexp_replace): handle null value #59186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/core/expression/qgsexpressionfunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,13 +1609,20 @@ static QVariant fcnRegexpReplace( const QVariantList &values, const QgsExpressio
QString str = QgsExpressionUtils::getStringValue( values.at( 0 ), parent );
QString regexp = QgsExpressionUtils::getStringValue( values.at( 1 ), parent );
QString after = QgsExpressionUtils::getStringValue( values.at( 2 ), parent );
bool isValid_after{ values.at( 2 ).isValid() };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool isValid_after{ values.at( 2 ).isValid() };
bool isValid_after{ values.at( 2 ).isValid() && !values.at( 2 ).isNull() };

Not sure of myself here. But I'm also wondering if just the isNull() condition wouldn't be enough ?

In your test, if you replace QVariant with QVariant( QMetaType::String ) for instance, it would be valid and null, and so your fix wouldn't work, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always use QgsVariantUtils::isNull for these checks.


QRegularExpression re( regexp, QRegularExpression::UseUnicodePropertiesOption );
if ( !re.isValid() )
{
parent->setEvalErrorString( QObject::tr( "Invalid regular expression '%1': %2" ).arg( regexp, re.errorString() ) );
return QVariant();
}

if ( ! isValid_after && str.contains( re ) ) // fix https://github.com/qgis/QGIS/issues/44274
{
return QVariant();
}

return QVariant( str.replace( re, after ) );
}

Expand Down Expand Up @@ -8546,7 +8553,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
<< new QgsStaticExpressionFunction( QStringLiteral( "length3D" ), QgsExpressionFunction::ParameterList() << QgsExpressionFunction::Parameter( QStringLiteral( "geometry" ) ), fcnLength3D, QStringLiteral( "GeometryGroup" ) )
<< new QgsStaticExpressionFunction( QStringLiteral( "replace" ), -1, fcnReplace, QStringLiteral( "String" ) )
<< new QgsStaticExpressionFunction( QStringLiteral( "regexp_replace" ), QgsExpressionFunction::ParameterList() << QgsExpressionFunction::Parameter( QStringLiteral( "input_string" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "regex" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "replacement" ) ), fcnRegexpReplace, QStringLiteral( "String" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "replacement" ) ), fcnRegexpReplace, QStringLiteral( "String" ),
QString(), false, QSet< QString >(), false, QStringList(), true )
<< new QgsStaticExpressionFunction( QStringLiteral( "regexp_substr" ), QgsExpressionFunction::ParameterList() << QgsExpressionFunction::Parameter( QStringLiteral( "input_string" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "regex" ) ), fcnRegexpSubstr, QStringLiteral( "String" ) )
<< new QgsStaticExpressionFunction( QStringLiteral( "substr" ), QgsExpressionFunction::ParameterList() << QgsExpressionFunction::Parameter( QStringLiteral( "string" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "start" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "length" ), true ), fcnSubstr, QStringLiteral( "String" ), QString(),
false, QSet< QString >(), false, QStringList(), true )
Expand Down
3 changes: 3 additions & 0 deletions tests/src/core/testqgsexpression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1695,6 +1695,9 @@ class TestQgsExpression: public QObject
QTest::newRow( "regexp_replace non greedy" ) << "regexp_replace('HeLLo','(?<=H).*?L', '-')" << false << QVariant( "H-Lo" );
QTest::newRow( "regexp_replace cap group" ) << "regexp_replace('HeLLo','(eL)', 'x\\\\1x')" << false << QVariant( "HxeLxLo" );
QTest::newRow( "regexp_replace invalid" ) << "regexp_replace('HeLLo','[[[', '-')" << true << QVariant();
QTest::newRow( "regexp_replace match, with null" ) << "regexp_replace('aaaa','a',NULL)" << false << QVariant( ); // fix https://github.com/qgis/QGIS/issues/44274
QTest::newRow( "regexp_replace not full match, with null" ) << "regexp_replace('aaabbb','b',NULL)" << false << QVariant( );
QTest::newRow( "regexp_replace no match with null" ) << "regexp_replace('zzz', 'b', NULL)" << false << QVariant( "zzz" );
QTest::newRow( "substr" ) << "substr('HeLLo', 3,2)" << false << QVariant( "LL" );
QTest::newRow( "substr named parameters" ) << "substr(string:='HeLLo',start:=3,length:=2)" << false << QVariant( "LL" );
QTest::newRow( "substr negative start" ) << "substr('HeLLo', -4)" << false << QVariant( "eLLo" );
Expand Down
Loading