pdphilip / laravel-elasticsearch

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

[Fixed] Error when running migrations: Call to undefined Method PDPhilip\Elasticsearch\Schema\Builder::hasTable() #14

Closed henrygermany closed 6 months ago

henrygermany commented 6 months ago

Package version

v2.9.7 Laravel v9.52.16 Elasticssearch 8.12.2

Describe the bug

First, thanks so much for this great package. Your dedication for implementing the whole Eloquent feature set is really impressive. As I understand it, the migration feature of this package should be usable like Eloquent's migrations, right? So, creating schema and then migrating with artisan migrate.

(Edit) Elasticsearch is the only data source for my project. I saw that in your laravel-elasticsearch-tests repository, all migrations get called by client code, not by the laravel framework like it's the case with artisan migrate. If the migration feature of this package is not intended to be used like this, please excuse me. (End of Edit)

After running artisan migrate, Laravel throws the following error:

 Call to undefined method PDPhilip\Elasticsearch\Schema\Builder::hasTable()

  at vendor/laravel/framework/src/Illuminate/Database/Migrations/DatabaseMigrationRepository.php:169
    165▕     public function repositoryExists()
    166▕     {
    167▕         $schema = $this->getConnection()->getSchemaBuilder();
    168▕
  ➜ 169▕         return $schema->hasTable($this->table);
    170▕     }
    171▕
    172▕     /**
    173▕      * Delete the migration repository data store.

      +21 vendor frames
  22  artisan:35
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

To Reproduce

Steps to reproduce the behavior:

use Illuminate\Database\Migrations\Migration; use PDPhilip\Elasticsearch\Schema\IndexBlueprint; use PDPhilip\Elasticsearch\Schema\Schema;

class CreatAuditIndex extends Migration { /**

Expected behavior

As I understand it, an index should be created.

Component Versions (Paste in the require section from your composer.json file):

  "require": {
   "php": "^8.1",
        "ext-json": "*",
        "laravel/framework": "^9.0",
        "laravel/tinker": "2.7",
        "pdphilip/elasticsearch": "~2.9"
  },

Additional context: I'm not sure, but I think Laravel expends your custom Schema or Schema Builder to have the same methods as Illuminate\Database\Schema\Builder, e.g hasTable() or dropAllTables()

When running artisan migrate:fresh for example, instead of hasTable() the method dropAllTables() is not found.

pdphilip commented 6 months ago

Thank you for the detailed report @henrygermany

The package's schema builder is a complete rebuild given that there is almost no overlap with how ES handles index management compared to MYSQL migrations.

That's not a problem unless ES is your only data source (as in your case), so what's happening is when you try to run your first migration Laravel tries to create a migrations table to keep track of your migrations, which triggers the hasTable() method and other MYSQL related blueprint methods.

I can bake in a fix for this, but the problem will pop up again if any Laravel packages have migrations in them, ex: Sanctum.

If it's within the scope of your project I would recommend not having ES as your primary datastore, but that's just my personal opinion.

Anyway, I'll publish a fix for this soon.

Side Note: I see you're mapping nested fields. I'm in the process of rebuilding the bridge to compile DSL queries instead of the current query_string. With that, you'll be able to do nested queries (like: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-nested-query.html).

Until then, if you map $index->array('updated_fields'); instead Then you'll be able to query values in there ex:

Audit::where('updated_fields.name','test')->get()

The only potential issue with doing that will be that if the values in your updated_fields array have many different object structures then ES will index each new structure it finds which means that you could eventually hit the index limit. But if the objects inside your field have the same structure then it's the better way to go.

pdphilip commented 6 months ago

Please update package to v2.9.8 and try again 👍

henrygermany commented 6 months ago

Thanks for the fast response and fix! artisan migrate now works.

Also thank you for the elaboration, this makes total sense and I feel a bit dumb not thinking about the migrations table.

I can understand your concerns about having ES as the only data source. In our case, the project is one small service in a distributed application and its only concern is keeping track of and offering historical data about the lifecycle of business objects saved elsewhere, so it does not really need another data source. But it should be no problem hooking up a slim RDBMS like sqlite for handling the migrations table in a way Laravel expects, so if you feel like my (edge) case shouldn't be a problem of the package, I totally understand.


Aside: I noticed that artisan migrate:rollback fails with a 400 Bad Request from ES:

{
   "error":{
      "root_cause":[
         {
            "type":"illegal_argument_exception",
            "reason":"Fielddata is disabled on [migration] in [migrations].
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 [migration] in order to load field data by uninverting the inverted index. Note that this can use significant memory."
         }
      ]

I'm not sure as of yet where this comes from and I'll be investigating more in the next few days.

henrygermany commented 6 months ago

Oh and about the nested fields: Thanks for the tip. I'll be experimenting with using nested fields more. As each indexed document in my project represents a change in a business object and contains the changed fields, the updated_fields field will differ heavily from document to document. I'm not sure if I even need the nested field.

pdphilip commented 6 months ago

Ah, that's a follow on bug in the migration index itself, thanks for flagging. It's a quick fix and I'll patch that along with another small update tomorrow (will be 2.9.9 for you). Good luck with your project.

pdphilip commented 6 months ago

Latest version v2.9.9 will fix the second error 👍

henrygermany commented 6 months ago

Thanks!