Add filterType source param and use QGIS EXP_FILTER when param is qgis

[MattiasSp]

Fixes #1139.

Adds support for QGIS Server's EXP_FILTER parameter wherever GeoServer's CQL_FILTER is currently used.

QGIS Server layers now need to specify the filterType config option on the source to make use of layer filters:

"source": {
    "qgisserver_wfs": {
      "url": "https://my.qgis.server/wfs",
      "filterType": "qgis"
    }
  },

WFS layers now create a WfsSource with filterType: 'cql' by default to preserve current behaviour.

[steff-o]

I have pretty much the same comment as on #1535 that the double implementation in both getFeature() and WfsSource can be avoided by making getFeature() a wrapper to WfsLayer. In addition I wonder if it is even possible to test the offline implementation, as to my understanding the offline functionality is dead code that is not included anywhere.

[MattiasSp]

I have pretty much the same comment as on #1535 that the double implementation in both getFeature() and WfsSource can be avoided by making getFeature() a wrapper to WfsLayer.

I thought about it while writing this PR and #1535 but settled on handling the removal/wrapping of getFeature() separately. For one, I didn't feel confident about catching all occurences (e g I wasn't aware of its use in the API) and, secondly, if anything were to break I thought it would be better not to have to roll back any commits needed for this PR at the same time.

I do agree with you about purging getFeature() but would prefer it to be a separate PR.

Just to add to the complexities:

• getFeature() uses hard coded WFS version 1.0.0, while WfsSource uses hard coded version 1.1.0
• getFeature() handles AGS_FEATURE layers which WfsSource does not.
  • Should we move the getFeature() functionality up one level to a new VectorSource parent class that is inherited both in agsfeature.js and in wfssource.js?
  • Or do we implement an equivalent functionality for AGS_FEATURE sources?
  • Or do we make only the non-AGS_FEATURE-part of getFeature() into a wrapper?

[MattiasSp]

In addition I wonder if it is even possible to test the offline implementation, as to my understanding the offline functionality is dead code that is not included anywhere.

I see, so maybe I changed those parts unnecessarily. Would you prefer that we don't touch them, that I revert that specific commit?

[steff-o]

Tested with geoserver only as I don't have qgis. It works as before for Geoserver, so it does not break anything.

Apart from that I have not tested offline, as it would not work anyway. Leave that code, it might be a heads up if someone picks up development later.

Regarding making getFeature() into a wrapper I would have no problems with just making it for WFS. The api experience would still be the same. But as you point out, it possibly should be another issue. But considering the amount of double work it would have been nice to have it done before more features are added to the source as it already is inconsistent behavior between the layer source and getFeature.

[steff-o]

This won't work when using double underscore notation or the 'id'-option on the layer. 'id' should be used instead of 'name' if it really is necessary to prepend the layername. Geoserver can handle with or without layername, but it may be dangerous to assume that it always shall be prepended, When calling from search the search result would most likely not not have layername prepended, but if calling from some arbitrary code it will probably already be prepended.

[jokd]

@MattiasSp Is this still a work in progress or outdated? Otherwise can you solve the conflicts and we can have another look at it.

[steff-o]

@MattiasSp Is this PR still relevant now that getFeature() has been completely rewritten? The parts in WfsSource is probably still relevant but probably hard to merge.

Please review this PR and update it if possible or remove it.

[MattiasSp]

Hi @steff-o and @jokd! I've merged this with master and put some more work into it. But now the commit history is rather messy. Do you think I should keep it as it is or would you prefer that I refactor the changes into a new PR?

[MattiasSp]

The gist of the PR is that the _loaderHelper() function of WfsSource now calls a createQueryFilter() function where the extent and filter parameters are generated together. In the _loaderHelper(), the complete url is put together using either FeatureId, if ids are provided, or else the integrated query filter from createQueryFilter().

Both the integrated query filter and the FeatureId parameter need to be handled differently between GeoServer and QGIS Server, so for both of those parts there is a switch based on the new source config option filterType. filterType has cql set as the default - retaining the current behaviour. Setting "filterType": "qgis" on a source means the url will be generated using QGIS Server's EXP_FILTER syntax and any FeatureId parameter will have the layer name (id) prepended to the ids.

Thus, the WfsSource is also ready to have further filter types such as OGC filters added - which to my knowledge is supported by both GeoServer and QGIS Server and possibly other map servers as well.

Some background to explain the changes;

• The BBOX and CQL_FILTER URL parameters are incompatible in GeoServer and give an error if used together
  • GeoServer: <ows:ExceptionText>bbox and cql_filter both specified but are mutually exclusive</ows:ExceptionText>
  • This is the reason extents and other layer filters are integrated in createQueryFilter(). In the cases where features should be filtered both by an extent and by e g attributes, the extent filter is also set in the CQL_FILTER or EXP_FILTER rather than using the BBOX parameter.
• BBOX and FeatureId are incompatible in both GeoServer and QGIS Server
  • GeoServer: <ows:ExceptionText>featureId and bbox both specified but are mutually exclusive</ows:ExceptionText>
  • QGIS Server: <ServiceException code="RequestNotWellFormed">FEATUREID FILTER and BBOX parameters are mutually exclusive</ServiceException>
  • This was handled by advising against calling _loaderHelper() with extent or layer filters together with FeatureId. Now, the FeatureId parameter will instead take precedence if the ids parameter is included in the function call. The change should not break any code that used to work, unless they rely on the request failing 😅
