Skip to content

Commit

Permalink
fix: revert POST for document search requests (#8263)
Browse files Browse the repository at this point in the history 
* Revert "fix: ensure csrf cookie for searches (#8260)"

This reverts commit 622ded5.

* Revert "refactor: eliminate single-use helper (#8226)"

This reverts commit 6608c9d.

* Revert "feat: POST for document search requests (#8206)"

This reverts commit b65a37b.

* test: add back test fix

* refactor: eliminate single-use helper
  • Loading branch information
@jennifer-richards
jennifer-richards authored Nov 22, 2024
1 parent ec02890 commit 9681d9f
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 196 deletions.
1 change: 0 additions & 1 deletion ietf/api/serializer.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ def end_object(self, obj):
field_value = None
else:
field_value = field
# Need QuerySetAny instead of QuerySet until django-stubs 5.0.1
if isinstance(field_value, QuerySetAny) or isinstance(field_value, list):
self._current[name] = dict([ (rel.pk, self.expand_related(rel, name)) for rel in field_value ])
else:
Expand Down
155 changes: 44 additions & 111 deletions ietf/doc/tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,163 +73,96 @@


class SearchTests(TestCase):
def test_search_handles_querystring_parameters(self):
"""Search parameters via querystring should not actually search"""
url = urlreverse("ietf.doc.views_search.search")
r = self.client.get(url + "?name=some-document-name&oldDrafts=on")
# Check that we got a valid response and that the warning about query string parameters is shown.
self.assertContains(
r,
"Searching via the URL query string is no longer supported.",
status_code=200,
)
# Check that the form was filled in correctly (not an exhaustive check, but different from the
# form defaults)
pq = PyQuery(r.content)
self.assertEqual(
pq("form#search_form input#id_name").attr("value"),
"some-document-name",
"The name field should be set in the SearchForm",
)
self.assertEqual(
pq("form#search_form input#id_olddrafts").attr("checked"),
"checked",
"The old drafts checkbox should be selected in the SearchForm",
)
self.assertIsNone(
pq("form#search_form input#id_rfcs").attr("checked"),
"The RFCs checkbox should not be selected in the SearchForm",
)
self.assertIsNone(
pq("form#search_form input#id_activedrafts").attr("checked"),
"The active drafts checkbox should not be selected in the SearchForm",
)

def test_search(self):
draft = WgDraftFactory(
name="draft-ietf-mars-test",
group=GroupFactory(acronym="mars", parent=Group.objects.get(acronym="farfut")),
authors=[PersonFactory()],
ad=PersonFactory(),
)

draft = WgDraftFactory(name='draft-ietf-mars-test',group=GroupFactory(acronym='mars',parent=Group.objects.get(acronym='farfut')),authors=[PersonFactory()],ad=PersonFactory())
rfc = WgRfcFactory()
draft.set_state(State.objects.get(used=True, type="draft-iesg", slug="pub-req"))
old_draft = IndividualDraftFactory(
name="draft-foo-mars-test",
authors=[PersonFactory()],
title="Optimizing Martian Network Topologies",
)
old_draft = IndividualDraftFactory(name='draft-foo-mars-test',authors=[PersonFactory()],title="Optimizing Martian Network Topologies")
old_draft.set_state(State.objects.get(used=True, type="draft", slug="expired"))
url = urlreverse("ietf.doc.views_search.search")

base_url = urlreverse('ietf.doc.views_search.search')

# only show form, no search yet
r = self.client.get(url)
r = self.client.get(base_url)
self.assertEqual(r.status_code, 200)

# no match
r = self.client.post(url, {"activedrafts": "on", "name": "thisisnotadocumentname"})
r = self.client.get(base_url + "?activedrafts=on&name=thisisnotadocumentname")
self.assertEqual(r.status_code, 200)
self.assertContains(r, "No documents match")
r = self.client.post(url, {"rfcs": "on", "name": "xyzzy"})

r = self.client.get(base_url + "?rfcs=on&name=xyzzy")
self.assertEqual(r.status_code, 200)
self.assertContains(r, "No documents match")
r = self.client.post(url, {"olddrafts": "on", "name": "bar"})

r = self.client.get(base_url + "?olddrafts=on&name=bar")
self.assertEqual(r.status_code, 200)
self.assertContains(r, "No documents match")
r = self.client.post(url, {"olddrafts": "on", "name": "foo"})

r = self.client.get(base_url + "?olddrafts=on&name=foo")
self.assertEqual(r.status_code, 200)
self.assertContains(r, "draft-foo-mars-test")
r = self.client.post(url, {"olddrafts": "on", "name": "FoO"}) # mixed case

r = self.client.get(base_url + "?olddrafts=on&name=FoO") # mixed case
self.assertEqual(r.status_code, 200)
self.assertContains(r, "draft-foo-mars-test")

# find by RFC
r = self.client.post(url, {"rfcs": "on", "name": rfc.name})
r = self.client.get(base_url + "?rfcs=on&name=%s" % rfc.name)
self.assertEqual(r.status_code, 200)
self.assertContains(r, rfc.title)

# find by active/inactive

draft.set_state(State.objects.get(type="draft", slug="active"))
r = self.client.post(url, {"activedrafts": "on", "name": draft.name})
r = self.client.get(base_url + "?activedrafts=on&name=%s" % draft.name)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

draft.set_state(State.objects.get(type="draft", slug="expired"))
r = self.client.post(url, {"olddrafts": "on", "name": draft.name})
r = self.client.get(base_url + "?olddrafts=on&name=%s" % draft.name)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

draft.set_state(State.objects.get(type="draft", slug="active"))

# find by title
r = self.client.post(url, {"activedrafts": "on", "name": draft.title.split()[0]})
r = self.client.get(base_url + "?activedrafts=on&name=%s" % draft.title.split()[0])
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by author
r = self.client.post(
url,
{
"activedrafts": "on",
"by": "author",
"author": draft.documentauthor_set.first().person.name_parts()[1],
},
)
r = self.client.get(base_url + "?activedrafts=on&by=author&author=%s" % draft.documentauthor_set.first().person.name_parts()[1])
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by group
r = self.client.post(
url,
{"activedrafts": "on", "by": "group", "group": draft.group.acronym},
)
r = self.client.get(base_url + "?activedrafts=on&by=group&group=%s" % draft.group.acronym)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

r = self.client.post(
url,
{"activedrafts": "on", "by": "group", "group": draft.group.acronym.swapcase()},
)

r = self.client.get(base_url + "?activedrafts=on&by=group&group=%s" % draft.group.acronym.swapcase())
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by area
r = self.client.post(
url,
{"activedrafts": "on", "by": "area", "area": draft.group.parent_id},
)
r = self.client.get(base_url + "?activedrafts=on&by=area&area=%s" % draft.group.parent_id)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by area
r = self.client.post(
url,
{"activedrafts": "on", "by": "area", "area": draft.group.parent_id},
)
r = self.client.get(base_url + "?activedrafts=on&by=area&area=%s" % draft.group.parent_id)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by AD
r = self.client.post(url, {"activedrafts": "on", "by": "ad", "ad": draft.ad_id})
r = self.client.get(base_url + "?activedrafts=on&by=ad&ad=%s" % draft.ad_id)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

# find by IESG state
r = self.client.post(
url,
{
"activedrafts": "on",
"by": "state",
"state": draft.get_state("draft-iesg").pk,
"substate": "",
},
)
r = self.client.get(base_url + "?activedrafts=on&by=state&state=%s&substate=" % draft.get_state("draft-iesg").pk)
self.assertEqual(r.status_code, 200)
self.assertContains(r, draft.title)

Expand All @@ -238,15 +171,15 @@ def test_search_became_rfc(self):
rfc = WgRfcFactory()
draft.set_state(State.objects.get(type="draft", slug="rfc"))
draft.relateddocument_set.create(relationship_id="became_rfc", target=rfc)
url = urlreverse("ietf.doc.views_search.search")
base_url = urlreverse('ietf.doc.views_search.search')

# find by RFC
r = self.client.post(url, {"rfcs": "on", "name": rfc.name})
r = self.client.get(base_url + f"?rfcs=on&name={rfc.name}")
self.assertEqual(r.status_code, 200)
self.assertContains(r, rfc.title)

# find by draft
r = self.client.post(url, {"activedrafts": "on", "rfcs": "on", "name": draft.name})
r = self.client.get(base_url + f"?activedrafts=on&rfcs=on&name={draft.name}")
self.assertEqual(r.status_code, 200)
self.assertContains(r, rfc.title)

Expand Down
120 changes: 40 additions & 80 deletions ietf/doc/views_search.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,31 +33,28 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


import hashlib
import json
import re
import datetime
import copy
import hashlib
import json
import operator

from collections import defaultdict
from functools import reduce

from django import forms
from django.conf import settings
from django.contrib import messages
from django.core.cache import cache, caches
from django.urls import reverse as urlreverse
from django.db.models import Q
from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect
from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect, QueryDict
from django.shortcuts import render
from django.utils import timezone
from django.utils.html import strip_tags
from django.utils.cache import _generate_cache_key # type: ignore
from django.utils.text import slugify
from django.views.decorators.csrf import ensure_csrf_cookie
from django_stubs_ext import QuerySetAny


import debug # pyflakes:ignore

Expand Down Expand Up @@ -149,29 +146,6 @@ def clean(self):
q['irtfstate'] = None
return q

def cache_key_fragment(self):
"""Hash a bound form to get a value for use in a cache key
Raises a ValueError if the form is not valid.
"""
def _serialize_value(val):
# Need QuerySetAny instead of QuerySet until django-stubs 5.0.1
if isinstance(val, QuerySetAny):
return [item.pk for item in val]
else:
return getattr(val, "pk", val) # use pk if present, else value

if not self.is_valid():
raise ValueError(f"SearchForm invalid: {self.errors}")
contents = {
field_name: _serialize_value(field_value)
for field_name, field_value in self.cleaned_data.items()
if field_name != "sort" and field_value is not None
}
contents_json = json.dumps(contents, sort_keys=True)
return hashlib.sha512(contents_json.encode("utf-8")).hexdigest()


def retrieve_search_results(form, all_types=False):
"""Takes a validated SearchForm and return the results."""

Expand Down Expand Up @@ -284,65 +258,51 @@ def retrieve_search_results(form, all_types=False):
return docs


@ensure_csrf_cookie
def search(request):
"""Search for a draft"""
# defaults for results / meta
results = []
meta = {"by": None, "searching": False}

if request.method == "POST":
form = SearchForm(data=request.POST)
if form.is_valid():
cache_key = f"doc:document:search:{form.cache_key_fragment()}"
cached_val = cache.get(cache_key)
if cached_val:
[results, meta] = cached_val
else:
results = retrieve_search_results(form)
results, meta = prepare_document_table(
request, results, form.cleaned_data
)
cache.set(
cache_key, [results, meta]
) # for settings.CACHE_MIDDLEWARE_SECONDS
log(f"Search results computed for {form.cleaned_data}")
meta["searching"] = True
else:
if request.GET:
# backwards compatibility - fill in the form
get_params = request.GET.copy()
if "activeDrafts" in request.GET:
get_params["activedrafts"] = request.GET["activeDrafts"]
if "oldDrafts" in request.GET:
get_params["olddrafts"] = request.GET["oldDrafts"]
if "subState" in request.GET:
get_params["substate"] = request.GET["subState"]
form = SearchForm(data=get_params)
messages.error(
request,
(
"Searching via the URL query string is no longer supported. "
"The form below has been filled in with the parameters from your request. "
'To execute your search, please click "Search".'
),
)
def _get_cache_key(params):
fields = set(SearchForm.base_fields) - {'sort'}
kwargs = dict([(k, v) for (k, v) in list(params.items()) if k in fields])
key = "doc:document:search:" + hashlib.sha512(json.dumps(kwargs, sort_keys=True).encode('utf-8')).hexdigest()
return key

if request.GET:
# backwards compatibility
get_params = request.GET.copy()
if 'activeDrafts' in request.GET:
get_params['activedrafts'] = request.GET['activeDrafts']
if 'oldDrafts' in request.GET:
get_params['olddrafts'] = request.GET['oldDrafts']
if 'subState' in request.GET:
get_params['substate'] = request.GET['subState']

form = SearchForm(get_params)
if not form.is_valid():
return HttpResponseBadRequest("form not valid: %s" % form.errors)

cache_key = _get_cache_key(get_params)
cached_val = cache.get(cache_key)
if cached_val:
[results, meta] = cached_val
else:
form = SearchForm()
results = retrieve_search_results(form)
results, meta = prepare_document_table(request, results, get_params)
cache.set(cache_key, [results, meta]) # for settings.CACHE_MIDDLEWARE_SECONDS
log(f"Search results computed for {get_params}")
meta['searching'] = True
else:
form = SearchForm()
results = []
meta = { 'by': None, 'searching': False }
get_params = QueryDict('')

return render(
request,
"doc/search/search.html",
context={"form": form, "docs": results, "meta": meta},
return render(request, 'doc/search/search.html', {
'form':form, 'docs':results, 'meta':meta, 'queryargs':get_params.urlencode() },
)


@ensure_csrf_cookie
def frontpage(request):
form = SearchForm()
return render(request, 'doc/frontpage.html', {'form':form})


def search_for_name(request, name):
def find_unique(n):
exact = Document.objects.filter(name__iexact=n).first()
Expand Down
Loading

0 comments on commit 9681d9f

Please sign in to comment.