-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
[WFS] Fix filtering by QGIS expression in order to support "@geometry" (Fix #60094) #60105
[WFS] Fix filtering by QGIS expression in order to support "@geometry" (Fix #60094) #60105
Conversation
908712f
to
91bdc51
Compare
cb63312
to
cdb91f5
Compare
for more information, see https://pre-commit.ci
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
src/core/qgsogcutils.cpp
Outdated
@@ -2318,7 +2318,7 @@ static bool isGeometryColumn( const QgsExpressionNode *node ) | |||
|
|||
const QgsExpressionNodeFunction *fn = static_cast<const QgsExpressionNodeFunction *>( node ); | |||
QgsExpressionFunction *fd = QgsExpression::Functions()[fn->fnIndex()]; | |||
return fd->name() == QLatin1String( "$geometry" ); | |||
return fd->name() == QLatin1String( "$geometry" ) || fn->referencedVariables().contains( QLatin1String( "geometry" ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sure about that but couldn't there be situations where a complicated function would be used that would reference the "geometry" variable but also others, and thus would not just be "@geometry" ? If so, the following could be more appropriate:
return fd->name() == QLatin1String( "$geometry" ) || fn->referencedVariables().contains( QLatin1String( "geometry" ) ); | |
return fd->name() == QLatin1String( "$geometry" ) || ( fn->referencedVariables().size() == 1 && fn->referencedVariables().contains( QLatin1String( "geometry" ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! Anyway, thinking about it more carefully, it looks like even fn->referencedVariables().size() == 1 && fn->referencedVariables().contains( QLatin1String( "geometry" ) )
wouldn't reject an expression like intersects(centroid(@geometry), geomFromWKT(‘POINT (5 6)’))
while it should be.
It seems to me it should be used instead: fd->name() == QLatin1String( "var" ) && fn->referencedVariables().contains( QLatin1String( "geometry" ) )
so we ensure that the first argument is actually a "var" QgsExpressionNode::ntFunction which can only reference 1 variable, and not a generic function that references a "geometry" variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t seems to me it should be used instead:
fd->name() == QLatin1String( "var" ) && fn->referencedVariables().contains( QLatin1String( "geometry" ) )
yes that looks even better
Description
Adds support to
@geometry
(#50134) besides$geometry
in WFS filtering by QGIS expression.The
isGeometryColumn()
method has been updated to also recognize the@geometry
variable in order for theexpressionFunctionToOgcFilter()
method to correctly traslate expressions containing the@geometry
variable.(The
expressionFromOgcFilter()
has not been updated to use@geometry
instead$geometry
)Added tests for expressions using
@geometry
.Also updated the WFS section of the QgsVectorLayer documentation (following up #60095) substituting
$geometry
with@geometry
(ref. #60095 (comment) @uclaros).Fixes #60094.
I'm unsure about the correctness of the error message at:
QGIS/src/core/qgsogcutils.cpp
Line 2385 in 42e7ad1
Should it be amended somehow?