Remove search_data and only use search_document so all submission search works the same

hypha/apply/funds/management/commands/rebuild_search_index.py

@@ -0,0 +1,97 @@
+from django.core.management.base import BaseCommand
+from django.db import DatabaseError, transaction
+
+from hypha.apply.funds.models.submissions import ApplicationSubmission
+
+CHUNK_SIZE = 500
+
+
+class Command(BaseCommand):
+    help = "Rebuild the full text search index (search_document) for all submissions."
+
+    def handle(self, *args, **options):
+        # Only load what prepare_search_vector() needs. This also keeps the
+        # command safe to run from migration 0140, before later schema changes.
+        self.submissions = ApplicationSubmission.objects.only(
+            "id", "public_id", "form_data", "form_fields"
+        )
+        pks = list(
+            ApplicationSubmission.objects.order_by("pk").values_list("pk", flat=True)
+        )
+        total = len(pks)
+        self.failed = []
+        indexed = 0
+
+        for start in range(0, total, CHUNK_SIZE):
+            chunk = []
+            for submission in self.load_chunk(pks[start : start + CHUNK_SIZE]):
+                try:
+                    submission.search_document = submission.prepare_search_vector()
+                except Exception as e:
+                    self.report_failure(submission.pk, e)
+                else:
+                    chunk.append(submission)
+
+            indexed += self.save_chunk(chunk)
+            processed = min(start + CHUNK_SIZE, total)
+            self.stdout.write(f"Processed {processed}/{total} submissions")
+
+        self.stdout.write(
+            self.style.SUCCESS(f"Rebuilt search index for {indexed} submissions.")
+        )
+        if self.failed:
+            pks = ", ".join(str(pk) for pk in self.failed)
+            self.stderr.write(
+                self.style.ERROR(
+                    f"Could not index {len(self.failed)} of {total} submissions: {pks}"
+                )
+            )
+
+    def load_chunk(self, pks):
+        """
+        Load the whole chunk in one query. Form data is deserialized while
+        loading, if that fails (e.g. a block type that no longer exists) load
+        row by row to find the culprits.
+        """
+        try:
+            return list(self.submissions.filter(pk__in=pks))
+        except Exception:
+            pass
+
+        loaded = []
+        for pk in pks:
+            try:
+                loaded.extend(self.submissions.filter(pk=pk))
+            except Exception as e:
+                self.report_failure(pk, e)
+        return loaded
+
+    def save_chunk(self, chunk):
+        """
+        Save the whole chunk in one query. If the database rejects it (e.g. a
+        tsvector over the size limit) save row by row to find the culprits.
+        """
+        try:
+            with transaction.atomic():
+                ApplicationSubmission.objects.bulk_update(chunk, ["search_document"])
+            return len(chunk)
+        except DatabaseError:
+            pass
+
+        saved = 0
+        for submission in chunk:
+            try:
+                with transaction.atomic():
+                    ApplicationSubmission.objects.filter(pk=submission.pk).update(
+                        search_document=submission.search_document
+                    )
+                saved += 1
+            except DatabaseError as e:
+                self.report_failure(submission.pk, e)
+        return saved
+
+    def report_failure(self, pk, error):
+        self.failed.append(pk)
+        self.stderr.write(
+            self.style.WARNING(f"Could not index submission {pk}: {error}")
+        )

hypha/apply/funds/migrations/0140_remove_applicationsubmission_search_data.py

@@ -0,0 +1,44 @@
+from django.core.management import call_command
+from django.db import migrations, models
+
+
+def rebuild_search_index(apps, schema_editor):
+    # Nothing to index on a new install, and that way only sites upgrading
+    # through this migration depend on the current model code.
+    if not apps.get_model("funds", "ApplicationSubmission").objects.exists():
+        return
+
+    # Submissions from before search_document was added (0111) were never
+    # indexed, and older indexed ones may have "id:None" instead of their
+    # public id. Rebuild the full weighted vector for every submission before
+    # dropping search_data. Uses the real model, but the command only loads
+    # columns that exist at this point. Submissions that can't be indexed
+    # (e.g. over the tsvector size limit) are reported and skipped.
+    call_command("rebuild_search_index")
+
+
+class Migration(migrations.Migration):
+    # Let the rebuild commit chunk by chunk instead of holding row locks on
+    # every submission in one long transaction.
+    atomic = False
+
+    dependencies = [
+        # Loading form fields queries the categories tables.
+        ("categories", "0009_alter_option_options"),
+        ("funds", "0139_alter_applicationbasedeterminationform_options_and_more"),
+    ]
+
+    operations = [
+        migrations.RunPython(rebuild_search_index, migrations.RunPython.noop),
+        # Give the field a default so that reversing the RemoveField can re-add
+        # the NOT NULL column to a table with existing rows.
+        migrations.AlterField(
+            model_name="applicationsubmission",
+            name="search_data",
+            field=models.TextField(default=""),
+        ),
+        migrations.RemoveField(
+            model_name="applicationsubmission",
+            name="search_data",
+        ),
+    ]

