11578 make search-filters request.method agnostic

[whatisgalen]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Issues Solved

Partly addresses #11578

Checklist

• I targeted one of these branches:
  • dev/8.0.x (under development): features, bugfixes not covered below
  • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
  • dev/6.2.x (extended support): major security issues, data loss issues
• I added a changelog in arches/releases
• I submitted a PR to arches-docs (if appropriate)
• Unit tests pass locally with my changes
• I added tests that prove my fix is effective or that my feature works
• My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic	Changed	Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

• Sponsored by:
• Found by: @
• Tested by: @
• Designed by: @

Further comments

[chiatt]

This change doesn't seem like it addresses a crashing bug or anything that wasn't already in previous versions of Arches. I could be wrong, but it seems like the target should be 8.0.

[whatisgalen]

This change doesn't seem like it addresses a crashing bug or anything that wasn't already in previous versions of Arches. I could be wrong, but it seems like the target should be 8.0.

One could argue that issues like #10630 are crashing bugs. Previously, to crash the vanilla map-filter on the frontend you would have had to draw an unrealistic number of vertices. However, given the new "feature by filter" functionality in 7.6.0, you can select an existing geometry that might have a very high vertex-count and this breaks the entire search request as it could exceed the number of characters permitted in the GET header.

The vanilla map filter looks like this on the backend:

class MapFilter(BaseSearchFilter):
    def append_dsl(self, search_query_object, **kwargs):
        permitted_nodegroups = kwargs.get("permitted_nodegroups")
        include_provisional = kwargs.get("include_provisional")
        search_query = Bool()
        querystring_params = kwargs.get("querystring", "{}")

so it's actually already request-agnostic; a developer could make a search-view that sends a POST request instead of a GET request. The issue is that other filters are hard-coded to expect a GET request, so if those other filters were used in combination with a map-filter POST request, only part of the search request would be fulfilled. Those other search filters are: paging-filter, sort-results, and term-filter. The standard search-view already looks for a POST request or a GET request so the scope is just limited to making sure that gets honored fully in all search filters.

[whatisgalen]

Regarding my comment on PR #10827:

Per my comment, the featureid was removed from the request payload to the backend (map-filter) because if the user edited the drawn polygon on the map, (for example to cover a larger spatial query area) the feature identified by featureid would differ from the query geometry, and there wasn't a great way to confirm whether the geometry had actually changed (given potential precision differences). This would have yielded search results out of sync with the search query.

However, because the only payload is the geojson feature itself, this means large coordinate sets can exceed the character limit on the GET request header (and url length in the browser). Unfortunately, those requests then fail, hence #11578 and the need for #11582

...There's now a solid category of search requests that will always fail whenever a high-vertex feature is used as a map filter. This means that the feature is inherently bugged until the search request can be sent as a POST request rather than GET.

[apeters]

General comments about structure.

1. I think we should push most of the logic to parse the request into the base_search_view.py module and centralize it there.
2. Just like we put the the request object on self, we should put the search_request (currently called search_request_object) on self as well.
3. We should then not have to pass it into the methods on the BaseSearchFilter

Do that first and then I'll re-review.

[apeters]

for clarity, and to improve on something that should have been done in the first place, this should default to False and not to search_request_object

[whatisgalen]

This actually already does default to False, see full method from string_utils.py:

def get_str_kwarg_as_bool(
    key, request_dict: Union[Dict[str, Any], QueryDict], default: bool = False
) -> bool:
    value = request_dict.get(key, str(default))
    if isinstance(value, bool):
        return value
    return str_to_bool(str(value))

[apeters]

same as above, should default to False

[whatisgalen]

see comment above

[whatisgalen]

General comments about structure.

1. I think we should push most of the logic to parse the request into the base_search_view.py module and centralize it there.

✅ (moved it into SearchFilterFactory which leverages the SearchView.create_query_dict() method anyhow)

2. Just like we put the the request object on self, we should put the search_request (currently called search_request_object) on self as well.

✅

3. We should then not have to pass it into the methods on the BaseSearchFilter

✅

Do that first and then I'll re-review.