qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
117 stars 37 forks source link

Allow filter to use both FeatureId and Expressions #258

Open dmarteau opened 1 year ago

dmarteau commented 1 year ago

QGIS Enhancement: Allow filter to use both FeatureId and Expressions

Date 2022/11/08

Author David Marteau (@dmarteau)

Contact dmarteau at 3liz dot com

maintainer @dmarteau

Version QGIS 3.30

Summary

This QEP address a situation in server where Control access filters are used with feature id filter, but the proposed solution may be beneficial to Qgis core and this is why we do not tag this QEP as server only.

In the actual situation a QgsFeatureRequest object handle be of two type only:

These type are mutually exclusive which means that cannot filter at the same time using feature's ID and expression.

This leads to the situation where a server plugins controlling acl use a layerFilterExpression in order to restrict the list of objects available, Qgis will apply the expression to the QgsFeatureRequest without controlling the current type of the filter.

If the QgsFeatureRequest is of type QgsFeatureRequest::FilterFids the call above will change the type of the filter to QgsFeatureRequest::FilterExpression, discarding the filtering by feature ids.

How Filter's realization are implemented are left to providers and separating filter's type is a way to enable providers to execute them in the most efficient way.

While some providers, like Shapefile's provider, must implement selection by id and by expressions in different way; others like Postgresql's provider just translate selection by id to expressions.

The fact that selection by feature id and expression are mutually exclusive is a constraint which is not justified for some providers. This leads to some situations where both filters are desirable, but achieving this in the actuel state may be totally inefficient for some providers like PostgreSQL.

Proposed Solution

Since the responsibility of the realization of the filters is left to provider's implementation, we think that there is no fundamental reason to restrict limit QgsFeatureRequest to either one type of another.

Without breaking api, a new QgsFeatureRequest type could be used in situation where both expression and selection by id is required. It would be the responsibility of the provider to combine both filters in the most efficient way: for postgres simple combinaison of expression is used - as it actually turns request by id as expression.

Other providers could simply implement a two stage selection if it appears that they cannot be combined efficiently.

Example(s)

Define a new QgsFeatureRequest::FilterType: FilterIdAndExpression

Since a QgsFeatureRequest object can actually hold both a set of ids and expressions, no other changes would be required but set the appropriate flags depending on the QgsFeatureRequest state.

Affected Files

For each provider:

Performance Implications

We do not expect any performance loss since in the worst case this would reduce to following calls of existing implementations (selection by feature, selection by expressions).

In the best case scenario, selection by ids is simply combined with the expression and passed as a single request as for the postgresql provider.

Votes

(required)

troopa81 commented 1 year ago

If the QgsFeatureRequest is of type QgsFeatureRequest::FilterFids the call above will change the type of the filter to QgsFeatureRequest::FilterExpression, discarding the filtering by feature ids.

Isn't that possible to convert the QgsFeatureRequest::FilterFids into an expression and to combine it with the expression that filter access control in the plugin ?

Have you already thought about some other part of QGIS that could benefit from this improvment?

roya0045 commented 1 year ago

If the QgsFeatureRequest is of type QgsFeatureRequest::FilterFids the call above will change the type of the filter to QgsFeatureRequest::FilterExpression, discarding the filtering by feature ids.

Isn't that possible to convert the QgsFeatureRequest::FilterFids into an expression and to combine it with the expression that filter access control in the plugin ?

Have you already thought about some other part of QGIS that could benefit from this improvment?

I vaguely had a use case years ago when I proposed a PR with similar intent, I don't recall much why and don't really need it anymore. Though the aspect of just adding it to an expression vs a dedicated filter was briefly mentionned.

Nyall's comment might be helpful https://github.com/qgis/QGIS/pull/9915#issuecomment-505734932

dmarteau commented 1 year ago

@troopa81

Isn't that possible to convert the QgsFeatureRequest::FilterFids into an expression and to combine it with the expression that filter access control in the plugin ?

It would be the solution if all providers could handle it, which is not the case: as mentionned above, shapefiles cannot handle selection by Ids as expression.

dmarteau commented 1 year ago

@roya0045 Whoa ! Thanks for the link ! I need to check it more in detail but your PR appears to be pretty much what we want to do, may be it can be revived as an implementation proposal for this QEP. ?

nyalldawson commented 1 year ago

@dmarteau just make sure it's super clear upfront: any changes to the feature request API need to be implemented for all providers and QgsFeatureSource subclasses upfront. Make sure you've factored all that into your plans -- there's a LOT of subclasses which need updating 🙃

dmarteau commented 1 year ago

there's a LOT of subclasses which need updating

Yes I know, that's why may be we can use this QEP to explicitely address what's need to be done before lauching a huge refactoring boat...

roya0045 commented 1 year ago

@roya0045 Whoa ! Thanks for the link ! I need to check it more in detail but your PR appears to be pretty much what we want to do, may be it can be revived as an implementation proposal for this QEP. ?

Strangely the branch still exist, I nrealy deleted it last week (usually I delete what is merged or what isn't merged and aren't in my homebrew).

If you want it you can take the changes and adapt them to your needs.

dmarteau commented 1 year ago

@roya0045 I took the branch, thanks.