PHPORM-28 Add Scout engine to index into MongoDB Search #3205

GromNaN · 2024-11-13T16:35:58Z

Fix PHPORM-28

And "index" stands for a MongoDB collection with an Atlas Search index named "scout".
The collection name MUST be prefixed to prevent error of indexing in the same MongoDB Collection as the source documents.

Checklist

Add tests and ensure they pass

src/Scout/ScoutEngine.php

GromNaN · 2024-11-13T16:46:43Z

composer.json

@@ -70,7 +71,8 @@
        "laravel": {
            "providers": [
                "MongoDB\\Laravel\\MongoDBServiceProvider",
-                "MongoDB\\Laravel\\MongoDBBusServiceProvider"
+                "MongoDB\\Laravel\\MongoDBBusServiceProvider",
+                "MongoDB\\Laravel\\Scout\\ScoutServiceProvider"


This class could be placed at the root of the package and given a name consistent with the other classes.

Suggested change

"MongoDB\\Laravel\\Scout\\ScoutServiceProvider"

"MongoDB\\Laravel\\MongoDBScoutServiceProvider"

Would "ScoutServiceProvider" likely conflict with other short class names? There looks to be redundancy in both suggestions here, but I'll defer to whatever you think is most consistent.

It's been a couple of weeks since I picked up the Telescope PR but IIRC I opted to use a namespace there as there would be multiple classes. Seemed preferable than keeping a service provider in a separate namespace from other classes associated with the feature.

GromNaN · 2024-11-15T13:21:54Z

tests/Scout/ScoutEngineTest.php

+                            'fuzzy' => [
+                                'maxEdits' => 2,
+                            ],


fuzzy is required to return results for partial matches (search for "lar" must return "Laravel Framework").

GromNaN · 2024-11-15T13:22:26Z

tests/Scout/ScoutEngineTest.php

+                            'path' => [
+                                'wildcard' => '*',
+                            ],


By default we don't know on which fields to search.

GromNaN · 2024-11-15T14:22:55Z

src/Scout/ScoutEngine.php

+
+        return $model->getScoutModelsByIds($builder, $ids)
+            ->filter(fn (Model $model) => in_array($model->getScoutKey(), $ids))
+            ->sortBy(fn (Model $model) => array_search($model->getScoutKey(), $ids))


This is suspicious.

Can you elaborate?

At first I didn't understood why the index was retrieved from an array, but now I understand that it's intended to sort in the initial order of the results. It's clever. That's copilot that refactored this code copied from MeilisearchEngine: https://github.com/laravel/scout/blob/13cb6460eb9802ff95a28dc11e178b1abebdcc02/src/Engines/MeilisearchEngine.php#L290

Wouldn't flipping the array (i.e. each $id becomes a key) be more efficient to derive a sort order? That would replace repetitive array_search() calls with HashTable lookups.

GromNaN · 2024-11-15T14:23:52Z

src/Scout/ScoutEngine.php

+     */
+    public function delete($models): void
+    {
+        assert($models instanceof Collection, new TypeError(sprintf('Argument #1 ($models) must be of type %s, %s given', Collection::class, get_debug_type($models))));


assert calls compensate for the lack of native types in the parent class.

src/Scout/ScoutEngine.php

GromNaN · 2024-11-21T15:04:39Z

Integration tests require an atlas or local-atlas cluster to run.
I need to find a way to test the result as the order is not the same for each reindexation.

jmikola

Leaving an incomplete round of feedback. We'll need to schedule some time to actually go through this PR together if you need a meaningful review, as a lot of it is over my head.

tests/Models/SearchableModel.php

jmikola · 2024-11-21T19:37:25Z

tests/Models/SearchableModel.php

+    }
+
+    /**
+     * This method must be overridden when the `getScoutKey` method is also overridden,


If getScoutKey() is overridden, isn't it possible that it no longer calls getScoutKeyName() at all? It's not clear to me why this was broken out to a separate method if it's only called from getScoutKey(), and a user could just as easily override that on its own.

getScoutKeyName is necessary for searching original model objects from the results of the search engine.
Example: if in a SQL database, the "Scout Key" is scout_key, a search query will return a list of objects with the scout_key property and the following query is executed: SELECT * FROM model WHERE scout_key IN (<ids>).

That answer is over my head. My question was more basic, as I was just looking for what might actually cal getScoutKeyName(). I think I found my answer in the Searchable trait, though.

It'd have been helpful if any overridden methods mentioned that they were overriding something from the trait (maybe using @see with a reference to the base method).

tests/Models/User.php

jmikola · 2024-11-21T19:44:37Z

tests/Scout/ScoutEngineTest.php

+
+                return $builder;
+            },
+            array_replace_recursive($defaultPipeline, [


Correct me if I'm wrong, but in this case you're actually just replacing $defaultPipeline[0] since there's an equality matching between key 0 in $defaultPipeline and the lone key in the replacement array.

Basing this on my read of array_replace_recursive.

Does this actually need to use the recursive variant if you're really just replacing an aggregation pipeline stage in its entirety?

Yes, I'm using array_replace_recursive to replace the numeric key (instead of append in cas of array_merge), and this adds the filter key-value to $defaultPipeline['$search']['compound']

tests/Scout/ScoutEngineTest.php

jmikola · 2024-11-21T19:57:33Z

tests/Scout/ScoutIntegrationTest.php

+        ], $results->pluck('name', 'id')->all());
+    }
+
+    public function testItCanUseBasicSearchToFetchKeys()


Is this test seems redundant in light of testItCanUseBasicSearch()?

jmikola · 2024-11-21T20:05:41Z

src/Scout/ScoutEngine.php

+     */
+    private function wait(Closure $callback): void
+    {
+        $timeout = hrtime()[0] + self::WAIT_TIMEOUT_SEC;


This is likely a very minor edge case, but hrtime() could be unsupported and return false:

https://github.com/php/php-src/blob/php-8.4.1/ext/standard/basic_functions.stub.php#L2186

https://github.com/php/php-src/blob/php-8.4.1/ext/standard/hrtime.c#L49

https://github.com/php/php-src/blob/php-8.4.1/Zend/zend_hrtime.h#L57

But if static analysis doesn't care about this then perhaps you shouldn't either.

src/Scout/ScoutEngine.php

jmikola · 2024-11-21T20:08:29Z

src/Scout/ScoutEngine.php

+            $mapping['fields']['__soft_deleted'] ??= [
+                'type' => 'number',
+                'representation' => 'int64',
+                'indexDoubles' => false,


Noted this pertains to https://www.mongodb.com/docs/atlas/atlas-search/field-types/number-type/#configure-fts-field-type-field-properties

Consider including a comment that links to some docs on this, as it's not clear what this mapping means otherwise.

src/Scout/ScoutEngine.php

jmikola · 2024-11-27T03:41:07Z

tests/Models/SearchableModel.php

+    }
+
+    /**
+     * This method must be overridden when the `getScoutKey` method is also overridden,


That answer is over my head. My question was more basic, as I was just looking for what might actually cal getScoutKeyName(). I think I found my answer in the Searchable trait, though.

It'd have been helpful if any overridden methods mentioned that they were overriding something from the trait (maybe using @see with a reference to the base method).

jmikola · 2024-11-27T03:43:12Z

tests/Models/User.php

+    public function toSearchableArray(): array
+    {
+        return [
+            'id' => (int) $this->id,


Why is the $id property documented as a string in the class docblock, but cast to an integer here? Is it expected to be an ObjectId within the database that's represented as a string in a hydrated model instance?

src/Scout/ScoutEngine.php

jmikola · 2024-11-27T03:50:06Z

src/Scout/ScoutEngine.php

+        $collection = $this->getIndexableCollection($models);
+
+        if ($this->softDelete && $this->usesSoftDelete($models)) {
+            $models->each->pushSoftDeleteMetadata();


Noted that this calls the method in the Searchable trait.

Yes, and there is no interface for "simplicity" I guess. I don't even see how I could add an assert(...) with this sequence of magic function calls.

jmikola · 2024-11-27T03:53:38Z

src/Scout/ScoutEngine.php

+    private function usesSoftDelete(Model|EloquentCollection $model): bool
+    {
+        if ($model instanceof EloquentCollection) {
+            $model = $model->first();


Is it safe to assume that SoftDeletes is used simply because the first model has it? I assume the trait is applied on the model class and an EloquentCollection can be assumed to always contain the same class. I dunno if Eloquent supports anything like Doctrine's multi-collection inheritance, but that might make it possible for there to be various classes here (considering an edge case where only some children utilize the trait).

But even if that were the case, would a single Scout collection cover all of the classes in that hierarchy? Perhaps not.

jmikola · 2024-11-27T04:59:09Z

src/Scout/ScoutEngine.php

+    /**
+     * Delete a "search index", i.e. a MongoDB collection.
+     */
+    public function deleteIndex($name): void


I'm not sure where this would be called, but should there be some validation that it's actually an index name and not just an arbitrary MongoDB collection?

One sanity check would be to ensure it starts with $this->prefix, although that's still a bit risky per my comment below on deleteAllIndexes().

src/Scout/ScoutEngine.php

…ypeMap)

src/Scout/ScoutEngine.php

+        while (hrtime()[0] < $timeout) {
+            if ($callback()) {
+                return;
+            }


github-advanced-security bot found potential problems Nov 13, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN changed the title ~~PHPORM-28 Add Scout engined to index into MongoDB Search~~ PHPORM-28 Add Scout engine to index into MongoDB Search Nov 14, 2024

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from fa3e0a9 to 12f3323 Compare November 15, 2024 13:20

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN force-pushed the PHPORM-28 branch from 12f3323 to 3a23e91 Compare November 15, 2024 14:20

GromNaN commented Nov 15, 2024

View reviewed changes

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

src/Scout/ScoutEngine.php Fixed Show fixed Hide fixed

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 13ff6ec to 4647cf0 Compare November 20, 2024 14:44

GromNaN marked this pull request as ready for review November 21, 2024 15:03

GromNaN requested a review from a team as a code owner November 21, 2024 15:03

GromNaN requested a review from jmikola November 21, 2024 15:03

jmikola reviewed Nov 21, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from 1b39753 to 95343b1 Compare November 23, 2024 09:57

jmikola reviewed Nov 27, 2024

View reviewed changes

GromNaN force-pushed the PHPORM-28 branch 2 times, most recently from ee6edd2 to 3d030ba Compare December 2, 2024 11:40

GromNaN added 8 commits December 17, 2024 17:32

PHPORM-28 Add Scout engined to index into MongoDB Search

97a3ad2

WIP tests

c9c9bcf

Pagination

8c79ada

Implement where/whereIn/whereNotIn

cb673ba

Complete unit tests

bd401b9

Fix integration tests

3580d4e

Factorize integration tests

2e5d46f

Clean-ups

2b6c8de

GromNaN added 9 commits December 17, 2024 17:32

Rename the ScoutServiceProvider

29ae17f

Improve search query and integration tests reliability

4e06ddf

Fix phpstan issues

f2c9f71

Remove unused getAtlasSearchDefinition

c92a25e

Get map algo from Meilisearch implem

4852062

Cleanups

c3fbdb3

Run atlas-local with search features locally

6afc320

Cleaning

caaf925

Convert __soft_deleted to bool, and return root documents as array (t…

3b1d68e

…ypeMap)

GromNaN force-pushed the PHPORM-28 branch from 3d030ba to 3b1d68e Compare December 17, 2024 16:32

Remove unspecified searchableMapping

831fa21

GromNaN force-pushed the PHPORM-28 branch from 69fe000 to 831fa21 Compare December 17, 2024 18:55

Remove unsafe deleteAllIndexes

73f06ef

github-advanced-security bot found potential problems Dec 17, 2024

View reviewed changes

Fix incorrect convertion of UUID to int

80b0c38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

GromNaN commented Nov 13, 2024 •

edited

Loading

GromNaN Nov 13, 2024

jmikola Nov 21, 2024

GromNaN Nov 15, 2024

GromNaN Nov 15, 2024

GromNaN Nov 15, 2024

jmikola Nov 21, 2024

GromNaN Nov 21, 2024

jmikola Nov 27, 2024

GromNaN Nov 15, 2024 •

edited

Loading

GromNaN commented Nov 21, 2024 •

edited

Loading

jmikola left a comment

jmikola Nov 21, 2024

GromNaN Nov 21, 2024

jmikola Nov 27, 2024

jmikola Nov 21, 2024

GromNaN Nov 21, 2024

jmikola Nov 21, 2024

jmikola Nov 21, 2024

jmikola Nov 21, 2024

jmikola Nov 27, 2024

jmikola Nov 27, 2024

jmikola Nov 27, 2024

GromNaN Dec 17, 2024

jmikola Nov 27, 2024

jmikola Nov 27, 2024

	"MongoDB\\Laravel\\Scout\\ScoutServiceProvider"
	"MongoDB\\Laravel\\MongoDBScoutServiceProvider"

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

Are you sure you want to change the base?

PHPORM-28 Add Scout engine to index into MongoDB Search #3205

Conversation

GromNaN commented Nov 13, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

GromNaN commented Nov 21, 2024 • edited Loading

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Nov 13, 2024 •

edited

Loading

GromNaN Nov 15, 2024 •

edited

Loading

GromNaN commented Nov 21, 2024 •

edited

Loading