Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
1b9b515
feat: add multivalue faceted search logic
Serj-N Jul 9, 2025
bece993
test: add unit tests
Serj-N Jul 9, 2025
fa68434
test: add integration tests
Serj-N Jul 9, 2025
d0f2b0e
refactor: add make command to run test in container
Serj-N Jul 9, 2025
718b223
fix: add whitespace around operator
Serj-N Jul 16, 2025
2c721ff
fix: run make quality
Serj-N Jul 16, 2025
82c5cad
test: run meilisearch service in github CI
Serj-N Jul 16, 2025
b315d5c
fix: add allowed hosts, edit ci.yml
Serj-N Jul 16, 2025
d273949
fix: change meilisearch url to localhost
Serj-N Jul 16, 2025
27ef1a3
feat: implement multivalue search for elastic
Serj-N Jul 15, 2025
a90b9d4
chore: change get_filter_rules after rebase
NiedielnitsevIvan Jul 24, 2025
b93e1e5
style: remove extra indents
NiedielnitsevIvan Jul 25, 2025
5ebcdeb
chore: add conftest, test_network, create network in makefile
Serj-N Jul 29, 2025
f4f698b
fix: adjust exclude logic, fix/add test
Serj-N Jul 29, 2025
5cf92ce
fix: remove unused import, fix import issue
Serj-N Jul 29, 2025
592d1b9
test: add test-all command, adjust test-elastic
Serj-N Jul 29, 2025
517e237
chore: add manage.py migrate to make commands
Serj-N Jul 29, 2025
e2c6dbc
chore: configure github ci to run tests
Serj-N Jul 29, 2025
f3760a7
chore: remove migrate from github ci
Serj-N Jul 29, 2025
7514511
test: use manage.py test instead of pytest, remove conftest.py
Serj-N Jul 29, 2025
e6f6246
test: run tests selectively using tox
Serj-N Jul 29, 2025
1503955
chore: fix indentation in ci.yml
Serj-N Jul 29, 2025
e19eb6e
fix: fix linter errors
Serj-N Jul 29, 2025
97149af
fix: fix lint error in a test
Serj-N Jul 29, 2025
580f0e0
test: restore base version of integration tests
Serj-N Aug 28, 2025
18a40b1
chore: make ci run all tests
Serj-N Aug 28, 2025
244a8b6
test: remove unused code
Serj-N Aug 28, 2025
b3680b3
test: cover nonfacet rules logic
Serj-N Aug 28, 2025
6c1ca11
test: adjust makefile to run tests
Serj-N Sep 8, 2025
fb406bc
fix: remove unused parameter
Serj-N Sep 8, 2025
44dbee7
test: move ES tests to test_elasticsearch.py
Serj-N Sep 13, 2025
f92d732
test: test global_aggs in multivalue aggregation
Serj-N Sep 13, 2025
08cd07a
test: remove tests asserting calls for aggregation processors
Serj-N Sep 13, 2025
d74f8dd
test: use assertGreaterEqual to compare dicts
Serj-N Sep 13, 2025
cfafa9e
chore: move version to top in docker-compose.yml
Serj-N Sep 13, 2025
4c09f37
docs: add comment for is_multivalue
Serj-N Sep 13, 2025
487419f
chore: change meilisearch image version
Serj-N Sep 13, 2025
79864c6
build: remove MEILISEARCH_URL from docker-compose, Makefile
Serj-N Sep 13, 2025
5e6653e
docs: edit docstring for _process_multivalue_aggregations
Serj-N Sep 13, 2025
4594bef
docs: add type annotations to new functions
Serj-N Sep 13, 2025
c84c7f1
style: rename rule_dict items to field_name, field_value
Serj-N Sep 13, 2025
8697093
test: run all tests via makefile
Serj-N Sep 15, 2025
6d516c5
fix: assert not optional in OR branch
Serj-N Sep 15, 2025
704c520
style: remove extra blank line
Serj-N Sep 15, 2025
0187b5a
refactor: pass or_fields to get_filter_rules
Serj-N Sep 15, 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
8 changes: 7 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,21 @@ jobs:
python-version: ['3.11', '3.12']
toxenv: [django42, django52, quality]

env:
MEILISEARCH_URL: http://localhost:7700

steps:
- uses: actions/checkout@v2
- name: setup python
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}

- name: Start Meilisearch
run: make meili-up

- name: Start Elasticsearch
run: make test.start_elasticsearch
run: make elastic-up

