Skip to content

fix: make the Typesense search backend usable - #289

Open
blarghmatey wants to merge 1 commit into
openedx:masterfrom
mitodl:tmacey/fix-typesense-search-params
Open

fix: make the Typesense search backend usable#289
blarghmatey wants to merge 1 commit into
openedx:masterfrom
mitodl:tmacey/fix-typesense-search-params

Conversation

@blarghmatey

Copy link
Copy Markdown
Contributor

Description

Every forum search on the Typesense backend fails against a real Typesense server. There are two independent defects, both present since the backend shipped in #225, and either one alone is enough to break search.

1. per_page=1000 exceeds Typesense's hard cap of 250. build_search_parameters() passes per_page=FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT (1000) on every search. Typesense caps per_page at 250 and rejects anything higher with HTTP 422, so this fires on every search, filtered or not.

The constant is shared with the other two backends, which is how it slipped through. Elasticsearch passes it as size (es.py:670) under index.max_result_window, and Meilisearch as limit (meilisearch.py:198) under maxTotalHits. Neither is set anywhere in this repo, so both take their product default — 10000 and 1000 respectively, read back from the e2e containers to check. Only Typesense caps low enough to break, and Meilisearch only just clears it.

Rather than quietly lower the deep-search budget for this backend alone, get_thread_ids() now walks pages of 250 until the budget is covered or a short page says the results are exhausted. Searches returning under 250 hits — nearly all of them — still cost exactly one request.

2. The topic filter names a field the collection does not have. collection_schema() declares commentable_id; build_search_parameters() filtered on commentable_ids, so any search scoped to discussion topics failed with HTTP 400 Could not find a filter field named 'commentable_ids' in the schema. Corrected to the singular name the collection actually declares. No reindex needed — document_from_thread() always wrote the field under the right name, only the filter was wrong.

Testing

Measured against Typesense 30.2, replicating build_search_parameters() against a collection built from collection_schema() and holding 600 threads:

search parameters result
per_page=1000 + commentable_ids (as shipped) 400 Could not find a filter field
per_page=250 + commentable_ids (bug 2 alone) 400 Could not find a filter field
per_page=1000 + commentable_id (bug 1 alone) 422 Only upto 250 hits can be fetched per page
per_page=250 + commentable_id, pages 1–3 200, found=600, 250 + 250 + 100 hits
get_thread_ids() after this change 600 of 600 threads returned

Neither defect was reachable from the existing tests, which mock the Typesense client and so cannot observe a rejection from the server — the unit test asserting commentable_ids and per_page=1000 passed happily against both bugs. That gap is the reason this shipped, so this PR closes it: tests/e2e/ gains a Typesense service in its compose file and a test_search_typesense.py covering both defects against a real server. Three of the four new tests fail on master with exactly the errors above (the fourth only builds and validates a collection, so it passes either way):

tests/e2e/test_search_typesense.py::test_insert_document
E   typesense.exceptions.ObjectUnprocessable: [Errno 422] Only upto 250 hits can be fetched per page.
tests/e2e/test_search_typesense.py::test_search_filtered_by_commentable_id
E   typesense.exceptions.RequestMalformed: [Errno 400] Could not find a filter field named `commentable_ids` in the schema.
tests/e2e/test_search_typesense.py::test_deep_search_past_the_per_page_limit
E   typesense.exceptions.RequestMalformed: [Errno 400] Could not find a filter field named `commentable_ids` in the schema.
3 failed, 1 passed

