Orchard creates content item even when validation fails.

[DamienLaw]

I'll confess that the title only speaks the half truth. Now that I have your attention, let's try this out and reproduce the issue. Below is what happening in Orchard when validation fails:

1. Create a content item, let's say create a new Page from the dashboard.
2. Leave the form empty and save it.
3. Content item Page gets created and about to be inserted into database through a transaction.
4. Validate form. Since the required field Title is empty, validation will fail.
5. Transaction is cancelled and rolled back after validation failed.

Why bother creating the content item when the database transaction is gonna get cancelled anyway and waste precious database resources (the Identity counter of the database table Orchard_Framework_ContentItemRecord is increased every time a content item is created and never backs down regardless of transaction rollback)?

Why not swap Step #3 with #4? Validate the form first and if it fails, then there's no need to even create the content item thus no transaction rollback necessary.

I'll point you to the Controller's Action in question to save you the trouble, Orchard.Core.Contents.Controllers.AdminController.CreatePOST(string, string, Action<ContentItem>)

_contentManager.Create(contentItem, VersionOptions.Draft); //content item is created first

var model = _contentManager.UpdateEditor(contentItem, this); //validation happens after content item creation

if (!ModelState.IsValid) {
    _transactionManager.Cancel(); //database transaction is cancelled when validation fails
    return View(model);
}

I suggest changing the order to

var model = _contentManager.UpdateEditor(contentItem, this); //validate the form first

if (!ModelState.IsValid) {
    _transactionManager.Cancel(); //rollback transaction when validation fails - probably doesn't even require this since database is not even touched yet
    return View(model);
}

_contentManager.Create(contentItem, VersionOptions.Draft); //create content item after validation passed

By doing this, we'll be able to tell apart a content item that has been created in the database from one that has not. A new content item (including those that fail validation) has an Id of 0. We can use this data in our ContentPartDriver to serve different shapes depending on whether if the content item is new or already created.

protected override DriverResult Editor(MetadataPart part, dynamic shapeHelper) {            
        //only serve a shape if the content item already inserted into the database
        if (part.ContentItem.Id != 0) {
            return ContentShape("Parts_Metadata_Edit", () =>
                shapeHelper.EditorTemplate(TemplateName: "Parts/Metadata.Edit", Model: _metadataService.BuildEditorViewModel(part), Prefix: Prefix)
            );
        }

        //don't serve any shapes for new content items
        return null;
    }

Currently, the above doesn't work well since part.ContentItem.Id is never 0 because Orchard always creates the content item before validation occurs (the above codes run during the validation stage).

[sebastienros]

Try this change, and run all the specflow tests first. Just to see if it doesn't break any unexpected feature.
Also, a safer solution might be to actually do the Update twice, first without the new content item, then if succeeded create a Draft and call Update again.

[sebastienros]

Anyone who wants to try my proposition is free to create a PR for that. And also do it for Comments if it works fine and gets sign-off.

[DamienLaw]

I would report back earlier but running all the tests after every modification is painfully slow.

This is what I've attempted:
1. Get the Orchard 1.10 pre-release source (Orchard.Source.1.10.zip) from https://github.com/OrchardCMS/Orchard/releases/tag/1.10
2. Extract, open Orchard.sln, build solution.
3. Modify the method Orchard.Core.Contents.Controllers.AdminController.CreatePOST(string, string, Action<ContentItem>) from:

_contentManager.Create(contentItem, VersionOptions.Draft);

var model = _contentManager.UpdateEditor(contentItem, this);

if (!ModelState.IsValid) {
    _transactionManager.Cancel();
    return View(model);
}

to:

// _contentManager.Create(contentItem, VersionOptions.Draft); // move this line below

var model = _contentManager.UpdateEditor(contentItem, this); // validate user input from the view

if (!ModelState.IsValid) {
    //_transactionManager.Cancel(); // don't need this anymore since database wasn't written onto yet at this point.
    return View(model);
}

_contentManager.Create(contentItem, VersionOptions.Draft); // create content item after validation passed
_contentManager.UpdateEditor(contentItem, this); // run this again to repopulate user input into content item since IContentManager.Create() above might have overwritten some user input data that has been populated during validation, a few lines above.

4. Run all Tests and Specs
5. All passed but one failed:
Orchard.Specs.BooleanFieldFeature.CreatingAndUsingBooleanFields. Error on the last test. This test is regarding "intentionally failing validation" which is very much related to how this issue is opened in the first place.

The test failed because of "object moved" which has been raised before at http://stackoverflow.com/questions/23626889/orchard-cms-1-8-specflow-tests-failing-with-object-moved-302-redirection. It might've related to the AppDomain restart.

I then decided to run this test manually by hooking up to IIS and SQL Server Express, installing Orchard, intentionally failing the test, old school style, just like everything described in the automated test. It behaved exactly as expected. No app restart. No redirection. Everything is just okay.

The last time this SpecFlow "object moved" issue happened was after the .NET Framework version was retargetted. This time, Orchard 1.10 also had its .NET Framework version retargetted too and to confirm my speculation, I ran the same test on Orchard 1.9.2 and passed flawlessly. No complaint of "object moved" whatsoever. Therefore, the retargetting of the .NET Framework might have something to do with the failed test.

However, this test passed on Orchard 1.10 before my modifications. As to how my modifications cause it to fail (regardless how remotely unrelated it might be) still eludes me.

[jtkech]

👍 to minimize table rows. I also agree with #6385 to have a Versionable option with content types.

[DamienLaw]

@jtkech
this issue has nothing to do with database tables having unnecessary records though.

This issue is about the handling of failed validation. Even currently, Orchard is fully capable of rolling back transactions when validation fails effectively preventing unnecessary records from being inserted into database tables. The most damage it does is probably just increasing the Identity counter of the database tables Orchard_Framework_ContentItemRecord and Orchard_Framework_ContentItemVersionRecord. The Id column of these 2 tables is of Identity which is responsible for assigning the next running integer to be used as an, well "Id". Like a serial number. Every database table with a column associated with Identity has a counter that keeps track of the current Identity value which will increase when new records are added (even through transactions) and never decreases (even in the event of transaction rollback or record deletion).

[jtkech]

Ok, i see, i've read too fast ;)

[sebastienros]

The most damage it does is probably just increasing the Identity counter of the database tables

I think the most damage is increasing the transaction log which can lead to the server to be down if there is no auto-growth, or if there is a hard limit on the hoster.