When changing Unicorn configuration it gives new paths for the yml files, resulting in errors in package

[mortenengel]

We're trying out Sitecore Courier for a new project. In this process we've been changing the Unicorn config, resulting in the exact same items being stored on different filepaths in the source and target library.

When this is the case, a deleted item won't be kept as an item deletion, but is removed by the line:
commands.RemoveAll(c => c is DeleteItemCommand && ((DeleteItemCommand)c).ItemPath.StartsWith(command.Deleted.ItemPath));

I took a quick look at the sourcecode, and considered doing a change myself, but wasn't sure if I'm misunderstanding something.

Basically I am confused by the entire GenerateDiff. Why compare all items of all types to eachother, and make way more differences than needed, and try to clean it afterwards?
In my mind, it would be simpler first taking all source items derived from ContentDataItem and comparing these to target ContentDataItem by Sitecore ID rather than path, and then just explicitly creating an add/remove/update based on differences.

Files would still need to be done exclusively on path as currently, but I think i'd work with sets on this as well.

If you wish, I'd be happy to do a pull request for such a change. I'm just unsure whether there's some reason I don't see for doing it the current way.

[adoprog]

Could you please provide a few sample items in the source and the target folders that would illustrate a problem or describe the scenario in more details?

As far as I understand, you have something like this:

Source:
...
/sitecore/content/Home/Item1
...

Target:
...
/sitecore/content/Home/Item1_Renamed (same ID as Item1)
...

And the resulting package does not contact "Delete Item" operation?

[mortenengel]

testcase.zip

Attached a zip of a very simple YML testcase, a template where I removed 1 field, while i also change the configuration name of the Unicorn, resulting in a different path. Creating a package of this source and target, results in the field not being deleted.

