ContentPublishingNotification is published to early - inconsistency during publishing

[wtct]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

11.2.0

Bug summary

We know that all "ing" notifications are published in the scopes. However, the data consistency could not be ensured.

Let's imagine that you want to create and publish a Child node when saving a parent - Trigger node.

Then, if the Child node is published successfully, the Trigger node should contain Another child node as a draft ready for further editing.

I invented this situation to illustrate the problem only, but we have come across similar issue.

I have analyzed the source codes of the ContentService.

Probably the issue could be solved by changing the place of publishing the ContentPublishingNotification.

In my opinion, the ContentPublishingNotification shouln't be published before validation of properties in the StrategyCanPublish method:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L3056

Sorry, but it's weird that this notification is published at this stage.

Instead, it could be published at the beginning of the StrategyPublish method when the publication really occurs:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L3237

Another issue is related to completing a scope without publication result checking:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L1166

However, this is a different story mentioned in another issue:

#13957

Best!

cc: @piotrbach

Specifics

No response

Steps to reproduce

1. Create a new Test document type.
2. Add required Title property in Test document type.
3. Allow to create a Test node at root.
4. Allow to create a Test node under Test node.
5. Create a Trigger node in the root of the Content section, as below:
   
6. Fill the Title property and save the Trigger node.
7. Create a new TestComposer and a new ContentNotificationHandler, as below:

public class TestComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddNotificationHandler<ContentSavingNotification, ContentNotificationsHandler>();
        builder.AddNotificationHandler<ContentPublishingNotification, ContentNotificationsHandler>();
    }
}

public class ContentNotificationsHandler : INotificationHandler<ContentSavingNotification>, INotificationHandler<ContentPublishingNotification>
{
    private readonly IContentService _contentService;

    public ContentNotificationsHandler(IContentService contentService)
    {
        _contentService = contentService;
    }

    public void Handle(ContentSavingNotification notification)
    {
        var entity = notification.SavedEntities.First();

        if (entity.Name == "Trigger")
        {
            var child = _contentService.Create("Child", entity.Id, entity.ContentType.Alias);

            var result = _contentService.SaveAndPublish(child);

            if (!result.Success)
                notification.Messages.Add(new EventMessage("Test", "Child is not published.", EventMessageType.Warning));
        }
    }

    public void Handle(ContentPublishingNotification notification)
    {
        var entity = notification.PublishedEntities.First();

        if (entity.Name == "Child")
        {
            var anotherChild = _contentService.Create("Another child", entity.ParentId, entity.ContentType.Alias);

            _contentService.Save(anotherChild);
        }
    }
}

8. Save the Trigger node.
9. Reload the Content section.
10. Take a look at children of the Trigger node.

Expected result / actual result

Actual result:

The Another Child node is created under the Trigger node.

Expected result:

The Another Child node shouldn't be created under the Trigger node, because Child node is finally not published due to validation errors.

[github-actions[bot]]

Hi there @wtct!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[nikolajlauridsen]

Hey @wtct as I understand it's intended behaviour that the node is saved as a draft even though the publication failed, this arises from the desire to allow editors to work freely, make mistakes, but then come back and fix it before publishing. This would not be possible if we didn't save the node even though the publication failed. I know @nielslyngsoe talks a bit about it here.

About your comment about the place we publish the notification is weird, I'll have to disagree, validation is a part of publishing in my opinion, so it makes sense to say "We are publishing something", now we at this point do not guarantee that the publishing will succeed, there could be errors (such as validation errors), or the operation may be cancelled in a separate notification handler, so you can't rely on this always succeeding, which your implementation does.

Alternatively, you could use the Saved and Published events instead, since that this point the consistency is guaranteed since the operation already happened.

Hope it helps.

[wtct]

Hi @nikolajlauridsen,

All right, let's assume that the place where the notification is published is not weird. Of course, we can make a poll, where is the best place to send this notification, but in my opinion it should be published after validation. Then you can check if it's cancelled or not.

However, it seems that you probably don't understand the data consistency issue I'm referring to.

It's not possible to use Saved/Published notifications because you cannot revert the performed operations if your custom operations fail.

Could you please imagine that you need to store important data in the custom table when a node is published?

