diff --git a/src/app/Console/Commands/AlgoliaSyncIndexSettings.php b/src/app/Console/Commands/AlgoliaSyncIndexSettings.php new file mode 100644 index 00000000..821da2ff --- /dev/null +++ b/src/app/Console/Commands/AlgoliaSyncIndexSettings.php @@ -0,0 +1,64 @@ +option('dry-run')) { + $this->info('[dry-run] would set attributesForFaceting on index: ' . $indexName); + $this->line(json_encode(self::ATTRIBUTES_FOR_FACETING, JSON_PRETTY_PRINT)); + return self::SUCCESS; + } + + $appId = config('algolia.algolia_app_id'); + $apiKey = config('algolia.algolia_write_api_key'); + + if (!$appId || !$apiKey || !$indexName) { + $this->error('Missing ALGOLIA_APP_ID / ALGOLIA_WRITE_API_KEY / ALGOLIA_INDEX'); + return self::FAILURE; + } + + // Resolved lazily (only reached once credentials are confirmed present) so + // dry-run and the missing-credentials path never touch the container binding. + $syncer = new AlgoliaIndexSettingsSyncer($this->laravel->make(SearchClient::class)); + $syncer->syncAttributesForFaceting($indexName, self::ATTRIBUTES_FOR_FACETING); + + $this->info("Done: attributesForFaceting synced on index={$indexName}"); + + return self::SUCCESS; + } +} diff --git a/src/app/Packages/Domains/README.md b/src/app/Packages/Domains/README.md index 541f53cd..daf5047f 100644 --- a/src/app/Packages/Domains/README.md +++ b/src/app/Packages/Domains/README.md @@ -51,4 +51,18 @@ AlgoliaAdapter SearchResult (Ports) ↓ Dto -``` \ No newline at end of file +``` + +--- + +## Algolia Index Settings + +`attributesForFaceting` is managed as code via `App\Console\Commands\AlgoliaSyncIndexSettings::ATTRIBUTES_FOR_FACETING`, +applied with `php artisan algolia:sync-index-settings` (use `--dry-run` to preview without writing). +Every attribute that `AlgoliaWorldHeritageSearchAdapter` filters or optional-filters on must be +listed there, or Algolia rejects the request. + +All currently declared attributes are plain facets (facet counts enabled), none use `filterOnly()`. +`country_names_jp`, `region`, and `study_regions` are not used as filters by the Adapter today, so +they are candidates for `filterOnly()` to shrink the index — but this has not been changed yet, +since whether the frontend relies on their facet counts has not been confirmed. \ No newline at end of file diff --git a/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaIndexSettingsSyncer.php b/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaIndexSettingsSyncer.php new file mode 100644 index 00000000..0aabf350 --- /dev/null +++ b/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaIndexSettingsSyncer.php @@ -0,0 +1,24 @@ +client->setSettings($indexName, [ + 'attributesForFaceting' => $attributesForFaceting, + ]); + + $taskId = $res['taskID'] ?? null; + if ($taskId !== null) { + $this->client->waitForTask($indexName, $taskId); + } + } +} diff --git a/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaWorldHeritageSearchAdapter.php b/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaWorldHeritageSearchAdapter.php index f4aab0ad..388d44eb 100644 --- a/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaWorldHeritageSearchAdapter.php +++ b/src/app/Packages/Domains/WorldHeritage/Adapter/AlgoliaWorldHeritageSearchAdapter.php @@ -6,6 +6,7 @@ use App\Packages\Domains\WorldHeritage\Ports\Dto\HeritageSearchResult; use App\Packages\Domains\WorldHeritage\Ports\WorldHeritageSearchPort; use App\Packages\Features\QueryUseCases\ListQuery\AlgoliaSearchListQuery; +use Illuminate\Support\Facades\Log; class AlgoliaWorldHeritageSearchAdapter implements WorldHeritageSearchPort { @@ -30,30 +31,40 @@ public function search( */ $filters = []; + /** + * Optional filters: unlike $filters, these never exclude a record — + * they only boost the ranking of records that match. Used when a + * country has not been confirmed via ISO3, so an input that fails to + * resolve to a real country (e.g. "Jap") still falls back to plain + * full-text search instead of returning 0 results. + */ + $optionalFilters = []; + /** * Country filtering strategy: - * - If ISO3 is explicitly provided, use strict ISO3 filtering and disable full-text search. + * - If ISO3 is explicitly provided, filter by state_party_codes:. + * This is a confirmed country, so it narrows results like any other filter. * - Otherwise, attempt to treat the provided countryName as one of: * - English country name (e.g. "Japan" / "japan") * - Japanese name (e.g. "日本") * - ISO3 (e.g. "JPN") * - * In that case we build an OR-filter across relevant fields. + * This is unconfirmed, so it goes into $optionalFilters: matching + * records rank higher, but a non-matching countryName does not + * reduce the full-text search results to zero. + * + * Either way, the country signal never blanks out the keyword: like + * every other filter (region/category/year/criteria/is_endangered), + * it narrows/boosts alongside the keyword search rather than + * replacing it. A keyword combined with an explicit country param + * (e.g. keyword="temple" & country_iso3=JPN) must narrow by both. */ $queryString = $query->keyword ?? ''; if ($this->hasValue($query->countryIso3)) { - // Requirement: ISO3 -> query='' and filter by state_party_codes: $filters[] = 'state_party_codes:' . $this->escapeToken($query->countryIso3); - $queryString = ''; } elseif ($this->hasValue($query->countryName)) { - $filters[] = $this->buildCountryOrFilter($query->countryName); - /** - * IMPORTANT: - * Do not blank out query here. - * If the input was not a real country (e.g. "japan" typo / random word), - * we still want full-text search to work rather than returning "top hits". - */ + $optionalFilters[] = $this->buildCountryOrFilter($query->countryName); } /** @@ -102,13 +113,14 @@ public function search( /** * Guardrail: - * Never execute Algolia with query='' AND no filters, + * Never execute Algolia with query='' AND no filters AND no optional filters, * otherwise you will get "top results" unrelated to the user input. */ $hasAnyFilter = $filters !== []; + $hasOptionalFilter = $optionalFilters !== []; $hasQueryText = $this->hasValue($queryString); - if (!$hasAnyFilter && !$hasQueryText) { + if (!$hasAnyFilter && !$hasOptionalFilter && !$hasQueryText) { // Prefer returning empty result rather than misleading "top hits". return new HeritageSearchResult( ids: [], @@ -127,6 +139,7 @@ public function search( 'page' => $firstPage, 'hitsPerPage' => $perPage, 'filters' => $hasAnyFilter ? implode(' AND ', $filters) : null, + 'optionalFilters' => $hasOptionalFilter ? $optionalFilters : null, /** * Only the id is ever read from the response (see below): * the full record is re-fetched from the DB via findByIdsPreserveOrder. @@ -140,11 +153,26 @@ public function search( $hits = $response['hits'] ?? []; - $ids = array_values(array_filter(array_map(static function (array $h) { - return isset($h['id']) - ? (int) $h['id'] - : (isset($h['objectID']) ? (int) $h['objectID'] : null); - }, $hits))); + $ids = []; + foreach ($hits as $hit) { + $rawId = $hit['id'] ?? $hit['objectID'] ?? null; + + if ($rawId === null) { + continue; + } + + if (!is_numeric($rawId)) { + // Don't silently coerce a non-numeric id to 0 and drop it via + // array_filter: that would hide an Algolia/DB data inconsistency. + Log::warning('Algolia search hit has a non-numeric id', [ + 'id' => $hit['id'] ?? null, + 'objectID' => $hit['objectID'] ?? null, + ]); + continue; + } + + $ids[] = (int) $rawId; + } $nbHits = (int) ($response['nbHits'] ?? 0); $nbPages = (int) ($response['nbPages'] ?? 0); @@ -161,11 +189,14 @@ public function search( } /** - * Build an OR-filter that can match different country input shapes: + * Build an OR-expression that can match different country input shapes: * - English: country (exact match) * - Japanese: country_name_jp (exact match) * - ISO3-like: state_party_codes: * + * Used as an entry in $optionalFilters (not $filters): a non-match + * does not exclude a record, it just skips the ranking boost. + * * Example output: * (country:"Japan" OR country_name_jp:"日本" OR state_party_codes:JPN) */ diff --git a/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaIndexSettingsSyncerTest.php b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaIndexSettingsSyncerTest.php new file mode 100644 index 00000000..b9ce2e47 --- /dev/null +++ b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaIndexSettingsSyncerTest.php @@ -0,0 +1,43 @@ +shouldReceive('setSettings') + ->once() + ->with('world_heritages', ['attributesForFaceting' => ['category', 'country']]) + ->andReturn(['taskID' => 42]); + $client->shouldReceive('waitForTask')->once()->with('world_heritages', 42); + + $syncer = new AlgoliaIndexSettingsSyncer($client); + $syncer->syncAttributesForFaceting('world_heritages', ['category', 'country']); + } + + #[DoesNotPerformAssertions] + public function test_does_not_wait_when_algolia_returns_no_task_id(): void + { + $client = Mockery::mock(SearchClient::class); + $client->shouldReceive('setSettings')->once()->andReturn([]); + $client->shouldNotReceive('waitForTask'); + + $syncer = new AlgoliaIndexSettingsSyncer($client); + $syncer->syncAttributesForFaceting('world_heritages', ['category']); + } +} diff --git a/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaSyncIndexSettingsTest.php b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaSyncIndexSettingsTest.php new file mode 100644 index 00000000..20c6a9c0 --- /dev/null +++ b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaSyncIndexSettingsTest.php @@ -0,0 +1,83 @@ + true]); + + $this->assertSame(0, $exitCode); + + $output = Artisan::output(); + foreach (AlgoliaSyncIndexSettings::ATTRIBUTES_FOR_FACETING as $attribute) { + $this->assertStringContainsString($attribute, $output); + } + } + + public function test_fails_when_write_credentials_are_missing(): void + { + config(['algolia.algolia_write_api_key' => null]); + + $exitCode = Artisan::call('algolia:sync-index-settings'); + + $this->assertSame(1, $exitCode); + $this->assertStringContainsString('Missing', Artisan::output()); + } + + public function test_syncs_attributes_for_faceting_when_credentials_are_present(): void + { + config([ + 'algolia.algolia_app_id' => 'dummy-app', + 'algolia.algolia_write_api_key' => 'dummy-key', + 'algolia.algolia_index' => 'world_heritages', + ]); + + $client = Mockery::mock(SearchClient::class); + $client->shouldReceive('setSettings') + ->once() + ->with('world_heritages', ['attributesForFaceting' => AlgoliaSyncIndexSettings::ATTRIBUTES_FOR_FACETING]) + ->andReturn(['taskID' => 123]); + $client->shouldReceive('waitForTask')->once()->with('world_heritages', 123); + + $this->app->instance(SearchClient::class, $client); + + $exitCode = Artisan::call('algolia:sync-index-settings'); + + $this->assertSame(0, $exitCode); + $this->assertStringContainsString('Done', Artisan::output()); + } + + public function test_attributes_for_faceting_covers_every_attribute_the_adapter_filters_on(): void + { + // Every attribute AlgoliaWorldHeritageSearchAdapter uses in $filters + // or $optionalFilters must be declared here, or Algolia rejects the + // facet filter request. + $usedByAdapter = [ + 'state_party_codes', + 'country', + 'country_name_jp', + 'study_region', + 'category', + 'criteria', + 'is_endangered', + ]; + + foreach ($usedByAdapter as $attribute) { + $this->assertContains($attribute, AlgoliaSyncIndexSettings::ATTRIBUTES_FOR_FACETING); + } + } +} diff --git a/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaWorldHeritageSearchAdapterTest.php b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaWorldHeritageSearchAdapterTest.php index 9ce743be..888c1c45 100644 --- a/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaWorldHeritageSearchAdapterTest.php +++ b/src/app/Packages/Domains/WorldHeritage/Tests/AlgoliaWorldHeritageSearchAdapterTest.php @@ -6,8 +6,9 @@ use App\Enums\StudyRegion; use App\Packages\Domains\WorldHeritage\Adapter\AlgoliaWorldHeritageSearchAdapter; use App\Packages\Features\QueryUseCases\ListQuery\AlgoliaSearchListQuery; +use Illuminate\Support\Facades\Log; use Mockery; -use PHPUnit\Framework\TestCase; +use Tests\TestCase; class AlgoliaWorldHeritageSearchAdapterTest extends TestCase { @@ -45,8 +46,9 @@ public function test_search_builds_algolia_params_with_filters_and_paging(): voi ); // Arrange: Algolia expects 0-based page indexing, so currentPage=2 becomes page=1. + // The keyword must stay intact even though a country filter is also present. $expectedParams = [ - 'query' => '', + 'query' => 'galapagos', 'page' => 1, 'hitsPerPage' => 30, 'filters' => @@ -78,7 +80,7 @@ public function test_search_builds_algolia_params_with_filters_and_paging(): voi $this->assertSame(1, $result->lastPage); } - public function test_search_by_country_name_without_iso3_does_not_filter_on_state_party(): void + public function test_search_by_country_name_without_iso3_uses_optional_filter_not_filter(): void { $indexName = 'world_heritage'; $adapter = new AlgoliaWorldHeritageSearchAdapter($this->client, $indexName); @@ -101,7 +103,51 @@ public function test_search_by_country_name_without_iso3_does_not_filter_on_stat 'query' => '', 'page' => 0, 'hitsPerPage' => 30, - 'filters' => '(country:"Ecuador" OR country_name_jp:"Ecuador")', + 'optionalFilters' => ['(country:"Ecuador" OR country_name_jp:"Ecuador")'], + 'attributesToRetrieve' => ['objectID', 'id'], + ]; + + $this->client + ->shouldReceive('searchSingleIndex') + ->once() + ->with($indexName, $expectedParams) + ->andReturn([ + 'hits' => [['id' => 1]], + 'nbHits' => 1, + 'nbPages' => 1, + 'page' => 0, + 'hitsPerPage' => 30, + ]); + + $result = $adapter->search($q, currentPage: 1, perPage: 30); + + $this->assertSame([1], $result->ids); + } + + public function test_search_by_lowercase_country_name_boosts_ranking_via_optional_filter(): void + { + $indexName = 'world_heritage'; + $adapter = new AlgoliaWorldHeritageSearchAdapter($this->client, $indexName); + + $q = new AlgoliaSearchListQuery( + keyword: null, + countryName: 'japan', + countryIso3: null, + region: null, + category: null, + yearFrom: null, + yearTo: null, + criteria: [], + isEndangered: null, + currentPage: 1, + perPage: 30, + ); + + $expectedParams = [ + 'query' => '', + 'page' => 0, + 'hitsPerPage' => 30, + 'optionalFilters' => ['(country:"japan" OR country_name_jp:"japan")'], 'attributesToRetrieve' => ['objectID', 'id'], ]; @@ -110,15 +156,155 @@ public function test_search_by_country_name_without_iso3_does_not_filter_on_stat ->once() ->with($indexName, $expectedParams) ->andReturn([ - 'hits' => [], - 'nbHits' => 0, - 'nbPages' => 0, + 'hits' => [['id' => 42]], + 'nbHits' => 1, + 'nbPages' => 1, + 'page' => 0, + 'hitsPerPage' => 30, + ]); + + $result = $adapter->search($q, currentPage: 1, perPage: 30); + + $this->assertSame([42], $result->ids); + } + + public function test_search_by_country_name_that_does_not_resolve_still_runs_full_text_search(): void + { + $indexName = 'world_heritage'; + $adapter = new AlgoliaWorldHeritageSearchAdapter($this->client, $indexName); + + // "zzz" does not match any real country (and isn't a plausible ISO3 + // either), but should not force a 0-result short-circuit the way a + // mandatory $filters entry would. + $q = new AlgoliaSearchListQuery( + keyword: null, + countryName: 'zzz', + countryIso3: null, + region: null, + category: null, + yearFrom: null, + yearTo: null, + criteria: [], + isEndangered: null, + currentPage: 1, + perPage: 30, + ); + + $expectedParams = [ + 'query' => '', + 'page' => 0, + 'hitsPerPage' => 30, + 'optionalFilters' => ['(country:"zzz" OR country_name_jp:"zzz" OR state_party_codes:ZZZ)'], + 'attributesToRetrieve' => ['objectID', 'id'], + ]; + + $this->client + ->shouldReceive('searchSingleIndex') + ->once() + ->with($indexName, $expectedParams) + ->andReturn([ + 'hits' => [['id' => 7], ['id' => 8]], + 'nbHits' => 2, + 'nbPages' => 1, + 'page' => 0, + 'hitsPerPage' => 30, + ]); + + $result = $adapter->search($q, currentPage: 1, perPage: 30); + + // The point of optionalFilters: a non-matching country term does not + // force an empty result via the adapter's own guardrail. + $this->assertSame([7, 8], $result->ids); + } + + public function test_search_logs_and_skips_a_hit_with_a_non_numeric_id_instead_of_silently_dropping_it(): void + { + $indexName = 'world_heritage'; + $adapter = new AlgoliaWorldHeritageSearchAdapter($this->client, $indexName); + + $q = new AlgoliaSearchListQuery( + keyword: 'temple', + countryName: null, + countryIso3: null, + region: null, + category: null, + yearFrom: null, + yearTo: null, + criteria: [], + isEndangered: null, + currentPage: 1, + perPage: 30, + ); + + $this->client + ->shouldReceive('searchSingleIndex') + ->once() + ->andReturn([ + 'hits' => [ + ['id' => 10], + ['id' => 'not-a-number', 'objectID' => 'broken-1'], + ], + 'nbHits' => 2, + 'nbPages' => 1, + 'page' => 0, + 'hitsPerPage' => 30, + ]); + + Log::shouldReceive('warning') + ->once() + ->with('Algolia search hit has a non-numeric id', [ + 'id' => 'not-a-number', + 'objectID' => 'broken-1', + ]); + + $result = $adapter->search($q, currentPage: 1, perPage: 30); + + // The valid hit survives; the broken one is logged and skipped rather + // than silently coerced to 0 and dropped via array_filter. + $this->assertSame([10], $result->ids); + } + + public function test_search_combines_keyword_with_country_iso3_filter_instead_of_dropping_keyword(): void + { + $indexName = 'world_heritage'; + $adapter = new AlgoliaWorldHeritageSearchAdapter($this->client, $indexName); + + $q = new AlgoliaSearchListQuery( + keyword: 'temple', + countryName: null, + countryIso3: 'JPN', + region: null, + category: null, + yearFrom: null, + yearTo: null, + criteria: [], + isEndangered: null, + currentPage: 1, + perPage: 30, + ); + + $expectedParams = [ + 'query' => 'temple', + 'page' => 0, + 'hitsPerPage' => 30, + 'filters' => 'state_party_codes:JPN', + 'attributesToRetrieve' => ['objectID', 'id'], + ]; + + $this->client + ->shouldReceive('searchSingleIndex') + ->once() + ->with($indexName, $expectedParams) + ->andReturn([ + 'hits' => [['id' => 913], ['id' => 1277]], + 'nbHits' => 2, + 'nbPages' => 1, 'page' => 0, 'hitsPerPage' => 30, ]); $result = $adapter->search($q, currentPage: 1, perPage: 30); - $this->assertSame([], $result->ids); + $this->assertSame([913, 1277], $result->ids); } } diff --git a/src/app/Providers/AppServiceProvider.php b/src/app/Providers/AppServiceProvider.php index a12b9fe0..369163c6 100644 --- a/src/app/Providers/AppServiceProvider.php +++ b/src/app/Providers/AppServiceProvider.php @@ -31,6 +31,13 @@ public function register(): void return new AlgoliaWorldHeritageSearchAdapter(client: $client, indexName: config('algolia.algolia_index')); }); + + // Write-key client for index administration (e.g. algolia:sync-index-settings). + // Resolved lazily via the container so callers only pay for it, and tests + // can swap it out, when they actually reach a code path that needs it. + $this->app->bind(SearchClient::class, function () { + return SearchClient::create(config('algolia.algolia_app_id'), config('algolia.algolia_write_api_key')); + }); } /**