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
+72 −41
Open

11578 make search-filters request.method agnostic #11582

Show file tree
Hide file tree
Changes from 5 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
20 changes: 12 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,25 +17,28 @@

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)
search_request_object = kwargs.get("search_request_object", self.request.GET)
export = search_request_object.get("export", None)
mobile_download = search_request_object.get("mobiledownload", None)
page = (
1
if self.request.GET.get(self.componentname) == ""
else int(self.request.GET.get(self.componentname, 1))
if search_request_object.get(self.componentname) == ""
or isinstance(search_request_object.get(self.componentname), dict)
else int(search_request_object.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 = search_request_object.get("resourcecount")
else:
limit = settings.SEARCH_ITEMS_PER_PAGE
limit = int(self.request.GET.get("limit", limit))
limit = int(search_request_object.get("limit", limit))
search_query_object["query"].start = limit * int(page - 1)
search_query_object["query"].limit = limit

def post_search_hook(self, search_query_object, response_object, **kwargs):
search_request_object = kwargs.get("search_request_object", self.request.GET)
total = (
response_object["results"]["hits"]["total"]["value"]
if response_object["results"]["hits"]["total"]["value"]
Expand All @@ -44,8 +47,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 search_request_object.get(self.componentname) == ""
or isinstance(search_request_object.get(self.componentname), dict)
else int(search_request_object.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
20 changes: 14 additions & 6 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 @@ -122,6 +122,7 @@
class StandardSearchView(BaseSearchView):

def append_dsl(self, search_query_object, **kwargs):
search_request_object = kwargs.get("search_request_object", self.request.GET)
search_query_object["query"].include("graph_id")
search_query_object["query"].include("root_ontology_class")
search_query_object["query"].include("resourceinstanceid")
Expand All @@ -132,15 +133,16 @@ 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", search_request_object)
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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))

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)
search_request_object = kwargs.get("search_request_object", self.request.GET)
for_export = get_str_kwarg_as_bool("export", search_request_object)
Copy link
Member

Choose a reason for hiding this comment

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

same as above, should default to False

Copy link
Member Author

Choose a reason for hiding this comment

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

see comment above

pages = search_request_object.get("pages", None)
total = int(search_request_object.get("total", "0"))
resourceinstanceid = search_request_object.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 @@ -220,6 +222,7 @@ def handle_search_results_query(
permitted_nodegroups=permitted_nodegroups,
include_provisional=include_provisional,
querystring=querystring,
search_request_object=sorted_query_obj,
)
append_instance_permission_filter_dsl(self.request, search_query_object)
except Exception as err:
Expand All @@ -235,7 +238,11 @@ def handle_search_results_query(
for filter_type, querystring in list(sorted_query_obj.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,
search_request_object=sorted_query_obj,
)

if response_object["results"] is not None:
# allow filters to modify the results
Expand All @@ -246,6 +253,7 @@ def handle_search_results_query(
search_query_object,
response_object,
permitted_nodegroups=permitted_nodegroups,
search_request_object=sorted_query_obj,
)

search_query_object.pop("query")
Expand Down
3 changes: 2 additions & 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,8 @@ 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", "*")
search_request_object = kwargs.get("search_request_object", self.request.GET)
language = search_request_object.get("language", "*")
for term in JSONDeserializer().deserialize(querystring_params):
if term["type"] == "term" or term["type"] == "string":
string_filter = Bool()
Expand Down