KevinJump / uSync

Database syncing tool for Umbraco CMS
https://jumoo.co.uk/usync/
Mozilla Public License 2.0
111 stars 65 forks source link

DataType Update during packageMigration completed then overwritten by uSync import at startup #640

Open mistyn8 opened 6 months ago

mistyn8 commented 6 months ago

umb 13.1.1, uSync 13.1.1

I've written a package migration that as part of it's role updates a dataype configuration (blocklist)

However, although this works as expected, adding uSync into the mix causes the update to be reverted as importAtStartup : Settings is in use.

Is it that the notification service isn't present to let uSync know that a dataType has changed? If so can we manually notify uSync? I do seem to remember there was a savewithevents in days gone past is that something that's missing?

Any pointers so that my migration supports uSync greatfully recieved

simplified code

public class PackageMigration1 : PackageMigrationBase
{
   public IglooPackageMigration1(IPackagingService packagingService, .... , IDataTypeService dataTypeService)
   {
       _dataTypeService = dataTypeService;
   }

   protected override void Migrate()
   {
      var contentBlockList = _dataTypeService.GetDataType(new Guid("e52e988a-65e8-45b0-87d7-dfa013357177"));
      // update our datatype and save it
      _dataTypeService.Save(contentBlockList);
      // not generating uSync config file....

   }
KevinJump commented 6 months ago

Hi,

I think the issue is probibly that the migrations are running before uSync has been able to register for the notification events, so it can't listen for them.

if you can you could look at triggering the migrations after the uSyncComposer has ran, but i don't think you can do that with a package migration, you would have to look at using the core migrations (which are very similar) as you have greater control over when they run in the sequence.

I don't think we can get uSync to be registred before package migrations because they happen in the core.

Kevin

mistyn8 commented 6 months ago

That makes perfect sense, thank you for the swift response.

mistyn8 commented 6 months ago

Pushing my luck.. so core migrators, are we talking https://cornehoskam.com/posts/how-to-create-run-migrations-umbraco-v10 and the https://docs.umbraco.com/umbraco-cms/extending/database#schema-class-and-migrations approach?

rather than https://docs.umbraco.com/umbraco-cms/extending/packages/creating-a-package#package-migration

KevinJump commented 6 months ago

Yes,

also some samples here https://github.com/KevinJump/DoStuffWithUmbraco/tree/NetCore/src/DoStuff.Core/Migrations

mistyn8 commented 6 months ago

superstar! I presume I just loose the run package migrations bit and visibility in the back office? by it not being a package migration.. any thing else of note?

KevinJump commented 6 months ago

Also, if you add the ComposeAfter tag it will run after uSync's composer.

    [ComposeAfter(typeof(uSync.BackOffice.uSyncBackOfficeComposer))]
    public class MyComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            throw new System.NotImplementedException();
        }
    }

in order to ensure that uSync (and umbraco) has fully started up before you do anything, you should run your migration code in a UmbracoApplicationStartedNotification (not starting as in the examples).

Started happens slightly later, and after the DB and everything are installed and randomly the language service has been populated with all the correct translations (so will work after installs and upgrades everytime)

KevinJump commented 6 months ago

yes, you loose the 'run migration' button, your package will still showup in the back office just without that.

mistyn8 commented 6 months ago

Definately pushing my luck now.. ;-) [ComposeAfter(typeof(uSync.BackOffice.uSyncBackOfficeComposer))] what happens if no uSync (optional) and not wanting to introduce a dependancy on uSync in my package?

KevinJump commented 6 months ago

Oh, I think you have to be very clever with reflection, but i am not 100% sure how.

mistyn8 commented 6 months ago

So looks like things get done in the right order now at least according to the logs.. but that no notifications are raised from a _dataTypeService.Save(dataType); in a migration plan

[00:32:16 INF] uSync Import: 8 handlers, processed 236 items, 0 changes in 1123ms
[00:32:16 INF] uSync: Startup Complete 1280ms
[00:32:16 INF] Starting 'DataTypeWidgetAddition'...
[00:32:16 INF] At fc40ffce-2e6a-4fe8-a0a6-afcca740baaa
[00:32:16 INF] Done

