Show recipe error instead if throwing an exeption

[MikeAlhayek]

Currently when importing a recipe and something goes throw, an exception is thrown. In a production environment, the user gets a white page which leaves them lost on what had happened.

Instead, we should catch these exceptions and show the error to the user so they have feedback instead of dumping exception or a white page.

[MikeAlhayek]

@hyzx86 I thought you had a PR or an issue for this that i can't find. If not, do you like to submit a PR for this?

[hyzx86]

But how do we do that? Do we just output the description of the internal exception to the user? Use Notify.ErrorAsync?

[MikeAlhayek]

@hyzx86 you can just use ModelState to add an error to the recipe input.

[hyzx86]

Just like this ?

[Piedone]

That looks good, just please don't output the exception message.

[MikeAlhayek]

Yes that looks good to me. Probably better not to color the JSON on the input.

At least with this we get feedback on what the error could be.

[hyzx86]

Just like : Import failed, see the server log for details. ？

But if the error message is not displayed, the user has to check the server log.

I feel that the import function should be for administrators, even if some security related information is exported in between.

[MikeAlhayek]

I think we should write the message from the exception. Each step controls the message of the exception so I think it is safe to make that visible. Otherwise, yes the user has no idea why it failed. The importing user may not have visibility into the log file in most cases

@Piedone thoughts since you suggested against that?

[Piedone]

The problem is that an exception can potentially include technical details that the user can't do anything about, or worse, expose private information. If we have a specific type of exception just for this (in O1 we had one with a localized message) then OK, but displaying just any exception is risky.

[MikeAlhayek]

@Piedone Another options would be to add a new property to

OrchardCore/src/OrchardCore/OrchardCore.Recipes.Abstractions/Models/RecipeExecutionContext.cs

Line 6 in 6679b28

public class RecipeExecutionContext

that would collect errors like a Dictionary of some sort So that this way each implementation of IRecipeStepHandler can add an error instead of throwing an exception.

Then in the controller, we can say step ABC failed with these errors. and if there is an exception with no errors provided by the step, we can display a generic message like "Failed to import the given recipe due to unknown error."

But we should try to provide guidance with these errors otherwise the user will be in the dark since the recipe can fail for many reasons.

[Piedone]

That would be an option too, or a ModelStateDictionary. Note though that some issues must halt the import process between two steps, so if not with exceptions, then somehow else that should still happen.

[MikeAlhayek]

In the recipe executor, we can check for errors before proceeding to the next step to ensure the existing logic stay intact. This chance will need to be documented here and also added to the change log so that anyone is implementing a custom step, they can set the errors instead of throwing exceptions.

@hyzx86 please add a new property to the RecipeExecutionContext Dictionary<string, string> Errors { get; } = []. Then update all implementation of RecipeExecutionContext to populate errors into the Errors property instead of throwing exception.

Then the line

OrchardCore/src/OrchardCore/OrchardCore.Recipes.Core/Services/RecipeExecutor.cs

Line 100 in 768459c

stepResult.IsSuccessful = true;

would be changed to something like this

if (recipeStep.Errors.Count > 0)
{
       stepResult.IsSuccessful = false;
       stepResult.ErrorMessage = string.Join(' ', recipeStep.Errors.Values);
}
else 
{
       stepResult.IsSuccessful = true;
}

You may even change RecipeStepResult.ErrorMessage from string to Dictionary<string, string> since that would be more useful since we can report the exact step within the recipe where the error occurred.

[hishamco]

@MikeAlhayek I remembered @hyzx86 submitted a PR for this last few weeks, right?

[MikeAlhayek]

No. There was a PR that throws an exception. Here we write exception and provide guidance instances of throwing an exception