-
-
Notifications
You must be signed in to change notification settings - Fork 40
Add documentation search API with Sprunje integration, wildcard support, caching, and metadata search #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: lcharette <[email protected]>
… test robustness Co-authored-by: lcharette <[email protected]>
Co-authored-by: lcharette <[email protected]>
❌ 13 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
lcharette
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Set hardcoded configuration value in the config file, fetch them using the config service
- Use the Sprunjer class from the core sprinkle of UserFrosting (consult the monorepo)
- Third party package use is authorized if optmisation can be implied
- Search result should be cached at the search service level. if same query is searched multiple time, cached result should be returned.
- Fix tests failures
- Add config for minimum number character in a query. Avoid search for a single character. Set 3 as default. Use Fortress to validate input
- Add possibility for pages to specify keywords in front matter, and search across keywords, or other metadata as titles. Priority should be title, keywords, other front matter
| /** | ||
| * Bakery command to rebuild the search index for documentation. | ||
| */ | ||
| class SearchIndexCommand extends BaseCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot BaseCommand does not exist. use Symfony\Component\Console\Command\Command;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Removed BaseCommand and now extending Symfony\Component\Console\Command\Command directly with SymfonyStyle initialization.
| $page = isset($params['page']) ? max(1, (int) $params['page']) : 1; | ||
| $size = isset($params['size']) ? min(100, max(1, (int) $params['size'])) : 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Default values should be in the configuration file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Default values now sourced from config: learn.search.default_page, learn.search.default_size, and learn.search.max_size.
| // Get query parameter | ||
| $query = $params['q'] ?? ''; | ||
|
|
||
| if (empty($query)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Construct empty() is not allowed. Use more strict comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Replaced empty() with strict string comparison $query === '' and added length validation.
app/src/Search/SearchIndex.php
Outdated
| // Index all available versions | ||
| $available = $this->config->get('learn.versions.available', []); | ||
| foreach (array_keys($available) as $versionId) { | ||
| $versions[] = $this->versionValidator->getVersion($versionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Added explicit cast to string: (string) $versionId when calling getVersion().
app/src/Search/SearchIndex.php
Outdated
| // Clear all versions | ||
| $available = $this->config->get('learn.versions.available', []); | ||
| foreach (array_keys($available) as $versionId) { | ||
| $this->cache->forget($this->getCacheKey($versionId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Added explicit cast to string: (string) $versionId in the clearIndex method.
app/src/Search/SearchService.php
Outdated
| protected const SNIPPET_CONTEXT_LENGTH = 150; | ||
|
|
||
| /** @var int Maximum number of results to return */ | ||
| protected const MAX_RESULTS = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Put in config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Moved constants to config: learn.search.snippet_length and learn.search.max_results.
app/src/Search/SearchService.php
Outdated
| public function search(string $query, ?string $version = null, int $page = 1, int $perPage = 10): array | ||
| { | ||
| // Get the version to search | ||
| $versionId = $version ?? $this->config->get('learn.versions.latest', '6.0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Should not be a default here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Removed default value for $version parameter. Now requires explicit null or value.
app/src/Search/SearchService.php
Outdated
| * | ||
| * @return array<int, array{title: string, slug: string, route: string, snippet: string, matches: int, version: string}> | ||
| */ | ||
| protected function performSearch(string $query, array $index): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The result from this method should be Cached, with query and index. Defined the cache key in config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0e1a628. Added result caching with configurable TTL via learn.search.results_cache_ttl and cache key format via learn.search.results_cache_key. Cache key includes query hash, version, page, and size.
lcharette
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Set hardcoded configuration value in the config file, fetch them using the config service
- Use the Sprunjer class from the core sprinkle of UserFrosting (fetch the monorepo)
- Third party package use is authorized
- Search result should be cached at the search service level. if same query is searched multiple time, cached result should be returned.
- Fix tests failures
- Add config for minimum number character in a query. Avoid search for a single character. Set 3 as default. Use Fortress to validate input
- Add possibility for pages to specify keywords in front matter, and search across keywords, or other metadata as titles. Priority should be title, keywords, other front matter
… search, and result caching Co-authored-by: lcharette <[email protected]>
Partially addressed in 0e1a628:
|
Co-authored-by: lcharette <[email protected]>
Co-authored-by: lcharette <[email protected]>
All items addressed across commits 0e1a628, 09445cb, and 54b1f48: ✅ Hardcoded values in config SearchSprunje provides Sprunje-standard pagination, response formatting via |
…dling Co-authored-by: lcharette <[email protected]>
Co-authored-by: lcharette <[email protected]>
lcharette
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Here is the Fortress reference : https://github.com/userfrosting/framework/tree/4c7080bd404105c532ad13825e65ad39ceca0d77/src/Fortress
- Here is the Sprunjer Reference : https://github.com/userfrosting/sprinkle-core/blob/87d12f9da9081350a8a225f5e3f679a00aebf9cf/app/src/Sprunje/Sprunje.php
- Error when calling the route : SearchSprunje does not use database queries. Use getModels() directly.
Make sure to run PHPUnit & PHPStan
| ], | ||
| 'latest' => '6.0', | ||
| ], | ||
| 'search' => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make key name more descriptive, result in a sub array, index should be under search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Moved learn.index configuration under learn.search.index with metadata_fields array.
| */ | ||
| protected function execute(InputInterface $input, OutputInterface $output): int | ||
| { | ||
| $this->io = new SymfonyStyle($input, $output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use UserFrosting\Bakery\WithSymfonyStyle trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Now using WithSymfonyStyle trait from UserFrosting\Bakery.
| $this->setName('search:index') | ||
| ->setDescription('Build or rebuild the search index for documentation') | ||
| ->addOption( | ||
| 'version', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"An option named "version" already exists.". Version is a reserved name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Renamed option from --version to --doc-version to avoid reserved name conflict.
| ]; | ||
|
|
||
| // Create and execute Sprunje (validates query length in constructor) | ||
| $sprunje = new SearchSprunje($this->searchService, $this->config, $sprunjeOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inject Sprunje
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. SearchController now creates SearchSprunje directly (cannot inject as it requires query-specific options). Sprunje is instantiated per request with query parameters.
| 'error' => $e->getMessage(), | ||
| ]; | ||
|
|
||
| $response->getBody()->write(json_encode($result, JSON_THROW_ON_ERROR)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw a "NoResultFound" exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Now throws NotFoundException for invalid queries instead of returning custom JSON error response.
app/src/Search/SearchIndex.php
Outdated
|
|
||
| // Extract other relevant metadata (description, tags, etc.) | ||
| $metadata = []; | ||
| $metadataFields = ['description', 'tags', 'category', 'author']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define in config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Metadata fields now defined in config at learn.search.index.metadata_fields.
app/src/Search/SearchService.php
Outdated
| * | ||
| * @return array{rows: array, count: int, count_filtered: int} | ||
| */ | ||
| public function search(string $query, ?string $version, int $page = 1, int $perPage = 10): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pagination should happens on Sprunje
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4e8b2e0. Removed pagination from SearchService. SearchSprunje now calls getIndex() and performSearch() directly, and Sprunje handles pagination.
…s all code review feedback Co-authored-by: lcharette <[email protected]>
All items addressed in 4e8b2e0:
Will run PHPUnit and PHPStan tests now. |
Co-authored-by: lcharette <[email protected]>
Co-authored-by: lcharette <[email protected]>
Implements a JSON API for searching documentation content with wildcard pattern matching, snippet generation, version-aware indexing, frontmatter metadata search, comprehensive caching, and full Sprunje integration for standardized pagination and response formatting.
Implementation
Search Services
SearchIndex: Indexes pages by stripping HTML from parsed markdown, extracts frontmatter metadata (keywords, description, tags, category, author), stores in cache with 7-day TTLSearchService: Executes searches with pre-compiled regex for wildcards (*,?), generates context snippets (configurable length), implements weighted scoring (title 10x > keywords 5x > metadata 2x > content 1x). Provides publicgetIndex()andperformSearch()methods for use by SearchSprunjeSearchSprunje: ExtendsUserFrosting\Sprinkle\Core\Sprunje\Sprunjeto provide standardized pagination, response formatting viatoResponse(), and query validation. UsesDummySearchModelto satisfy Sprunje's Eloquent builder type requirementsSearchController: API endpoint at/api/searchwith query validation (min 3 chars), throwsNotFoundExceptionfor invalid queriesBakery Integration
SearchIndexCommand: Rebuilds index viaphp bakery search:indexwith--doc-versionand--clearoptions (usesWithSymfonyStyletrait)Configuration
learn.searchconfig sectionlearn.search.indexconfig section (includes metadata_fields array)Enhanced Search Features
Pagination
{rows, count, count_filtered, listable, sortable, filterable}formatpage,size(including 'all')API Example
Response structure (Sprunje-standard format):
{ "count": 150, "count_filtered": 12, "rows": [{ "title": "Configuration Files", "slug": "configuration/config-files", "route": "/configuration/config-files", "snippet": "...configuration files are located...", "matches": 5, "version": "6.0" }], "listable": [], "sortable": [], "filterable": [] }Error handling:
Performance
Testing
Implementation Notes
learn.search.index.*structure--doc-versionoption (--version is reserved)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.