- name: Install pip
run: pip install -r requirements/pip.txt
Expand Down
31 changes: 31 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,34 @@ test: test_with_es ## run tests and generate coverage report
install-local: ## installs your local edx-search into the LMS and CMS python virtualenvs
docker exec -t edx.devstack.lms bash -c '. /edx/app/edxapp/venvs/edxapp/bin/activate && cd /edx/app/edxapp/edx-platform && pip uninstall -y edx-search && pip install -e /edx/src/edx-search && pip freeze | grep edx-search'
docker exec -t edx.devstack.cms bash -c '. /edx/app/edxapp/venvs/edxapp/bin/activate && cd /edx/app/edxapp/edx-platform && pip uninstall -y edx-search && pip install -e /edx/src/edx-search && pip freeze | grep edx-search'

test-all: create-test-network meili-up elastic-up
@MEILISEARCH_MASTER_KEY=test_master_key python manage.py test || true
@$(MAKE) meili-down
@$(MAKE) elastic-down


meili-up: create-test-network
@echo "Starting Meilisearch..."
@docker compose up -d test_meilisearch
@echo "Waiting for Meilisearch to be healthy..."
@timeout 15 bash -c \
'until curl -sf http://localhost:7700/health > /dev/null; do echo "Waiting..."; sleep 1; done'

meili-down:
@echo "Shutting down Meilisearch..."
@docker compose down test_meilisearch


elastic-up: create-test-network
@echo "Starting Elasticsearch..."
@docker compose up -d test_elasticsearch
@echo "Waiting for Elasticsearch to be healthy..."
@timeout 30 bash -c 'until curl -s http://localhost:9200/_cluster/health | grep -q "status"; do echo "Waiting..."; sleep 2; done'

elastic-down:
@echo "Shutting down Elasticsearch..."
docker compose down test_elasticsearch

create-test-network:
docker network inspect test_network >/dev/null 2>&1 || docker network create --driver bridge test_network
26 changes: 26 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
version: '3.8'

services:

test_elasticsearch:
Expand All @@ -18,7 +20,31 @@ services:
- data01:/usr/share/elasticsearch/data
ports:
- "9200:9200"
networks:
- test_network

test_meilisearch:
# Keep in sync with DOCKER_IMAGE_MEILISEARCH
# in https://github.com/overhangio/tutor/blob/main/tutor/templates/config/defaults.yml
image: getmeili/meilisearch:v1.8.4
ports:
- "7700:7700"
networks:
- test_network
environment:
MEILISEARCH_MASTER_KEY: test_master_key
healthcheck:
test: ["CMD", "curl", "-f", "http://localhost:7700/health"]
interval: 2s
timeout: 1s
retries: 10

volumes:
data01:
driver: local

networks:
test_network:
name: test_network
driver: bridge
external: true
13 changes: 12 additions & 1 deletion edxsearch/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
# This is just a container for running tests
DEBUG = True

ALLOWED_HOSTS = []
ALLOWED_HOSTS = ['testserver', 'localhost', '127.0.0.1']

TEMPLATES = [
{
Expand Down Expand Up @@ -128,3 +128,14 @@
}
}
}

MEILISEARCH_API_KEY = os.environ.get("MEILISEARCH_MASTER_KEY", "test_master_key")
MEILISEARCH_URL = os.environ.get("MEILISEARCH_URL", "http://localhost:7700")

ELASTIC_SEARCH_CONFIG = [
{
'use_ssl': False,
'host': 'localhost',
'port': 9200
}
]
2 changes: 2 additions & 0 deletions search/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ def course_discovery_search(
from_=0,
field_dictionary=None,
enable_course_sorting_by_start_date=False,
is_multivalue=False, # Determines whether to use single-faceted or multi-faceted search
):
"""
Course Discovery activities against the search engine index of course details
Expand Down Expand Up @@ -166,6 +167,7 @@ def course_discovery_search(
exclude_dictionary=exclude_dictionary,
aggregation_terms=course_discovery_aggregations(),
sort_by=sort_by,
is_multivalue=is_multivalue,
)

return results
106 changes: 87 additions & 19 deletions search/elastic.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
RESERVED_CHARACTERS = "+=><!(){}[]^~*:\\/&|?"


def _translate_hits(es_response):
def _translate_hits(es_response, aggregation_terms, is_multivalue=False):
"""
Provide result set in our desired format from elasticsearch results.

Expand Down Expand Up @@ -99,7 +99,7 @@ def translate_result(result):

def translate_agg_bucket(bucket, agg_result):
"""
Any conversion from ES aggregations result into our search engine syntax
Convert ES aggregation result following our search engine syntax.

agg_result argument needs for getting total number of
documents per bucket.
Expand All @@ -109,19 +109,29 @@ def translate_agg_bucket(bucket, agg_result):
:return: dict
"""
agg_item = agg_result[bucket]
terms = {
bucket["key"]: bucket["doc_count"]
for bucket in agg_item["buckets"]
}
total_docs = (
agg_result[_get_total_doc_key(bucket)]["value"]
+ agg_item["sum_other_doc_count"]
+ agg_item["doc_count_error_upper_bound"]
)

