jolicode / elastically

🔍 JoliCode's Elastica wrapper to bootstrap Elasticsearch PHP integrations
248 stars 36 forks source link

dynamic index_class_mapping ? #173

Open dgoosens opened 10 months ago

dgoosens commented 10 months ago

hello,

so I need to build a lot of indexes...
the naming of these indexes is dynamic, but I do use a predefined prefix av_%%indexname%% all the indexes do use the same mapping and map to the same class

as the naming is dynamic, I can not list all of them in the configuration...
at runtime, the request tells me on which index I should use.

requests work fine...
BUT when building the resultSet at the end of \Elastica\Search::search, using the injected BuilderInterface the request fails because the class mapping is unknown

so before looking into injecting a custom BuilderInterface, I was wondering if there were ways to either use wildcards for the mapping or inject a mapping at runtime ?

thanks a lot Dmitri

dgoosens commented 10 months ago

hello

So I managed to handle this with a custom ResultSetBuilder that uses a custom IndexNameMapper
Creating these was rather straightforward

In my case, all I had to do was overwriting IndexNameMapper::getClassFromIndexName() to return the class I wanted
(could have gone through the trouble of matching the prefix and/or a dynamic class map)

then, when consuming the service, instead of doing

$index = $this->esClient->getIndex($indexName);
$query = .... ;
$response = $index->search($query, ['limit' => 30], Request::GET);

did

$index = $this->esClient->getIndex($indexName);
$query = .... ;

$search = $index->createSearch($query, ['limit' => 30], $myCustomResultBuilder);
$response = $search->search('', null, Request::GET);

yet.... I find this really over-complicated, just to inject a custom class mapping at runtime
I see two options to avoid this trouble:

  1. allow the injection of a class to be used by the ResultBuilder directly in the search() method
  2. allow the injection of class-mapping into the IndexNameMapper, but this is currently no possible (or I did not figure out how)

If PRs are accepted for either option, let me know...
think I should be able to deal with this

damienalexandre commented 10 months ago

Hello, thanks for this detailed report!

This library does not support wildcard for class to index mapping as you found out.

I find this really over-complicated, just to inject a custom class mapping at runtime

You should be able to do the second one. The ResultSetBuilder is one of the required argument of the Client: https://github.com/jolicode/elastically/blob/2a97fc8ee18eb5abb5b7b52e7d398fa96d1018e5/src/Client.php#L34

So you can just inject your custom one when creating the Client, which depending on your framework / context may differ.

dgoosens commented 10 months ago

hi @damienalexandre

thanks for getting back at me....
and yes, as you can see in my update, that's what I did

yet... this does not seem very efficient

so wondered if I submit a PR to enable:

it would have a chance to be merged...
or if this is a deliberate choice by the devs to keep this as simple as possible ?

damienalexandre commented 10 months ago

We tend to avoid adding options and features to keep the library easy to maintain :grimacing: - so my first answer would be to not accept such PR.

BUT there is room for improvement, we could do like we did for the mapping provider.

We added a MappingProviderInterface allowing users to bring their own mapping the way the want (Yaml, Php, Json... whatever).

We could add a IndexNameMapperInterface, and make it easier / clearer how to switch from one to another.

WDYT?

Cheers.

dgoosens commented 10 months ago

We tend to avoid adding options and features to keep the library easy to maintain 😬 - so my first answer would be to not accept such PR.

BUT there is room for improvement, we could do like we did for the mapping provider.

We added a MappingProviderInterface allowing users to bring their own mapping the way the want (Yaml, Php, Json... whatever).

We could add a IndexNameMapperInterface, and make it easier / clearer how to switch from one to another.

WDYT?

Cheers.

I can do that....
not sure I will be able to deal with this immediately, but soon though