babenkoivan / elastic-migrations

Elasticsearch migrations for Laravel
MIT License
187 stars 32 forks source link

Supply nested `Mapping` instance when creating object fields #23

Closed stevebauman closed 3 years ago

stevebauman commented 3 years ago

Description

When creating migrations and adding fields using the Mapping class, I'm unable to create sub-fields for a property, as shown in the Elasticsearch docs here:

https://www.elastic.co/guide/en/elasticsearch/reference/current/properties.html

Example

In the above docs link, they have a manager field with age and name sub-fields, but I am unable to create this with the current API, as adding any field always adds the type key:

https://github.com/babenkoivan/elastic-adapter/blob/d3b5daa27386956f341c1cf6e1c1d4e872e96496/src/Indices/Mapping.php#L104

Here's a work-around attempt:

use ElasticAdapter\Indices\Mapping;

$mapping = new Mapping();

$mapping->properties('manager', [
    'properties' => [
        'age' => [
          'type' => 'integer',
        ],
        'name' => [
          'type' => 'text',
        ],
    ],
]);

$mapping->toArray();

Result:

[
  "properties" => [
    "manager" => [
      "type" => "properties",
      "age" => [
        "type" => "integer",
      ],
      "name" => [
        "type" => "text",
      ],
    ],
  ],
]

Possible Solutions

It'd be great to be able to simply add values to the $properties array via the Mapping class, via:

$mapping = new Mapping();

$mapping->addProperty('manager', $attributes = []);

And it'd be even more awesome if we could supply a closure that automatically injected a new nested mapping instance:

$mapping = new Mapping();

$mapping->addProperty('manager', function (Mapping $mapping) {
    $mapping->integer('age');
    $mapping->text('name');
});

Or maybe even an object() method if you'd prefer to be explicit:

$mapping = new Mapping();

$mapping->object('manager', function (Mapping $mapping) {
    $mapping->integer('age');
    $mapping->text('name');
});

If you're dead-set on the API, then we could simply create the Mapping instance from the Laravel IoC so developers can extend the current (if the Mapping class has final removed) or swapped out with their own implementation:

From:

https://github.com/babenkoivan/elastic-migrations/blob/1e9290d18a59f26e1aefaa95d0c0c964903db9fe/src/Adapters/IndexManagerAdapter.php#L26-L32

To:

public function create(string $indexName, ?callable $modifier = null): IndexManagerInterface {
    $prefixedIndexName = prefix_index_name($indexName);

    if (isset($modifier)) {
        $mapping = resolve(Mapping::class);
        $settings = resolve(Settings::class);

        // ...

What are your thoughts? I'd be happy to create a PR for the above implementations. Let me know! 👍

stevebauman commented 3 years ago

Scratch this issue -- I discovered that the type field can be supplied in object fields when creating/updating indexes 👍

This works:

use ElasticAdapter\Indices\Mapping;

$mapping = new Mapping();

$mapping->object('manager', [
  'properties' => [
      'age' => [
          'type' => 'integer',
      ],
      'name' => [
          'type' => 'text',
      ],
  ],
]);

However, it would be great to automatically pass in new nested mappings so so we don't lose the fluent Mapping interface with sub-fields. Thoughts?

babenkoivan commented 3 years ago

Hey @stevebauman, thank you for your suggestion. I'll look into it and think about how it can be improved to provide a better interface. This has a low priority though, cause nothing stops you from creating objects or nested types.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days