hypha/apply/funds/migrations/0141_applicationsubmission_form_data_gin.py

@@ -0,0 +1,23 @@
+import django.contrib.postgres.indexes
+from django.contrib.postgres.operations import AddIndexConcurrently
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    # Concurrent index creation can't run inside a transaction.
+    atomic = False
+
+    dependencies = [
+        ("funds", "0140_remove_applicationsubmission_search_data"),
+    ]
+
+    operations = [
+        AddIndexConcurrently(
+            model_name="applicationsubmission",
+            index=django.contrib.postgres.indexes.GinIndex(
+                fields=["form_data"],
+                name="funds_appsub_form_data_gin",
+                opclasses=["jsonb_path_ops"],
+            ),
+        ),
+    ]

hypha/apply/funds/models/submissions.py

@@ -8,13 +8,19 @@
 from django.contrib.auth.models import AbstractBaseUser, AnonymousUser, Group
 from django.contrib.contenttypes.fields import GenericRelation
 from django.contrib.postgres.indexes import GinIndex
-from django.contrib.postgres.search import SearchVector, SearchVectorField
+from django.contrib.postgres.search import (
+    SearchQuery,
+    SearchRank,
+    SearchVector,
+    SearchVectorField,
+)
 from django.core.exceptions import PermissionDenied
-from django.db import models
+from django.db import models, transaction
 from django.db.models import (
     Avg,
     Count,
     Exists,
+    F,
     FloatField,
     OuterRef,
     Q,
@@ -118,6 +124,12 @@ class ApplicationSubmissionQueryset(JSONOrderable):
     def active(self):
         return self.filter(status__in=active_statuses, is_archive=False)
 
+    def search(self, term):
+        query = SearchQuery(term, search_type="websearch")
+        return self.filter(search_document=query).annotate(
+            rank=SearchRank(F("search_document"), query)
+        )
+
     def inactive(self):
         return self.exclude(Q(status__in=active_statuses) | Q(is_archive=True))
 
@@ -258,12 +270,10 @@ def for_table(self, user):
             )
             .prefetch_related("screening_statuses")
             .defer(
-                "search_data",
                 "search_document",
                 # Only used to check if a previous stage exists, skip the large fields.
                 "previous__form_data",
                 "previous__form_fields",
-                "previous__search_data",
                 "previous__search_document",
             )
         )
