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.

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

Mixed Filters #81

Closed
HelenwoodsJNCC opened this issue May 16, 2019 · 15 comments
Closed

Mixed Filters #81

HelenwoodsJNCC opened this issue May 16, 2019 · 15 comments

Comments

@HelenwoodsJNCC
Copy link

Can we look into the issue the occurs when combining simple and complex filters on a single layer - first raised by Graeme in issue #37

@JamesPe
Copy link
Collaborator

JamesPe commented May 16, 2019

@andyb-esdm - not sure we ever anticipated this - can you have a check in your code to see if its something simple like you skipping out after you have found the simple one thinking tehre won't be a complex one as well ?

@andyb-esdm
Copy link
Collaborator

@JamesPe No we didn't anticipate this (I don't really understand how it could work server side to be honest). I also perceived it as a mistake at the time to put type on the filter table instead of filterType on the layer table.
So, currently the mapper checks what kind of filter (simple or complex) it needs to apply by testing every filter. That is, if every filter is simple or every filter is complex apply the filter else do nothing. It then sets a single parameter on the layer source, either viewParams or CQL_FILTER.
To implement combined filter types, it would have to be a bit more intelligent about managing a pair of filter params for the layer source (instead of a single string) where one of them might not be set. It would also have to combine the two types of filters when it sets them on the layer source.
I'm afraid the assumption of a single type of filter is a bit embedded across the code. I'm not sure how much refactoring it would require. So time estimate between 0.5d and 1d (max).

@HelenwoodsJNCC
Copy link
Author

@andyb-esdm ok thanks Andy, @JordanPinder @GMDuncan do you want to focus on this issue this month and save most of the other issues for later in the support contract?

@JordanPinder
Copy link

@HelenwoodsJNCC given that this would take the most time I'd agree in focusing on mixed filters this month using the 12 hours we have (8 regular & 4 carried from last month).

@HelenwoodsJNCC
Copy link
Author

@andyb-esdm can we focus on this issue for the support contract this month then please. I will close the other issues and re-open another month. Please let us know if you think it will take longer to sort out i.e if you would need to use any of next months hours. Thanks!

@andyb-esdm
Copy link
Collaborator

@HelenwoodsJNCC Okay. I'll warn you if it looks like it will take longer but it really shouldn't.
I'll use this issue to double-check I understand what @GMDuncan requires.

@GMDuncan
Copy link

@andyb-esdm I think the final behaviour would be that both viewparams and cqlfilter would be passed through to the WMS request. So each would behave as they currently do, but are not mutually exclusive. I believe that Geoserver can handle both simultaneously, as I'm fairly certain that I tested this back at the beginning of the Geoserver move but I can confirm this for you if you want.

An example would be for Annex I maps from survey. Here:

  • a GUI is passed through as a complex filter - feeding through to a "Like" statement in the view so that for example a user can type in "GB" to select all British maps. viewParams not passed to WMS query if no input in text box (same as current viewparam behaviour).
  • a user may wish to select multiple AnnexI habitat types (via checkboxes), which would be passed through as a simple/cql filter. cqlfilter not passto WMS if no checkboxes are enabled (same as current cqlfilter behaviour).

I didn't realise that the code structure was set up this way, though. So happy for alternatives to be found if they're more sensible?

@andyb-esdm
Copy link
Collaborator

@GMDuncan My fault about the code structure! I had been thinking about creating a filter service class to provide more sophisticated filter handling, instead of just concatenating a string and setting a single parameter on the request. So now is the chance to do this.

@GMDuncan
Copy link

Warning: thought below, not to be taken as a request, just for comments!
@andyb-esdm That sounds interesting! It might actually tie up with something that was a potential for "future support", which was to get advice on handling "Like andy" type statements in filters, which again is something that would be incredibly useful for especially EUNIS habitats and checkboxes where you're looking for partial matches against multiple options.

It's not currently possible without some horrendous SQL behind the scenes, but it would.... i think ... be possible if there was the option to "wrap" or "decorate" (depending on what word works best!) filter values before they get concatenated together, which sounds potentially in the same area of thinking of your thoughts above.

The behaviour would be that if a user selected hab_type values of "A5,2" "A5.3" and "A2", and the wrapper on the filter was set as "{}%" with {} representing the value, the resulting query string would be something like &viewparams=hab_type:'A5.2%','A5.3%',A2%' which could be easily passed into the SQL view.

This is something that is very much not required at the moment, but might make a good addition later. And if you're delving into this area in this ticket, i'm wondering if it would be beneficial to do anything at once?

Keen on hearing your thoughts on this (whether it's a terrible idea, whether it's better done other ways etc!) just for consideration before delving into filter code twice...

Also keen on @HelenwoodsJNCC @JordanPinder and @helen-lillis thoughts!

@andyb-esdm
Copy link
Collaborator

@GMDuncan Something like this is entirely possible. Right now the filter vaues are reformatted to escape special characters etc. and that reformatting could be extended for different filter types. The detail here would be how these are configured server-side I guess?
Anyway, will refactor existing code with this potentially in mind.

@GMDuncan
Copy link

GMDuncan commented May 20, 2019

Sounds good - we can discuss this internally on a requirement level then and add on in future if necessary.

I was thinking that client side, the Filter table could have an "InputWrapper" column which stores the pattern for value replacement in some easyish to understand way with a placeholder for the value. One question concerns behaviour for multiselect vs single value, whether to enable the behaviour on both or only enable for multiselect? To me it's probably safest to go with what's easier to code, as it's less necessary on a single value filter (because the wrapper can potentially just be stored in the SQL view itself), but depending on the code, enabling a default behaviour might be easier (or the same effort) as restricting the behaviour to one type!

Anyway, will have a chat with everyone in-house about this.

Also I just realised that I wrote "Like andy" in the comment above, obviously I meant "like any", but I think the typo works well too haha!

@andyb-esdm
Copy link
Collaborator

@GMDuncan @HelenwoodsJNCC
I've implemented the mixed filters for a layer so you can now have viewParams and cql_filter in the same request.
I'm afraid my testing has been limited to changing the filter configuration and examining the request to the WMS. The wms services we have don't support mixed filters 'for real'!
I've deployed to our test server. Can we discuss how to test this against a real layer that supports mixed filters?

@andyb-esdm
Copy link
Collaborator

One other thing to note here is that the CQL_FILTER doesn't currently support free text. It's lookups only. I spoke to SA about this and neither of us can remember why that decision was made. Can either of you remember? If not, should I add it?

@GMDuncan
Copy link

Can't remember that decision reasoning myself, and freetext in CQL seems sensible, so I don't see why it couldn't be added if it's relatively painless? Thoughts @helen-lillis ?

@andyb-esdm
Copy link
Collaborator

I've looked again at the CQL_FILTER. It creates a literal IN clause for look-ups i.e. it returns hab_type IN ('A1', 'A2')
So now it's become a bit clearer why I didn't implement anything for free text - it's difficult to anticipate what the CQL where clause should be (with viewParams it's just a straight substitution).

For example, we could wrap the search term with a LIKE and wildcards: 'mud' would translate to hab_type LIKE '%mud%' or hab_type LIKE 'mud%' etc.

Terms with spaces could translate into IN (similar to lookup example) e.g. 'mud sand' would translate to hab_type IN ('mud', 'sand')
Or it could translate to hab_type LIKE '%mud%' OR hab_type LIKE '%sand%'

So if I do implement this, then we'd need to settle on a single way for free text being handled I think? Maybe the complex (viewParams) approach makes this unnecessary because you can define how you want the search text handled in the view definition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants