pdphilip / laravel-elasticsearch

Laravel Elasticsearch: An Elasticsearch implementation of Laravel's Eloquent ORM
MIT License
86 stars 16 forks source link

[Fixed] Accessors are not working on "Multi fields" using ".keyword" #11

Closed alirezaImani-f4L3e closed 6 months ago

alirezaImani-f4L3e commented 7 months ago

Describe the bug

I have a model named Audit and it has a auditable_type field which is multi field with types keyword and text . I tried to use accessors to format the retrevived auditable_type and realized that accessors are not working . even accessor function did not called .

Audit.php

<?php

...

class Audit extends Model
{
    ...

    public function getAuditableTypeAttribute(string $value)
    {
        return collect(array_filter(explode('\\', $value), function ($item) {
            return $item != null && $item != '';
        }))->last();
    }

}

AuditRepository.php

$column = "auditable_type.keyword";
Audit::distinct($column)->get($column)->pluck([$column]); // we expect that accessor format the field .

To Reproduce

Create a model using laravel-elasticsearch package and try to implement an accessor for a text field .

Expected behavior

We Expect that model accessor format the desired field correctly .

pdphilip commented 7 months ago

Hi, is hard to see what you're trying to do, but I can spot a few issues with what you have shared.

Your getAuditableTypeAttribute() accessor is faulty. Accessor methods do not have arguments, like (string $value). They work as attributes of a current record (or an instantiated model class):

  public function getFullNameAttribute()
  {
      return $this->first_name.' '.$this->last_name;   // returns $user->full_name
  }

Secondly, you can't perform database actions on accessors since these columns do not exist at a database level. They are handled by Laravel further up the stack. Getters create attributes from existing records, and setters manipulate fields before saving.

If you try to find, sort, etc on an accessor-defined field with MySQL you will run into the same limitations.

alirezaImani-f4L3e commented 7 months ago

About first tip :

based on the laravel@8 documentation :

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class User extends Model
{
    /**
     * Get the user's first name.
     *
     * @param  string  $value
     * @return string
     */
    public function getFirstNameAttribute($value)
    {
        return ucfirst($value);
    }
}

As you can see, the original value of the column is passed to the accessor, allowing you to manipulate and return the value

And about second tip :

I have been mentioned that auditable_type is a text field on my elasticsearch instance and therefore it exists at databse level .

Screenshot 2024-01-30 at 14 03 39

Anyway I guess the problem is about using .keyword because of that auditable_type is multi field .

pdphilip commented 7 months ago

I see, I was mistaken on both.

I did run a simple test on my end and it should work, this is what I did:

public function getCodeAttribute(int $value)
{
    return $value + 1000000;
}
// code is a field with a possible value of 1 - 5

UserLog::distinct('code')->get('code'); returned:

Screenshot 2024-01-30 at 12 04 00

UserLog::distinct('code')->get('code')->pluck(['code']); produced:

Screenshot 2024-01-30 at 12 04 09

The .keyword would throw it off for sure, what happens if you omit it? ie:

$column = "auditable_type";
Audit::distinct($column)->get($column)->pluck([$column]);
alirezaImani-f4L3e commented 7 months ago

I'm facing this error when i ignore the .keyword :

[{\"type\":\"illegal_argument_exception\",\"reason\":\"Fielddata is disabled on [auditable_type] in [audits]. Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [auditable_type] in order to load field data by uninverting the inverted index. Note that this can use significant memory.\"}]

pdphilip commented 7 months ago

Can you try re-index by having the field as a keyword first, then (assuming you want full-text search for this field) as text

$index->keyword('auditable_type');

..then:

$index->text('auditable_type');

Then the operation should use the keyword type as the default and you can omit .keyword

See: https://github.com/pdphilip/laravel-elasticsearch?tab=readme-ov-file#migrations

Let me know

alirezaImani-f4L3e commented 7 months ago

I have been re-index with your explained changes and sorting and filtering have been fixed . but in my search i have to use .text extenstion for this field .

pdphilip commented 7 months ago

Yeah, multi-field is specific to Elasticsearch, so the package needs to assume which type to apply the query to, which will be the first indexed type. Extending by .keyword or .text is a tool to give us control over that. But you would need to use one or the other no matter what.

The issue is these extensions are not Laravel native so they are throwing off some of the built-in modeling/data features of Laravel.

I recognize tho that it would be useful to not have to worry about index ordering so I'll leave this open for now and will see if I can get to a reasonable upgrade where .keyword doesn't affect Laravel's accessors.

Which version of this package are you using?

alirezaImani-f4L3e commented 7 months ago

I'm using laravel@8 with pdphilip/laravel-elasticsearch@2.8 . Thanks for your attention .

pdphilip commented 6 months ago

@alirezaImani-f4L3e the latest version v2.8.7 fixes this. You'll be able to execute distinct queries with the keyword flag, and the mutators will parse it correctly:

$column = "auditable_type.keyword";
Audit::distinct()->get($column);