MAGE-1170: Handle Index options with IndexNameFetcher

[damcou]

This PR contains:

• Added an IndexOptions model implementing IndexOptionsInterface which would be the component of every index operation coming to AlgoliaConnector in the future
• Updated the deprecated AlgoliaHelper class to convert all calls to the new format including IndexOptions model
• Updated the AlgoliaConnector class so it fetches all index names according to the InderNameFetcher service
• Removed all the static methods in AlgoliaConnector to be able to use the IndexNameFetcher everywhere, notably in the setLastOperationInfo methid

[cammonro]

Really like where you going with this. The IndexOptions is a very useful abstraction.

Added some questions and comments. Please let me know what you think.

[cammonro]

Would it make sense to relocate this getIndexName logic into the IndexOptions object itself?

[damcou]

That's what I was thinking at the beginning, but this would require to inject the IndexNameFetcher into IndexOptions and I would like to keep its instantiation as easy as it is right now (only with one array parameter for data) . Do you see a way to perform such thing ?

[cammonro]

Ah yes this is a good thing to think about... 🤔

You know... Originally I was thinking we could make the IndexOptions a "smart object" (kind of like the "Active Record" pattern) but maybe that makes this class a bit too heavy (and I believe does kind of violate SRP).

Instead, another idea would be to keep IndexOptions as a lightweight DTO (data transfer object) and use the "Builder" pattern. i.e create an IndexOptionsBuilder that generates the options to be passed to the AlgoliaConnector.

Essentially that would mean factoring out both AlgoliaHelper::convertToIndexOptions and AlgoliaConnector::getIndexName into the new IndexOptionsBuilder which honors SRP I think.

The Builder (which can be a singleton and can inject the IndexNameFetcher via DI) has one job and one job only - create a valid IndexOptions object. Now instead of having IndexOptions::getEnforcedIndexName it's just IndexOptions::getIndexName because the Builder will have already figured out what the name is. Any methods that use IndexOptions can just call $indexOptions->getIndexName() and they're good to go. And now responsibilities are properly separated.

I think I like this approach a bit better than the "Active Record" pattern. What do you think?

[damcou]

I think this is a great idea !

I committed an IndexOptionsBuilder class, following your description of it as best as I could.
It has two separate methods:

• convertToIndexOptions which replaces the previous AlgoliaHelper::convertToIndexOptions to ensure to be able to set directly an index name if needed (for the AlgoliaHelper class, or for testing purposes)
• createIndexOptions which will be the preferred way to create IndexOptions objects in the future (starting 3.16.0) which takes index suffix as an argument and computes the index name with the IndexNameFetcher

At the beginning I wanted to use only one method but this would make a lot a calls a bit weird with a lot of null params since everything would be optional.

As a result, IndexOptionsInterface::ENFORCED_INDEX_NAME with its related getter is not useful anymore and we can use directly IndexOptionsInterface::INDEX_NAME as you suggested.

Let me know what you think about it.

[cammonro]

I think we're almost there! I added some suggestions to further centralize responsibility and (hopefully) keep things cleaner. But this is already much improved.

[cammonro]

This might just be my preference, but I think a single entry point for the Builder with a simple build method accepting an array of parameters would be cleaner. Let Builder::build handle the responsibility of deciding how to set the index name not the caller.

However, I can also see the benefit of having these convenience methods. These could accept the method signatures you have defined and call the main build "under the hood".

Meanwhile perhaps we should self-document more and rename accordingly:

Suggested change

	public function convertToIndexOptions(?string $indexName = null, ?int $storeId = null): IndexOptions
	public function buildWithEnforcedIndex(?string $indexName = null, ?int $storeId = null): IndexOptionsInterface

... using a buildWithX convention that calls build.

(Should probably return IndexOptionsInterface as well)

[cammonro]

I would probably put the call to computeIndexName in a central build method.

[cammonro]

I was trying to think of something that would work with the buildWithX convention ...

Suggested change

	public function createIndexOptions(?string $indexSuffix = null, ?int $storeId = null, ?bool $isTmp = false): IndexOptions
	public function buildWithComputedIndex(?string $indexSuffix = null, ?int $storeId = null, ?bool $isTmp = false): IndexOptionsInterface

[cammonro]

Suggestion to centralize the logic that decides the index name (enforced always wins) ...

Suggested change

	{
	{
	if (isset($indexOptions->getIndexName())) {
	return $indexOptions->getIndexName();
	}