Fix: typesense missing scoutmetadata #868
Conversation
fix: missing scout metadata for typesense search
…-metadata Revert "fix: missing scout metadata for typesense search"
| $result = $hits[$objectIdPositions[$model->getScoutKey()]] ?? []; | ||
|
|
||
| foreach ($result as $key => $value) { | ||
| if ($key === 'document') { |
There was a problem hiding this comment.
Could you add a test for this case specifically?
| @@ -392,6 +392,19 @@ public function map(Builder $builder, $results, $model) | |||
| ->filter(static function ($model) use ($objectIds) { | |||
| return in_array($model->getScoutKey(), $objectIds, false); | |||
| }) | |||
| ->map(static function ($model) use ($hits, $objectIdPositions) { | |||
| $result = $hits[$objectIdPositions[$model->getScoutKey()]] ?? []; | |||
There was a problem hiding this comment.
There should be a test for the fallback of the empty array
tests/Unit/TypesenseEngineTest.php
Outdated
| $model->shouldReceive(['getScoutKeyName' => 'id']); | ||
| $model->shouldReceive('getScoutModelsByIds')->andReturn( | ||
| Collection::make([ | ||
| new SearchableModel([ |
There was a problem hiding this comment.
Could you add some more response data to verify handling of multiple results?
|
Drafting pending response to feedback. |
|
Thank you @tharropoulos for your feedback. |
All looks well, but could you lastly add some integration tests as well? As we are mocking the Typesense Client itself, we could miss some details in the process. Otherwise, this covers every branch inside the function. |
@tharropoulos I have need of this functionality for Typesense and was ready to write the tests myself, but I see that by default the codebase excludes the integration tests for external services in phpunit.xml.dist. What is your recommendation to write and test one to meet your requirements? |
One thing I did is run the integration tests locally, to ensure that it works for an actual Typesense server instance. As soon as you add the tests to SC, we'll be able to test them against a local Typesense instance |
@tharropoulos Please forgive me, I am new to contributing to Laravel packages. I played around with the integration tests last night. It seems like I would need to install this package on a functioning Laravel app to run the external-network group integration tests. Does that sound about right? If not, how are you guys running them? Otherwise the current integration tests fail as they are for the group external-network since it cannot grab a working config even after making the needed adjustments to phpunit.xml.dist to add the TYPESENSE_API_KEY env variable. |
On my end I just remove the skip group from the phpunit config and run:
|
For me all 8 current integration tests fail with the following error.
I know that error can be rectified by setting up the scout config file, but that doesn't appear to make a difference in the integration tests. Just wondering what I need to do to get it up and running or what is different about your testing environment from mine. |
In the User::class => [
'collection-schema' => [
'fields' => [
[
'name' => 'id',
'type' => 'string',
],
[
'name' => 'name',
'type' => 'string',
],
[
'name' => 'created_at',
'type' => 'int64',
],
],
'default_sorting_field' => 'created_at',
],
'search-parameters' => [
'query_by' => 'name'
],
],while also importing the fixture itself: use Laravel\Scout\Tests\Fixtures\User; |
Yes, I can confirm I have tried and I am doing all of those things, yet still getting the aforementioned error messages for each Typesense integration test. Thank you for taking the time to troubleshoot. |
|
Hi @tharropoulos. I am giving this another go. In addition to your instructions above I added the following line to phpunit.xml.dist <env name="TYPESENSE_API_KEY" value="xyz"/>I imported the user fixture into the Scout config file, and it seems all the Typesense integration tests (except one) return an empty array instead of the expected data, resulting in 7 failures out of 8 assertions and 8 tests. This is also true with a fresh clone of the latest version of Scout. Are you getting this too? |
Hey there, yeah, seems so. I got around it by first removing the - 'id' => (int) $this->id,
+ 'id' => (string) $this->id,and adding this to the TestCase abc: use Orchestra\Testbench\Concerns\WithWorkbench;
use function Orchestra\Testbench\artisan;
+use Illuminate\Support\Facades\DB;
+
use function Orchestra\Testbench\remote;
abstract class TestCase extends \Orchestra\Testbench\TestCase
{
use WithWorkbench;
+ protected function getEnvironmentSetUp($app)
+ {
+ // Setup SQLite in-memory database
+ $app['config']->set('database.default', 'testbench');
+ $app['config']->set('database.connections.testbench', [
+ 'driver' => 'sqlite',
+ 'database' => ':memory:',
+ 'prefix' => '',
+ ]);
+ }
+
+ protected function setUp(): void
+ {
+ parent::setUp();
+
+ $this->artisan('migrate:fresh');
+
+ DB::reconnect();
+
+ if (method_exists($this, 'defineDatabaseMigrations')) {
+ $this->defineDatabaseMigrations();
+ }
+
+ if (method_exists($this, 'afterRefreshingDatabase')) {
+ $this->afterRefreshingDatabase();
+ }
+ }This way it actually indexes the collection to Typesense. The tests may not match the actual sort order (depends on your typesense version), so just confirm the length / existence of the metadata itself in the test |
This Pull Request fixes the missing
scoutMetadatafor typesense search results.As written in the Typesense documentation, there are additional data that gets returned by typesense.
For example
highlights, which shows what to highlight in your search results.For me specifically the
geo_distance_metersvalue was very intresting if you query for a geo search:This PR adds all this additional data to the model via scouts
scoutMetadata