Name of IContent is/was never "Dirty" in ContentService events / notifications

jerpenol commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

8.17.0

Bug summary

After changing the name of a Content Node and saving and publishing, the property Name is/was never dirty on ContentService events Saving, Saved, Publishing and Published. TreeEntityBase suggests that the name should be listed as dirty (added to ContentBase._currentChanges IsDirty or ContentBase._savedChanges = WasDirty).

Furthermore on the ContentService.Published event (could be on the other events as well) the name of the version with id that equals IContent.VersionId is exactly the same as the version with id equals IContent.PublishedVersionId, while these two Id's differ (PublishedVersionId is the older one). They both hold the newly edited name of the content node.

This combined looks like in some way content gets saved twice before publishing. The weird thing would be that "normal" properties (not base properties like Name) are dirty in all these cases.

I have a multilingual setup, but I am not sure if that has any influence. Furthermore, this might occur as well in Umbraco 9, since I have been experiencing similar issues in Umbraco 9.

Specifics

No response

Steps to reproduce

Change the name of an existing node
Hook into ContentService.Saving, ContentService.Saved, ContentService.Publishing and ContentService.Published
Loop through PublishEventArgs<IContent>.PublishedEntities or ContentSavedEventArgs.SavedEntities
Check if the name was listed in IContent.GetDirtyProperties() and through IContent.WasPropertyDirty("Name") and through IContent.IsPropertyDirty("Name")

Expected result / actual result

Expected: The Name property is listed in the list of dirty properties, and therefore dirty when checked Actual: The Name property is never listed in the list of dirty properties, and therefore never dirty when checked

nul800sebastiaan commented 2 years ago

I don't know if the node name was ever intended to be included in the list of dirty properties as that was supposed to be for custom properties as far as I can remember. You can get the current node name from the content service and compare it to see the difference, the variant names are also available, for example

Note that I did test this on 9.4 and we recently fixed something for that for the upcoming 8.18.1 and 9.4.0 releases so you might not have enough info at the moment but will when those releases are out. https://github.com/umbraco/Umbraco-CMS/pull/12069

I'm going to send this off to the team as it feel like it would be nice if we had a IsNodeNameDirty / WasNodeNameDirty or something like that.

I haven't checked the Saved or Published notifciations, (as opposed to the Saving and Publishing ones), but I am sure you can't see the difference at that point since ContentService will have the same content as the Entities. I'd be curious what exactly you'd like to be able to do with the information that the node name was changed? It seems to me that it's most useful to know what the previous vs. current node name is but maybe I'm missing an interesting use case.

jerpenol commented 2 years ago

Thanks for your response @nul800sebastiaan. I don't know why, but suddenly it seems to work, so it recognizes that the name has changed.

I think it would be nice if we could check if the name has been changed in a legit way. We use a lot of custom lucene indexes for search overviews (along with the URL's of the results). When the name of a node changes, in most cases, the URL also changes along. If the name does not change we want to only update the page within the index, when it does change we want to update the descendants as well. Additionally it would be nice if we could check a property or name change specifically for a culture, for us that would remove a lot of indexing overhead to Saving and Publishing.

nul800sebastiaan commented 2 years ago

Additionally it would be nice if we could check a property or name change specifically for a culture, for us that would remove a lot of indexing overhead to Saving and Publishing.

See screenshot above. GetCultureName gives you the node name per culture. Similarly you can get the value of each property per culture and compare to see if they are equal to know if you need to do something.. for example:

We use a lot of custom lucene indexes for search overviews (along with the URL's of the results). When the name of a node changes, in most cases, the URL also changes along.

Yikes, okay, I can't judge but that doesn't sound like a great use of custom indexes. :-) As you already noted indexing upon saving and publishing is expensive. 🙈

jerpenol commented 2 years ago

@nul800sebastiaan The problem is that we update the Index OnPublished, because we fetch the data from the ContentCache. As I said in my initial bug, the version behind VersionId and PublishedVersionId contain the same properties and names, so I cannot compare those two.

Additionally, I think it's a bit overkill that I have to manually compare per culture while this information is being collected by the Dirty-interfaces, why not specify a change by culture?

I have a feeling you misunderstood the kind of indexes we have. This is not Examine, but a bunch of raw barebones Lucene indexes we very carefully maintain and update according to changes in the CMS.

nul800sebastiaan commented 2 years ago

Additionally, I think it's a bit overkill that I have to manually compare per culture while this information is being collected by the Dirty-interfaces, why not specify a change by culture?

Yeah I know it can always be better, to be honest some of the culture stuff was kinda bolted on later and I think it was difficult to retrofit it.

the version behind VersionId and PublishedVersionId contain the same properties and names, so I cannot compare those two.

Hmm, I didn't see that earlier... you said:

Furthermore on the ContentService.Published event (could be on the other events as well) the name of the version with id that equals IContent.VersionId is exactly the same as the version with id equals IContent.PublishedVersionId, while these two Id's differ (PublishedVersionId is the older one). They both hold the newly edited name of the content node.

That sounds correct. at that point the new content has been published and you're looking at (in the editor) the exact content that was just published, so indeed the versions should be the same. We don't create a new version at this point until you make another edit and Save or Publish it.

I'm going to have to conclude this discussion here since we need to make decisions, but first off: however overkill / convoluted - you now know the current state of events/notifications and what we bubble up and why it's an imperfect system that has kind of been bolted on. Be sure to do whatever you need to do to get the information you need to get your lucene indexes updated.

For improvements, we don't have anything currently on the roadmap to improve the notifications. The only thing I have to offer right now is that we can have a look at small changes if you send us a PR. I'll emphasize "small" since it seems like an area where it's easy to get lost and try to improve "everything". Note: we're no longer looking at v8 PRs but encourage contributions to v9.

However, I will close this issue as "works as intended" for now, but feel free to refer to it if you're interested in making a PR for this. I'll also add the idea label for us to filter on later in case we do some more large work on notifications, then we can easily find the information about the current friction again.

umbraco / Umbraco-CMS