Skip to content
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

Fix: typesense missing scoutmetadata #868

Draft
wants to merge 11 commits into
base: 10.x
Choose a base branch
from

Conversation

arminwinkt
Copy link

This Pull Request fixes the missing scoutMetadata for 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.

Screenshot 2024-09-24 at 13 23 00

For me specifically the geo_distance_meters value was very intresting if you query for a geo search:

Screenshot 2024-09-24 at 13 23 12

This PR adds all this additional data to the model via scouts scoutMetadata

Copy link
Contributor

@tharropoulos tharropoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some notes on testing to cover all edge cases, otherwise looks good!

$result = $hits[$objectIdPositions[$model->getScoutKey()]] ?? [];

foreach ($result as $key => $value) {
if ($key === 'document') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()]] ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a test for the fallback of the empty array

$model->shouldReceive(['getScoutKeyName' => 'id']);
$model->shouldReceive('getScoutModelsByIds')->andReturn(
Collection::make([
new SearchableModel([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some more response data to verify handling of multiple results?

@taylorotwell taylorotwell marked this pull request as draft September 25, 2024 15:17
@taylorotwell
Copy link
Member

Drafting pending response to feedback.

@arminwinkt arminwinkt marked this pull request as ready for review October 3, 2024 20:52
@arminwinkt
Copy link
Author

Thank you @tharropoulos for your feedback.
Added some more tests as you mentioned.
Looking forward to your second review.

@tharropoulos
Copy link
Contributor

Thank you @tharropoulos for your feedback. Added some more tests as you mentioned. Looking forward to your second review.

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.

@taylorotwell taylorotwell marked this pull request as draft October 4, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants