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.

Sign up for GitHub

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

11578 make search-filters request.method agnostic #11582

Open
wants to merge 14 commits into
base: dev/7.6.x
Choose a base branch
from
Open

11578 make search-filters request.method agnostic #11582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c4023c1
use search_request query obj in lieu of request.GET for all methods c…
whatisgalen Oct 27, 2024
0315aac
use kwargs.search_request_object instead of request.GET in search_fil…
whatisgalen Oct 27, 2024
6599a43
handle case in tests of empty dict value for paging-filter search par…
whatisgalen Oct 28, 2024
30e3dac
Merge branch 'archesproject-dev/7.6.x' into 11578_search_req_method
whatisgalen Nov 14, 2024
fa81b4f
set dynamic requestMethod for search query method on frontend, re #11578
whatisgalen Nov 14, 2024
a1f34c4
search_request_object --> SearchView.search_request; set self.search_…
whatisgalen Nov 20, 2024
e68f68b
move self.search_request into BaseSearchFilter via SearchFilterFactor…
whatisgalen Nov 20, 2024
9f46aa9
rm search_request as kwarg given self has it now, re #11578
whatisgalen Nov 20, 2024
9abbbba
resolve race conditions between searchfilter factory and searchview, …
whatisgalen Nov 20, 2024
3efea56
rm redundant assignment, re #11578
whatisgalen Nov 20, 2024
de2dd67
Merge branch 'archesproject-dev/7.6.x' into 11578_search_req_method
whatisgalen Jan 2, 2025
52a7b05
make search_results csrf_exempt, re #11578
whatisgalen Jan 2, 2025
127d1d4
access appropriate method body in SearchResultsExporter __init__, re …
whatisgalen Jan 2, 2025
72289c8
fix typo search_request
whatisgalen Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions arches/app/media/js/views/components/search/base-search-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ define([
doQuery: function() {
const queryObj = JSON.parse(this.queryString());
if (self.updateRequest) { self.updateRequest.abort(); }
const requestMethod = JSON.stringify(queryObj).length > 1800 ? "POST" : "GET";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where did this number come from? 2048 seems like the most Edge can handle but most other browsers much more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaves a generous buffer in case the host is really long as can sometimes be the case with development hosts using Azure, for example: "https://longsubdomain.azure.cloudapp.westus3.developmentserverlongname." Is there a reason you think we should increase this limit + forego compatibility with Edge?

Copy link
Contributor

@bferguso bferguso Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe another option is to build the full GET URL string and compare that to the 2048 limit? May need to URL Encode the query string to as that can add a large number of characters to the URL (eg: " " -> "%20", etc).

Something like this might work to get the full URL length?:

const urlLength = (window.location.origin+arches.urls.export_results+"?"+$.param(payload)).length

Also - there's at least one other place that we'll need to handle this on the front end. Maybe worth wrapping the logic in a separate util function as it feels like this may be implemented in a number of places?

arches/arches/app/media/js/views/components/search/search-export.js

Lines 77 to 80 in debacb9

$.ajax({
type: "GET",
url: arches.urls.export_results,
data: payload

self.updateRequest = $.ajax({
type: "GET",
type: requestMethod,
url: arches.urls.search_results,
data: queryObj,
context: this,
Expand Down Expand Up @@ -82,7 +83,8 @@ define([
},
complete: function(request, status) {
self.updateRequest = undefined;
window.history.pushState({}, '', '?' + $.param(queryObj).split('+').join('%20'));
if (requestMethod === "GET")
window.history.pushState({}, '', '?' + $.param(queryObj).split('+').join('%20'));
this.sharedStateObject.loading(false);
}
});
Expand Down
15 changes: 10 additions & 5 deletions arches/app/search/components/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@


class BaseSearchFilter:
def __init__(self, request=None, user=None, componentname=None):
def __init__(
self, request=None, search_request=None, user=None, componentname=None
):
self.request = request
self.search_request = search_request
self.user = user
self.componentname = componentname

Expand Down Expand Up @@ -54,6 +57,10 @@ def __init__(self, request=None, user=None):
for search_filter in SearchComponent.objects.all()
}
self.search_filters_instances = {}
request_object = (
self.request.GET if self.request.method == "GET" else self.request.POST
)
self.search_request = request_object.dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should rename this to "self.querystring" because essentially that's what it is. This would reduce confusion with "self.request"


def get_filter(self, componentname):
if componentname in self.search_filters:
Expand All @@ -71,7 +78,7 @@ def get_filter(self, componentname):
)
if class_method:
filter_instance = class_method(
self.request, self.user, componentname
self.request, self.search_request, self.user, componentname
)
self.search_filters_instances[search_filter.componentname] = (
filter_instance
Expand All @@ -83,10 +90,8 @@ def get_filter(self, componentname):
def get_searchview_name(self):
if not self.request:
searchview_component_name = None
elif self.request.method == "POST":
searchview_component_name = self.request.POST.get("search-view", None)
else:
searchview_component_name = self.request.GET.get("search-view", None)
searchview_component_name = self.search_request.get("search-view", None)
Comment on lines 91 to +94
Copy link
Member

@apeters apeters Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only need the "else" portion of this statement. self.request is already assumed to exist in the __init__ method. If it doesn't exist, then that method will fail. Will self.request ever be None?


if not searchview_component_name:
# get default search_view component
Expand Down
12 changes: 10 additions & 2 deletions arches/app/search/components/base_search_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,15 @@ class BaseSearchView(BaseSearchFilter):
how to execute a search in the search_results method
"""

def __init__(self, request=None, user=None, componentname=None):
super().__init__(request=request, user=user, componentname=componentname)
def __init__(
self, request=None, search_request=None, user=None, componentname=None
):
super().__init__(
request=request,
search_request=search_request,
user=user,
componentname=componentname,
)
self.searchview_component = SearchComponent.objects.get(
componentname=componentname
)
Expand Down Expand Up @@ -95,6 +102,7 @@ def __init__(self, request=None, user=None, componentname=None):
item.componentname, float("inf")
),
)
self.search_request = self.create_query_dict(self.search_request)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this is neccessary.

Copy link
Member Author

@whatisgalen whatisgalen Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_query_dict is a searchview class method, so if your search view has logic, for example, whitelisting/blacklisting certain query parameters, this method is where that's happening. I can see however that because this variable is called search_request, it should probably have some of the other properties of the request object like user etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also due to the inheritance logic of search-view classes, setting the correct search-view on a blank request here:

def create_query_dict(self, query_dict):
        # check that all searchview required linkedSearchFilters are present
        query_dict[self.searchview_component.componentname] = True # set the database default search-view

... is what enables the python class of that search-view to be initialized. I know it looks circular but this came about as an intentional fix.


@property
def required_search_filters(self):
Expand Down
18 changes: 10 additions & 8 deletions arches/app/search/components/paging_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,22 @@

class PagingFilter(BaseSearchFilter):
def append_dsl(self, search_query_object, **kwargs):
export = self.request.GET.get("export", None)
mobile_download = self.request.GET.get("mobiledownload", None)
export = self.search_request.get("export", None)
mobile_download = self.search_request.get("mobiledownload", None)
page = (
1
if self.request.GET.get(self.componentname) == ""
else int(self.request.GET.get(self.componentname, 1))
if self.search_request.get(self.componentname) == ""
or isinstance(self.search_request.get(self.componentname), dict)
else int(self.search_request.get(self.componentname, 1))
)

if export is not None:
limit = settings.SEARCH_RESULT_LIMIT
elif mobile_download is not None:
limit = self.request.GET["resourcecount"]
limit = self.search_request.get("resourcecount")
else:
limit = settings.SEARCH_ITEMS_PER_PAGE
limit = int(self.request.GET.get("limit", limit))
limit = int(self.search_request.get("limit", limit))
search_query_object["query"].start = limit * int(page - 1)
search_query_object["query"].limit = limit

Expand All @@ -44,8 +45,9 @@ def post_search_hook(self, search_query_object, response_object, **kwargs):
)
page = (
1
if self.request.GET.get(self.componentname) == ""
else int(self.request.GET.get(self.componentname, 1))
if self.search_request.get(self.componentname) == ""
or isinstance(self.search_request.get(self.componentname), dict)
else int(self.search_request.get(self.componentname, 1))
)

paginator, pages = get_paginator(
Expand Down
2 changes: 1 addition & 1 deletion arches/app/search/components/sort_results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

class SortResults(BaseSearchFilter):
def append_dsl(self, search_query_object, **kwargs):
sort_param = self.request.GET.get(self.componentname, None)
sort_param = kwargs.get("querystring", None)
Copy link
Member

@apeters apeters Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be

sort_param = self.search_request.get(self.componentname, None)


if sort_param is not None and sort_param != "":
search_query_object["query"].sort(
Expand Down
24 changes: 12 additions & 12 deletions arches/app/search/components/standard_search_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,15 @@ def append_dsl(self, search_query_object, **kwargs):
search_query_object["query"].include("map_popup")
search_query_object["query"].include("provisional_resource")
search_query_object["query"].include("permissions")
load_tiles = get_str_kwarg_as_bool("tiles", self.request.GET)
load_tiles = get_str_kwarg_as_bool("tiles", self.search_request)
if load_tiles:
search_query_object["query"].include("tiles")

def execute_query(self, search_query_object, response_object, **kwargs):
for_export = get_str_kwarg_as_bool("export", self.request.GET)
pages = self.request.GET.get("pages", None)
total = int(self.request.GET.get("total", "0"))
resourceinstanceid = self.request.GET.get("id", None)
for_export = get_str_kwarg_as_bool("export", self.search_request)
pages = self.search_request.get("pages", None)
total = int(self.search_request.get("total", "0"))
resourceinstanceid = self.search_request.get("id", None)
dsl = search_query_object["query"]
if for_export or pages:
results = dsl.search(index=RESOURCES_INDEX, scroll="1m")
Expand Down Expand Up @@ -206,13 +206,10 @@ def handle_search_results_query(
se = SearchEngineFactory().create()
search_query_object = {"query": Query(se)}
response_object = {"results": None}
sorted_query_obj = search_filter_factory.create_search_query_dict(
list(self.request.GET.items()) + list(self.request.POST.items())
)
permitted_nodegroups = get_permitted_nodegroups(self.request.user)
include_provisional = get_provisional_type(self.request)
try:
for filter_type, querystring in list(sorted_query_obj.items()):
for filter_type, querystring in list(self.search_request.items()):
search_filter = search_filter_factory.get_filter(filter_type)
if search_filter:
search_filter.append_dsl(
Expand All @@ -232,14 +229,17 @@ def handle_search_results_query(
if returnDsl:
return response_object, search_query_object

for filter_type, querystring in list(sorted_query_obj.items()):
for filter_type, querystring in list(self.search_request.items()):
search_filter = search_filter_factory.get_filter(filter_type)
if search_filter:
search_filter.execute_query(search_query_object, response_object)
search_filter.execute_query(
search_query_object,
response_object,
)

if response_object["results"] is not None:
# allow filters to modify the results
for filter_type, querystring in list(sorted_query_obj.items()):
for filter_type, querystring in list(self.search_request.items()):
search_filter = search_filter_factory.get_filter(filter_type)
if search_filter:
search_filter.post_search_hook(
Expand Down
2 changes: 1 addition & 1 deletion arches/app/search/components/term_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def append_dsl(self, search_query_object, **kwargs):
include_provisional = kwargs.get("include_provisional")
search_query = Bool()
querystring_params = kwargs.get("querystring", "[]")
language = self.request.GET.get("language", "*")
language = self.search_request.get("language", "*")
for term in JSONDeserializer().deserialize(querystring_params):
if term["type"] == "term" or term["type"] == "string":
string_filter = Bool()
Expand Down
32 changes: 22 additions & 10 deletions arches/app/search/search_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,28 @@ class SearchResultsExporter(object):
def __init__(self, search_request=None):
if search_request is None:
raise Exception("Need to pass in a search request")
search_request.GET = search_request.GET.copy()
search_request.GET["tiles"] = True
search_request.GET["export"] = True
self.report_link = search_request.GET.get("reportlink", False)
self.format = search_request.GET.get("format", "tilecsv")
self.export_system_values = get_str_kwarg_as_bool(
"exportsystemvalues", search_request.GET
)
self.compact = search_request.GET.get("compact", True)
self.precision = int(search_request.GET.get("precision", 5))
if search_request.method == "GET":
search_request.GET = search_request.GET.copy()
search_request.GET["tiles"] = True
search_request.GET["export"] = True
self.report_link = search_request.GET.get("reportlink", False)
self.format = search_request.GET.get("format", "tilecsv")
self.export_system_values = get_str_kwarg_as_bool(
"exportsystemvalues", search_request.GET
)
self.compact = search_request.GET.get("compact", True)
self.precision = int(search_request.GET.get("precision", 5))
else:
search_request.POST = search_request.POST.copy()
search_request.POST["tiles"] = True
search_request.POST["export"] = True
self.report_link = search_request.POST.get("reportlink", False)
self.format = search_request.POST.get("format", "tilecsv")
self.export_system_values = get_str_kwarg_as_bool(
"exportsystemvalues", search_request.POST
)
self.compact = search_request.POST.get("compact", True)
self.precision = int(search_request.POST.get("precision", 5))
Comment on lines +49 to +70
Copy link
Member

@apeters apeters Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if/else could be boiled down to a single chunk of code using something like this:

        the_request = getattr(search_request, method).copy()
        the_request["tiles"] = True
        the_request["export"] = True
        self.report_link = the_request.get("reportlink", False)
        self.format = the_request.get("format", "tilecsv")
        self.export_system_values = get_str_kwarg_as_bool(
            "exportsystemvalues", the_request
        )
        self.compact = the_request.get("compact", True)
        self.precision = int(the_request.get("precision", 5))
        setattr(search_request, method, the_request)

if self.format == "shp" and self.compact is not True:
raise Exception("Results must be compact to export to shapefile")
self.search_request = search_request
Expand Down
2 changes: 2 additions & 0 deletions arches/app/views/search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from django.http import Http404
from django.shortcuts import render
from django.utils.translation import gettext as _
from django.views.decorators.csrf import csrf_exempt
from arches.app.models.models import (
MapMarker,
GraphModel,
Expand Down Expand Up @@ -349,6 +350,7 @@ def get_dsl_from_search_string(request):
return JSONResponse(dsl)


@csrf_exempt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the need for this?

def search_results(request, returnDsl=False):
search_filter_factory = SearchFilterFactory(request)
searchview_component_instance = search_filter_factory.get_searchview_instance()
Expand Down
Loading