Even added a simple

public class DoStuffContentNotificationHandler : INotificationHandler<DataTypeSavingNotification>
{
    private readonly ILogger _logger;

    public DoStuffContentNotificationHandler(ILogger<DoStuffContentNotificationHandler> logger)
    {
        _logger = logger;
    }

    public void Handle(DataTypeSavingNotification notification)
    {
        _logger.LogDebug("Don't shout");
    }
}

public class DontShoutComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddNotificationHandler<DataTypeSavingNotification, DoStuffContentNotificationHandler>();
    }
}

with

    [ComposeAfter(typeof(DontShoutComposer))]
    public class HeroWidgetInnerOverlayStrengthMigrationComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            builder.AddNotificationHandler<UmbracoApplicationStartedNotification, HeroWidgetInnerOverlayStrengthMigration>();
        }
    }

Notification gets handled when using the backoffice, but not from the migration plan :-(

Think I exhausted the breath of my capability now.. will have to ship with some directions on uSync integration...

I also noticed that the core packagemigrator, persists dataypesection folder keys (guids) as well as names from the xml, uSync configs only carry the name..

<DocumentType Folders="Widgets/Grid/Accordion" FolderKeys="fadd2193-84ec-4cd5-96f7-d6901c842a7c/dad5dd1b-321a-4809-8942-5beea83b6c38/06a8a294-5815-4692-b270-3b9ef3f15d21"> just peeked my interest..

mistyn8 commented 6 months ago

FYI just tested with a cloud project and uda's aren't created by umbraco Deploy either...

KevinJump commented 6 months ago

Hi,

for the last bit yes, usync doesn't sync folders explicitly, rather it syncs items in folders and creates the folders as needed, we don't sync the key of the folder because it doesn't matter, and we try to store the minimal amount of info, so we can reduce the amount of false changes (its all about performance, and portability.).

instead when a doctype is saved/created/etc the folder is located by name or created new if needed.

mistyn8 commented 6 months ago

I think there is a scenario where folder keys have merit. (much like why node keys are bestpractice UID) package installs a folder, user changes name of if.. package ships an update and wants to put item in the same folder, with a key we can find it. But name only we generate a new folder with the old name? Edge case and a few moving parts between packagemigrartion/usync prob seen in a dev team, not the end of the world to have two folders now and sort after the fact ;-)

Also packagemigration created doctypes seem to get a uda created.. but not manual use of the datatypeservice.save() can't see anything extra going on in https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Infrastructure/Packaging/PackageDataInstallation.cs 🤔

Luuk1983 commented 5 days ago

I'm running into the same issues here as well and for me this is a very VERY serious issue.

I have a package that has a migration that adds an additional property to a media type. When using the PackageMigration, it doesn't work as mentioned above, because the property is added in the migration and uSync removes it again if it runs after that.

So I don't mind rewriting the PackageMigration to a BaseMigration also as mentioned above. So I did that and that works....until the site is restarted. I don't want my package to have a dependency on uSync, so I can't specify ComposeAfter, but still, it seems that the migration is run after uSync.

So far so good, but there is no usync file that gets updated, like @mistyn8 mentioned. So the next time uSync runs, it will again remove the property from my media type.

This is a very serious issue for me, because that makes migrations practically useless for anything that touches uSync controlled content. Isn't there a way to tell uSync to export that specific media type, so it updates the files? I see the ContentTypeHandler as a good candidate, but the parameters seem quite complex :P

mistyn8 commented 5 days ago

after a discussion on discord.. the functionality is by design. to stop several packages blowing each other up..

workaround is to hook into the post migration handler.. (fired when all package migration required at runtime are complete)

you can then do something like this to once if save to do go and use the services to resave any changes you've made and uSync I think will get it's notifications that things have changed.