You need to have a transaction to achieve data consistency between published node and data in the custom table.

Unfortunately, it's not possible now without hacks.

Therefore I raised this issue.

Please let me know if now everything is clear, if so please reopen this issue.

Best!

cc: @piotrbach

[nikolajlauridsen]

No I'm not entirely sure I understand your specific use case.

But regardless, an issue is maybe not the best place for this, since it's not a bug or specific problem, but more an issue with the overall architecture/structure.

So I think this might be better as a discussion, so I'll go ahead and convert this to a discussion instead so we can get some more input.

[wtct]

Thanks @nikolajlauridsen for another chance for clarification 👍

The "ing" notifications are published in the scopes. One of the most interesting example of the scopes could be found here:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L998

It means that all related, custom changes performed in the notifications handlers will be committed or reverted together with changes applied to the node, because those operations are wrapped with the same ambient scope.

So thanks to the scopes we can ensure data consistency between nodes and custom operations, rows in custom tables etc.

It's working as expected for the ContentSavingNotification, but it's worth to mention that there is a corner case reported here: #13957

Unfortunately, it's not possible to achieve the same data consistency during handling the ContentPublishingNotification, because this notification is published before validation. So if we have any custom operations in the handler of ContentPublishingNotification then it will be committed regardless of validation errors. But we need to commit it only when node is finally published...

Actually it's caused by two issues:

1. The first one is related to completing scope regardless of PublishResult: https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L1166
2. The second one is related to publishing the ContentPublishingNotification to early before validation: https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Services/ContentService.cs#L3056

Therefore I raised this issue.

Please let me know if it is clear now.

[nikolajlauridsen]

Hi @wtct I'm quite aware of how scopes works 😉 My point was more that I hadn't yet quite seen what the actual underlying problem was, but I didn't communicate that quite well, that's my bad.

Now I've given this some thought, and this is what I think.

I see your issue, however, I don't think this issue stems from where the notification is published, or something like that, those are the symptoms of the real problem, which is much more architectural.

The problem is really that there is a single SaveAndPublish operation, just the name itself is a very bad code smell. This operation is very much two separate and distinct operations: Saving the content, and publishing the content, and these two operations are at odds here.

The two opposing ideas are:

1. We very much want the content to be saved, even though it cannot be published, we do not want the editor to lose all their progress, just because they typed in 1 character too many.
2. We want to roll back any changes made in the publishing scope if the operation fails.

But as long as SaveAndPublish is done as 1 operation, in 1 transaction, one of these points must win over the other, we cannot simultaneously commit and roll back the transaction.

So my proposed solution to this would be to split these two apart completely, into two separate operations, done in two separate transactions, this way we can save and commit our content, but at the same time roll back a failed publish, and have data consistency, we can have our cake and eat it too.

When that said, all of these changes are very much breaking, behaviorally at least, so I don't think this is something we can do right now, one way or the other. But I'll mention this to the team as well.

[wtct]

Thanks @nikolajlauridsen for understanding!

I have the same thoughts about the process performed by SaveAndPublish method.

There could be still one public SaveAndPublish method in the ContentService, but the implementation could be splited into two separate processes wrapped by separate scopes.

Unfortunately, there will be still the same issue related to the publication of ContentPublishingNotification if we have validation during publication.

If the ContentPublishingNotification is published before validation and the scope is completed in the case of invalid properties then data consistency couldn't be ensured. Regardless if we split SaveAndPublish or not.

However, if we could publish the ContentPublishingNotification after validation then we are sure that all operation should be performed if there is no other issue with db/connection etc.

Moving the publication of the ContentPublishingNotification is not a breaking change in my opinion and for now the another breaking changes are not required to achieve expected result.

Please consider moving the publication of the ContentPublishingNotification.

[Zeegaan]

This feature has now been implemented in #16331 🚀

[piotrbach]

Hi @Zeegaan @Migaroez, I see that the issue is fixed for v13. Is there a chance to fix v11 and v12 as well?

[bergmania]

Hi @piotrbach

V11 is EOL and v12 is in security phase, so this will not be back-ported

[piotrbach]

Hi, @bergmania, thanks & have a great day!