Recipes within recipes overwrite variables configuration

[deanmarcussen]

When running a recipe inside a recipe with the Recipes step the variables section / _variablesMethodProvider is overwritten with the values from the inner recipe.

Example

{
  "name": "CKEditorsample",
  "displayName": "CKEditor Sample",
  "description": "CKEditor Sample based on TheBlogTheme",
  "author": "ThisNetWorks",
  "website": "https://github.com/ThisNetWorks",
  "version": "1",
  "issetuprecipe": true,
  "categories": [ "default" ],
  "tags": [ "blog" ],  
  "variables": {
    "ckeditorSampleContentItemId": "[js:uuid()]"      <----- this variable is overwritten by the variables section in the blog recipe
  },
  "steps": [
     {
      "name": "Recipes",
      "Values": [
        {
          "ExecutionId" : "",
          "Name" : "Blog"                <----- this recipe has a variables section and overwrites the variablesmethodprovider
        }
      ]
     },
    {
      "name": "Feature",
      "enable": [
        "ThisNetWorks.OrchardCore.CKEditor"
      ],
      "disable": [
      ]
    }
  ]
}

This happens because the IRecipeExecutor is a Singleton, and assigns the variableMethodsProvider to a private variable as it reads the recipe.json, so on the second recipe run it overwrites the value.

This should be easy to fix, probably with a dictionary wrapping the variablesMethodsProviders for each recipe, keyed to the execution id, which is always a new guid.

Hmm, does it need to be concurrent?

@jtkech you are fixing some recipe issues at the moment, but I think this is unrelated / a different issue.

[jtkech]

This should be easy to fix, probably with a dictionary wrapping the variablesMethodsProviders for each recipe, keyed to the execution id, which is always a new guid.

Yes easy to fix for this one but see below, i think that more things need to be fixed.

Hmm, does it need to be concurrent?

Not just because there is a nested recipe, but yes if recipes are executed concurently (e.g. through the admin UI) on the same instance. Otherwise, if only one recipe is executed, all steps are executed asynchronously in their own scope but the whole flow of a given recipe (including nested ones) is intended to be sequential.

About the recipe executor i did some updates related to the usage of the shell scopes but didn't get a closer look on other things, after a quick look it seems that there are other issues, e.g on each step scope we resolve IScriptingManager that is a singleton (so could be resolved once) and we update its GlobalMethodProviders which is a non thread safe List.

Also, if you run multiple times some recipes the GlobalMethodProviders will grow up again and again, see the below image. Note: After a setup the shell is reloaded, so that' okay. So i think that the RecipeExecutor will need to be reviewed

@jtkech you are fixing some recipe issues at the moment, but I think this is unrelated / a different issue.

Yes it is now unrelated because it just revealed another issue that i fixed without changing any code related to recipes, see my last comment in #6648 whose i changed the title.

As a reminder, maybe a recipe should be an exclusive task but in a future PR, here just thread safe, idem e.g. for workflow singletons, exclusive background tasks, but these are all pending things (as many other ones) since a while because my distributed PR is already too big ;) and may need another pending PR to continue working on it.

Finally, re-running the blog recipe fails on Your alias already exists here related to "Alias": "main-menu". I understand that the idempotency on importing content items is based on their ids, but when a recipe re-generate itself new ids it is not idempotent.

[jtkech]

So only the VariablesMethodProvider and the ParametersMethodProvider need to be tied to a given recipe as they use parameters passed to the recipe executor ExecuteAsync(). The ConfigurationMethodProvider only needs the ShellConfiguration that is a singleton so it can be injected once (as the IScriptingManager itself).

Then i think this is the recipe executor that would need to hold, e.g. in a concurrent dictionary, the additional providers tied to a given recipe executionId key, then when calling the scripting manager Evaluate(), in place of passing null to the scopedMethodProviders parameter, we could pass these additional providers. And at the end of a recipe execution we could cleanup the providers tied to a given recipe executionId.

I'll try to work on it tomorrow

[deanmarcussen]

So i think that the RecipeExecutor will need to be reviewed

Yes, there are a few things in here. (so not so simple)

Then i think this is the recipe executor that would need to hold, e.g. in a concurrent dictionary, the additional providers tied

Agree.
Or just for ideas, maybe the recipe executor should be transient, rather than a singleton. And when running inner recipes, resolve a new instance.
But equally this may cause other complications.

maybe a recipe should be an exclusive task but in a future PR

Probably, but for later as you say.

Finally, re-running the blog recipe fails on Your alias already exists here related to "Alias": "main-menu". I understand that the idempotency on importing content items is based on their ids, but when a recipe re-generate itself new ids it is not idempotent.

Yes, we discussed this, and agreed setup recipes were only intended to be ran once. Otherwise they cannot be idempotent.
That is not something we have ever documented anywhere. But there are many things that happen when you run a setup recipe twice (multiple layer widgets... etc ... )

Cool, leaving you to see what works best.

For info I made some other changes here to the executor #6421 but that pr is not ready, and will be refactored, so better to fix these issues here, and separately.

[jtkech]

Or just for ideas, maybe the recipe executor should be transient, rather than a singleton. And when running inner recipes, resolve a new instance.

Good idea but as it is implemented the RecipesStep just updates the InnerRecipes of the "parent" RecipeExecutionContext