• FeatureId and CQL_FILTER are incompatible in GeoServer, but in QGIS Server the FeatureId will override any EXP_FILTER
  • GeoServer: <ows:ExceptionText>featureId and cql_filter both specified but are mutually exclusive</ows:ExceptionText>

[steff-o]

I'll have a look soon, missed the notification.

Don't think the commit history matters, it will be squashed when the PR is merged.

[steff-o]

Tested on Geoserver only as I don't have access to a Qgis server. Seems to work as expected on Geoserver so at least it doesn't break anything when I tested related tables, MultiSelect and plain Wfs.

Some issues that are not related to functionality, but bugs me:

• The changes to offline/wfs.js. Those changes can not possibly be tested and will only add to the confusion whether offline exists or not. (It doesn't, it's just some dead code.)
• The changes to getfeature.js is not necessary or even useful. The filterType and filter will only be used when trying to fetch features from a WMS-layer using the quirky "assume that there is a WFS layer at the same adress as the WMS layer" functionality that is only used by Multiselect plugin. And still it won't work unless using some extra configurations:
  • The WMS layer will need to have a "filterType" setting, which is not documented and not used anywhere.
  • The WMS layer would need to have a property called "filter" (getfeature.js row 98), which is not documented and not used anywhere. In order for a WMS layer to have a filter that ends up in the WMS request, it is set on the "sourceParam"-property which will not be transferred to the WFS request.
• Function createQueryFilter should be marked as private unless you want to support it indefinitely as an instance of wfssource can be accessed from api.
• The comment in line 128 of wfssource is incorrect; it should be "the filter parameter and BBOX" instead of featureid (even if that also is correct, but not relevant at that part of the code)
• Related tables may not work with qgis server due to row 59 in relatedTables.js if Qgis uses a different syntax. I don't have a Qgis server to test on, but if it doesn't work it's not a big deal as it didn't work before either. Just a heads up.

Other than that it seems like a stellar upgrade for the ever increasing Qgis server following!

Edit: First version stated that it was not possible to configure for qgis. My bad I put the configuration at layer level.

[MattiasSp]

Thanks for the review @steff-o!

I'll respond to your feedback and edit this comment as I adjust the PR:

• The changes to offline/wfs.js
  • Agreed - I reverted the related commits
• The changes to getfeature.js
  • This one was a bit complex - but I think you are right to suggest I remove it - done!
  • I think that GeoServer's WMS layers could support filtering with these and a couple more changes. Maybe that might be a future addition. For QGIS Server WMS layers, the filter syntax is not the same for WMS as for WFS layers, so that would mean even more specialized filter handling.
• The comment in line 128 of wfssource is incorrect
  • Fixed!
• Function createQueryFilter should be marked as private
  • Done, although what is your take on the AirBnB rule 23.4 - no-underscore-dangle? Is it enough that I put a leading underscore and added a comment?
• Related tables may not work with qgis server due to row 59 in relatedTables.js
  • Displaying related table attributes seems to work, but I didn't get editing to work yet. I will leave that work for another PR.
• Should it work as expected when putting the filterType param at layer level?
  • I changed the default for WFS layers, to use filterType from the layer config if there is one. The source level setting will still take precedence over layer settings.

[MattiasSp]

There @steff-o, I think I have addressed all of your comments now - ready for another look :)

[steff-o]

Surely layer configuration should override source configuration, right? Seems more logical to me.

Other than that it looks OK. I'm totally fine with dangling underscores, In fact OpenLayers rely on it and up until recently it was the best we had. In the future we may decide to allow hash-privates as it has been along for quite a while now.

Yes, editing is a completely different beast and should be kept out of this PR.

[MattiasSp]

Surely layer configuration should override source configuration, right? Seems more logical to me.

I considered that initially. Setting a default on the source and a more specific override on the layer would definitely feel more logical in the general case.

However, a single source can not handle multiple different filterTypes, so overriding a source value on the layer level with a different filterType must mean that one of the values are incorrect and will not work. Either the overriding layer or all the other layers using that source will be incorrectly configured.

Allowing filterType on the layer level was really only meant as a courtesy to administrators, so that it will still work if they set the value on the layer instead of the source - but, in my opinion, it's really an aspect of the source that is being defined and it should ideally be set at the source level.

[steff-o]

A single source COULD be able to support different filters for different layers. At least Geoserver also supports the OGC filter parameter FILTER. Qgis probably does that as well as it is in the WMS specification. Right now it is not possible to specify filterType = OGC, but having the source's setting overriding the layer's setting will make it impossible to implement that later.

Having the possibility to use different filter types on same source would be the reason to call the setting "filterType" instead of having a server type setting on the source, which could be used for more vendor specific features without having to specify each vendor specific feature. For instance WMS sources have a ```type`` `setting that can be used to utilize different vendor specific features (probably not documented).

I think the simplest solution would be removing the possibility to configure it at layer level for now. If someone actually implements more filter types it can be added at layer level to override source setting if desired.

[steff-o]

LGTM. I will merge soon unless anyone objects.

[MattiasSp]

A single source COULD be able to support different filters for different layers [...] Having the possibility to use different filter types on same source would be the reason to call the setting "filterType" instead of having a server type setting on the source

Completely agree and I think what was part of my original thinking even though I seem to have forgotten xD

LGTM. I will merge soon unless anyone objects.

Thank you @steff-o! And thanks again for all the constructive feedback along the way.