refactor search suggestions - #4105
Conversation
340b5d4 to
c86360d
Compare
|
Todo: requests to /search/suggest without language slug return 400. implement search suggestions for organizations and users |
…or-search-suggestions
…e to get multiple results
MizukiTemma
left a comment
There was a problem hiding this comment.
Thank you so much, that's a great work 😻 and it comes even with clean code, good structure and tests 🚀
I have though two concerns:
-
In Media library (both network management and region) it seems the completion is not impelmented. If it was not intented, we should add it, as it's currently available on the prod system.
-
It is not a bug but should be informed to Service Team that users may have to type more to see the first suggestion (completion) compared to the current implementation.
Currently users get suggestions directly after the first letter tipped (if any maches), but after this PR they have to type at least two letters and no completion may be suggested with very two letters. I observed completion appeard first with >3 letters (as tested locally with test data. The performance may be better with real data).
This makes them maybe confused. Service Team should inform users of this change and also of an option to hit the enter key or click the lupe icon to search for short string.
Thank you very much! I wanted to keep I changed the |
MizukiTemma
left a comment
There was a problem hiding this comment.
Thank you so much 😸 Looks good 🚀
| :param prefix_match: Whether the token starts with the query | ||
| :return: The weighted score for this token | ||
| """ | ||
| score = similarity * weight |
There was a problem hiding this comment.
Why I would prefer the bounded weights: This way , when adding a new weight, the actual weight of other weights would not move since the maximum is fixed. With unbounded weights, it shifts the relative weigthiness of every other weight if we add a new "max" weight. But this is also not such a strong opinion for me, that if you want to stay with the current form, that I would be unhappy about it
| for field in fields: | ||
| q_filter |= Q(**{f"{field}__icontains": query}) | ||
|
|
||
| qs: QuerySet[Any] = cls.objects.filter(q_filter) |
There was a problem hiding this comment.
How to reproduce the page situation:
- Create a new page with title "Title first version", save it.
- Change the title to "Title second version" save it.
- Go to the list view. Type "first version" See suggestion "Title first version" Select that, see that no search results are shown
6c702a8 to
c190203
Compare
hannaseithe
left a comment
There was a problem hiding this comment.
Thank you for implementing the change requests. I had a second a little more thorough look and came up with a few more issues:
| :return: Dict with "suggestions" key containing list of {suggestion, score} dicts | ||
| """ | ||
| file_result = super().suggest_tokens(query, region=region, archived=archived) | ||
| dir_result = Directory.suggest_tokens(query, region=region, archived=archived) |
There was a problem hiding this comment.
I am a bit uncomfortable with how Directory is handled here. This seems like a bit hacky solution. I think the core issue comes from the archtiectural assumption that we only search on one object_type in the view, which forces you to hide the directory away here inside media_file.py Maybe a better way would be to let the view accept a list of object_types and then merge and sort them together at the end - especially since all the fields are not tokenized, so accumulating score over tokesn wont be a cocnern.
There was a problem hiding this comment.
the issue is that, when a user is searching the media library, we don't know if they are searching for a directory or a media file object. from a technical perspective, the object_type is hardcoded to mediafile here. we do resolve the object_type into a model class in integreat_cms/cms/views/search/utils.py:get_model_class_from_object_type, and we could change our logic to accept a list of classes such that the mediafile object type resolves to [MediaFile, Directory], however, the utils function is currently also used by the legacy search_content_ajax, which expects a different return type. so the options if see are:
- keep the (arguably hacky) approach that the
Directorytokens are merged into theMediaFiletokens inside theMediaFile.suggest_tokensmethod - wrap
get_model_class_from_object_typeforsearch_suggestto handle themediafilespecial case for search token suggestions only while keeping the logic for the legacy endpoint untouched
which do you think is better?
There was a problem hiding this comment.
My suggestion would actually to be, to change the return value from the frontend:
- so in library.tsx line 225:
data-object-type="mediafile,directory"
- in search-query.ts line 85:
tableSearchInput.getAttribute("data-object-type").split(","),
- and search-query.ts from line 3
const queryObjects = async (url: string, types: string[], queryString: string, archived: boolean) => {
if (queryString.trim().length === 0) {
document.getElementById("table-search-suggestions").classList.add("hidden");
return;
}
const response = await fetch(url, {
method: "POST",
headers: {
"X-CSRFToken": getCsrfToken(),
},
body: JSON.stringify({
query_string: queryString,
object_types: types,
archived,
}),
});
- and then we could just loop over the list in search_suggest.py and therfore be able to split the two suggest_tokens to the two models where they belong.
I hope I didn't miss anything, but this way we would not touch the old ajax endpoint, but still end up with a cleaner architecture. What do you think?
| latest_translations=models.FilteredRelation( | ||
| "translations", | ||
| condition=models.Q(translations__id__in=latest_translation_ids), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I get the error
May 18 10:56:45 DEBUG integreat_cms.cms.views.search.search_suggest - Search suggest for 'page' with query 'Ka'
May 18 10:56:46 ERROR django.request - 500 Internal Server Error: /augsburg/ajax/de/search/suggest/
Traceback ....
....
integreat-cms/.venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 116, in get_child_with_renamed_prefix
raise ValueError(
ValueError: Passing a QuerySet within a FilteredRelation is not supported.
There was a problem hiding this comment.
seems like this worked in Django 4.2, but is now rejected in newer versions. I'll check for a different solution
| # Build filter for any field containing the query | ||
| q_filter = Q() | ||
| for field in fields: | ||
| q_filter |= Q(**{f"{field}__icontains": query}) |
There was a problem hiding this comment.
I think there is an issue with the filtering pipeline.
- you do an icontains. That filters out any fuzzyness from the start
- . You apply trigram similarity when we only have exact matches already (so similarity only gets applied by accident to alternative fields - where we already have an exact match on another field of the same object)
I remember you saying that the fuzzyness is intentional for typos and such. In that case icontains is the wrong prefilter.
There was a problem hiding this comment.
The scope of this PR was initially to only use similarity for scoring, not for finding the matches. Using similarity for finding matches was planned to be handled in a separate PR. But since the PR is already huge, it doesn't really hurt to include it here as well, i.e. replace the icontains with similarity matching. If you'd prefer that, I can do it. The corresponding issue is #4098
There was a problem hiding this comment.
If this is not too much work for you, I would prefer to have it implemented here. But I leave it up to you to decide.
…go version > 4.2 compability
…or-search-suggestions
|
@MizukiTemma could you also have a look again? quite some things changed since you reviewed 🙈 |
|
@jonbulz It's not a bug but I found one concern: completions for users are not filtered by the region and users may see names or e-mail addresses of users of the other regions. Example and how to reproduce locally |
hannaseithe
left a comment
There was a problem hiding this comment.
The language filter and the archived filter make sense to me. I left a minor architectural comment. That might be worth considering.
| unpublished drafts. | ||
| """ | ||
| qs = super().get_suggest_queryset(region=region, archived=archived) | ||
| qs = super().get_suggest_queryset( |
There was a problem hiding this comment.
This tightly couples the SearchSuggestMixin to the abstract_content_translation class. Maybe it would be better to inherit from the SearchSuggestMixing directly here in the AbstractContentTranslation class instead of the concrete child classes?
|
converted to draft until this is ready to be picked up again |

Short description
Currently, suggesting search term completions in the Search input field and record suggestions when linking to internal references use the same logic. This PR introduces
SearchSuggestMixinto cleanly separate the logic of search term suggestions from suggesting records. Instead of just producing a list of record names,suggest_tokensextracts useful search tokens from the existing records and ranks them by similarity score.Proposed changes
SearchSuggestMixinto extend search term suggestion to different modelssearch/suggestto separate search term completion from record suggestionsearch_content_ajax.pyfromviews/utilstoviews/searchfor convenienceSEARCH_FIELDSin a central place to handle configurationregion_filter_fieldandarchived_filter_fieldto allow filtering by region and archived stateSide effects
search_content_ajaxendpoint and itssuggest()model methods are unchanged. List view search inputs now hit the newsearch/suggest/endpoint instead. The "Insert link" editor dialog continues to usesearch_content_ajax.Faithfulness to issue description and design
There are no intended deviations from the issue and design. The old
suggest()/search()methods on individual models are intentionally left in place for thesearch_content_ajaxendpoint and will be unified in a follow-up.How to test
Resolved issues
Fixes: #4095
Fixes: #4096
Pull Request Review Guidelines