Refactor ContentIndexSettings to IContentIndexSettings ♻️

[Skrypt]

Refactor ContentIndexSettings as IContentIndexSettings: We can now have multiple indexing providers' content indexing settings. BuildIndexContext now needs to pass an IContentIndexSettings in its ctor; This allows to determine which indexing provider is used for the ContentItemIndexCoordinator. ✨ ♻️

I'm separating "core" element changes from the ElasticSearch PR so that I can eventually test it inside my Nuget Package solution. These changes are required to support implementing multiple indexing providers in OC.

TODO:

• Migration from ContentIndexSettings to LuceneContentIndexSettings in the Content Definitions.
• Backward compatibility with old recipe.json files : ContentIndexSettings should be imported as LuceneContentIndexSettings.
• Recipe backward compatibility refactor if needed.

[hishamco]

Are we considering the breaking changes in release versions?

[Skrypt]

I marked ContentIndexSettings as obsolete.

[hishamco]

I marked ContentIndexSettings as obsolete.

I didn't see the code, but I expect you will do that ;)

[Skrypt]

Needs to be tested first with some websites to make sure the migration works fine. I think @sebastienros is afraid that the migration might break actual websites which I can understand. Better test it than be sorry later. 😄

[hishamco]

@Skrypt is it ready to review, while you are testing the migrations?

[Skrypt]

The PR is here for that. Unless I would refactor it completely; I think that's about it. I extracted these changes from the Elastic Search PR because I thought it was a sensible part, so feel free to propose ideas.

[hishamco]

I need to submit few PRs, then I can have a look to this

[Skrypt]

The only thing remaining that I'm not 100% sure of is where should I register this LuceneRecipeEventHandler since these recipes can be executed on setup. I've moved it to the OrchardCore.Deployment module for now. And also, it is now registered as a transient service as suggested by Dean and Jtkech.

@deanmarcussen @jtkech @sebastienros

[deanmarcussen]

@jtkech when you have some time can you take a look at this?
I vaguely remember you saying recipe event handlers would need to be registered differently, after some changes you made? But it might have been something completely different.

@Skrypt my question for you is couldn't we do this in the Content Step itself? It feels like both would still execute anyway, with much room for confusion?

It seems odd to have this in Deployment, which is not a content related module.

I also wonder about the value of changing from ContentIndexSettings to LuceneContentIndexSettings - lot of code to achieve that, I know it makes the end result cleaner, but much room for breaking, whereas staying with the original name is much less room for breaking (but room for confusion about ContentIndexSettings vs ElasticIndexSettings)

[Skrypt]

The idea is that I needed to change the Drivers models so that there is no confusion for anyone when trying to build a new Indexing Provider. Here, the issue is that on the Elastic Search PR the ContentIndexSettings we're used for both Lucene and Elastic at the same time. I'm trying to make it evident that these are supposed to be specific per provider so that next time someone tries to build a new search provider that he does not use ContentIndexSettings and overwrite the current existing settings. There are some parts in the Lucene modules where there is a lot of code duplication and that's probably also why the Elastic Search module doesn't work right now. We need to clean up things in there, not make it more confusing.

I had the same discussion with @jtkech and I kept it as a LuceneContentIndexSettings because I think it is what it is. Now, for the recipe event handler, it is registered as transient as @jtkech suggested to me. And, there is no place where this handler belongs to else than probably having it registered in a core module to have it enabled all the time to handle the backward compatibility in every possible use case. Maybe OrchardCore.Contents.Core ... open for suggestions.

As for the Content Step itself, I felt like having a handler that could be removed quickly, later on, was simpler. It will achieve the same thing in the end ... unless you think it would be better in some way?

I refactored the code this week thinking about scenarios where deployment could be made on a server that is not updated with the handler and made sure that when it would be updated that it won't fail. I can't think of a use case where it would break things because I thought about the most common use cases. And made these changes with @jtkech.

[Skrypt]

@deanmarcussen Yes, the LuceneRecipeEventHandler.RecipeStepExecutingAsync() will be executed on every step. But it will process only on 2 specific steps which are ReplaceContentDefinition and ContentDefinition. For all the other steps it will just return Task.CompletedTask;. So, it can happen that the Handler be processing twice in the same recipe. Though, the recipeExecutor is responsible for the orders of the steps so it doesn't matter if it passes twice. It will just migrate the JObject of these 2 different steps.

I tried moving it to the RecipeExecutinAsync() method but it would require streaming the entire recipe file as we do in the RecipeExecutor which doesn't make sense since it will be streaming it just afterward. So instead of creating 2 different processes that will stream the file, it takes less code and time to simply just migrate it in process.

[jtkech]

@deanmarcussen @Skrypt

I vaguely remember you saying recipe event handlers would need to be registered differently

Yes refer to the comments of #6731 and where I added the following code comment

OrchardCore/src/OrchardCore/OrchardCore.Recipes.Abstractions/Events/IRecipeEventHandler.cs

Lines 6 to 11 in 9b01525

	/// <summary>
	/// Note: The recipe executor creates for each step a scope that may be based on a new shell if the features have changed,
	/// so an 'IRecipeEventHandler', that is also used in each step scope, can't be a tenant level service, otherwise it may
	/// be used in a shell container it doesn't belong, so it should be an application level transient or singleton service.
	/// </summary>
	public interface IRecipeEventHandler

Yes, keeping ContentIndexSettings doesn' hurt me

But I understand that @Skrypt want to make things cleaner ;)

I will think about it again this week end.

[Skrypt]

We don't need to access the scoped tenant data here, we parse the current recipe.json file that we want to execute. In case of a deployment made from a tenant, the RecipeExecutor will only receive data that will be parsed in the LuceneHandler. In this case, the data is already pre-fetched. So, now there are 2 people that want to keep ContentIndexSettings, I'm about to drop the ball on that PR. I've refactored it twice now. Maybe it's just a bad idea or we just don't agree on the fact that it will make us reach the same goal in the end but differently. Yes, I'm probably stubborn in nature so I prefer having a migration than being obligated to use ContentIndexSettings which is less descriptive. Feel free to refactor it a third time. Sorry, probably had a bad day. But now it makes 3 weeks this PR is up and I won't argue more.

[hishamco]

@Skrypt why it's closed?

[Skrypt]

Because I think we need to agree on how we should handle backward compatibility with recipes moving forward.

This is one of the probably many other data migrations that will happen over time. Now that 1.0 is shipped we need to make sure that everything is backward compatible but I can't agree that we should adapt the code to handle these changes instead of coding it as it would naturally be. If we have a naming change then the data and recipes need to be migrated accordingly and we don't have any strategy about this.

To make it more evident, if a "Dog" Class becomes a "Cat" class then should I keep the Dog class as "Animal" just to keep the data like it was before because I just had an "Animal" class? At this point, things are certainly unclear on what we should do exactly.

I pushed enough time on this, I have other things to do. The branch is still there to be refactored. I think there are other issues that are more important to fix right now than making Elastic Search work in OC. We know it will eventually work but there is too much work to do still. And if the smallest change like this one takes 2 weeks to get merged we won't have it soon.

[hishamco]

Thanks for the work you did

[Skrypt]

Closing as it is merged back in #11052