if is_multivalue:
values_agg = agg_item["values"]
terms = {
bucket["key"]: bucket["doc_count"]
for bucket in values_agg["buckets"]
Comment on lines +114 to +117

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
values_agg = agg_item["values"]
terms = {
bucket["key"]: bucket["doc_count"]
for bucket in values_agg["buckets"]
terms = {
bucket["key"]: bucket["doc_count"]
for bucket in agg_item["values"]

Nit: it's easier to see the difference between this part of the if and the else: clause below if you structure these similar lines in exactly the same way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here: the two dict comprehensions seem to be structrured in the same way - the only difference being the object after "in"

@bradenmacdonald bradenmacdonald Sep 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, don't worry about this one, which was really a nitpick. I got a bit confused reading this code because these two parts look exactly the same (but they're not):

Screenshot 2025-09-17 at 10 04 28 AM

I think it's slightly more clear if you write it this way, which makes it more obvious that they're different:

Screenshot 2025-09-17 at 10 09 31 AM

I realize now my suggestion above in my first comment had a bug/error, so yeah don't worry about it.

}
total_docs = sum(terms.values())
else:
terms = {
bucket["key"]: bucket["doc_count"]
for bucket in agg_item["buckets"]
}
total_docs = (
agg_result[_get_total_doc_key(bucket)]["value"]
+ agg_item["sum_other_doc_count"]
+ agg_item["doc_count_error_upper_bound"]
)

return {
"terms": terms,
"total": total_docs,
"other": agg_item["sum_other_doc_count"],
"other": agg_item.get("sum_other_doc_count", 0),
}

results = list(map(translate_result, es_response["hits"]["hits"]))
Expand All @@ -132,11 +142,19 @@ def translate_agg_bucket(bucket, agg_result):
"results": results,
}
if "aggregations" in es_response:
response["aggs"] = {
bucket: translate_agg_bucket(bucket, es_response["aggregations"])
for bucket in es_response["aggregations"]
if "total_" not in bucket
}
if is_multivalue:
global_aggs = es_response["aggregations"].get("global_aggs", {})
response["aggs"] = {
facet: translate_agg_bucket(facet, global_aggs)
for facet in global_aggs
if facet in aggregation_terms
}
else:
response["aggs"] = {
bucket: translate_agg_bucket(bucket, es_response["aggregations"])
for bucket in es_response["aggregations"]
if "total_" not in bucket
}

return response

Expand Down Expand Up @@ -241,6 +259,51 @@ def _process_aggregation_terms(aggregation_terms):
return elastic_aggs


def _process_multivalue_aggregations(aggregation_terms: dict, field_dictionary: dict) -> dict:
"""
Compute the aggregation requests that we'll send with the query when searching with multi-value faceting.
Typical shape of the args:
aggregation_terms={'language': {}, 'modes': {}, 'org': {}}
field_dictionary={'enrollment_start': <search.utils.DateRange object at ...>,
'language': ['en', 'fr']}
"""
aggs = {}
for facet_field, options in aggregation_terms.items():
filters_excluding_facet = {
field: value for field, value in field_dictionary.items()
if field != facet_field
}
filter_clauses = [
_get_filter_field(field, value)
for field, value in filters_excluding_facet.items()
if value
]
facet_filter = {
"bool": {
"must": filter_clauses
}
} if filter_clauses else {"match_all": {}}

aggs[facet_field] = {
"filter": facet_filter,
"aggs": {
"values": {
"terms": {
"field": facet_field,
**options
}
}
}
}

return {
"global_aggs": {
"global": {},
"aggs": aggs
}
}


class ElasticSearchEngine(SearchEngine):
"""
ElasticSearch implementation of SearchEngine abstraction
Expand Down Expand Up @@ -654,8 +717,13 @@ def search(self,
}

body = {"query": query}

is_multivalue = kwargs.pop("is_multivalue", False)
if aggregation_terms:
body["aggs"] = _process_aggregation_terms(aggregation_terms)
if is_multivalue:
body["aggs"] = _process_multivalue_aggregations(aggregation_terms, field_dictionary)
else:
body["aggs"] = _process_aggregation_terms(aggregation_terms)
if sort_by:
body["sort"] = self._transform_sort_by(sort_by)

Expand All @@ -668,7 +736,7 @@ def search(self,
log.exception("error while searching index - %r", ex)
raise

return _translate_hits(es_response)
return _translate_hits(es_response, aggregation_terms, is_multivalue)

def _transform_sort_by(self, fields: list[SortField]):
"""
Expand Down
Loading