(That run used master's typesense.py with only TYPESENSE_MAX_PER_PAGE = 250 appended, since the new tests reference that constant. It adds a name, not behaviour.)

test_insert_document is worth a look: it is an ordinary unfiltered search, and it fails too. Forum search on this backend is non-functional as shipped, not merely degraded for topic-scoped queries.

With the fix: pytest tests 200 passed, pytest tests/e2e 42 passed, and mypy, pylint, pycodestyle and pydocstyle are clean over src/forum tests test_utils manage.py.

TYPESENSE_URLS in the test settings pointed at 0.0.0.0:8108, where nothing runs. It now points at the new e2e container, mapped to 5108 to match the port-shifting convention Elasticsearch (5200) and Meilisearch (5700) already follow there.

Scope

Out of scope, unchanged, and already noted in the backend's own comments: get_suggested_text() still returns None, so there is no "did you mean" spelling correction here, which the Elasticsearch backend does provide via phrase suggesters (es.py:545); group_ids and sort_criteria also remain unsupported. Anyone moving an environment onto this backend should weigh the loss of spelling correction separately.

I could not find an existing issue or PR covering either defect: no issue matches typesense, per_page or commentable_ids in any state, and the only PRs to have touched search/typesense.py are #225 itself plus #268 and #283, which dropped Python 3.11 and moved the repo to uv and a src layout. Neither buggy line has changed since #225 introduced it.

Merge checklist:
Check off if complete or not applicable:

  • Version bumped — N/A, semantic-release derives it from the fix: commit
  • Changelog record added — N/A, same
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Every forum search on the Typesense backend fails against a real server.
Two independent defects, both present since the backend shipped in openedx#225:

`build_search_parameters()` passes `per_page=FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT`
(1000). Typesense caps `per_page` at 250 and rejects anything higher with
HTTP 422, so this fires on every search, filtered or not. The constant is
shared with the Elasticsearch backend, which passes it as `size` under a
`max_result_window` of 10000, and with Meilisearch, which passes it as `limit`
under a `maxTotalHits` of 1000 -- only Typesense caps low enough to break.
Rather than lower the deep-search budget for this backend alone, walk pages of
250 until the budget is covered or a short page says the results are exhausted.
Searches returning fewer than 250 hits, which is nearly all of them, still cost
one request.

`collection_schema()` declares `commentable_id`; `build_search_parameters()`
filtered on `commentable_ids`, so scoping a search to discussion topics failed
with HTTP 400 "Could not find a filter field". Corrected to the singular field
name the collection actually declares -- no reindex needed.

Neither defect was reachable from the existing tests, which mock the Typesense
client and so cannot observe a rejection from the server. Add a Typesense
service to the e2e compose file and cover both against it; three of the four
new tests fail on the current code with the errors above.

`TYPESENSE_URLS` in the test settings pointed at 0.0.0.0:8108, where nothing
runs; it now points at the e2e container, mapped to 5108 to match the ports
Elasticsearch and Meilisearch already use there.
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @blarghmatey!

This repository is currently maintained by @openedx/wg-maintainers-forums.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@blarghmatey

Copy link
Copy Markdown
Contributor Author

@openedx/wg-maintainers-forums — this is ready for engineering review whenever someone has time. All checks are green, including the new e2e job.

Short version of why it might be worth prioritising: the Typesense search backend is currently non-functional for every search, not just some. Two independent defects, both present since the backend shipped in #225:

  • per_page is set to FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT (1000), and Typesense rejects anything above 250 with HTTP 422. This fires on every search, filtered or not.
  • The topic filter uses commentable_ids, but the collection schema declares commentable_id, so any topic-scoped search fails with HTTP 400.

Neither was reachable from the existing tests, which mock the Typesense client and so cannot observe a rejection from the server — the unit test asserting the broken parameters passed happily. That is why the PR adds a Typesense service to the e2e compose file and covers both defects against a real server; three of the four new tests fail on master with exactly those errors.

The change is confined to search/typesense.py plus its tests, and touches neither the Elasticsearch nor the Meilisearch backend, so the blast radius is limited to operators who have explicitly opted into Typesense for forum search.

Happy to rebase or split it if that helps review. No rush on our end beyond wanting it upstream rather than carried as a patch.

blarghmatey added a commit to mitodl/lehrer that referenced this pull request Sep 1, 2026
… fork (#208)

* fix(mit-ol): install forum from our branch, drop the edx-django-utils fork

Two override changes, in opposite directions.

Forum: the Typesense search backend is non-functional on every search, not
just some -- per_page is set above Typesense's hard cap of 250, and the topic
filter names a field the collection schema does not declare. Both have been
there since the backend shipped. Fix is openedx/forum#289, which is green and
awaiting review; until it merges and releases, the master cells install forum
from the branch. Scoped to master only: our branch is based on forum master,
so pointing verawood or ulmo at it would jump them across release boundaries
for no benefit -- both still have forum search on Elasticsearch.

This changes nothing at runtime today, since typesense:forum_search_enabled is
false everywhere. It is what makes flipping that toggle possible.

edx-django-utils: openedx/edx-django-utils#549 merged and shipped in 8.0.2, so
the fork branch is no longer needed. edx-platform master already pins 8.0.2 and
the three master cells simply drop the override. The verawood and ulmo cells
cannot: those branches pin 8.0.1, which predates the fix, so they take an
explicit 8.0.2 pin instead. Deleting the override outright would have quietly
regressed OpenTelemetryBackend.create_span() on four of the seven cells.

* fix(mit-ol): extend the forum override to verawood

I had scoped this to the master cells out of caution rather than evidence.
Having actually diffed 0.4.2 against forum master, verawood can take it too.

The delta is ten commits, and only three modules are genuinely removed once
the src/ layout move is normalised away: backends/mongodb/*,
forum_create_mongodb_indexes, and toggles.py.

None of that reaches us. All twelve environments already run
forum_v2.enable_mysql_backend for everyone, so nobody is on the Mongo data
backend. Nothing in openedx-platform imports forum.backends.mongodb or
forum.toggles, and nothing in ol-infrastructure or lehrer invokes the removed
management command. toggles.py only ever defined the enable_mysql_backend
waffle flag itself, which is redundant now that MySQL is the sole backend --
worth noting that our set_waffle_flags entries for it become dead config.

The rest is packaging (uv, src layout, semantic-release) and typing.cast()
wrappers around request.data in the views, which are no-ops at runtime.

ulmo stays on 0.4.1, now for a concrete reason rather than caution: forum
master declares requires-python >=3.12 and the ulmo cell builds on 3.11, so
it cannot install. verawood builds on 3.12, and forum master's only other
constraint is Django>=4.2.

Verified by installing the branch into a clean 3.12 venv: builds, reports
0.4.5, and the installed forum/search/typesense.py carries
TYPESENSE_MAX_PER_PAGE = 250 and the singular commentable_id filter.
blarghmatey added a commit to mitodl/ol-infrastructure that referenced this pull request Sep 2, 2026
…ments

lehrer's build manifest now installs forum from the branch behind
openedx/forum#289 on six of its seven cells, so the Typesense forum backend is
finally usable in those images. Until now the toggle could not be turned on
anywhere: the shipped backend is non-functional, passing per_page=1000 against
Typesense's hard cap of 250 (HTTP 422 on every search) and filtering on
commentable_ids where the collection schema declares commentable_id (HTTP 400
on topic-scoped searches).

That is the likely explanation for April 2026. 87fcc05 (#4501, 11:04) enabled
Typesense and set this key to "true" on mitxonline CI; d6c2824 set it back to
"false" 48 minutes later with no reason recorded; 386057f rolled Typesense
out more widely at 13:48 the same day while deliberately keeping the forum key
"false". It has stayed off everywhere since.

Flipping only where the fixed forum is actually deployed. Cross-referencing
version_matrix.py's env-to-release map against the manifest's cells, seven of
the eight CI/QA environments run master or verawood and qualify. xpro QA is the
exception: it runs ulmo, which pins openedx-forum==0.4.1 because the manifest's
release_python puts ulmo on 3.11 against master and verawood on 3.12, and forum
master dropped 3.11. It keeps Elasticsearch and is left untouched. Production is
untouched everywhere.

All seven already have typesense:enabled true, so the new value is reached at
k8s_configmaps.py:293. Verified by preview on mitxonline.CI, which shows
FORUM_SEARCH_BACKEND becoming forum.search.typesense.TypesenseBackend in the
interpolated configmap.

Run rebuild_forum_indices per environment after applying, then confirm search
returns results. The failure mode is total rather than partial, so it is
obvious if you look.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW
blarghmatey added a commit to mitodl/ol-infrastructure that referenced this pull request Sep 2, 2026
…ments (#5718)

lehrer's build manifest now installs forum from the branch behind
openedx/forum#289 on six of its seven cells, so the Typesense forum backend is
finally usable in those images. Until now the toggle could not be turned on
anywhere: the shipped backend is non-functional, passing per_page=1000 against
Typesense's hard cap of 250 (HTTP 422 on every search) and filtering on
commentable_ids where the collection schema declares commentable_id (HTTP 400
on topic-scoped searches).

That is the likely explanation for April 2026. 87fcc05 (#4501, 11:04) enabled
Typesense and set this key to "true" on mitxonline CI; d6c2824 set it back to
"false" 48 minutes later with no reason recorded; 386057f rolled Typesense
out more widely at 13:48 the same day while deliberately keeping the forum key
"false". It has stayed off everywhere since.

Flipping only where the fixed forum is actually deployed. Cross-referencing
version_matrix.py's env-to-release map against the manifest's cells, seven of
the eight CI/QA environments run master or verawood and qualify. xpro QA is the
exception: it runs ulmo, which pins openedx-forum==0.4.1 because the manifest's
release_python puts ulmo on 3.11 against master and verawood on 3.12, and forum
master dropped 3.11. It keeps Elasticsearch and is left untouched. Production is
untouched everywhere.

All seven already have typesense:enabled true, so the new value is reached at
k8s_configmaps.py:293. Verified by preview on mitxonline.CI, which shows
FORUM_SEARCH_BACKEND becoming forum.search.typesense.TypesenseBackend in the
interpolated configmap.

Run rebuild_forum_indices per environment after applying, then confirm search
returns results. The failure mode is total rather than partial, so it is
obvious if you look.


Claude-Session: https://claude.ai/code/session_012c3Ua8fPhRYnEdQE8snFNW

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Sep 2, 2026
@mphilbrick211
mphilbrick211 requested a review from a team September 2, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

4 participants