From c85b23f731fad6dcb0a8612788b10b094ab138ea Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Wed, 24 Apr 2024 11:26:44 +0200 Subject: [PATCH 01/12] feat: wip - display of facets --- frontend/public/off.html | 2 +- frontend/src/event-listener-setup.ts | 2 +- frontend/src/events.ts | 1 + frontend/src/search-a-licious.ts | 1 + frontend/src/search-ctl.ts | 10 +- frontend/src/search-facets.ts | 507 +++++++++++++++++++++++ frontend/src/search-pages.ts | 58 +-- frontend/src/search-results-ctl.ts | 91 ++++ frontend/src/search-results.ts | 38 +- frontend/src/test/search-pages_test.ts | 3 +- frontend/src/test/search-results_test.ts | 3 +- 11 files changed, 634 insertions(+), 82 deletions(-) create mode 100644 frontend/src/search-facets.ts create mode 100644 frontend/src/search-results-ctl.ts diff --git a/frontend/public/off.html b/frontend/public/off.html index ba2f88c6..d30a74d8 100644 --- a/frontend/public/off.html +++ b/frontend/public/off.html @@ -251,7 +251,7 @@
- **TODO** Facets +
diff --git a/frontend/src/event-listener-setup.ts b/frontend/src/event-listener-setup.ts index 0066a3dd..5a160f4d 100644 --- a/frontend/src/event-listener-setup.ts +++ b/frontend/src/event-listener-setup.ts @@ -6,7 +6,7 @@ // eslint-disable-next-line @typescript-eslint/no-explicit-any type Constructor = new (...args: any[]) => T; -export declare class EventRegistrationInterface { +export interface EventRegistrationInterface { /** * Calls window.addEventListener if not remove before next AnimationFrame * @param event - event name diff --git a/frontend/src/events.ts b/frontend/src/events.ts index 6e90f8bb..847e59d8 100644 --- a/frontend/src/events.ts +++ b/frontend/src/events.ts @@ -14,6 +14,7 @@ export interface SearchResultDetail extends BaseSearchDetail { pageCount: number; pageSize: number; currentPage: number; + aggregations: Object; // FIXME: we could be more precise } /** diff --git a/frontend/src/search-a-licious.ts b/frontend/src/search-a-licious.ts index 5812a026..bc9cdc46 100644 --- a/frontend/src/search-a-licious.ts +++ b/frontend/src/search-a-licious.ts @@ -1,4 +1,5 @@ export {SearchaliciousBar} from './search-bar'; export {SearchaliciousButton} from './search-button'; export {SearchaliciousPages} from './search-pages'; +export {SearchaliciousFacets} from './search-facets'; export {SearchaliciousResults} from './search-results'; diff --git a/frontend/src/search-ctl.ts b/frontend/src/search-ctl.ts index 57f617cf..fbe5d078 100644 --- a/frontend/src/search-ctl.ts +++ b/frontend/src/search-ctl.ts @@ -1,6 +1,9 @@ import {LitElement} from 'lit'; import {property, state} from 'lit/decorators.js'; -import {EventRegistrationMixin} from './event-listener-setup'; +import { + EventRegistrationInterface, + EventRegistrationMixin, +} from './event-listener-setup'; import {SearchaliciousEvents} from './enums'; import { ChangePageEvent, @@ -12,7 +15,8 @@ import { // eslint-disable-next-line @typescript-eslint/no-explicit-any type Constructor = new (...args: any[]) => T; -export declare class SearchaliciousSearchInterface { +export interface SearchaliciousSearchInterface + extends EventRegistrationInterface { query: string; name: string; baseUrl: string; @@ -177,6 +181,7 @@ export const SearchaliciousSearchMixin = >( */ async search(page?: number) { const response = await fetch(this._searchUrl(page)); + // FIXME data should be typed… const data = await response.json(); this._results = data.hits; this._count = data.count; @@ -191,6 +196,7 @@ export const SearchaliciousSearchMixin = >( pageCount: this._pageCount!, currentPage: this._currentPage!, pageSize: this.pageSize, + aggregations: data.aggregations, }; this.dispatchEvent( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { diff --git a/frontend/src/search-facets.ts b/frontend/src/search-facets.ts new file mode 100644 index 00000000..e899742f --- /dev/null +++ b/frontend/src/search-facets.ts @@ -0,0 +1,507 @@ +import {LitElement, html} from 'lit'; +import {customElement, property} from 'lit/decorators.js'; +import {repeat} from 'lit/directives/repeat.js'; + +import {SearchaliciousResultCtlMixin} from './search-results-ctl'; +import {SearchResultEvent} from './events'; +import keys from 'lodash-es/keys'; + +type Aggregations = Record; + +interface FilterData { + buckets: Array; +} + +interface Bucket { + key: string; +} + +interface DocCountBucket extends Bucket { + doc_count: number; +} + +/** + * Component to display search filters (aka facets) on in a column + */ +@customElement('searchalicious-facets') +export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( + LitElement +) { + // the last search facets + @property({attribute: false}) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + aggregations?: Aggregations; + + override handleResults(event: SearchResultEvent) { + this.aggregations = event.detail.aggregations as Aggregations; + } + + renderBucket(bucket: Bucket) { + const dcBucket = bucket as DocCountBucket; + return html` + + `; + } + + renderFilter(filterName: string, filterData: FilterData) { + return html` +
+ + ${filterName} + ${repeat( + filterData.buckets, + (item: Bucket) => item.key, + (item: Bucket) => this.renderBucket(item) + )} +
+ `; + } + + override render() { + if (this.aggregations) { + return html`
+ ${repeat( + keys(this.aggregations), + (item: string) => item, + (item: string) => this.renderFilter(item, this.aggregations![item]) + )} +
`; + } else { + return html``; // FIXME + } + } +} + +declare global { + interface HTMLElementTagNameMap { + 'searchalicious-facets': SearchaliciousFacets; + } +} + +/* + +things to add to API: +1. selected filters (if easy) --> we put it in a specific but we can match +2. widget type to use for each facet --> may also be separate to simplify ? +3. translations of filter names + +{ + "aggregations": { + "nova_groups": { + "doc_count_error_upper_bound": 0, + "sum_other_doc_count": 0, + "buckets": [ + { + "key": "4", + "doc_count": 559 + }, + { + "key": "3", + "doc_count": 158 + }, + { + "key": "1", + "doc_count": 95 + }, + { + "key": "2", + "doc_count": 50 + } + ] + }, + "owner": { + "doc_count_error_upper_bound": 0, + "sum_other_doc_count": 21, + "buckets": [ + { + "key": "org-carrefour", + "doc_count": 9 + }, + { + "key": "org-systeme-u", + "doc_count": 9 + }, + { + "key": "org-les-mousquetaires", + "doc_count": 6 + }, + { + "key": "org-idiffusion", + "doc_count": 5 + }, + { + "key": "org-scamark", + "doc_count": 5 + }, + { + "key": "org-auchan-apaw", + "doc_count": 4 + }, + { + "key": "org-casino", + "doc_count": 3 + }, + { + "key": "org-groupe-ldc", + "doc_count": 3 + }, + { + "key": "org-albert-menes", + "doc_count": 2 + }, + { + "key": "org-alnatura", + "doc_count": 2 + } + ] + }, + "categories_tags": { + "doc_count_error_upper_bound": 58, + "sum_other_doc_count": 5438, + "buckets": [ + { + "key": "en:plant-based-foods-and-beverages", + "doc_count": 419 + }, + { + "key": "en:plant-based-foods", + "doc_count": 367 + }, + { + "key": "en:snacks", + "doc_count": 266 + }, + { + "key": "en:sweet-snacks", + "doc_count": 202 + }, + { + "key": "en:dairies", + "doc_count": 163 + }, + { + "key": "en:beverages", + "doc_count": 143 + }, + { + "key": "en:fermented-foods", + "doc_count": 127 + }, + { + "key": "en:cereals-and-potatoes", + "doc_count": 123 + }, + { + "key": "en:fermented-milk-products", + "doc_count": 123 + }, + { + "key": "en:meats-and-their-products", + "doc_count": 118 + } + ] + }, + "brands_tags": { + "doc_count_error_upper_bound": 8, + "sum_other_doc_count": 1759, + "buckets": [ + { + "key": "bonarea", + "doc_count": 17 + }, + { + "key": "carrefour", + "doc_count": 15 + }, + { + "key": "lidl", + "doc_count": 15 + }, + { + "key": "nestle", + "doc_count": 13 + }, + { + "key": "tesco", + "doc_count": 13 + }, + { + "key": "u", + "doc_count": 13 + }, + { + "key": "auchan", + "doc_count": 8 + }, + { + "key": "coop", + "doc_count": 7 + }, + { + "key": "hacendado", + "doc_count": 7 + }, + { + "key": "ferrero", + "doc_count": 5 + } + ] + }, + "ecoscore_grade": { + "doc_count_error_upper_bound": 0, + "sum_other_doc_count": 0, + "buckets": [ + { + "key": "unknown", + "doc_count": 2350 + }, + { + "key": "b", + "doc_count": 260 + }, + { + "key": "d", + "doc_count": 220 + }, + { + "key": "c", + "doc_count": 167 + }, + { + "key": "e", + "doc_count": 98 + }, + { + "key": "a", + "doc_count": 37 + }, + { + "key": "not-applicable", + "doc_count": 29 + } + ] + }, + "states_tags": { + "doc_count_error_upper_bound": 581, + "sum_other_doc_count": 25942, + "buckets": [ + { + "key": "en:to-be-completed", + "doc_count": 3153 + }, + { + "key": "en:characteristics-to-be-completed", + "doc_count": 3084 + }, + { + "key": "en:origins-to-be-completed", + "doc_count": 3047 + }, + { + "key": "en:packaging-code-to-be-completed", + "doc_count": 3007 + }, + { + "key": "en:product-name-completed", + "doc_count": 3001 + }, + { + "key": "en:expiration-date-to-be-completed", + "doc_count": 2783 + }, + { + "key": "en:packaging-to-be-completed", + "doc_count": 2767 + }, + { + "key": "en:photos-uploaded", + "doc_count": 2709 + }, + { + "key": "en:photos-to-be-validated", + "doc_count": 2628 + }, + { + "key": "en:packaging-photo-to-be-selected", + "doc_count": 2620 + } + ] + }, + "nutrition_grades": { + "doc_count_error_upper_bound": 0, + "sum_other_doc_count": 0, + "buckets": [ + { + "key": "unknown", + "doc_count": 1991 + }, + { + "key": "d", + "doc_count": 314 + }, + { + "key": "c", + "doc_count": 246 + }, + { + "key": "e", + "doc_count": 206 + }, + { + "key": "a", + "doc_count": 173 + }, + { + "key": "b", + "doc_count": 169 + }, + { + "key": "not-applicable", + "doc_count": 62 + } + ] + }, + "lang": { + "doc_count_error_upper_bound": 0, + "sum_other_doc_count": 61, + "buckets": [ + { + "key": "fr", + "doc_count": 1186 + }, + { + "key": "en", + "doc_count": 1040 + }, + { + "key": "es", + "doc_count": 353 + }, + { + "key": "it", + "doc_count": 239 + }, + { + "key": "de", + "doc_count": 220 + }, + { + "key": "nl", + "doc_count": 17 + }, + { + "key": "pt", + "doc_count": 16 + }, + { + "key": "pl", + "doc_count": 14 + }, + { + "key": "ru", + "doc_count": 12 + }, + { + "key": "cs", + "doc_count": 11 + } + ] + }, + "countries_tags": { + "doc_count_error_upper_bound": 9, + "sum_other_doc_count": 411, + "buckets": [ + { + "key": "en:france", + "doc_count": 1017 + }, + { + "key": "en:united-states", + "doc_count": 619 + }, + { + "key": "en:spain", + "doc_count": 378 + }, + { + "key": "en:germany", + "doc_count": 273 + }, + { + "key": "en:italy", + "doc_count": 252 + }, + { + "key": "en:united-kingdom", + "doc_count": 127 + }, + { + "key": "en:belgium", + "doc_count": 93 + }, + { + "key": "en:switzerland", + "doc_count": 92 + }, + { + "key": "en:canada", + "doc_count": 81 + }, + { + "key": "en:ireland", + "doc_count": 51 + } + ] + }, + "labels_tags": { + "doc_count_error_upper_bound": 18, + "sum_other_doc_count": 1172, + "buckets": [ + { + "key": "en:organic", + "doc_count": 200 + }, + { + "key": "en:no-gluten", + "doc_count": 177 + }, + { + "key": "en:eu-organic", + "doc_count": 144 + }, + { + "key": "en:green-dot", + "doc_count": 126 + }, + { + "key": "en:vegetarian", + "doc_count": 115 + }, + { + "key": "en:vegan", + "doc_count": 104 + }, + { + "key": "en:nutriscore", + "doc_count": 84 + }, + { + "key": "fr:ab-agriculture-biologique", + "doc_count": 61 + }, + { + "key": "en:no-preservatives", + "doc_count": 56 + }, + { + "key": "en:no-lactose", + "doc_count": 40 + } + ] + } + } +} +*/ diff --git a/frontend/src/search-pages.ts b/frontend/src/search-pages.ts index 1e15182b..0ca354a1 100644 --- a/frontend/src/search-pages.ts +++ b/frontend/src/search-pages.ts @@ -3,15 +3,17 @@ import {customElement, property, state} from 'lit/decorators.js'; import {repeat} from 'lit/directives/repeat.js'; import range from 'lodash-es/range'; -import {EventRegistrationMixin} from './event-listener-setup'; import {SearchaliciousEvents} from './enums'; import {SearchResultEvent} from './events'; +import {SearchaliciousResultCtlMixin} from './search-results-ctl'; /** * A component to display pagination for search results. */ @customElement('searchalicious-pages') -export class SearchaliciousPages extends EventRegistrationMixin(LitElement) { +export class SearchaliciousPages extends SearchaliciousResultCtlMixin( + LitElement +) { static override styles = css` ul { list-style-type: none; @@ -25,13 +27,6 @@ export class SearchaliciousPages extends EventRegistrationMixin(LitElement) { } `; - /** - * the search we display result for, - * this corresponds to `name` attribute of corresponding search-bar - */ - @property({attribute: 'search-name'}) - searchName = 'searchalicious'; - /** * Wether or not we should add the jump to first shortcuts */ @@ -50,12 +45,6 @@ export class SearchaliciousPages extends EventRegistrationMixin(LitElement) { @property({attribute: 'displayed-pages', type: Number}) displayedPages = 5; - /** - * Do we have an already launched search - */ - @state() - searchLaunched = false; - /** * Number of pages in current search */ @@ -97,18 +86,16 @@ export class SearchaliciousPages extends EventRegistrationMixin(LitElement) { /** * Update state on search received */ - _updatePages(event: Event) { - const detail = (event as SearchResultEvent).detail; - if (detail.searchName === this.searchName) { - this.searchLaunched = true; - this._pageCount = detail.pageCount; - this._currentPage = detail.currentPage; - [this._startRange, this._endRange] = this._computeRange( - this._currentPage, - this._pageCount - ); - this.requestUpdate(); - } + override handleResults(event: SearchResultEvent) { + const detail = event.detail; + this.searchLaunched = true; + this._pageCount = detail.pageCount; + this._currentPage = detail.currentPage; + [this._startRange, this._endRange] = this._computeRange( + this._currentPage, + this._pageCount + ); + this.requestUpdate(); } /** @@ -253,23 +240,6 @@ export class SearchaliciousPages extends EventRegistrationMixin(LitElement) { ); } } - - /** - * Connect search event handlers. - */ - override connectedCallback() { - super.connectedCallback(); - this.addEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => - this._updatePages(event) - ); - } - // connect to our specific events - override disconnectedCallback() { - super.disconnectedCallback(); - this.removeEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => - this._updatePages(event) - ); - } } declare global { diff --git a/frontend/src/search-results-ctl.ts b/frontend/src/search-results-ctl.ts new file mode 100644 index 00000000..3ff217d2 --- /dev/null +++ b/frontend/src/search-results-ctl.ts @@ -0,0 +1,91 @@ +import {LitElement} from 'lit'; +import {property, state} from 'lit/decorators.js'; + +import { + EventRegistrationInterface, + EventRegistrationMixin, +} from './event-listener-setup'; +import {SearchaliciousEvents} from './enums'; +import {SearchResultEvent} from './events'; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type Constructor = new (...args: any[]) => T; + +export interface SearchaliciousResultsCtlInterface + extends EventRegistrationInterface { + searchName: string; + searchLaunched: Boolean; + + // sub class must override this one to dislpay results + handleResults(event: SearchResultEvent): void; + // this is the registered handler + _handleResults(event: Event): void; +} + +/** + * Common Mixin for elements that react upon a search result + * It can be to display results, facets or pagination. + */ +export const SearchaliciousResultCtlMixin = >( + superClass: T +) => { + /** + * The search mixin, encapsulate the logic of dialog with server + */ + class SearchaliciousResultCtlMixinClass extends EventRegistrationMixin( + superClass + ) { + /** + * the search we display result for, + * this corresponds to `name` attribute of corresponding search-bar + */ + @property({attribute: 'search-name'}) + searchName = 'searchalicious'; + + /** + * this will be true if we already launched a search + */ + @state() + searchLaunched = false; + + handleResults(_: SearchResultEvent) { + throw Error( + 'You must provide a handleResults function in you implementation' + ); + } + + /** + * event handler for NEW_RESULT events + */ + _handleResults(event: Event) { + // check if event is for our search + const resultEvent = event as SearchResultEvent; + const detail = resultEvent.detail; + if (detail.searchName === this.searchName) { + this.searchLaunched = true; + // call the real method + this.handleResults(resultEvent); + } + } + + /** + * Connect search event handlers. + */ + override connectedCallback() { + super.connectedCallback(); + this.addEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => + this._handleResults(event) + ); + } + // connect to our specific events + override disconnectedCallback() { + super.disconnectedCallback(); + this.removeEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => + this._handleResults(event) + ); + } + } + + return SearchaliciousResultCtlMixinClass as Constructor & + T; +}; diff --git a/frontend/src/search-results.ts b/frontend/src/search-results.ts index 48959aa0..79e09c71 100644 --- a/frontend/src/search-results.ts +++ b/frontend/src/search-results.ts @@ -2,9 +2,8 @@ import {LitElement, html} from 'lit'; import {customElement, property, state} from 'lit/decorators.js'; import {repeat} from 'lit/directives/repeat.js'; -import {EventRegistrationMixin} from './event-listener-setup'; +import {SearchaliciousResultCtlMixin} from './search-results-ctl'; import {SearchResultEvent} from './events'; -import {SearchaliciousEvents} from './enums'; import { MissingResultTemplateError, MultipleResultTemplateError, @@ -22,18 +21,9 @@ type htmlType = typeof html; * It reacts to the `searchalicious-result` event fired by the search controller. */ @customElement('searchalicious-results') -export class SearchaliciousResults extends EventRegistrationMixin(LitElement) { - /** - * the search we display result for, - * this corresponds to `name` attribute of corresponding search-bar - */ - @property({attribute: 'search-name'}) - searchName = 'searchalicious'; - - // did we already launched a search - @state() - searchLaunched = false; - +export class SearchaliciousResults extends SearchaliciousResultCtlMixin( + LitElement +) { // the last search results @state() // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -143,18 +133,12 @@ export class SearchaliciousResults extends EventRegistrationMixin(LitElement) { /** * event handler for NEW_RESULT events */ - _displayResults(event: Event) { - // check if event is for our search - const detail = (event as SearchResultEvent).detail; - if (detail.searchName === this.searchName) { - this.searchLaunched = true; - this.results = detail.results; // it's reactive, should trigger rendering - } + override handleResults(event: SearchResultEvent) { + this.results = event.detail.results; // it's reactive, should trigger rendering } /** * Create our resultRenderer, using last opportunity to read innerHTML before shadowRoot creation. - * Also connect search event handlers. */ override connectedCallback() { // we do this before calling super, to be sure we still have our innerHTML intact @@ -162,16 +146,6 @@ export class SearchaliciousResults extends EventRegistrationMixin(LitElement) { // as _buildResultRenderer needs the slot named result to already exists this.resultRenderer = this._buildResultRenderer(); super.connectedCallback(); - this.addEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => - this._displayResults(event) - ); - } - // connect to our specific events - override disconnectedCallback() { - super.disconnectedCallback(); - this.removeEventHandler(SearchaliciousEvents.NEW_RESULT, (event) => - this._displayResults(event) - ); } } diff --git a/frontend/src/test/search-pages_test.ts b/frontend/src/test/search-pages_test.ts index 120ec565..1a81f590 100644 --- a/frontend/src/test/search-pages_test.ts +++ b/frontend/src/test/search-pages_test.ts @@ -19,8 +19,9 @@ suite('searchalicious-pages', () => { pageCount: pageCount, currentPage: currentPage, pageSize: 10, + aggregations: {}, }; - el._updatePages( + el._handleResults( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { bubbles: true, composed: true, diff --git a/frontend/src/test/search-results_test.ts b/frontend/src/test/search-results_test.ts index 93e5a303..102ab8a5 100644 --- a/frontend/src/test/search-results_test.ts +++ b/frontend/src/test/search-results_test.ts @@ -22,8 +22,9 @@ suite('searchalicious-results', () => { pageCount: 1, currentPage: 1, pageSize: 10, + aggregations: {}, }; - el._displayResults( + el._handleResults( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { bubbles: true, composed: true, From 46384e250733b7a6b2b68a65664935ad1810567d Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Wed, 24 Apr 2024 20:29:41 +0200 Subject: [PATCH 02/12] feat: wip on facets api --- Makefile | 2 +- app/config.py | 162 ++++++++++++++++++++++++++++++++- app/facets.py | 72 +++++++++++++++ data/config/openfoodfacts.yml | 10 +++ tests/unit/test_config.py | 164 ++++++++++++++++++++++++++++++++++ 5 files changed, 407 insertions(+), 3 deletions(-) create mode 100644 app/facets.py create mode 100644 tests/unit/test_config.py diff --git a/Makefile b/Makefile index 256f79cd..bdec6eb9 100644 --- a/Makefile +++ b/Makefile @@ -114,7 +114,7 @@ test: _ensure_network test_api test_front test_api: @echo "🔎 Running API tests..." - ${DOCKER_COMPOSE_TEST} run --rm api pytest tests/ + ${DOCKER_COMPOSE_TEST} run --rm api pytest ${args} tests/ test_front: @echo "🔎 Running front-end tests..." diff --git a/app/config.py b/app/config.py index f98a490e..52f5d183 100644 --- a/app/config.py +++ b/app/config.py @@ -1,13 +1,16 @@ import json +import logging from enum import StrEnum, auto from pathlib import Path -from typing import Annotated +from typing import Annotated, Optional import yaml from pydantic import BaseModel, Field, HttpUrl, field_validator, model_validator from pydantic.json_schema import GenerateJsonSchema from pydantic_settings import BaseSettings +log = logging.getLogger(__name__) + class LoggingLevel(StrEnum): NOTSET = "NOTSET" @@ -232,6 +235,12 @@ class ESIndexConfig(BaseModel): class TaxonomyIndexConfig(BaseModel): + """We have an index storing multiple taxonomies + + It enables functions like auto-completion, or field suggestions + as well as enrichment of requests with synonyms + """ + name: Annotated[ str, Field(description="name of the taxonomy index alias to use"), @@ -245,6 +254,12 @@ class TaxonomyIndexConfig(BaseModel): class TaxonomyConfig(BaseModel): + """Configuration of taxonomies, + that is collections of entries with synonyms in multiple languages + + Field may be linked to taxonomies. + """ + sources: Annotated[ list[TaxonomySourceConfig], Field(description="configurations of used taxonomies"), @@ -256,7 +271,7 @@ class TaxonomyConfig(BaseModel): "to be always exported during indexing. During indexing, we use the taxonomy " "to translate every taxonomized field in a language-specific subfield. The list " "of language depends on the value defined here and on the optional " - "`taxonomy_langs` field that can be defined in each document." + "`taxonomy_langs` field that can be defined in each document.", ), ] index: Annotated[ @@ -267,7 +282,71 @@ class TaxonomyConfig(BaseModel): ] +DEFAULT_FACET_NAME = "default" +FACET_ORDER_ELLIPSIS = "..." + + +class FacetsConfig(BaseModel): + """configure a set of facets to be displayed to users""" + + name: Annotated[ + str, + Field( + description="the name of the facet, used to identify it. You must have a facet named default" + ), + ] = "" + include: Annotated[ + Optional[list[str]], + Field( + description="a list of facets to include in the response. " + "If not provided, all facets will be returned but excluded." + "It must be a field with `bucket_agg` true." + ), + ] = None + exclude: Annotated[ + Optional[list[str]], + Field( + description="a list of facets to exclude. It is mutually exclusive with `include`" + ), + ] = None + order: Annotated[ + Optional[list[str]], + Field( + description="the order in which facets should be displayed. " + "If you have one ... entry all non listed entries will be added here in alpha order " + "otherwise they will be appended." + ), + ] = None + + def get_facets_order(self, index_config: "IndexConfig") -> list[str]: + """return a list to facets in right order. + + You usually call the method with same name on IndexConfig + + :param index_config: we need the index config facets belongs to, + to get the field configs + """ + facets_order = list(self.order or []) + fields = set(self.include or []) or ( + set(index_config.get_fields_with_bucket_agg()) - set(self.exclude or []) + ) + non_listed_facets = sorted(fields - set(facets_order)) + try: + position_for_non_listed = facets_order.index(FACET_ORDER_ELLIPSIS) + facets_order[position_for_non_listed : position_for_non_listed + 1] = ( + non_listed_facets + ) + except ValueError: + facets_order.extend(non_listed_facets) + return facets_order + + class IndexConfig(BaseModel): + """Inside the config file we can have several indexes defined. + + This object gives configuration for one index. + """ + index: Annotated[ ESIndexConfig, Field(description="configuration of the Elasticsearch index") ] @@ -278,6 +357,15 @@ class IndexConfig(BaseModel): "names and values contain the field configuration" ), ] + facets: Annotated[ + dict[str, FacetsConfig], + Field( + description="configuration for facets to be displayed to users. " + "You should have a facet named 'default' that will be used as the default facet configuration." + "If don't define one, a default facet will be created with all fields included in alpha order.", + default_factory=dict, + ), + ] split_separator: Annotated[ str, Field( @@ -386,6 +474,54 @@ def field_references_must_exist_and_be_valid(self): return self + @model_validator(mode="after") + def add_default_facets_if_none_exists(self): + """Validator that adds a default facet configuration + if none is defined.""" + if DEFAULT_FACET_NAME not in self.facets: + # create default config + default_config = FacetsConfig( + name=DEFAULT_FACET_NAME, + ) + self.facets[DEFAULT_FACET_NAME] = default_config + return self + + @model_validator(mode="after") + def ensure_all_facets_fields_are_agg(self): + errors, warnings = [], [] + for facet_name, facet in self.facets.items(): + for field_name in facet.include or []: + if field_name not in self.fields: + errors.append( + f"Unknown field name in facet include: {field_name} in _{facet_name}" + ) + elif not self.fields[field_name].bucket_agg: + errors.append( + f"Non aggregation field name in facet include: {field_name} in _{facet_name}" + ) + for field_name in facet.exclude or []: + if field_name not in self.fields: + warnings.append( + f"Unknown field name in facet exclude: {field_name} in _{facet_name}" + ) + elif not self.fields[field_name].bucket_agg: + warnings.append( + f"Non aggregation field name in facet exclude: {field_name} in _{facet_name}" + ) + field_for_order = set(facet.include or []) or ( + set(self.fields) - set(facet.exclude or []) + ) + for field_name in facet.order or []: + if field_name not in field_for_order: + warnings.append( + f"Field {field_name} is listed in `order` but not present in facet" + ) + if warnings: + log.warning("\n".join(warnings)) + if errors: + raise ValueError("\n".join(errors)) + return self + @field_validator("fields") @classmethod def add_field_name_to_each_field(cls, fields: dict[str, FieldConfig]): @@ -393,6 +529,14 @@ def add_field_name_to_each_field(cls, fields: dict[str, FieldConfig]): field_item.name = field_name return fields + @field_validator("facets") + @classmethod + def add_facet_name_to_each_facet(cls, facets: Optional[dict[str, FacetsConfig]]): + if facets: + for facet_name, facet_item in facets.items(): + facet_item.name = facet_name + return facets + def get_supported_langs(self) -> set[str]: """Return the set of supported languages for `text_lang` fields. @@ -413,8 +557,22 @@ def get_taxonomy_langs(self) -> set[str]: # langs will be stored in a unique `other` subfield return (set(self.taxonomy.exported_langs)) & set(ANALYZER_LANG_MAPPING) + def get_fields_with_bucket_agg(self): + return [ + field_name for field_name, field in self.fields.items() if field.bucket_agg + ] + + def get_facets_order(self, facets_name=DEFAULT_FACET_NAME) -> list[str]: + return self.facets[facets_name].get_facets_order(self) + class Config(BaseModel): + """This is the global config object that reflects + the yaml configuration file. + + Validations will be performed while we load it. + """ + indices: dict[str, IndexConfig] = Field( description="configuration of indices. " "The key is the ID of the index that can be referenced at query time. " diff --git a/app/facets.py b/app/facets.py new file mode 100644 index 00000000..a8a7dfbc --- /dev/null +++ b/app/facets.py @@ -0,0 +1,72 @@ +""" +A module to help building facets from aggregations +""" + +from typing import Any + +from . import config +from ._types import SearchResponse + + +def build_facets( + search_result: SearchResponse, + index: str | None = None, + facet_name: str = config.DEFAULT_FACET_NAME, +): + """Given a search result with aggregations, + build a list of facets for API response""" + facets: list[dict[str, Any]] = [] + if config.CONFIG is None: + return facets + _, index_config = config.CONFIG.get_index_config(index) + aggregations = search_result.aggregations + facet_fields = index_config.get_facets_order() + for field_name in facet_fields: + if field_name not in aggregations: + pass + agg_data = aggregations[field_name] + agg_buckets = agg_data.get("buckets", []) + # best values + facet_items = [ + { + "key": bucket["key"], + # TODO: compute name in target language if there is a taxonomy + "name": bucket["name"], + "count": bucket["doc_count"], + # TODO: add if selected + } + for bucket in agg_buckets + ] + # add other values + if agg_data["sum_other_doc_count"]: + facet_items.append( + { + "key": "--other--", + # TODO: translate in target language ? + "name": "other", + "count": agg_data["sum_other_doc_count"], + } + ) + items_count = sum(item["count"] for item in facet_items) + # and no values + if (not search_result.is_count_exact) and search_result.count > items_count: + facet_items.append( + { + "key": "--none--", + # Note: this depends on search_result.is_count_exact, + # but we leave it to user to verify + "count": search_result.count - items_count, + } + ) + # append our facet information + facets.append( + { + "key": field_name, + # FIXME translate + "name": field_name, + "items": facet_items, + "count_error_margin": aggregations["doc_count_error_upper_bound"], + } + ) + + return facets diff --git a/data/config/openfoodfacts.yml b/data/config/openfoodfacts.yml index a7eeccb9..41cc8f25 100644 --- a/data/config/openfoodfacts.yml +++ b/data/config/openfoodfacts.yml @@ -123,6 +123,16 @@ indices: type: integer completeness: type: float + facets: + # include all fields + default: + order: + - categories_tags + - brands_tags + - labels_tags + - ecoscore_grade + - nova_groups + - ecoscore_grade document_denylist: - '8901552007122' lang_separator: _ diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py new file mode 100644 index 00000000..cbe4574f --- /dev/null +++ b/tests/unit/test_config.py @@ -0,0 +1,164 @@ +import app.config + +BASE_CONFIG = """ +indices: + test: + index: + number_of_replicas: 1 + number_of_shards: 1 + name: test_index + id_field_name: mykey + last_modified_field_name: last_modified + fields: + mykey: + required: true + type: keyword + mybool: + required: true + type: bool + last_modified: + required: true + type: date + mytext: + full_text_search: true + type: text_lang + myagg: + required: true + type: keyword + bucket_agg: true + # more fields + # facets + supported_langs: ["en", "fr"] + taxonomy: + sources: [] + exported_langs: [] + index: + number_of_replicas: 1 + number_of_shards: 1 + name: test_taxonomy + document_fetcher: app._import.BaseDocumentFetcher +default_index: test +""" +AGGS_FIELDS = """ + other_agg: + type: keyword + bucket_agg: true + more_agg: + type: keyword + bucket_agg: true + somuch_agg: + type: keyword + bucket_agg: true +""" + + +def _config_with_aggs(tmpdir, facets=""): + my_config = tmpdir / "config.yaml" + conf_content = BASE_CONFIG.replace(" # more fields\n", AGGS_FIELDS) + conf_content = conf_content.replace(" # facets\n", facets + "\n") + open(my_config, "w").write(conf_content) + return my_config + + +def test_facets_default_created(tmpdir): + conf_file = _config_with_aggs(tmpdir) + conf = app.config.Config.from_yaml(conf_file) + assert conf.indices["test"].facets == { + "default": app.config.FacetsConfig( + name="default", include=None, exclude=None, order=None + ) + } + + +def test_facets_with_include(tmpdir): + facets = """ + facets: + default: + include: ["somuch_agg", "myagg", "more_agg"] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + # only included, alpha sort + assert conf.indices["test"].get_facets_order() == [ + "more_agg", + "myagg", + "somuch_agg", + ] + + +def test_facets_with_exclude(tmpdir): + facets = """ + facets: + default: + exclude: ["myagg", "more_agg"] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + # without excluded, alpha sort + assert conf.indices["test"].get_facets_order() == ["other_agg", "somuch_agg"] + + +def test_facets_order_no_ellipsis(tmpdir): + facets = """ + facets: + default: + order: ["myagg", "somuch_agg"] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + # at end + assert conf.indices["test"].get_facets_order() == [ + "myagg", + "somuch_agg", + "more_agg", + "other_agg", + ] + + +def test_facets_order_with_end_ellipsis(tmpdir): + facets = """ + facets: + default: + order: ["myagg", "somuch_agg", "..."] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + # at end + assert conf.indices["test"].get_facets_order() == [ + "myagg", + "somuch_agg", + "more_agg", + "other_agg", + ] + + +def test_facets_order_with_ellipsis(tmpdir): + facets = """ + facets: + default: + order: ["myagg", "...", "more_agg"] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + assert conf.indices["test"].get_facets_order() == [ + "myagg", + "other_agg", + "somuch_agg", + "more_agg", + ] + + +def test_facets_order_with_start_ellipsis(tmpdir): + facets = """ + facets: + default: + order: ["...", "myagg", "more_agg"] + """ + conf_file = _config_with_aggs(tmpdir, facets) + conf = app.config.Config.from_yaml(conf_file) + assert conf.indices["test"].get_facets_order() == [ + "other_agg", + "somuch_agg", + "myagg", + "more_agg", + ] From 0b34aabc808bd7f36a7c978f5b5f0c9d3bd321bf Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Wed, 24 Apr 2024 20:36:36 +0200 Subject: [PATCH 03/12] feat: wip on facets api --- app/facets.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/app/facets.py b/app/facets.py index a8a7dfbc..8e87cab6 100644 --- a/app/facets.py +++ b/app/facets.py @@ -8,6 +8,18 @@ from ._types import SearchResponse +def safe_get_index_config( + index: str | None = None, configuration: config.Config | None = None +) -> config.IndexConfig | None: + """Safely get config""" + if configuration is None: + configuration = config.CONFIG + if configuration is None: + return None + _, index_config = configuration.get_index_config(index) + return index_config + + def build_facets( search_result: SearchResponse, index: str | None = None, @@ -16,11 +28,11 @@ def build_facets( """Given a search result with aggregations, build a list of facets for API response""" facets: list[dict[str, Any]] = [] - if config.CONFIG is None: - return facets - _, index_config = config.CONFIG.get_index_config(index) aggregations = search_result.aggregations - facet_fields = index_config.get_facets_order() + index_config = safe_get_index_config(index=index) + if index_config is None: + return facets + facet_fields = index_config.get_facets_order(facet_name) for field_name in facet_fields: if field_name not in aggregations: pass From 2dfe7c4852257c813ba26d4875b225177f138485 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 15:42:45 +0200 Subject: [PATCH 04/12] feat: add basic facets in API --- README.md | 10 +++ app/_types.py | 26 +++++++- app/api.py | 94 +++++++++------------------ app/config.py | 129 +------------------------------------- app/facets.py | 87 ++++++++++++++----------- app/query.py | 21 ++++--- app/search.py | 86 +++++++++++++++++++++++++ docker/dev.yml | 2 +- tests/unit/test_config.py | 106 ++----------------------------- 9 files changed, 222 insertions(+), 339 deletions(-) create mode 100644 app/search.py diff --git a/README.md b/README.md index 7512a0d3..c1687bae 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,16 @@ uvicorn app.api:app --reload --port=8001 --workers=4 ``` Note that it's important to use port 8001, as port 8000 will be used by the docker version of the search service. + + +To debug the backend app: +* stop api instance: `docker compose stop api` +* add a pdb.set_trace() at the point you want, +* then launch `docker compose run --rm --use-aliases api uvicorn app.api:app --proxy-headers --host 0.0.0.0 --port 8000 --reload`[^use_aliases] + +[^use_aliases]: the `--use-aliases` make it so that this container is reachable as "api" for the other containers in the compose + + ### Pre-Commit This repo uses [pre-commit](https://pre-commit.com/) to enforce code styling, etc. To use it: diff --git a/app/_types.py b/app/_types.py index 54ad45b5..b3cc9f89 100644 --- a/app/_types.py +++ b/app/_types.py @@ -7,6 +7,29 @@ JSONType = dict[str, Any] +class FacetItem(BaseModel): + """Describe an entry of a facet""" + + key: str + name: str + count: int + """The number of elements for this value""" + + +class FacetInfo(BaseModel): + """Search result for facet""" + + name: str + """The display name of the facet""" + items: list[FacetItem] + """Items in the facets""" + count_error_margin: int | None = None + + +FacetsInfos = dict[str, FacetInfo] +"""Information about facets for a search result""" + + class SearchResponseDebug(BaseModel): query: JSONType @@ -26,7 +49,8 @@ def is_success(self): class SuccessSearchResponse(BaseModel): hits: list[JSONType] - aggregations: JSONType + aggregations: JSONType | None = None + facets: FacetsInfos | None = None page: int page_size: int page_count: int diff --git a/app/api.py b/app/api.py index 33a57c2d..02d7de2a 100644 --- a/app/api.py +++ b/app/api.py @@ -8,44 +8,18 @@ from fastapi.responses import HTMLResponse, PlainTextResponse from fastapi.templating import Jinja2Templates +import app.search as app_search from app import config from app._types import SearchResponse, SuccessSearchResponse from app.config import check_config_is_defined, settings -from app.postprocessing import ( - BaseResultProcessor, - load_result_processor, - process_taxonomy_completion_response, -) -from app.query import ( - build_completion_query, - build_elasticsearch_query_builder, - build_search_query, - execute_query, -) +from app.facets import check_all_facets_fields_are_agg +from app.postprocessing import process_taxonomy_completion_response +from app.query import build_completion_query from app.utils import connection, get_logger, init_sentry logger = get_logger() -if config.CONFIG is None: - # We want to be able to import api.py (for tests for example) without - # failure, but we add a warning message as it's not expected in a - # production settings - logger.warning("Main configuration is not set, use CONFIG_PATH envvar") - FILTER_QUERY_BUILDERS = {} - RESULT_PROCESSORS = {} -else: - # we cache query builder and result processor here for faster processing - FILTER_QUERY_BUILDERS = { - index_id: build_elasticsearch_query_builder(index_config) - for index_id, index_config in config.CONFIG.indices.items() - } - RESULT_PROCESSORS = { - index_id: load_result_processor(index_config) - for index_id, index_config in config.CONFIG.indices.items() - } - - app = FastAPI( title="search-a-licious API", contact={ @@ -116,6 +90,13 @@ def get_document( return product +def check_facets_are_valid(index_id: str | None, facets: list[str] | None) -> None: + """Check that the facets are valid.""" + errors = check_all_facets_fields_are_agg(index_id, facets) + if errors: + raise HTTPException(status_code=400, detail=json.dumps(errors)) + + @app.get("/search") def search( q: Annotated[ @@ -130,7 +111,7 @@ def search( filter clause for categories and brands and look for "strawberry" in multiple fields. - The query is optional, but `sort_by` value must then be provided.""" +The query is optional, but `sort_by` value must then be provided.""" ), ] = None, langs: Annotated[ @@ -163,59 +144,44 @@ def search( and be sortable. If it is not provided, results are sorted by descending relevance score.""" ), ] = None, + facets: Annotated[ + str | None, + Query( + description="""Name of facets to return in the response as a comma-separated value. + If None (default) no facets are returned.""" + ), + ] = None, index_id: Annotated[ str | None, INDEX_ID_QUERY_PARAM, ] = None, ) -> SearchResponse: + # check and preprocess parameters check_config_is_defined() global_config = cast(config.Config, config.CONFIG) check_index_id_is_defined(index_id, global_config) - index_id, index_config = global_config.get_index_config(index_id) - result_processor = cast(BaseResultProcessor, RESULT_PROCESSORS[index_id]) + facets_list = facets.split(",") if facets else None + check_facets_are_valid(index_id, facets_list) if q is None and sort_by is None: raise HTTPException( status_code=400, detail="`sort_by` must be provided when `q` is missing" ) - - langs_set = set(langs.split(",") if langs else ["en"]) - logger.debug( - "Received search query: q='%s', langs='%s', page=%d, " - "page_size=%d, fields='%s', sort_by='%s'", - q, - langs_set, - page, - page_size, - fields, - sort_by, - ) - if page * page_size > 10_000: raise HTTPException( status_code=400, detail=f"Maximum number of returned results is 10 000 (here: page * page_size = {page * page_size})", ) - - query = build_search_query( + langs_list = langs.split(",") if langs else ["en"] + # search + return app_search.search( q=q, - langs=langs_set, - size=page_size, + langs=langs_list, + page_size=page_size, page=page, - config=index_config, + fields=fields.split(",") if fields else None, sort_by=sort_by, - # filter query builder is generated from elasticsearch mapping and - # takes ~40ms to generate, build-it before hand to avoid this delay - filter_query_builder=FILTER_QUERY_BUILDERS[index_id], - ) - logger.debug("Elasticsearch query: %s", query.to_dict()) - - projection = set(fields.split(",")) if fields else None - return execute_query( - query, - result_processor, - page=page, - page_size=page_size, - projection=projection, + facets=facets_list, + index_id=index_id, ) diff --git a/app/config.py b/app/config.py index 52f5d183..cb55edeb 100644 --- a/app/config.py +++ b/app/config.py @@ -2,7 +2,7 @@ import logging from enum import StrEnum, auto from pathlib import Path -from typing import Annotated, Optional +from typing import Annotated import yaml from pydantic import BaseModel, Field, HttpUrl, field_validator, model_validator @@ -282,65 +282,6 @@ class TaxonomyConfig(BaseModel): ] -DEFAULT_FACET_NAME = "default" -FACET_ORDER_ELLIPSIS = "..." - - -class FacetsConfig(BaseModel): - """configure a set of facets to be displayed to users""" - - name: Annotated[ - str, - Field( - description="the name of the facet, used to identify it. You must have a facet named default" - ), - ] = "" - include: Annotated[ - Optional[list[str]], - Field( - description="a list of facets to include in the response. " - "If not provided, all facets will be returned but excluded." - "It must be a field with `bucket_agg` true." - ), - ] = None - exclude: Annotated[ - Optional[list[str]], - Field( - description="a list of facets to exclude. It is mutually exclusive with `include`" - ), - ] = None - order: Annotated[ - Optional[list[str]], - Field( - description="the order in which facets should be displayed. " - "If you have one ... entry all non listed entries will be added here in alpha order " - "otherwise they will be appended." - ), - ] = None - - def get_facets_order(self, index_config: "IndexConfig") -> list[str]: - """return a list to facets in right order. - - You usually call the method with same name on IndexConfig - - :param index_config: we need the index config facets belongs to, - to get the field configs - """ - facets_order = list(self.order or []) - fields = set(self.include or []) or ( - set(index_config.get_fields_with_bucket_agg()) - set(self.exclude or []) - ) - non_listed_facets = sorted(fields - set(facets_order)) - try: - position_for_non_listed = facets_order.index(FACET_ORDER_ELLIPSIS) - facets_order[position_for_non_listed : position_for_non_listed + 1] = ( - non_listed_facets - ) - except ValueError: - facets_order.extend(non_listed_facets) - return facets_order - - class IndexConfig(BaseModel): """Inside the config file we can have several indexes defined. @@ -357,15 +298,6 @@ class IndexConfig(BaseModel): "names and values contain the field configuration" ), ] - facets: Annotated[ - dict[str, FacetsConfig], - Field( - description="configuration for facets to be displayed to users. " - "You should have a facet named 'default' that will be used as the default facet configuration." - "If don't define one, a default facet will be created with all fields included in alpha order.", - default_factory=dict, - ), - ] split_separator: Annotated[ str, Field( @@ -474,54 +406,6 @@ def field_references_must_exist_and_be_valid(self): return self - @model_validator(mode="after") - def add_default_facets_if_none_exists(self): - """Validator that adds a default facet configuration - if none is defined.""" - if DEFAULT_FACET_NAME not in self.facets: - # create default config - default_config = FacetsConfig( - name=DEFAULT_FACET_NAME, - ) - self.facets[DEFAULT_FACET_NAME] = default_config - return self - - @model_validator(mode="after") - def ensure_all_facets_fields_are_agg(self): - errors, warnings = [], [] - for facet_name, facet in self.facets.items(): - for field_name in facet.include or []: - if field_name not in self.fields: - errors.append( - f"Unknown field name in facet include: {field_name} in _{facet_name}" - ) - elif not self.fields[field_name].bucket_agg: - errors.append( - f"Non aggregation field name in facet include: {field_name} in _{facet_name}" - ) - for field_name in facet.exclude or []: - if field_name not in self.fields: - warnings.append( - f"Unknown field name in facet exclude: {field_name} in _{facet_name}" - ) - elif not self.fields[field_name].bucket_agg: - warnings.append( - f"Non aggregation field name in facet exclude: {field_name} in _{facet_name}" - ) - field_for_order = set(facet.include or []) or ( - set(self.fields) - set(facet.exclude or []) - ) - for field_name in facet.order or []: - if field_name not in field_for_order: - warnings.append( - f"Field {field_name} is listed in `order` but not present in facet" - ) - if warnings: - log.warning("\n".join(warnings)) - if errors: - raise ValueError("\n".join(errors)) - return self - @field_validator("fields") @classmethod def add_field_name_to_each_field(cls, fields: dict[str, FieldConfig]): @@ -529,14 +413,6 @@ def add_field_name_to_each_field(cls, fields: dict[str, FieldConfig]): field_item.name = field_name return fields - @field_validator("facets") - @classmethod - def add_facet_name_to_each_facet(cls, facets: Optional[dict[str, FacetsConfig]]): - if facets: - for facet_name, facet_item in facets.items(): - facet_item.name = facet_name - return facets - def get_supported_langs(self) -> set[str]: """Return the set of supported languages for `text_lang` fields. @@ -562,9 +438,6 @@ def get_fields_with_bucket_agg(self): field_name for field_name, field in self.fields.items() if field.bucket_agg ] - def get_facets_order(self, facets_name=DEFAULT_FACET_NAME) -> list[str]: - return self.facets[facets_name].get_facets_order(self) - class Config(BaseModel): """This is the global config object that reflects diff --git a/app/facets.py b/app/facets.py index 8e87cab6..68bb679c 100644 --- a/app/facets.py +++ b/app/facets.py @@ -2,10 +2,8 @@ A module to help building facets from aggregations """ -from typing import Any - from . import config -from ._types import SearchResponse +from ._types import FacetInfo, FacetItem, FacetsInfos, SearchResponse def safe_get_index_config( @@ -20,65 +18,82 @@ def safe_get_index_config( return index_config +def check_all_facets_fields_are_agg( + index_id: str | None, facets: list[str] | None +) -> list[str]: + """Check all facets are valid, + that is, correspond to a field with aggregation""" + errors: list[str] = [] + if facets is None: + return errors + config = safe_get_index_config(index_id) + if config is None: + raise ValueError(f"Cannot get index config for index_id {index_id}") + for field_name in facets: + if field_name not in config.fields: + errors.append(f"Unknown field name in facets: {field_name}") + elif not config.fields[field_name].bucket_agg: + errors.append(f"Non aggregation field name in facets: {field_name}") + return errors + + def build_facets( search_result: SearchResponse, - index: str | None = None, - facet_name: str = config.DEFAULT_FACET_NAME, -): + index_config: config.IndexConfig, + facets_names: list[str] | None, +) -> FacetsInfos: """Given a search result with aggregations, - build a list of facets for API response""" - facets: list[dict[str, Any]] = [] + build a list of facets for API response + """ aggregations = search_result.aggregations - index_config = safe_get_index_config(index=index) - if index_config is None: - return facets - facet_fields = index_config.get_facets_order(facet_name) - for field_name in facet_fields: + if facets_names is None or aggregations is None: + return {} + facets: FacetsInfos = {} + for field_name in facets_names: if field_name not in aggregations: pass agg_data = aggregations[field_name] agg_buckets = agg_data.get("buckets", []) # best values facet_items = [ - { - "key": bucket["key"], + FacetItem( + key=bucket["key"], # TODO: compute name in target language if there is a taxonomy - "name": bucket["name"], - "count": bucket["doc_count"], + name=bucket["key"], + count=bucket["doc_count"], # TODO: add if selected - } + ) for bucket in agg_buckets ] # add other values if agg_data["sum_other_doc_count"]: facet_items.append( - { - "key": "--other--", + FacetItem( + key="--other--", # TODO: translate in target language ? - "name": "other", - "count": agg_data["sum_other_doc_count"], - } + name="Other", + count=agg_data["sum_other_doc_count"], + ) ) - items_count = sum(item["count"] for item in facet_items) + items_count = sum(item.count for item in facet_items) # and no values if (not search_result.is_count_exact) and search_result.count > items_count: facet_items.append( - { - "key": "--none--", + FacetItem( + key="--none--", + # TODO: translate in target language ? + name="None", # Note: this depends on search_result.is_count_exact, # but we leave it to user to verify - "count": search_result.count - items_count, - } + count=search_result.count - items_count, + ) ) # append our facet information - facets.append( - { - "key": field_name, - # FIXME translate - "name": field_name, - "items": facet_items, - "count_error_margin": aggregations["doc_count_error_upper_bound"], - } + facets[field_name] = FacetInfo( + # FIXME translate + name=field_name, + items=facet_items, + count_error_margin=agg_data["doc_count_error_upper_bound"], ) return facets diff --git a/app/query.py b/app/query.py index 483e2192..373971db 100644 --- a/app/query.py +++ b/app/query.py @@ -193,14 +193,20 @@ def parse_sort_by_parameter(sort_by: str | None, config: IndexConfig) -> str | N return sort_by -def create_aggregation_clauses(config: IndexConfig) -> dict[str, Agg]: - """Create term bucket aggregation clauses for all relevant fields as - defined in the config. +def create_aggregation_clauses( + config: IndexConfig, facets: list[str] | None +) -> dict[str, Agg]: + """Create term bucket aggregation clauses + for all fields corresponding to facets, + as defined in the config """ clauses = {} - for field in config.fields.values(): - if field.bucket_agg: - clauses[field.name] = A("terms", field=field.name) + if facets is not None: + for field_name in facets: + field = config.fields[field_name] + if field.bucket_agg: + # TODO - aggregation might depend on agg type or field type + clauses[field.name] = A("terms", field=field.name) return clauses @@ -211,6 +217,7 @@ def build_search_query( page: int, config: IndexConfig, filter_query_builder: ElasticsearchQueryBuilder, + facets: list[str] | None = None, sort_by: str | None = None, ) -> Query: """Build an elasticsearch_dsl Query. @@ -244,7 +251,7 @@ def build_search_query( if filter_query: query = query.query("bool", filter=filter_query) - for agg_name, agg in create_aggregation_clauses(config).items(): + for agg_name, agg in create_aggregation_clauses(config, facets).items(): query.aggs.bucket(agg_name, agg) sort_by = parse_sort_by_parameter(sort_by, config) diff --git a/app/search.py b/app/search.py new file mode 100644 index 00000000..143eebbd --- /dev/null +++ b/app/search.py @@ -0,0 +1,86 @@ +import logging +from typing import cast + +from app import config +from app._types import SearchResponse +from app.facets import build_facets +from app.postprocessing import BaseResultProcessor, load_result_processor +from app.query import ( + build_elasticsearch_query_builder, + build_search_query, + execute_query, +) + +logger = logging.getLogger(__name__) + +if config.CONFIG is None: + # We want to be able to import api.py (for tests for example) without + # failure, but we add a warning message as it's not expected in a + # production settings + logger.warning("Main configuration is not set, use CONFIG_PATH envvar") + FILTER_QUERY_BUILDERS = {} + RESULT_PROCESSORS = {} +else: + # we cache query builder and result processor here for faster processing + FILTER_QUERY_BUILDERS = { + index_id: build_elasticsearch_query_builder(index_config) + for index_id, index_config in config.CONFIG.indices.items() + } + RESULT_PROCESSORS = { + index_id: load_result_processor(index_config) + for index_id, index_config in config.CONFIG.indices.items() + } + + +def search( + index_id: str | None, + q: str | None, + sort_by: str | None, + page: int, + page_size: int, + fields: list[str] | None, + langs: list[str], + facets: list[str] | None, +) -> SearchResponse: + """Run a search""" + global_config = cast(config.Config, config.CONFIG) + index_id, index_config = global_config.get_index_config(index_id) + result_processor = cast(BaseResultProcessor, RESULT_PROCESSORS[index_id]) + langs_set = set(langs) + logger.debug( + "Received search query: q='%s', langs='%s', page=%d, " + "page_size=%d, fields='%s', sort_by='%s'", + q, + langs_set, + page, + page_size, + fields, + sort_by, + ) + + query = build_search_query( + q=q, + langs=langs_set, + size=page_size, + page=page, + config=index_config, + sort_by=sort_by, + # filter query builder is generated from elasticsearch mapping and + # takes ~40ms to generate, build-it before hand to avoid this delay + filter_query_builder=FILTER_QUERY_BUILDERS[index_id], + facets=facets, + ) + logger.debug("Elasticsearch query: %s", query.to_dict()) + + projection = set(fields) if fields else None + search_result = execute_query( + query, + result_processor, + page=page, + page_size=page_size, + projection=projection, + ) + search_result.facets = build_facets(search_result, index_config, facets) + # remove aggregations to avoid sending too much information + search_result.aggregations = None + return search_result diff --git a/docker/dev.yml b/docker/dev.yml index e1c63faf..4086827c 100644 --- a/docker/dev.yml +++ b/docker/dev.yml @@ -26,7 +26,7 @@ services: api: <<: *api-base # uvicorn in reload mode - command: ["uvicorn", "app.api:app", "--proxy-headers", "--host", "0.0.0.0", "--port", "8000", "--reload"] + command: ["uvicorn", "app.api:app", "--proxy-headers", "--host", "0.0.0.0", "--port", "8000", "--reload", "--debug"] updater: <<: *api-base search_frontend: diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index cbe4574f..07436d23 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -27,7 +27,6 @@ type: keyword bucket_agg: true # more fields - # facets supported_langs: ["en", "fr"] taxonomy: sources: [] @@ -60,105 +59,8 @@ def _config_with_aggs(tmpdir, facets=""): return my_config -def test_facets_default_created(tmpdir): +def test_loading_config(tmpdir): conf_file = _config_with_aggs(tmpdir) - conf = app.config.Config.from_yaml(conf_file) - assert conf.indices["test"].facets == { - "default": app.config.FacetsConfig( - name="default", include=None, exclude=None, order=None - ) - } - - -def test_facets_with_include(tmpdir): - facets = """ - facets: - default: - include: ["somuch_agg", "myagg", "more_agg"] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - # only included, alpha sort - assert conf.indices["test"].get_facets_order() == [ - "more_agg", - "myagg", - "somuch_agg", - ] - - -def test_facets_with_exclude(tmpdir): - facets = """ - facets: - default: - exclude: ["myagg", "more_agg"] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - # without excluded, alpha sort - assert conf.indices["test"].get_facets_order() == ["other_agg", "somuch_agg"] - - -def test_facets_order_no_ellipsis(tmpdir): - facets = """ - facets: - default: - order: ["myagg", "somuch_agg"] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - # at end - assert conf.indices["test"].get_facets_order() == [ - "myagg", - "somuch_agg", - "more_agg", - "other_agg", - ] - - -def test_facets_order_with_end_ellipsis(tmpdir): - facets = """ - facets: - default: - order: ["myagg", "somuch_agg", "..."] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - # at end - assert conf.indices["test"].get_facets_order() == [ - "myagg", - "somuch_agg", - "more_agg", - "other_agg", - ] - - -def test_facets_order_with_ellipsis(tmpdir): - facets = """ - facets: - default: - order: ["myagg", "...", "more_agg"] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - assert conf.indices["test"].get_facets_order() == [ - "myagg", - "other_agg", - "somuch_agg", - "more_agg", - ] - - -def test_facets_order_with_start_ellipsis(tmpdir): - facets = """ - facets: - default: - order: ["...", "myagg", "more_agg"] - """ - conf_file = _config_with_aggs(tmpdir, facets) - conf = app.config.Config.from_yaml(conf_file) - assert conf.indices["test"].get_facets_order() == [ - "other_agg", - "somuch_agg", - "myagg", - "more_agg", - ] + # just test it loads for now + app.config.Config.from_yaml(conf_file) + # TODO add asserts on config From 55d609e26a639b525ac9887670c779046e9e0d9c Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 18:15:11 +0200 Subject: [PATCH 05/12] feat: facets component with display --- docker/dev.yml | 2 +- frontend/public/off.html | 5 +- frontend/src/events.ts | 2 +- frontend/src/search-ctl.ts | 27 ++++- frontend/src/search-facets.ts | 134 ++++++++++++++++------- frontend/src/test/search-pages_test.ts | 2 +- frontend/src/test/search-results_test.ts | 2 +- 7 files changed, 129 insertions(+), 45 deletions(-) diff --git a/docker/dev.yml b/docker/dev.yml index 4086827c..e1c63faf 100644 --- a/docker/dev.yml +++ b/docker/dev.yml @@ -26,7 +26,7 @@ services: api: <<: *api-base # uvicorn in reload mode - command: ["uvicorn", "app.api:app", "--proxy-headers", "--host", "0.0.0.0", "--port", "8000", "--reload", "--debug"] + command: ["uvicorn", "app.api:app", "--proxy-headers", "--host", "0.0.0.0", "--port", "8000", "--reload"] updater: <<: *api-base search_frontend: diff --git a/frontend/public/off.html b/frontend/public/off.html index d30a74d8..59d778d1 100644 --- a/frontend/public/off.html +++ b/frontend/public/off.html @@ -251,7 +251,10 @@
- + + + +
diff --git a/frontend/src/events.ts b/frontend/src/events.ts index 847e59d8..0cc7a208 100644 --- a/frontend/src/events.ts +++ b/frontend/src/events.ts @@ -14,7 +14,7 @@ export interface SearchResultDetail extends BaseSearchDetail { pageCount: number; pageSize: number; currentPage: number; - aggregations: Object; // FIXME: we could be more precise + facets: Object; // FIXME: we could be more precise } /** diff --git a/frontend/src/search-ctl.ts b/frontend/src/search-ctl.ts index fbe5d078..41aadb4f 100644 --- a/frontend/src/search-ctl.ts +++ b/frontend/src/search-ctl.ts @@ -11,6 +11,9 @@ import { SearchResultEvent, SearchResultDetail, } from './events'; +import {SearchaliciousFacets} from './search-facets'; + +import uniq from 'lodash-es/uniq'; // eslint-disable-next-line @typescript-eslint/no-explicit-any type Constructor = new (...args: any[]) => T; @@ -96,7 +99,22 @@ export const SearchaliciousSearchMixin = >( @state() _count?: number; - // TODO: this should be on results element instead + /** + * Get the list of facets we want to request + */ + _facets(): string[] { + // search facets elements, we can't filter on search-name because of default value… + const facetsElements = document.querySelectorAll('searchalicious-facets'); + const allFacets: string[] = []; + facetsElements.forEach((item) => { + const facetElement = item as SearchaliciousFacets; + if (facetElement.searchName == this.name) { + allFacets.push(...facetElement.getFacetsNames()); + } + }); + return uniq(allFacets) as string[]; + } + _searchUrl(page?: number) { // remove trailing slash const baseUrl = this.baseUrl.replace(/\/+$/, ''); @@ -107,6 +125,7 @@ export const SearchaliciousSearchMixin = >( page_size: string; page?: string; index?: string; + facets?: string; } = { q: this.query, langs: this.langs, @@ -116,6 +135,9 @@ export const SearchaliciousSearchMixin = >( if (page) { params.page = page.toString(); } + if (this._facets()) { + params.facets = this._facets().join(','); + } const queryStr = Object.entries(params) .filter( ([_, value]) => value != null // null or undefined @@ -176,6 +198,7 @@ export const SearchaliciousSearchMixin = >( this.search(detail.page); } } + /** * Launching search */ @@ -196,7 +219,7 @@ export const SearchaliciousSearchMixin = >( pageCount: this._pageCount!, currentPage: this._currentPage!, pageSize: this.pageSize, - aggregations: data.aggregations, + facets: data.facets, }; this.dispatchEvent( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { diff --git a/frontend/src/search-facets.ts b/frontend/src/search-facets.ts index e899742f..77678c1d 100644 --- a/frontend/src/search-facets.ts +++ b/frontend/src/search-facets.ts @@ -1,27 +1,31 @@ -import {LitElement, html} from 'lit'; -import {customElement, property} from 'lit/decorators.js'; +import {LitElement, html, css} from 'lit'; +import {customElement, property, queryAssignedNodes} from 'lit/decorators.js'; import {repeat} from 'lit/directives/repeat.js'; import {SearchaliciousResultCtlMixin} from './search-results-ctl'; import {SearchResultEvent} from './events'; -import keys from 'lodash-es/keys'; -type Aggregations = Record; +interface FacetsInfos { + [key: string]: FacetInfo; +} -interface FilterData { - buckets: Array; +interface FacetInfo { + name: string; + // TODO: add other types if needed + items: FacetItem[]; } -interface Bucket { +interface FacetItem { key: string; + name: string; } -interface DocCountBucket extends Bucket { - doc_count: number; +interface FacetTerm extends FacetItem { + count: number; } /** - * Component to display search filters (aka facets) on in a column + * Parent Component to display a side search filter (aka facets) */ @customElement('searchalicious-facets') export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( @@ -30,54 +34,108 @@ export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( // the last search facets @property({attribute: false}) // eslint-disable-next-line @typescript-eslint/no-explicit-any - aggregations?: Aggregations; + facets?: FacetsInfos; + + @queryAssignedNodes({flatten: true}) + facetNodes!: Array; + + /** + * Names of facets we need to query, + * this is the names of contained facets + */ + getFacetsNames(): string[] { + const _facets: string[] = []; + this.facetNodes.forEach((node) => { + if (node instanceof SearchaliciousFacet) { + _facets.push(node.name); + } + }); + return _facets; + } override handleResults(event: SearchResultEvent) { - this.aggregations = event.detail.aggregations as Aggregations; + this.facets = event.detail.facets as FacetsInfos; + if (this.facets) { + // dispatch to children + this.facetNodes.forEach((node) => { + if (node instanceof SearchaliciousFacet) { + node.infos = this.facets![node.name]; + } + }); + } + } + + override render() { + // we always want to render slot, baceauso we use queryAssignedNodes + // but we may not want to display them + const display = this.facets ? '' : 'display: none'; + return html`
`; } +} + +/** + * Base Component to display a side search filter (aka facets) + * + * This is a base class, implementations are specific based on facet type + */ +export class SearchaliciousFacet extends LitElement { + @property() + name = ''; - renderBucket(bucket: Bucket) { - const dcBucket = bucket as DocCountBucket; + // the last search infor for my facet + @property({attribute: false}) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + infos?: FacetInfo; + + renderFacet() { + throw new Error('renderFacet not implemented: implement in sub class'); + } + + override render() { + if (this.infos) { + return this.renderFacet(); + } else { + return html``; // FIXME: is this ok ? + } + } +} + +@customElement('searchalicious-facet-terms') +export class SearchaliciousTermsFacet extends SearchaliciousFacet { + static override styles = css` + .term-wrapper { + display: block; + } + `; + renderTerm(term: FacetTerm) { return html` - +
+ +
`; } - renderFilter(filterName: string, filterData: FilterData) { + override renderFacet() { return html` -
+
- ${filterName} + ${this.name} ${repeat( - filterData.buckets, - (item: Bucket) => item.key, - (item: Bucket) => this.renderBucket(item) + (this.infos!.items || []) as FacetTerm[], + (item: FacetTerm) => `${item.key}-${item.count}`, + (item: FacetTerm) => this.renderTerm(item) )}
`; } - - override render() { - if (this.aggregations) { - return html`
- ${repeat( - keys(this.aggregations), - (item: string) => item, - (item: string) => this.renderFilter(item, this.aggregations![item]) - )} -
`; - } else { - return html``; // FIXME - } - } } declare global { interface HTMLElementTagNameMap { 'searchalicious-facets': SearchaliciousFacets; + 'searchalicious-facet-terms': SearchaliciousTermsFacet; } } diff --git a/frontend/src/test/search-pages_test.ts b/frontend/src/test/search-pages_test.ts index 1a81f590..09dd784a 100644 --- a/frontend/src/test/search-pages_test.ts +++ b/frontend/src/test/search-pages_test.ts @@ -19,7 +19,7 @@ suite('searchalicious-pages', () => { pageCount: pageCount, currentPage: currentPage, pageSize: 10, - aggregations: {}, + facets: {}, }; el._handleResults( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { diff --git a/frontend/src/test/search-results_test.ts b/frontend/src/test/search-results_test.ts index 102ab8a5..8e3ea4a9 100644 --- a/frontend/src/test/search-results_test.ts +++ b/frontend/src/test/search-results_test.ts @@ -22,7 +22,7 @@ suite('searchalicious-results', () => { pageCount: 1, currentPage: 1, pageSize: 10, - aggregations: {}, + facets: {}, }; el._handleResults( new CustomEvent(SearchaliciousEvents.NEW_RESULT, { From 4b4e0582ecc25c4e8d9f76f0148a963aa0bf1f63 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 19:43:36 +0200 Subject: [PATCH 06/12] feat: facets selection now possible --- frontend/public/off.html | 2 + frontend/src/search-ctl.ts | 49 +++- frontend/src/search-facets.ts | 529 ++++++---------------------------- 3 files changed, 132 insertions(+), 448 deletions(-) diff --git a/frontend/public/off.html b/frontend/public/off.html index 59d778d1..ffe409f6 100644 --- a/frontend/public/off.html +++ b/frontend/public/off.html @@ -254,6 +254,8 @@ + +
diff --git a/frontend/src/search-ctl.ts b/frontend/src/search-ctl.ts index 41aadb4f..6808ff98 100644 --- a/frontend/src/search-ctl.ts +++ b/frontend/src/search-ctl.ts @@ -100,24 +100,56 @@ export const SearchaliciousSearchMixin = >( _count?: number; /** - * Get the list of facets we want to request + * @returns all searchalicious-facets elements linked to this search ctl */ - _facets(): string[] { + _facetsNodes(): SearchaliciousFacets[] { + const allNodes: SearchaliciousFacets[] = []; // search facets elements, we can't filter on search-name because of default value… const facetsElements = document.querySelectorAll('searchalicious-facets'); - const allFacets: string[] = []; facetsElements.forEach((item) => { const facetElement = item as SearchaliciousFacets; if (facetElement.searchName == this.name) { - allFacets.push(...facetElement.getFacetsNames()); + allNodes.push(facetElement); } }); - return uniq(allFacets) as string[]; + return allNodes; + } + + /** + * Get the list of facets we want to request + */ + _facets(): string[] { + return ( + uniq( + this._facetsNodes() + .map((facets) => facets.getFacetsNames()) + .flat() + ) || undefined + ); + } + + /** + * Get the filter linked to facets + * @returns an expression to be added to query + */ + _facetsFilters(): string { + const allFilters: string[] = this._facetsNodes() + .map((facets) => facets.getSearchFilters()) + .flat(); + return allFilters.join(' AND '); } _searchUrl(page?: number) { // remove trailing slash const baseUrl = this.baseUrl.replace(/\/+$/, ''); + const queryParts = []; + if (this.query) { + queryParts.push(this.query); + } + const facetsFilters = this._facetsFilters(); + if (facetsFilters) { + queryParts.push(facetsFilters); + } // build parameters const params: { q: string; @@ -127,7 +159,7 @@ export const SearchaliciousSearchMixin = >( index?: string; facets?: string; } = { - q: this.query, + q: queryParts.join(' '), langs: this.langs, page_size: this.pageSize.toString(), index: this.index, @@ -135,8 +167,9 @@ export const SearchaliciousSearchMixin = >( if (page) { params.page = page.toString(); } - if (this._facets()) { - params.facets = this._facets().join(','); + const facets = this._facets(); + if (facets) { + params.facets = facets.join(','); } const queryStr = Object.entries(params) .filter( diff --git a/frontend/src/search-facets.ts b/frontend/src/search-facets.ts index 77678c1d..b9aea756 100644 --- a/frontend/src/search-facets.ts +++ b/frontend/src/search-facets.ts @@ -4,6 +4,7 @@ import {repeat} from 'lit/directives/repeat.js'; import {SearchaliciousResultCtlMixin} from './search-results-ctl'; import {SearchResultEvent} from './events'; +import keys from 'lodash-es/keys'; interface FacetsInfos { [key: string]: FacetInfo; @@ -24,8 +25,18 @@ interface FacetTerm extends FacetItem { count: number; } +interface PresenceInfo { + [index: string]: boolean; +} + +function stringGuard(s: string | undefined): s is string { + return s != null; +} + /** - * Parent Component to display a side search filter (aka facets) + * Parent Component to display a side search filter (aka facets). + * + * It must contains a SearchaliciousFacet component for each facet we want to display. */ @customElement('searchalicious-facets') export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( @@ -37,30 +48,40 @@ export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( facets?: FacetsInfos; @queryAssignedNodes({flatten: true}) - facetNodes!: Array; + slotNodes!: Array; + + _facetNodes(): SearchaliciousFacet[] { + return this.slotNodes.filter( + (node) => node instanceof SearchaliciousFacet + ) as SearchaliciousFacet[]; + } /** * Names of facets we need to query, - * this is the names of contained facets + * this is the names of contained facetNodes. + * + * This is used by search-ctl to build the query */ getFacetsNames(): string[] { - const _facets: string[] = []; - this.facetNodes.forEach((node) => { - if (node instanceof SearchaliciousFacet) { - _facets.push(node.name); - } - }); - return _facets; + return this._facetNodes().map((node) => node.name); + } + + getSearchFilters(): string[] { + return this._facetNodes() + .map((node) => node.searchFilter()) + .filter(stringGuard); } + /** + * As a result is received, we dispatch facets values to facetNodes + * @param event search result event + */ override handleResults(event: SearchResultEvent) { this.facets = event.detail.facets as FacetsInfos; if (this.facets) { // dispatch to children - this.facetNodes.forEach((node) => { - if (node instanceof SearchaliciousFacet) { - node.infos = this.facets![node.name]; - } + this._facetNodes().forEach((node) => { + node.infos = this.facets![node.name]; }); } } @@ -79,6 +100,9 @@ export class SearchaliciousFacets extends SearchaliciousResultCtlMixin( * This is a base class, implementations are specific based on facet type */ export class SearchaliciousFacet extends LitElement { + /** + * The name of the facet we display + */ @property() name = ''; @@ -91,6 +115,10 @@ export class SearchaliciousFacet extends LitElement { throw new Error('renderFacet not implemented: implement in sub class'); } + searchFilter(): string | undefined { + throw new Error('renderFacet not implemented: implement in sub class'); + } + override render() { if (this.infos) { return this.renderFacet(); @@ -100,6 +128,9 @@ export class SearchaliciousFacet extends LitElement { } } +/** + * This is a "terms" facet, this must be within a searchalicious-facets element + */ @customElement('searchalicious-facet-terms') export class SearchaliciousTermsFacet extends SearchaliciousFacet { static override styles = css` @@ -107,16 +138,59 @@ export class SearchaliciousTermsFacet extends SearchaliciousFacet { display: block; } `; + + @property({attribute: false}) + selectedTerms: PresenceInfo = {}; + + /** + * Set wether a term is selected or not + */ + setTermSelected(e: Event) { + const element = e.target as HTMLInputElement; + const name = element.name; + if (element.checked) { + this.selectedTerms[name] = true; + } else { + delete this.selectedTerms[name]; + } + } + + /** + * Create the search term based upon the selected terms + */ + override searchFilter(): string | undefined { + const values = keys(this.selectedTerms) as String[]; + if (values.length === 0) { + return undefined; + } + let orValues = values.join(' OR '); + if (values.length > 1) { + orValues = `(${orValues})`; + } + return `${this.name}:${orValues}`; + } + + /** + * Renders a single term + */ renderTerm(term: FacetTerm) { return html`
-
`; } + /** + * Renders the facet content + */ override renderFacet() { return html`
@@ -138,428 +212,3 @@ declare global { 'searchalicious-facet-terms': SearchaliciousTermsFacet; } } - -/* - -things to add to API: -1. selected filters (if easy) --> we put it in a specific but we can match -2. widget type to use for each facet --> may also be separate to simplify ? -3. translations of filter names - -{ - "aggregations": { - "nova_groups": { - "doc_count_error_upper_bound": 0, - "sum_other_doc_count": 0, - "buckets": [ - { - "key": "4", - "doc_count": 559 - }, - { - "key": "3", - "doc_count": 158 - }, - { - "key": "1", - "doc_count": 95 - }, - { - "key": "2", - "doc_count": 50 - } - ] - }, - "owner": { - "doc_count_error_upper_bound": 0, - "sum_other_doc_count": 21, - "buckets": [ - { - "key": "org-carrefour", - "doc_count": 9 - }, - { - "key": "org-systeme-u", - "doc_count": 9 - }, - { - "key": "org-les-mousquetaires", - "doc_count": 6 - }, - { - "key": "org-idiffusion", - "doc_count": 5 - }, - { - "key": "org-scamark", - "doc_count": 5 - }, - { - "key": "org-auchan-apaw", - "doc_count": 4 - }, - { - "key": "org-casino", - "doc_count": 3 - }, - { - "key": "org-groupe-ldc", - "doc_count": 3 - }, - { - "key": "org-albert-menes", - "doc_count": 2 - }, - { - "key": "org-alnatura", - "doc_count": 2 - } - ] - }, - "categories_tags": { - "doc_count_error_upper_bound": 58, - "sum_other_doc_count": 5438, - "buckets": [ - { - "key": "en:plant-based-foods-and-beverages", - "doc_count": 419 - }, - { - "key": "en:plant-based-foods", - "doc_count": 367 - }, - { - "key": "en:snacks", - "doc_count": 266 - }, - { - "key": "en:sweet-snacks", - "doc_count": 202 - }, - { - "key": "en:dairies", - "doc_count": 163 - }, - { - "key": "en:beverages", - "doc_count": 143 - }, - { - "key": "en:fermented-foods", - "doc_count": 127 - }, - { - "key": "en:cereals-and-potatoes", - "doc_count": 123 - }, - { - "key": "en:fermented-milk-products", - "doc_count": 123 - }, - { - "key": "en:meats-and-their-products", - "doc_count": 118 - } - ] - }, - "brands_tags": { - "doc_count_error_upper_bound": 8, - "sum_other_doc_count": 1759, - "buckets": [ - { - "key": "bonarea", - "doc_count": 17 - }, - { - "key": "carrefour", - "doc_count": 15 - }, - { - "key": "lidl", - "doc_count": 15 - }, - { - "key": "nestle", - "doc_count": 13 - }, - { - "key": "tesco", - "doc_count": 13 - }, - { - "key": "u", - "doc_count": 13 - }, - { - "key": "auchan", - "doc_count": 8 - }, - { - "key": "coop", - "doc_count": 7 - }, - { - "key": "hacendado", - "doc_count": 7 - }, - { - "key": "ferrero", - "doc_count": 5 - } - ] - }, - "ecoscore_grade": { - "doc_count_error_upper_bound": 0, - "sum_other_doc_count": 0, - "buckets": [ - { - "key": "unknown", - "doc_count": 2350 - }, - { - "key": "b", - "doc_count": 260 - }, - { - "key": "d", - "doc_count": 220 - }, - { - "key": "c", - "doc_count": 167 - }, - { - "key": "e", - "doc_count": 98 - }, - { - "key": "a", - "doc_count": 37 - }, - { - "key": "not-applicable", - "doc_count": 29 - } - ] - }, - "states_tags": { - "doc_count_error_upper_bound": 581, - "sum_other_doc_count": 25942, - "buckets": [ - { - "key": "en:to-be-completed", - "doc_count": 3153 - }, - { - "key": "en:characteristics-to-be-completed", - "doc_count": 3084 - }, - { - "key": "en:origins-to-be-completed", - "doc_count": 3047 - }, - { - "key": "en:packaging-code-to-be-completed", - "doc_count": 3007 - }, - { - "key": "en:product-name-completed", - "doc_count": 3001 - }, - { - "key": "en:expiration-date-to-be-completed", - "doc_count": 2783 - }, - { - "key": "en:packaging-to-be-completed", - "doc_count": 2767 - }, - { - "key": "en:photos-uploaded", - "doc_count": 2709 - }, - { - "key": "en:photos-to-be-validated", - "doc_count": 2628 - }, - { - "key": "en:packaging-photo-to-be-selected", - "doc_count": 2620 - } - ] - }, - "nutrition_grades": { - "doc_count_error_upper_bound": 0, - "sum_other_doc_count": 0, - "buckets": [ - { - "key": "unknown", - "doc_count": 1991 - }, - { - "key": "d", - "doc_count": 314 - }, - { - "key": "c", - "doc_count": 246 - }, - { - "key": "e", - "doc_count": 206 - }, - { - "key": "a", - "doc_count": 173 - }, - { - "key": "b", - "doc_count": 169 - }, - { - "key": "not-applicable", - "doc_count": 62 - } - ] - }, - "lang": { - "doc_count_error_upper_bound": 0, - "sum_other_doc_count": 61, - "buckets": [ - { - "key": "fr", - "doc_count": 1186 - }, - { - "key": "en", - "doc_count": 1040 - }, - { - "key": "es", - "doc_count": 353 - }, - { - "key": "it", - "doc_count": 239 - }, - { - "key": "de", - "doc_count": 220 - }, - { - "key": "nl", - "doc_count": 17 - }, - { - "key": "pt", - "doc_count": 16 - }, - { - "key": "pl", - "doc_count": 14 - }, - { - "key": "ru", - "doc_count": 12 - }, - { - "key": "cs", - "doc_count": 11 - } - ] - }, - "countries_tags": { - "doc_count_error_upper_bound": 9, - "sum_other_doc_count": 411, - "buckets": [ - { - "key": "en:france", - "doc_count": 1017 - }, - { - "key": "en:united-states", - "doc_count": 619 - }, - { - "key": "en:spain", - "doc_count": 378 - }, - { - "key": "en:germany", - "doc_count": 273 - }, - { - "key": "en:italy", - "doc_count": 252 - }, - { - "key": "en:united-kingdom", - "doc_count": 127 - }, - { - "key": "en:belgium", - "doc_count": 93 - }, - { - "key": "en:switzerland", - "doc_count": 92 - }, - { - "key": "en:canada", - "doc_count": 81 - }, - { - "key": "en:ireland", - "doc_count": 51 - } - ] - }, - "labels_tags": { - "doc_count_error_upper_bound": 18, - "sum_other_doc_count": 1172, - "buckets": [ - { - "key": "en:organic", - "doc_count": 200 - }, - { - "key": "en:no-gluten", - "doc_count": 177 - }, - { - "key": "en:eu-organic", - "doc_count": 144 - }, - { - "key": "en:green-dot", - "doc_count": 126 - }, - { - "key": "en:vegetarian", - "doc_count": 115 - }, - { - "key": "en:vegan", - "doc_count": 104 - }, - { - "key": "en:nutriscore", - "doc_count": 84 - }, - { - "key": "fr:ab-agriculture-biologique", - "doc_count": 61 - }, - { - "key": "en:no-preservatives", - "doc_count": 56 - }, - { - "key": "en:no-lactose", - "doc_count": 40 - } - ] - } - } -} -*/ From be2c15829775ebd6d718c7b4ae1f52c71811212d Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 21:16:15 +0200 Subject: [PATCH 07/12] refactor: remove lodash dependency --- frontend/package.json | 4 +--- frontend/src/search-ctl.ts | 13 ++++--------- frontend/src/search-facets.ts | 3 +-- frontend/src/search-pages.ts | 11 +++++++++-- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/frontend/package.json b/frontend/package.json index b34a41f7..82197acc 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -41,9 +41,7 @@ "author": "Open Food Facts", "license": "BSD-3-Clause", "dependencies": { - "@types/lodash-es": "^4.17.0", - "lit": "^3.0.0", - "lodash-es": "^4.17.21" + "lit": "^3.0.0" }, "devDependencies": { "@custom-elements-manifest/analyzer": "^0.6.3", diff --git a/frontend/src/search-ctl.ts b/frontend/src/search-ctl.ts index 6808ff98..10908590 100644 --- a/frontend/src/search-ctl.ts +++ b/frontend/src/search-ctl.ts @@ -13,8 +13,6 @@ import { } from './events'; import {SearchaliciousFacets} from './search-facets'; -import uniq from 'lodash-es/uniq'; - // eslint-disable-next-line @typescript-eslint/no-explicit-any type Constructor = new (...args: any[]) => T; @@ -119,13 +117,10 @@ export const SearchaliciousSearchMixin = >( * Get the list of facets we want to request */ _facets(): string[] { - return ( - uniq( - this._facetsNodes() - .map((facets) => facets.getFacetsNames()) - .flat() - ) || undefined - ); + const names = this._facetsNodes() + .map((facets) => facets.getFacetsNames()) + .flat(); + return [...new Set(names)]; } /** diff --git a/frontend/src/search-facets.ts b/frontend/src/search-facets.ts index b9aea756..7327ba7e 100644 --- a/frontend/src/search-facets.ts +++ b/frontend/src/search-facets.ts @@ -4,7 +4,6 @@ import {repeat} from 'lit/directives/repeat.js'; import {SearchaliciousResultCtlMixin} from './search-results-ctl'; import {SearchResultEvent} from './events'; -import keys from 'lodash-es/keys'; interface FacetsInfos { [key: string]: FacetInfo; @@ -159,7 +158,7 @@ export class SearchaliciousTermsFacet extends SearchaliciousFacet { * Create the search term based upon the selected terms */ override searchFilter(): string | undefined { - const values = keys(this.selectedTerms) as String[]; + const values = Object.keys(this.selectedTerms); if (values.length === 0) { return undefined; } diff --git a/frontend/src/search-pages.ts b/frontend/src/search-pages.ts index 0ca354a1..3eb38b43 100644 --- a/frontend/src/search-pages.ts +++ b/frontend/src/search-pages.ts @@ -1,12 +1,19 @@ import {LitElement, html, css} from 'lit'; import {customElement, property, state} from 'lit/decorators.js'; import {repeat} from 'lit/directives/repeat.js'; -import range from 'lodash-es/range'; import {SearchaliciousEvents} from './enums'; import {SearchResultEvent} from './events'; import {SearchaliciousResultCtlMixin} from './search-results-ctl'; +// small utility to have a range +// inspired from https://stackoverflow.com/a/44957114/2886726 +function _range(start: number, end: number): number[] { + return Array(end - start) + .fill(1) + .map((_, idx) => start + idx); +} + /** * A component to display pagination for search results. */ @@ -140,7 +147,7 @@ export class SearchaliciousPages extends SearchaliciousResultCtlMixin( if (!this._startRange || !this._endRange) { return []; } - return range(this._startRange, this._endRange + 1); + return _range(this._startRange, this._endRange + 1); } override render() { From 91a35f89ac34be54c1a81f63db1ecdd3a4f5e183 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 21:24:05 +0200 Subject: [PATCH 08/12] fix: fix python tests --- tests/unit/data/complex_query.json | 52 ------------ tests/unit/data/empty_query_with_sort_by.json | 52 ------------ .../empty_query_with_sort_by_and_facets.json | 29 +++++++ .../unit/data/non_existing_filter_field.json | 52 ------------ tests/unit/data/simple_filter_query.json | 52 ------------ tests/unit/data/simple_full_text_query.json | 52 ------------ .../data/simple_full_text_query_facets.json | 84 +++++++++++++++++++ tests/unit/data/sort_by_query.json | 52 ------------ tests/unit/test_query.py | 31 ++++++- 9 files changed, 140 insertions(+), 316 deletions(-) create mode 100644 tests/unit/data/empty_query_with_sort_by_and_facets.json create mode 100644 tests/unit/data/simple_full_text_query_facets.json diff --git a/tests/unit/data/complex_query.json b/tests/unit/data/complex_query.json index 78cb85ed..c8136960 100644 --- a/tests/unit/data/complex_query.json +++ b/tests/unit/data/complex_query.json @@ -113,58 +113,6 @@ "minimum_should_match": 1 } }, - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "size": 25, "from": 25 } \ No newline at end of file diff --git a/tests/unit/data/empty_query_with_sort_by.json b/tests/unit/data/empty_query_with_sort_by.json index fd15ab84..93d6f769 100644 --- a/tests/unit/data/empty_query_with_sort_by.json +++ b/tests/unit/data/empty_query_with_sort_by.json @@ -1,56 +1,4 @@ { - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "sort": [ "unique_scans_n" ], diff --git a/tests/unit/data/empty_query_with_sort_by_and_facets.json b/tests/unit/data/empty_query_with_sort_by_and_facets.json new file mode 100644 index 00000000..9aa2c6d6 --- /dev/null +++ b/tests/unit/data/empty_query_with_sort_by_and_facets.json @@ -0,0 +1,29 @@ +{ + "aggs": { + "brands_tags": { + "terms": { + "field": "brands_tags" + } + }, + "categories_tags": { + "terms": { + "field": "categories_tags" + } + }, + "nutrition_grades": { + "terms": { + "field": "nutrition_grades" + } + }, + "lang": { + "terms": { + "field": "lang" + } + } + }, + "sort": [ + "unique_scans_n" + ], + "size": 25, + "from": 25 +} \ No newline at end of file diff --git a/tests/unit/data/non_existing_filter_field.json b/tests/unit/data/non_existing_filter_field.json index 56c4c84c..28b7ecff 100644 --- a/tests/unit/data/non_existing_filter_field.json +++ b/tests/unit/data/non_existing_filter_field.json @@ -13,58 +13,6 @@ ] } }, - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "size": 25, "from": 25 } \ No newline at end of file diff --git a/tests/unit/data/simple_filter_query.json b/tests/unit/data/simple_filter_query.json index 5a0265ba..fad711fe 100644 --- a/tests/unit/data/simple_filter_query.json +++ b/tests/unit/data/simple_filter_query.json @@ -12,58 +12,6 @@ ] } }, - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "size": 25, "from": 25 } \ No newline at end of file diff --git a/tests/unit/data/simple_full_text_query.json b/tests/unit/data/simple_full_text_query.json index 8535d559..baa76cc1 100644 --- a/tests/unit/data/simple_full_text_query.json +++ b/tests/unit/data/simple_full_text_query.json @@ -57,58 +57,6 @@ ] } }, - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "size": 10, "from": 0 } \ No newline at end of file diff --git a/tests/unit/data/simple_full_text_query_facets.json b/tests/unit/data/simple_full_text_query_facets.json new file mode 100644 index 00000000..34906eb9 --- /dev/null +++ b/tests/unit/data/simple_full_text_query_facets.json @@ -0,0 +1,84 @@ +{ + "query": { + "bool": { + "should": [ + { + "match_phrase": { + "product_name.fr": { + "query": "flocons d'avoine", + "boost": 2.0 + } + } + }, + { + "match_phrase": { + "generic_name.fr": { + "query": "flocons d'avoine", + "boost": 2.0 + } + } + }, + { + "match_phrase": { + "categories.fr": { + "query": "flocons d'avoine", + "boost": 2.0 + } + } + }, + { + "match_phrase": { + "labels.fr": { + "query": "flocons d'avoine", + "boost": 2.0 + } + } + }, + { + "match_phrase": { + "brands": { + "query": "flocons d'avoine", + "boost": 2.0 + } + } + }, + { + "multi_match": { + "query": "flocons d'avoine", + "fields": [ + "product_name.fr", + "generic_name.fr", + "categories.fr", + "labels.fr", + "brands" + ] + } + } + ] + } + }, + "aggs": { + "brands_tags": { + "terms": { + "field": "brands_tags" + } + }, + "labels_tags": { + "terms": { + "field": "labels_tags" + } + }, + "nutrition_grades": { + "terms": { + "field": "nutrition_grades" + } + }, + "owner": { + "terms": { + "field": "owner" + } + } + }, + "size": 10, + "from": 0 +} \ No newline at end of file diff --git a/tests/unit/data/sort_by_query.json b/tests/unit/data/sort_by_query.json index e7cf95fb..7bee1935 100644 --- a/tests/unit/data/sort_by_query.json +++ b/tests/unit/data/sort_by_query.json @@ -57,58 +57,6 @@ ] } }, - "aggs": { - "brands_tags": { - "terms": { - "field": "brands_tags" - } - }, - "lang": { - "terms": { - "field": "lang" - } - }, - "owner": { - "terms": { - "field": "owner" - } - }, - "categories_tags": { - "terms": { - "field": "categories_tags" - } - }, - "labels_tags": { - "terms": { - "field": "labels_tags" - } - }, - "countries_tags": { - "terms": { - "field": "countries_tags" - } - }, - "states_tags": { - "terms": { - "field": "states_tags" - } - }, - "nutrition_grades": { - "terms": { - "field": "nutrition_grades" - } - }, - "ecoscore_grade": { - "terms": { - "field": "ecoscore_grade" - } - }, - "nova_groups": { - "terms": { - "field": "nova_groups" - } - } - }, "sort": [ { "unique_scans_n": { diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 5a471585..4ede37e8 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -116,13 +116,22 @@ def test_parse_lucene_dsl_query( @pytest.mark.parametrize( - "id_,q,langs,size,page,sort_by", + "id_,q,langs,size,page,sort_by,facets", [ - ("simple_full_text_query", "flocons d'avoine", {"fr"}, 10, 1, None), + ("simple_full_text_query", "flocons d'avoine", {"fr"}, 10, 1, None, None), + ( + "simple_full_text_query_facets", + "flocons d'avoine", + {"fr"}, + 10, + 1, + None, + ["brands_tags", "labels_tags", "nutrition_grades", "owner"], + ), # sort by descending number of scan count - ("sort_by_query", "flocons d'avoine", {"fr"}, 10, 1, "-unique_scans_n"), + ("sort_by_query", "flocons d'avoine", {"fr"}, 10, 1, "-unique_scans_n", None), # we change number of results (25 instead of 10) and request page 2 - ("simple_filter_query", 'countries_tags:"en:italy"', {"en"}, 25, 2, None), + ("simple_filter_query", 'countries_tags:"en:italy"', {"en"}, 25, 2, None, None), ( "complex_query", 'bacon de boeuf (countries_tags:"en:italy" AND (categories_tags:"en:beef" AND ' @@ -131,6 +140,7 @@ def test_parse_lucene_dsl_query( 25, 2, None, + None, ), ( "non_existing_filter_field", @@ -139,6 +149,7 @@ def test_parse_lucene_dsl_query( 25, 2, None, + None, ), ( "empty_query_with_sort_by", @@ -147,6 +158,16 @@ def test_parse_lucene_dsl_query( 25, 2, "unique_scans_n", + None, + ), + ( + "empty_query_with_sort_by_and_facets", + None, + {"en"}, + 25, + 2, + "unique_scans_n", + ["brands_tags", "categories_tags", "nutrition_grades", "lang"], ), ], ) @@ -157,6 +178,7 @@ def test_build_search_query( size: int, page: int, sort_by: str | None, + facets: list[str] | None, update_results: bool, default_config: IndexConfig, default_filter_query_builder: ElasticsearchQueryBuilder, @@ -169,6 +191,7 @@ def test_build_search_query( config=default_config, filter_query_builder=default_filter_query_builder, sort_by=sort_by, + facets=facets, ) if update_results: From 6f178719450fae97b8d73b276fe7821aeddf9340 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 21:53:46 +0200 Subject: [PATCH 09/12] fix: avoid adding facets parameter if empty in search-ctl --- frontend/src/search-ctl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/search-ctl.ts b/frontend/src/search-ctl.ts index 10908590..34488e0a 100644 --- a/frontend/src/search-ctl.ts +++ b/frontend/src/search-ctl.ts @@ -163,7 +163,7 @@ export const SearchaliciousSearchMixin = >( params.page = page.toString(); } const facets = this._facets(); - if (facets) { + if (facets && facets.length > 0) { params.facets = facets.join(','); } const queryStr = Object.entries(params) From b8f780c31dcdc631ed376524f1f4c2920356ecd2 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Thu, 30 May 2024 21:59:47 +0200 Subject: [PATCH 10/12] fix: add lazy loading to results images to avoid weird requests --- frontend/public/off.html | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/public/off.html b/frontend/public/off.html index ffe409f6..430fa731 100644 --- a/frontend/public/off.html +++ b/frontend/public/off.html @@ -279,6 +279,7 @@
@@ -288,10 +289,12 @@
From 3ff57e33dce507301263fd64633aa32d9a9a0891 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Fri, 31 May 2024 15:02:42 +0200 Subject: [PATCH 11/12] fix: typos in comments Co-authored-by: Pierre Slamich --- README.md | 2 +- app/_types.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index c1687bae..91588d6d 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ Note that it's important to use port 8001, as port 8000 will be used by the dock To debug the backend app: -* stop api instance: `docker compose stop api` +* stop API instance: `docker compose stop api` * add a pdb.set_trace() at the point you want, * then launch `docker compose run --rm --use-aliases api uvicorn app.api:app --proxy-headers --host 0.0.0.0 --port 8000 --reload`[^use_aliases] diff --git a/app/_types.py b/app/_types.py index b3cc9f89..af0a6c50 100644 --- a/app/_types.py +++ b/app/_types.py @@ -8,7 +8,7 @@ class FacetItem(BaseModel): - """Describe an entry of a facet""" + """Describes an entry of a facet""" key: str name: str @@ -17,7 +17,7 @@ class FacetItem(BaseModel): class FacetInfo(BaseModel): - """Search result for facet""" + """Search result for a facet""" name: str """The display name of the facet""" From 5be9a956a892435783ef3fd2572def77c0fa6342 Mon Sep 17 00:00:00 2001 From: Alex Garel Date: Fri, 31 May 2024 16:05:49 +0200 Subject: [PATCH 12/12] fix: remove non usefull fixme --- frontend/src/search-facets.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/search-facets.ts b/frontend/src/search-facets.ts index 7327ba7e..33421f39 100644 --- a/frontend/src/search-facets.ts +++ b/frontend/src/search-facets.ts @@ -122,7 +122,7 @@ export class SearchaliciousFacet extends LitElement { if (this.infos) { return this.renderFacet(); } else { - return html``; // FIXME: is this ok ? + return html``; } } }