Extensions on helper class does not save to DB

busrasengul commented 3 months ago

Which version of Skybrud Redirects are you using? (Please write the exact version, example: 4.0.8)

13.0.4

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

13.3.2

Bug description

Hey @abjerner !

Trying to extend RedirectsBackOfficeHelper as stated in the docs here

I'm overriding the Mapping functionality in order to change the destination URL, as far as I could understand that's the bit where the redirects get saved. Here's my code

    public override IEnumerable<RedirectModel> Map(IEnumerable<IRedirect> redirects)
    {
        foreach (var redirect in redirects)
        {
            redirect.Destination.Url = GetCurrentSiteDomain(redirect.Destination.Url);

            _redirectsService.SaveRedirect(redirect);
        }

        return base.Map(redirects);
    }

    public override RedirectModel Map(IRedirect redirect)
    {
        redirect.Destination.Url = GetCurrentSiteDomain(redirect.Destination.Url);

        _redirectsService.SaveRedirect(redirect);

        return base.Map(redirect);
    }

GetCurrentSiteDomain method is just getting the destination URL and adding a FE domain in front of it. I needed this extension so I can use redirects headlessly, as my BE URL is different than my FE URL.

But no matter I try and extend from RedirectsBackOfficeHelper, DB does not pick up my changes. I can see I add my custom URL to IRedirectDestination Destination model URL property, and FullUrl is picked up too, see the old and new Destination model in the screenshots, it doesn't save to DB still..

Am I missing anything or is this a bug? Thanks in advance :)

abjerner commented 3 months ago

Hi @busrasengul

The Map methods are used when the API controller is returning the redirects for the dashboard. The code in your example would therefore mean that each time a user sees the redirects dashboard, a number of redirects will be re-saved in the database. This is not intended, and actually quite bad.

We are pretty much only doing headless sites these days, and we haven't had any issues with the destination URL. Without knowing too much about your setup, adding the frontend domain (but not the backend domain) to the site node should generally ensure that the destination URLs are correct. Would this help in your situation as well?

If you still wish to update a redirect when it's saved to the database, this is currently not possible in the package. I've previously created an issue for Adding events/notifications for CRUD operations, which would be the best way to handle this. But at least for now, its not something I've had the time to look into.

Speaking of headless, some of our own sites suffer from the inbound URL column showing the backend domain instead of the frontend domain. There are a number of different scenarios to take into account, so I haven't yet found a good way to handle this directly in the package. But for the inbound URL, the Map(IRedirect redirect) method would be a good place to handle this.

busrasengul commented 3 months ago

Thank you @abjerner Yeah, I realized whenever I'm on a page in the backoffice it was updating the db without updating it 😄 Thank you for your suggestion but unfortunately, I have to add a custom BE domain to the node as well.. So it might add more complexity to the redirects? II doon't mind redirects show inbound URLs from BE domain as long as it does the redirect..

abjerner commented 3 months ago

@busrasengul since this is for a headless site, how are you getting the content (and redirects) from Umbraco?

The redirects package does contain two events/notifications that you could normally use for handling something like this for MVC sites, but if the user doesn't hit the normal request pipeline, you can't really use those.

The notifications:

Pre Look (via IRedirectPreLookupNotification) Executed right before the middleware will look for a redirect via IRedirectsService. You can use this event/notification to set a custom destination URL before the lookup, in which case the middleware won't look up a destination on its own.
Post Look (via IRedirectPostLookupNotification) Executed right after the middleware will look for a redirect via IRedirectsService. If a redirect was found, the notification will have a reference to the redirect.

Unfortunately it seems that I haven't documented those (although I should), but like mentioned, if you're in a headless context, you probably can't use them anyway.

skybrud / Skybrud.Umbraco.Redirects