OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.44k stars 2.4k forks source link

Exceptions in drivers don't prevent content item updates #4252

Open ns8482e opened 5 years ago

ns8482e commented 5 years ago

With reference to issue #4168

Currently, When Content Item is updated, if any attached part throws any Unhandled Exception during update process - the exception is not notified to to the calling controller i.e. EditPOST of Content AdminController and Controller menthod contiues to update the content item in database.

As result, data stored in database for that content item becomes corrupted/inconsistent.

In transnational scenario, if one Part fails to update the whole Edit action of Content Item should be cancelled - partial edit should not be allowed.

To reproduce, I have created following in a module

  1. Created Product Module with - ProductPart, ProductPartDisplayDriver, ProductPartViewModel and ProductPart.Edit.cshtml

In ProductPartDisplayDriver defined UpdateAsyc as following

public override async Task<IDisplayResult> UpdateAsync(ProductPart model, IUpdateModel updater)
        {            
            await updater.TryUpdateModelAsync(model, Prefix, t => t.Sku);

            return Edit(model);
        }

Created Content Type ProductWidget and addded TitlePart and ProductPart to it.

Create a Flow- Page, Added ProductWidget, set Title = "Product 1", SKU = "ABCDEF" and Saved it without any problem.

Now, I changed the code of ProductPartDisplayDriver to add an unhandled exception as below

public override async Task<IDisplayResult> UpdateAsync(ProductPart model, IUpdateModel updater)
        {            
            throw new Exception();
            await updater.TryUpdateModelAsync(model, Prefix, t => t.Sku);

            return Edit(model);
        }

Edited the Page created earlier - Modified ProductWidget's title to "Awesome Product" and SKU to "P1P2P3" and click on save.

UI notified that it Saved successfully, However it only updated the Title Part.

Ideally it should not update the entire Page, and should fail with some kind of error notification on UI.

jtkech commented 5 years ago

I will take a look

jtkech commented 5 years ago

The controller call indirectly the following

            await _partDisplayDrivers.InvokeAsync(async contentDisplay =>
            {
                var result = await contentDisplay.UpdateEditorAsync(part, typePartDefinition, context);
                if (result != null)
                {
                    await result.ApplyAsync(context);
                }
            }, Logger);

Where InvokeAsync do

    public static async Task InvokeAsync<TEvents>(this IEnumerable<TEvents> events, Func<TEvents, Task> dispatch, ILogger logger)
    {
        foreach (var sink in events)
        {
            try
            {
                await dispatch(sink);
            }
            catch (Exception ex)
            {
                HandleException(ex, logger, typeof(TEvents).Name, sink.GetType().FullName);
            }
        }
    }

You're right, the exception is catched and logged but the model is not invalidated.

Piedone commented 6 months ago

This is currently still the case. However, judging from the lack of responses here (and I've also searched for similar other issues without luck), is it really a problem? I'm inclined to say yes: if a driver couldn't update, then the content item's state is not completely valid, and thus shouldn't be saved. On the other hand, this prevents incorrectly implemented drivers from bringing down the editor (i.e. the editor is somewhat guarded against issues in specific modules).