@@ -504,7 +514,6 @@ class ApplicationSubmission(
     user = models.ForeignKey(
         settings.AUTH_USER_MODEL, on_delete=models.CASCADE, null=True
     )
-    search_data = models.TextField()
     search_document = SearchVectorField(null=True)
 
     # Workflow inherited from WorkflowHelpers
@@ -552,6 +561,11 @@ class ApplicationSubmission(
     class Meta:
         indexes = [
             GinIndex(fields=["search_document"]),
+            GinIndex(
+                fields=["form_data"],
+                name="funds_appsub_form_data_gin",
+                opclasses=["jsonb_path_ops"],
+            ),
         ]
         verbose_name = _("application submission")
         verbose_name_plural = _("application submissions")
@@ -666,7 +680,7 @@ def application_id(self):
     @property
     def application_id_nc(self):
         """
-        Only used in e-mail and other notifications where a cached_property gives a None value.
+        Only used in search, e-mail and other notifications where a cached_property gives a None value.
         """
         return self.public_id or self.id
 
@@ -775,7 +789,6 @@ def create_revision(
                     revision.is_draft = False
                     revision.save()
                 self.live_revision = revision
-                self.search_data = " ".join(list(self.prepare_search_values()))
                 self.search_document = self.prepare_search_vector()
 
             self.draft_revision = revision
@@ -791,7 +804,6 @@ def create_revision(
                 revision.is_draft = False
                 revision.save()
                 self.live_revision = revision
-                self.search_data = " ".join(list(self.prepare_search_values()))
                 self.search_document = self.prepare_search_vector()
                 self.save(skip_custom=True)
 
@@ -840,21 +852,23 @@ def save(self, *args, update_fields=None, skip_custom=False, **kwargs):
 
             # We need the submission id to correctly save the files
             files = self.extract_files()
+        elif not self.public_id:
+            self.public_id = f"{self.get_from_parent('submission_id_prefix')}{self.id}"
 
         self.clean_submission()
 
-        # add a denormed version of the answer for searching
-        # @TODO: remove 'search_data' in favour of 'search_document' for FTS
-        self.search_data = " ".join(self.prepare_search_values())
-        self.search_document = self.prepare_search_vector()
-
-        super().save(*args, **kwargs)
-
-        if self.id and not self.public_id:
-            self.public_id = f"{self.get_from_parent('submission_id_prefix')}{self.id}"
+        if not creating:
+            # add a denormed version of the answer for searching
+            self.search_document = self.prepare_search_vector()
+            return super().save(*args, **kwargs)
 
+        # New submissions get their public_id and search index from the save at
+        # the end, once they have an id. Run it all in one transaction so a
+        # failure can't leave a half created, unsearchable submission behind.
         # TODO: This functionality should be extracted and moved to a separate function, too hidden here
-        if creating:
+        with transaction.atomic():
+            super().save(*args, **kwargs)
+
             AssignedReviewers = apps.get_model("funds", "AssignedReviewers")
             ApplicationRevision = apps.get_model("funds", "ApplicationRevision")
 
@@ -956,19 +970,10 @@ def get_searchable_contents(self):
                     contents.append(value)
         return contents
 
-    def prepare_search_values(self):
-        values = self.get_searchable_contents()
-
-        # Add named fields into the search index
-        for field in ["full_name", "email", "title", "public_id"]:
-            if value := getattr(self, field):
-                values.append(value)
-        return values
-
     def index_components(self):
         return {
-            "A": " ".join([f"id:{self.application_id}", self.title]),
-            "C": " ".join([self.full_name, self.email]),
+            "A": " ".join(filter(None, [f"id:{self.application_id_nc}", self.title])),
+            "C": " ".join(filter(None, [self.full_name, self.email])),
             "B": " ".join(self.get_searchable_contents()),
         }
 

hypha/apply/funds/tables.py

@@ -313,7 +313,7 @@ def search_data_and_id(self, queryset, name, value):
         possible_id = re.search("^#(\\d+)$", value.strip())
         if possible_id:
             return queryset.filter(id=possible_id.groups()[0])
-        return queryset.filter(search_data__icontains=value)
+        return queryset.search(value)
 
     def filter_archived(self, queryset, name, value):
         if not value:

hypha/apply/funds/templates/funds/includes/table_filter_and_search.html

@@ -36,15 +36,15 @@ <h2 class="section-header">{{ heading }}</h2>
                 role="search"
                 class="relative grow"
             >
-                <label class="flex gap-2 items-center w-auto input input-border">
-                    {% heroicon_mini "magnifying-glass" size=20 class="opacity-50 h-[1em]" aria_hidden=true %}
+                <label class="flex gap-2 items-center w-full input input-border">
+                    {% heroicon_mini "magnifying-glass" size=20 class="text-fg-muted" aria_hidden=true %}
                     <input
                         type="search"
-                        placeholder="{% trans 'Search...' %}"
+                        class=""
+                        placeholder="{% trans 'Search…' %}"
                         name="query"
-                        {% if search_term %}value="{{ request.GET.query }}"{% endif %}
                         aria-label="{% trans 'Search input' %}"
-                        class="sm:w-auto w-[15ch]"
+                        {% if search_term %}value="{{ request.GET.query }}" autofocus{% endif %}
                     >
                 </label>
             </form>

hypha/apply/funds/templates/submissions/all.html

@@ -131,14 +131,12 @@
                 {% endif %}
             {% endfor %}
 
-            <label class="inline-flex relative flex-auto items-center">
-                <span class="flex absolute inset-y-0 items-center pointer-events-none start-0 ps-3">
-                    {% heroicon_mini "magnifying-glass" size=20 class="text-fg-muted" aria_hidden=true %}
-                </span>
+            <label class="flex gap-2 items-center w-full input input-border">
+                {% heroicon_mini "magnifying-glass" size=20 class="text-fg-muted" aria_hidden=true %}
                 <input
                     type="text"
                     id="search-navbar"
-                    class="w-full input"
+                    class=""
                     placeholder="{% trans 'Search…' %}"
                     name="query"
                     aria-label="{% trans 'Search submissions' %}"