public class IPostMigrationNotificationHandler10 : INotificationHandler<MigrationPlansExecutedNotification>
{
    private readonly IRuntimeState _runtimeState;
    private readonly IContentTypeService _contentTypeService;
    private readonly IDataTypeService _dataTypeService;
    private readonly ILogger<IglooPostMigrationNotificationHandler10> _logger;

    public IglooPostMigrationNotificationHandler10(IRuntimeState runtimeState, IContentTypeService contentTypeService, ILogger<IglooPostMigrationNotificationHandler10> logger, IDataTypeService dataTypeService)
    {
        _runtimeState = runtimeState;
        _contentTypeService = contentTypeService;
        _logger = logger;
        _dataTypeService = dataTypeService;
    }

    public void Handle(MigrationPlansExecutedNotification notification)
    {

        if (_runtimeState.Level is not RuntimeLevel.Run)
        {
            return;
        }

        // Check if we have run the right migration, otherwise skip
        if (HasMigrationRun(notification.ExecutedPlans) is false)
        {
            return;
        }

        var datatypeGuid = new Guid("f8786b4b-a700-4707-a7b3-619a12d80345");
        var navigationSettingsKey = new Guid("c21ca0cf-b285-419d-a454-bb2834de77d9");
        var footerNavigationSettingsKey = new Guid("7a5e0631-1d73-45e5-b521-6199880a639a");

        if (_dataTypeService.GetDataType(datatypeGuid) is IDataType navLayoutDatatype)
        {
            _dataTypeService.Save(navLayoutDatatype);
        }
        else
        {
            _logger.LogWarning("Can't find dataType - IG - Navigation Layout - Radio");
        }

        if (_contentTypeService.Get(navigationSettingsKey) is IContentType navigationSettingsContentType)
        {
            // resave the contentType
            _contentTypeService.Save(navigationSettingsContentType);
        }
        else
        {
            _logger.LogWarning("Can't find navigation settings");
        }

        IContentType footerNavigationSettingsContentType = _contentTypeService.Get(footerNavigationSettingsKey);
        if (footerNavigationSettingsContentType is not null)
        {
            // resave the contentType
            _contentTypeService.Save(footerNavigationSettingsContentType);
        }
        else
        {
            _logger.LogError("DocType update: Required elements not found for footer navigation settings");
        }
    }

    private static bool HasMigrationRun(IEnumerable<ExecutedMigrationPlan> executedMigrationPlans)
    {
        foreach (ExecutedMigrationPlan executedMigrationPlan in executedMigrationPlans)
        {
            foreach (MigrationPlan.Transition transition in executedMigrationPlan.CompletedTransitions)
            {
                if (transition.MigrationType == typeof(IglooPackageMigration10))
                {
                    return true;
                }
            }
        }

        return false;
    }
}
Luuk1983 commented 5 days ago

And the reason is that actions done in the migration itself does not trigger notifications that uSync relies upon? And this solution simply 'retriggers' the notifications by performing them again?

It might work, especially if I check if uSync is event installed. It's tricky though. If the migrations succeeds, but the 'MigrationPlansExecutedNotification' fails, you can get weird results.

Still think it would be nice if I could just tell uSync to export a certain content or media type when it's updated.

mistyn8 commented 5 days ago

Simply that a user installs two packages from different developers at the same time, my packages removes something that the other package relies on and boom we have a fire. Ill dig out the discord thread for you to read... Much better minds explained it better..

Luuk1983 commented 5 days ago

Simply that a user installs two packages from different developers at the same time, my packages removes something that the other package relies on and boom we have a fire. Ill dig out the discord thread for you to read... Much better minds explained it better..

Ok thanks :) I was thinking if I could use the eventAggregator to manually dispatch a (in my case) MediaTypeSaved notification during the migration :P

mistyn8 commented 5 days ago

yeah that's the issue though all notifications are suppressed during migrations..

https://discord.com/channels/869656431308189746/1245696446301208616/1245779272493170740 or if you aren't on discord https://discord-chats.umbraco.com/t/18860516/package-developers-i-see-i-can-use