I see multiple other similar problems though, when using Sitecore path rather than Sitecore ID for comparison. Including moved and renamed items (I haven't tested those yet though).

[adoprog]

I've ran Courier with the following command (note "-r" for Rainbow):

Sitecore.Courier.Runner.exe -s C:\Source -t C:\Target -o C:\Package.update -r

and here's what I get under changed items:

<?xml version="1.0" encoding="utf-8"?>
<changeItemCommand>
  <collisionbehavior></collisionbehavior>
  <databasename>master</databasename>
  <itemid>{fb233b77-e9b5-4847-bdd8-8c74416ea79e}</itemid>
  <itempath>/sitecore/templates/Template 1/__Standard Values</itempath>
  <parent>{2a54b494-68fe-44f6-85d9-88a3b57cf689}</parent>
  <commands>
    <deleteFieldCommand>
      <collisionbehavior></collisionbehavior>
      <fieldid>{79a314b1-82ad-4733-bb82-92cc62306c31}</fieldid>
      <fieldname></fieldname>
      <oldValue>1</oldValue>
    </deleteFieldCommand>
  </commands>
</changeItemCommand>

[mortenengel]

That is correct, and as expected. It is the deleted field (field2) which is no - it is the standard value no longer present. t in the update package (should have been a delete item)

[adoprog]

Are you talking about "__Standard Values" item? I can see it is present (and is identical) in both source and the target.
If I run WinMerge over the two folders, the only change it shows is Field2 missing.

Technically, the "target" folder in your example contains corrupt serialization (as Standard Values in that folder should not have the Field2 standard value if everything was serialized correctly).

[mortenengel]

I am no longer at my computer. You may be right about the corrupt serialization, I made it by hand, as my "production" example is very large and contains sensitive data.

What this example source should have been was:
A simple template "Template 1" containing two checkbox fields Field1 and Field2. This was serialized by the unicorn configuration named 1.

My target was the same, except Field2 is removed (resulting in it's standard value being gone as well) and the unicorn configuration is renamed to 2.

What should happen here is 1 item change (Field2 value removed from standard values) and 1 item delete (Field2) - but the item delete is missing

[mortenengel]

I've had the chance to check up on the example I provided now. The target does not have a Field2 standard value, only Field1 (which is still present).

I guess what you're talking about is that the changeItemCommand on "__Standard Values" item created by Sitecore Courier end up having a deleteFieldCommand on the no longer existing field?
Whether that's invalid I don't know. But it's something that's technically unavoiadable, since it's not a requirement that the template of items (Including the entire hierarchy of base templates) is included in the serialization. However personally I would expect the deleteFieldCommand be present for values on deleted fields.

[adoprog]

Ok, I see where the issue is now, thanks! 👍 I'll review it and get back with my comments soon.

[mortenengel]

I've done a bit of copy pasting to try and illustrate. What I am missing is a deleteditem of the field I removed from the template:

[adoprog]

Thanks, I've added quick fix for this scenario (published as Release v1.2.3). The diff algorithm is not ideal and PRs with improvements are welcome. especially if they are also covered by unit tests.

[mortenengel]

I have created a change, I don't have good enough datasets to test at home, but I will test it tomorrow and commit if it works as expected.

I am however in doubt how you want the unit tests to run? I don't see any examples in the repository. Do you have any current examples? (which I would need to test my code on as well, to make sure it doesn't break anything else I did not account for).

[mortenengel]

I've done a bit more testing. Would still love to have it tested if you have any unit tests ready.
I dont have access to push a new branch.

Just leaving the notes I made for the commit here for later use:

Changed DiffGenerator quite a bit.

Some thoughts behind the changes

1. Rather than run through all items in the other iterator to check for complex equalities, I created maps with keys given their individual uniqueness. This should give much better runtimes for large sets of data.
2. The ContentItem equality comparer was based on Sitecore serialization. Unicorn paths are not unique in the same way - and databasename is not a part of unicorn paths (and depending on how the source and target was made, not necesarily part of the Sitecore either). I create the map key by Database and ItemId which will always be unique.
3. I honestly don't see the need for anything but full path for files. Given the windows file/foldername conventions
4. Simplified the logic quite a bit (did I oversimplify?). There's no longer any cleanup of unnecesarily created commands.
5. I added some exceptions which should only happen on corrupted/incorrect data. It could easily move on silently and just choose 1. But I personally believe exceptions with some info is the better way to go.
6. The test example I supplied in the issue was flawed. It was made by hand and the 2 fields in the source had the same ID. The new code dectected and gave info on this. The old did not.

[adoprog]

I dont have access to push a new branch.

Please just use a forked repository and create a pull request when the change is ready. I'll take a look into creating some unit tests over the weekend.

[mortenengel]

Ahh alright, never tried forks. Created one and made a pull request.

For now I've made an entirely new DiffGenerator, with a new commandlineoption -n to use it (meaning it's only enabled in commandline tool). You can review and decide whether to implement it.

I have done a large amount of testing, by comparing 2 different local development sites, which i both serialized with Unicorn and Sitecore.

What I did was take a copy of the App_Config, bin and Views folders, as well as a full serialization of master:/sitecore/System folder. I did this for the current master version as well as the forked version.

source project:
659 files
2380 items

target project:
742 files
2348 items

The results were that for both versions of the serialization, some items were missing in the deleteitems folders (some languages and some other settings).

Other than that, there were namechanges to 2 file, MongoDB.Bson.XML and MongoB.Driver.XML - these were changed from uppercase extension to lowercase extension. The "old" version considered these as changed, which results in no name change on the file. My version does a delete and add. So the filenames are exactly the same again.

I added some stopwatches as well to check time. But the DiffGenerator itself only took around 3-4 seconds with either serialization and either implementation, so even though the new was a tad faster, it was too little to consider it any meaningful optimization.

Have a look, I'll be happy to answer any questions you might have.

[adoprog]

Thanks! I've merged the pull request, will do some further testing and then include into the next release. I've also added a few sample diff generation tests at DiffGeneratorTests.cs

[mortenengel]

I've added a unit test to the class, it's set to run with NewDiffGenerator and works, but if you change it to the old DiffGenerator (commented out) the test fails.

• made a pull request.

[adoprog]

Merged in #42