Remove deprecated Elasticsearch query framework#794
Open
axlewin wants to merge 41 commits into
Open
Conversation
This endpoint has been unsupported for over a year, and for two months has been inaccessible in production (since 3d839bf). Several deprecated methods were required for it to work, and without it these too can be removed. This gets rid of at least one more deprecated ES method. Since we no longer generate random gameboards, we no longer have any need to store temporary boards. The cache can be removed too, but for now I have just removed the "store new temporary board" method.
No longer needed since all of its methods are superseded by AbstractInstruction-based equivalents
and convertToBoolMap helper method
Deprecated; AbstractFilterInstruction-based searching should use AbstractInstructions instead
Deprecated; AbstractFilterInstruction-based searching should use AbstractInstructions instead
This is superseded by buildBaseInstructions, which is easier to work with for AbstractInstructions (as opposed to AbstractFilterInstructions).
Also remove the tests for the removed methods. These all used AbstractFilterInstructions. Going forward, AbstractInstruction-based searching should be used instead.
Deprecated in favour of BooleanInstructions
To allow e.g. nofilter quizzes to be viewed. If nofilter content needs to be excluded based on role, this should be handled outside of this manager (e.g. in QuizManager).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 40.28% 40.39% +0.10%
==========================================
Files 547 541 -6
Lines 23814 23389 -425
Branches 2898 2828 -70
==========================================
- Hits 9593 9447 -146
+ Misses 13320 13077 -243
+ Partials 901 865 -36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The "should" clauses should be grouped together and nested in a parent "must" to ensure at least one clause per group is satisfied.
ElasticSearchProvider no longer contains any public helper functions for building queries that can be tested in isolation. It only contains functions that directly interact with the Elasticsearch client which can't easily be tested with unit tests.
If a boolean instruction contains just one match clause, this can be simplified to just a match instruction. Overloading nestedMatchSearch allows us to remove the confusing nesting at the call site whilst still re-using all the existing boolean search functionality.
This nested instruction has no must/filter clauses, so "minimum should match" defaults to 1 anyway.
jsharkey13
reviewed
Jun 11, 2026
jsharkey13
left a comment
jsharkey13
left a comment
Member
There was a problem hiding this comment.
A few questions about subtle filtering behaviour where I think we were, deep under the hood, doing some filtering which this no longer does - because it does not use the new-style builder that does that by default.
Go back to excluding nofilter content by default, and explicitly set the `excludeNofilterContent` property on the base builder when required.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
If a search instruction is built without explicit type filters being passed in, we were defaulting to only returning pages that would be returned by sitewide search. This meant that the search instruction builder - and its more generally useful functionality such as setting base filters - couldn't easily be used in places where we want to return content of _any_ type, unless every possible content type was passed in to the builder. Instead, if no content types are provided, assume all content types should be returned. The existing places that need to filter by content type, such as sitewide search, already pass in the appropriate content types anyway so we weren't relying on the previous default behaviour. This also requires not explicitly setting minimumShouldMatch, since in the case where no content types are provided we no longer require at least one content type to match, so we should let it default to zero. In the case where content types _are_ provided, minimumShouldMatch will default to one anyway, since all of its nested clauses are shoulds.
All events should now have end dates, but this wasn't always the case. To be safe, the initial list of events considered here should include events with a date in the past, in case the end date isn't set. The logic further down still disregards the date if an end date _is_ set.
The search logic here was identical to that in getUnsafeCachedContentDTOsMatchingIds, so just call the method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes all usage of
BooleanSearchClauseandAbstractFilterInstruction-based searching in favour of the newerAbstractInstruction-based pattern. This removes a lot of duplicated logic; there should now be a single, consistent approach to building ES queries across the whole project.Also removes the
generateTemporaryGameboardendpoint, which has been unsupported for over a year. This change is technically unrelated but was needed in order to remove some of the ES framework.