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

Add Configurable Scout Key Type #752

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

geisi
Copy link
Contributor

@geisi geisi commented Jul 20, 2023

Hello,

this PR improves the Scout integration when using custom keys for Searchable Models.

Problem

This problem was already addressed in the following issue:
#741

In one of our applications, we use incrementing IDs as the primary key for our models. Additionally, we are using the HasUuids trait to add a uuid column on top of the incrementing id. We did that because we didn't want to expose our incrementing IDs to the front end (and the search engine) due to security concerns.

We have done that by modifying the `getScouteKey' method.

    public function getScoutKey(): string
    {
        return $this->uuid;
    }

So far, so good indexing of these models works. But when we want to use the Model::search()->get() method within the query builder, we get a database exception because Scout thinks our primary key is an integer.

This happens within the queryScoutModelsByIds method. The scout package determines the used key type (integer or string) via the Eloquent getKeyType() method. This behavior is not correct in this case. When you can define a custom Scout key column, you should also be able to define the column type of this custom key.

Workaround

Before this PR, The workaround for this problem is to override the queryScoutModelsByIds() method within the models so the builder is doing a whereIn() instead of a whereIntegerRaw() condition.

Solution

I have added a new getScoutKeyType(): string method, which, by default, returns the model key type. This new method allows you to customize the key type Scout uses for querying models.

This should not be a breaking change since it does not change the default behavior of Scout, but it adds a little more flexibility in how custom key types can be used in Scout. There is a minor risk of naming collisions, but since the new method is prefixed with the scout word, it should be no problem.

@driesvints
Copy link
Member

Hey @mmachatschek, does this change look okay to you?

@geisi
Copy link
Contributor Author

geisi commented Jul 21, 2023

I think if this PR gets merged, we should also create a PR for the Docs:

By default, Scout will use the primary key of the model as the model's unique ID / key that is stored in the search index. If you need to customize this behavior, you may override the getScoutKey, getScoutKeyName and getScoutKeyType methods on the model:

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Laravel\Scout\Searchable;

class User extends Model
{
    use Searchable;

    /**
     * Get the value used to index the model.
     */
    public function getScoutKey(): mixed
    {
        return $this->email;
    }

    /**
     * Get the key name used to index the model.
     */
    public function getScoutKeyName(): mixed
    {
        return 'email';
    }

    /**
     * Get the auto-incrementing key type for querying models.
     *
     * @return string
     */
    public function getScoutKeyType()
    {
        return 'string';
    }

This current example in the docs won't work with Model::search() when you use incrementing IDs as the model's primary key.

@mmachatschek
Copy link
Contributor

mmachatschek commented Jul 21, 2023

@driesvints @geisi the changes look good to me, even though I would prefer if we could add a test for this as well.

@geisi
Copy link
Contributor Author

geisi commented Jul 21, 2023

@mmachatschek Yes, you are right.

In the current test suite, I did not find any tests for the currently existing behavior, so I have not added some.

But I can create tests for the whole scoutKey logic.

@mmachatschek
Copy link
Contributor

@geisi yes that would be great

@geisi
Copy link
Contributor Author

geisi commented Jul 21, 2023

@mmachatschek Tests are done (at least) for the new logic

@taylorotwell taylorotwell merged commit 7886876 into laravel:10.x Jul 27, 2023
5 checks passed
@geisi geisi deleted the add_configurable_scout_key_type branch July 27, 2023 15:54
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.

4 participants