Update index settings before document import #391
Conversation
WalkthroughThe update moves the settings update operation in the Meilisearch import command to occur before batch imports for each index, rather than after. Corresponding test assertions are updated to expect settings update messages before batch import messages. No other logic, control flow, or public interface changes were made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MeilisearchImportCommand
participant SettingsUpdater
participant BatchImporter
User->>MeilisearchImportCommand: Execute import command
loop For each index
MeilisearchImportCommand->>SettingsUpdater: update(settings)
SettingsUpdater-->>MeilisearchImportCommand: Settings updated
loop For each batch
MeilisearchImportCommand->>BatchImporter: import(batch)
BatchImporter-->>MeilisearchImportCommand: Batch imported
end
end
MeilisearchImportCommand-->>User: Output progress and completion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
============================================
+ Coverage 88.37% 88.40% +0.02%
Complexity 1 1
============================================
Files 20 20
Lines 886 888 +2
============================================
+ Hits 783 785 +2
Misses 103 103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Command/MeilisearchImportCommand.php (1)
78-173:execute()breaches PHPMD thresholds – extract smaller private methodsCI flags show coupling = 14, CC = 12, NPath = 592.
Splitting the monolith will improve readability and testability:
- Extract “determineRuntimeOptions()” (parses CLI options, returns small DTO / array).
- Extract “processIndex(array $index, Options $opts, SymfonyStyle $io)” – contains the inner loop and settings update.
- Keep swapping logic in another helper.
This will drop complexity below the configured limits without behaviour changes.
♻️ Duplicate comments (1)
tests/Integration/Command/MeilisearchImportCommandTest.php (1)
441-444: Same brittleness comment applies hereConsider switching to partial-string assertions as suggested above.
🧹 Nitpick comments (1)
tests/Integration/Command/MeilisearchImportCommandTest.php (1)
209-218: Stop hard-coding the full console transcript – assert only relevant linesRelying on exact ordering of every console line is brittle; any internal change in setting iteration order (array ordering, library upgrade) will break the test while behaviour is still correct.
Refactor to:
$output = $commandTester->getDisplay(); self::assertStringContainsString('Importing for index Meilisearch\\Bundle\\Tests\\Entity\\Post', $output); self::assertStringContainsString('Indexed a batch of 6 / 6', $output); self::assertStringEndsWith("Done!\n", $output);You can still assert that each setting line appears with
assertMatchesRegularExpression()but without depending on their sequence.Repeat for other tests updated in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Command/MeilisearchImportCommand.php(1 hunks)tests/Integration/Command/MeilisearchImportCommandTest.php(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/Command/MeilisearchImportCommand.php
[warning] 24-24: PHP Mess Detector: The class MeilisearchImportCommand has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13.
[warning] 78-78: PHP Mess Detector: The method execute() has a Cyclomatic Complexity of 12. The configured cyclomatic complexity threshold is 10.
[warning] 78-78: PHP Mess Detector: The method execute() has an NPath complexity of 592. The configured NPath complexity threshold is 200.
| if ($updateSettings) { | ||
| $this->settingsUpdater->update($index['prefixed_name'], $responseTimeout, $prefix ? $prefix.$index['prefixed_name'] : null); | ||
| } | ||
|
|
There was a problem hiding this comment.
Catch & surface SettingsUpdater::update() failures to avoid silent aborts
$this->settingsUpdater->update() is now executed before any data is imported.
If this call throws (e.g. TimeOutException, connectivity issues), the whole import command terminates and no documents are indexed, potentially leaving the index empty.
Guard the call so the operator at least gets a clear-to-read error and, optionally, can continue with document import:
-if ($updateSettings) {
- $this->settingsUpdater->update($index['prefixed_name'], $responseTimeout, $prefix ? $prefix.$index['prefixed_name'] : null);
-}
+if ($updateSettings) {
+ try {
+ $this->settingsUpdater->update(
+ $index['prefixed_name'],
+ $responseTimeout,
+ $prefix ? $prefix.$index['prefixed_name'] : null
+ );
+ } catch (\Throwable $e) {
+ $output->writeln(
+ sprintf('<error>Failed updating settings for %s: %s</error>', $index['prefixed_name'], $e->getMessage())
+ );
+ // Decide whether to `return self::FAILURE;` or continue importing documents
+ throw $e;
+ }
+}This mirrors the explicit error handling already present inside formatIndexingResponse().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($updateSettings) { | |
| $this->settingsUpdater->update($index['prefixed_name'], $responseTimeout, $prefix ? $prefix.$index['prefixed_name'] : null); | |
| } | |
| if ($updateSettings) { | |
| try { | |
| $this->settingsUpdater->update( | |
| $index['prefixed_name'], | |
| $responseTimeout, | |
| $prefix ? $prefix.$index['prefixed_name'] : null | |
| ); | |
| } catch (\Throwable $e) { | |
| $output->writeln( | |
| sprintf( | |
| '<error>Failed updating settings for %s: %s</error>', | |
| $index['prefixed_name'], | |
| $e->getMessage() | |
| ) | |
| ); | |
| // Decide whether to return self::FAILURE; or continue importing documents | |
| throw $e; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Command/MeilisearchImportCommand.php around lines 117 to 120, the call to
$this->settingsUpdater->update() can throw exceptions that cause the entire
import command to terminate silently without indexing any documents. Wrap this
call in a try-catch block to catch exceptions like TimeOutException or
connectivity issues, then log or display a clear error message to the operator.
Optionally, allow the import process to continue despite the failure, similar to
the error handling in formatIndexingResponse().
There was a problem hiding this comment.
I think this is for a next PR deciding if partial import, when some indexes in the middle fails is okay 😛
There was a problem hiding this comment.
@norkunas Fair point! You're right that this PR is focused on the core timeout fix, and the error handling strategy for partial imports is indeed a separate concern that deserves its own focused discussion.
Would you like me to create a follow-up issue to track the error handling improvement for future consideration?
✏️ Learnings added
Learnt from: norkunas
PR: meilisearch/meilisearch-symfony#391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.057Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a follow-up issue to track the error handling improvement for settings updates.
@norkunas I've created a follow-up issue to track the error handling improvements for settings updates. The issue captures the context from our discussion and outlines the key considerations around partial import behavior when settings updates fail.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for error handling improvements in MeilisearchImportCommand
gh issue create \
--title "Improve error handling for settings updates in MeilisearchImportCommand" \
--body "## Problem
After PR #391, settings updates now occur before document import in the MeilisearchImportCommand. If \`SettingsUpdater::update()\` throws an exception (e.g., TimeOutException, connectivity issues), the entire import command terminates silently without indexing any documents, potentially leaving the index empty.
## Proposed Solution
Add error handling around the settings update call to:
- Catch exceptions and provide clear error messages to operators
- Decide whether to fail fast or continue with document import when settings updates fail
- Consider partial import behavior when some indexes fail in the middle of processing
## Context
This issue was identified during review of PR #391 where the settings update timing was changed to resolve timeout issues.
**Original PR:** https://github.com/meilisearch/meilisearch-symfony/pull/391
**Discussion:** https://github.com/meilisearch/meilisearch-symfony/pull/391#discussion_r2253088004
The error handling strategy for partial imports deserves focused discussion separate from the core timeout fix implemented in PR #391." \
--assignee norkunas
Length of output: 1281
@norkunas Perfect! I've successfully created issue #392 to track the error handling improvements for settings updates in the MeilisearchImportCommand.
The issue includes:
- Clear explanation of the problem (settings failures causing silent import aborts)
- Proposed solutions for error handling and partial import considerations
- Context linking back to this PR and our discussion
- Assignment to you for future prioritization
This keeps the current PR focused on the core timeout fix while ensuring the error handling improvements don't get lost. Thanks for keeping the scope clean! 🎯
🧬 Code Graph Analysis Results
Relevant snippets for understanding src/Command/MeilisearchImportCommand.php
File: src/Model/Aggregator.php (lines 12-80)
abstract class Aggregator implements NormalizableInterface
{
protected string $objectID;
protected $entity;
/**
* @param object $entity
*/
public function __construct($entity, array $entityIdentifierValues)
{
$this->entity = $entity;
if (\count($entityIdentifierValues) > 1) {
throw new InvalidEntityForAggregator("Aggregators don't support more than one primary key.");
}
$this->objectID = (string) reset($entityIdentifierValues);
}
/**
* Returns the entities class names that should be aggregated.
*
* @return string[]
*/
public static function getEntities(): array
{
return [];
}
/**
* Returns an entity id from the provided object id.
*/
public static function getEntityIdFromObjectId(string $objectId): string
{
return $objectId;
}
/**
* Returns an entity class name from the provided object id.
*
* @throws EntityNotFoundInObjectID
*/
public static function getEntityClassFromObjectID(string $objectId): string
{
$type = explode('::', $objectId)[0];
if (\in_array($type, static::getEntities(), true)) {
return $type;
}
throw new EntityNotFoundInObjectID("Entity class from ObjectID $objectId not found.");
}
public function normalize(NormalizerInterface $normalizer, ?string $format = null, array $context = []): array
{
return array_merge(['objectID' => $this->objectID], $normalizer->normalize($this->entity, $format, $context));
}
}- Abstract class for aggregators used in indexing.
- Has static method
getEntities()returning array of entity class names aggregated. - Used in
MeilisearchImportCommand::entitiesToIndex()to expand aggregator entities into their constituent entities.
File: src/Services/SettingsUpdater.php (lines 17-79)
final class SettingsUpdater
{
private const DEFAULT_RESPONSE_TIMEOUT = 5000;
private Client $searchClient;
private EventDispatcherInterface $eventDispatcher;
private Collection $configuration;
public function __construct(SearchService $searchService, Client $searchClient, EventDispatcherInterface $eventDispatcher)
{
$this->searchClient = $searchClient;
$this->eventDispatcher = $eventDispatcher;
$this->configuration = $searchService->getConfiguration();
}
/**
* @param non-empty-string $indice
* @param positive-int|null $responseTimeout
*/
public function update(string $indice, ?int $responseTimeout = null, ?string $prefixedName = null): void
{
$index = (new Collection($this->configuration->get('indices')))->firstWhere('prefixed_name', $indice);
if (!\is_array($index)) {
throw new InvalidIndiceException($indice);
}
if (!\is_array($index['settings'] ?? null) || [] === $index['settings']) {
return;
}
$indexName = $prefixedName ?? $index['prefixed_name'];
$indexInstance = $this->searchClient->index($indexName);
$responseTimeout = $responseTimeout ?? self::DEFAULT_RESPONSE_TIMEOUT;
foreach ($index['settings'] as $variable => $value) {
$method = \sprintf('update%s', ucfirst($variable));
if (!method_exists($indexInstance, $method)) {
throw new InvalidSettingName(\sprintf('Invalid setting name: "%s"', $variable));
}
if (isset($value['_service']) && $value['_service'] instanceof SettingsProvider) {
$value = $value['_service']();
} elseif (('distinctAttribute' === $variable || 'proximityPrecision' === $variable || 'searchCutoffMs' === $variable) && \is_array($value)) {
$value = $value[0] ?? null;
}
// Update
$task = $indexInstance->{$method}($value);
// Get task information using uid
$indexInstance->waitForTask($task['taskUid'], $responseTimeout);
$task = $indexInstance->getTask($task['taskUid']);
if ('failed' === $task['status']) {
throw new TaskException($task['error']['message']);
}
$this->eventDispatcher->dispatch(new SettingsUpdatedEvent($index['class'], $indexName, $variable));
}
}
}- Service used in
MeilisearchImportCommandto update index settings. - Throws exceptions on invalid index or setting.
- Waits for Meilisearch task completion and dispatches events on updates.
File: src/Command/IndexCommand.php (lines 13-60)
abstract class IndexCommand extends Command
{
protected const DEFAULT_RESPONSE_TIMEOUT = 5000;
protected SearchService $searchService;
private string $prefix;
public function __construct(SearchService $searchService)
{
$this->searchService = $searchService;
$this->prefix = $this->searchService->getConfiguration()->get('prefix');
parent::__construct();
}
protected function getEntitiesFromArgs(InputInterface $input, OutputInterface $output): Collection
{
$indices = new Collection($this->searchService->getConfiguration()->get('indices'));
$indexNames = new Collection();
if ($indexList = $input->getOption('indices')) {
$list = explode(',', $indexList);
$indexNames = (new Collection($list))->transform(function (string $item): string {
// Check if the given index name already contains the prefix
if (!str_contains($item, $this->prefix)) {
return $this->prefix.$item;
}
return $item;
});
}
if (0 === \count($indexNames) && 0 === \count($indices)) {
$output->writeln(
'<comment>No indices specified. Please either specify indices using the cli option or YAML configuration.</comment>'
);
return new Collection();
}
if (\count($indexNames) > 0) {
return $indices->reject(fn (array $item) => !\in_array($item['prefixed_name'], $indexNames->all(), true));
}
return $indices;
}
}- Base command class for Meilisearch commands.
- Provides method
getEntitiesFromArgs()to get configured indices filtered by CLI option. - Uses prefix from configuration to normalize index names.
File: src/SearchService.php (selected method signatures)
public function getConfiguration(): Collection;
public function isSearchable($className): bool;
public function index(ObjectManager $objectManager, $searchable): array;
public function deleteByIndexName(string $indexName): ?array;getConfiguration()returns configuration collection including indices.isSearchable()checks if a class is searchable.index()indexes entities and returns an array of indexing responses.deleteByIndexName()deletes an index by name.
File: src/Collection.php (selected methods)
- Implements array-like collection with methods:
all(): returns all items as array.forget($keys): removes items by keys.reject($callback): filters out items matching callback.transform($callback): transforms items by callback.firstWhere($key, $operator = null, $value = null): returns first item matching key-value.unique($key = null, bool $strict = false): returns unique items.
- Used extensively in
MeilisearchImportCommandfor index collections.
File: src/EventListener/ConsoleOutputSubscriber.php (lines 11-31)
final class ConsoleOutputSubscriber implements EventSubscriberInterface
{
private OutputStyle $io;
public function __construct(OutputStyle $io)
{
$this->io = $io;
}
public function afterSettingsUpdate(SettingsUpdatedEvent $event): void
{
$this->io->writeln('<info>Setting "'.$event->getSetting().'" updated of "'.$event->getIndex().'".</info>');
}
public static function getSubscribedEvents(): array
{
return [
SettingsUpdatedEvent::class => 'afterSettingsUpdate',
];
}
}- Event subscriber that outputs messages to console when settings are updated.
- Used in
MeilisearchImportCommandto provide console feedback.
File: src/Exception/TaskException.php (lines 7-9)
final class TaskException extends \LogicException
{
}- Exception thrown when a Meilisearch task fails.
- Used in
MeilisearchImportCommand::formatIndexingResponse().
File: src/Engine.php (index method, lines 28-58)
public function index($searchableEntities): array
{
if ($searchableEntities instanceof SearchableEntity) {
$searchableEntities = [$searchableEntities];
}
$data = [];
foreach ($searchableEntities as $entity) {
$searchableArray = $entity->getSearchableArray();
if ([] === $searchableArray) {
continue;
}
$indexUid = $entity->getIndexUid();
if (!isset($data[$indexUid])) {
$data[$indexUid] = [];
}
$data[$indexUid][] = $searchableArray + ['objectID' => $this->normalizeId($entity->getId())];
}
$result = [];
foreach ($data as $indexUid => $objects) {
$result[$indexUid] = $this->client
->index($indexUid)
->addDocuments($objects, 'objectID');
}
return $result;
}- Indexes entities by grouping them per index and adding documents to Meilisearch.
- Returns array of API responses keyed by index name.
File: src/Command/MeilisearchUpdateSettingsCommand.php (lines 19-93)
- Command class that updates settings for indices.
- Has similar
entitiesToIndex()method asMeilisearchImportCommandto expand aggregators. - Uses
SettingsUpdaterservice to update settings. - Adds
ConsoleOutputSubscriberfor console feedback.
File: src/Command/MeilisearchCreateCommand.php (lines 20-113)
- Command class that creates indices.
- Similar pattern of expanding aggregators in
entitiesToIndex(). - Uses
SettingsUpdaterto update settings optionally. - Adds
ConsoleOutputSubscriberfor console feedback.
File: src/Command/MeilisearchDeleteCommand.php (lines 14-50)
- Command class that deletes indices.
- Uses
SearchService::deleteByIndexName()to delete indices. - Provides console output for success or failure.
File: src/Command/MeilisearchClearCommand.php (lines 12-47)
- Command class that clears documents from indices.
- Uses
SearchService::clear()method. - Provides console output for success or failure.
Summary
The MeilisearchImportCommand class is a Symfony console command that imports entities into Meilisearch indices. It uses:
SearchServiceto check if entities are searchable and to perform indexing.SettingsUpdaterto update index settings optionally.ConsoleOutputSubscriberto provide console feedback on events.Collectionclass to manage index collections and filtering.Aggregatorabstract class to expand aggregator entities into their constituent entities.Engineclass to perform the actual indexing calls to Meilisearch.- Exception classes like
TaskExceptionto handle errors during indexing.
The command supports options for batch size, skipping batches, response timeout, updating settings, and swapping indices to avoid downtime. It handles batching of entities, waits for indexing tasks to complete, and can swap temporary indices with live ones.
These related classes and methods provide context for how the import command operates, manages entities, updates settings, and interacts with Meilisearch.
🧠 Learnings used
Learnt from: norkunas
PR: meilisearch/meilisearch-symfony#391
File: src/Command/MeilisearchImportCommand.php:117-120
Timestamp: 2025-08-05T04:46:01.057Z
Learning: In meilisearch-symfony project, the maintainer norkunas prefers to keep PRs focused and defer error handling improvements for settings updates to separate PRs, particularly when it involves decisions about partial import behavior when some indexes fail.
2 participants
Pull Request
Related issue
Fixes #376
What does this PR do?
This could be seen as a minor BC break because it changes command output order, but I think this is fine given that settings update always timeouts if index is big after document import
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
Bug Fixes
Tests