Skip to content
This repository has been archived by the owner on May 13, 2021. It is now read-only.

get index or create it with the model primary key if not exist #41

Merged
merged 1 commit into from
Sep 13, 2020

Conversation

shokme
Copy link
Collaborator

@shokme shokme commented Sep 2, 2020

Resolve #36

@bidoubiwa
Copy link

bidoubiwa commented Sep 2, 2020

Hey @shokme
Thanks for this new contribution!
Could you add a test that tries getOrCreateIndex two times in a row on the same index ? Just to be sure it is working correctly when the index already exists.

Also, is the primary key optionnal ? Could you add a test without any primary key given?

@shokme
Copy link
Collaborator Author

shokme commented Sep 2, 2020

Could you add a test that tries getOrCreateIndex two times in a row on the same index ? Just to be sure it is working correctly when the index already exists.

getOrCreateIndex is from the sdk, usually I avoid to test thing already tested. Then, I don't know if this is possible to do with mocked test ?

Also, is the primary key optionnal ? Could you add a test without any primary key given?

By default Laravel will set a $primaryKey = id in its model, each time a model is created/updated, even if the primary key is optional Laravel will always have it.

But, should we keep the possibility to create an index by using the cli ?

php artisan scout:index name -k optionalKey

if we are able to handle it automatically, I don't see this cli useful anymore (even If we have to keep a part of it to delete an index).

@bidoubiwa
Copy link

Ah yes, redundant tests are bad, you are right.

By default Laravel will set a $primaryKey = id in its model, each time a model is created/updated, even if the primary key is optional Laravel will always have it.

MeiliSearch infers the id when non is provided. In the documentation it is said that

If no primary key has been given either when the index was created or as a parameter of the add documents route, the primary key will be searched in the first document sent.
MeiliSearch will search for an attribute that contains the string id in a case-insensitive manner (i.e, uid, MovieId, ID, 123id123)

Does this mean that if I create a index with the laravel-scout it will enforce id as a primary key ? Meaning it removes the possibility that MeiliSearch infers for example uid as the primary key on document creation?

But, should we keep the possibility to create an index by using the cli ?

I'm not enough aware of the use case of this SDK, I prefer waiting on @curquiza opinion on this

@elfeffe
Copy link

elfeffe commented Sep 3, 2020

@bidoubiwa I think Meilisearch must not infer the ID in this case, it should use Laravel $primaryKey, which can or cannot include the string "id".
If I have a product, I think UPC to SKU can be the key, that's the reason why Laravel lets you set a $primaryKey.
In my opinion, the MS primary key should be the $primaryKey that you set in Laravel.
In fact, in some e-commerce project that I have created using Algolia, I use SKU as the primaryKey (foto24.com for example)

@shokme
Copy link
Collaborator Author

shokme commented Sep 3, 2020

it will be less error prone to let laravel handle the PK of meilisearch as you can have a PK != *id*

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

The MeiliSearch API will manage the automatic index creation in the future (meilisearch/meilisearch#918), but until the official release, this is a great fix!

@shokme shokme merged commit 91742b7 into meilisearch:master Sep 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I can't index my documents (they are ok at Algolia)
4 participants