typesense / laravel-scout-typesense-driver

Laravel Scout Driver for Typesense
https://typesense.org
MIT License
131 stars 38 forks source link

Error with deleting multiple documents #6

Closed manavo closed 3 years ago

manavo commented 3 years ago

Description

Bulk deleting isn't working for me, I get an error like:

Typesense\Exceptions\ObjectNotFound: Could not find a filter field named `feedback.id` in the schema

My collection schema is:

[
  {
    "created_at":1602963950,
    "default_sorting_field":"last_reply_time",
    "fields":[
      {
        "facet":false,
        "index":true,
        "name":"application_id",
        "optional":false,
        "type":"int64"
      },
      {
        "facet":false,
        "index":true,
        "name":"message",
        "optional":true,
        "type":"string"
      },
      {
        "facet":false,
        "index":true,
        "name":"email",
        "optional":true,
        "type":"string"
      },
      {
        "facet":false,
        "index":true,
        "name":"last_reply_time",
        "optional":false,
        "type":"int64"
      },
      {
        "facet":false,
        "index":true,
        "name":"properties",
        "optional":true,
        "type":"string[]"
      },
      {
        "facet":false,
        "index":true,
        "name":"member_name",
        "optional":true,
        "type":"string"
      },
      {
        "facet":false,
        "index":true,
        "name":"member_email",
        "optional":true,
        "type":"string"
      },
      {
        "facet":false,
        "index":true,
        "name":"reply_messages",
        "optional":true,
        "type":"string[]"
      },
      {
        "facet":false,
        "index":true,
        "name":"reply_emails",
        "optional":true,
        "type":"string[]"
      }
    ],
    "name":"feedback",
    "num_documents":2459169,
    "num_memory_shards":4
  }
]

The issue might be that I haven't explicitly defined id as a field?

For what it's worth, even the example in the readme doesn't indicate that we should have id as a field.

Metadata

Typsense Version: 0.17.0 and 0.21.0 both tested

OS: Ubuntu 20.04.2 LTS

hi019 commented 3 years ago

In order to link Typesense Documents to Laravel Models, the package uses getScoutKeyName and getScoutKey (more info). The first problem I see with this is that getScoutKeyName includes the table name, ex table_name.primary_key. In the delete method, we assume that getScoutKeyName is only primary_key. In my codebase, I've overriden the getScoutKeyName for an unrelated reason, prior to this package's development, so that's probably why I didn't notice the problem.

With that said, the error you're getting is due to the delete method trying to query the affected documents based on the getScoutKeyName field which as mentioned above is invalid.

Looking at your schema, maybe getScoutKeyName should be email? Try adding these to your model:

    public function getScoutKey()
    {
        return $this->email;
    }

    public function getScoutKeyName()
    {
        return 'email';
    }
manavo commented 3 years ago

I see what you're saying, but this library also needs to work with the default configuration of Scout and models, which isn't to override the Scout Key.

So yes what you're saying would in theory work (although email would not be the correct primary key for my use case).

But we also need to make it work with the default id of the model being the id in Typesense.

manavo commented 3 years ago

I've proposed #9 to fix this, and go back to deleting one document at a time in a loop.

As much as I agree that it is a good optimisation to do if deleting multiple documents, it is more important for the library to work with normal defaults, and then try and optimise it.

Just my opinion, at it at least allows a first proper version of the library to be tagged and released. This can then be further optimised if there is a way for it to work with the defaults.

hi019 commented 3 years ago

I agree that this is a bug - perhaps I wasn't clear in my previous reply. Anyway, thanks for the contributions and I will test your PR later today.

manavo commented 3 years ago

Thanks @hi019 🙂