Are wildcard querystrings supported?

[tormnator]

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

4.0.19

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

12.3.6

Question

We have a need to forward any querystring on the original URL to the destination URL. Let's say original URL is "/drawing" and destination URL is "/planner/drawing", and Forward query string is set to Enabled. Then whenever a request is made to /drawing, e.g. /drawing?group-id=1213, the redirect should go to /planner/drawing?group-id=1213.

This doesn't seem to work currently. Whenever the original URL has a querystring, the redirect does not happen. Only if we hardcode the querystring as a part of the original URL and the user happens to link using that exact querystring, does it work.

Is this as designed? If yes, could we add a feature to support the described scenario?

[abjerner]

Hi @tormnator

It's seems that I accidentally introduced a bug in the forward query string logic so this isn't working in v4.0.19 and v4.0.20. I will look into fixing this 😉

Meanwhile you should be able to downgrade to v4.0.18 from before the bug was introduced.

[tormnator]

Since this is a showstopper situation for us I went ahead and found a workaround. The following code seems to do the job, but a solution without having to add custom code would of course be appreciated.

/// <summary>
/// This class provides a workaround for redirects with querystrings not being handled by the
/// Skybrud Redirects package. The notification will only be called when a request's url is not
/// found in our site. See the comments on the Handle() method for how the workaround works.
/// </summary>
public class QueryStrRedirectPreLookupNotification : INotificationHandler<RedirectPreLookupNotification>
{
    public QueryStrRedirectPreLookupNotification(IRedirectsService redirectsService)
        => RedirectsService = redirectsService;

    private readonly IRedirectsService RedirectsService;

    /// <summary>
    /// Removes the querystring from the URL and then calls the RedirectService.GetRedirectByUri() to
    /// see if there's a redirect for the URL. If yes, and this redirect is configured to forward the
    /// querystring to the destination URL, then assign the redirect object to the notification's
    /// Redirect property. This will cause the RedirectsMiddleware's code to use that redirect. The
    /// code will reapply the querystring when it calls RedirectService.GetDestinationUrl() internally.
    /// </summary>
    public void Handle(RedirectPreLookupNotification notification)
    {
        int index = notification.RawUrl?.IndexOf('?') ?? -1;
        if (index > -1)
        {
            var uriWithoutQueryStr = new Uri(notification.RawUrl.Remove(index));
            var redirect = RedirectsService.GetRedirectByUri(uriWithoutQueryStr);
            if (redirect != null && redirect.ForwardQueryString)
            {
                notification.Redirect = redirect;
            }
        }
    }
}

I registered the notification handler by using this call during application startup (the necessary using statements were also added):

builder.AddNotificationHandler<RedirectPreLookupNotification, QueryStrRedirectPreLookupNotification>();

[tormnator]

@abjerner, of course the timing.... oh well. I will deploy with my workaround for now, and then hopefully remove it soon when your fix is released.

[abjerner]

@tormnator I pushed a commit with a potential fix here: https://github.com/skybrud/Skybrud.Umbraco.Redirects/commit/07f257a4d6220c245a4023c9294cc940a55c5242

I've set up a few test cases that all now pass, but I feel this is something that I should probably test a bit more, so I won't be able to push a new release today (it's getting late here).

I will try wrapping this up tomorrow evening instead 😉

[abjerner]

This should now be fixed in v4.0.21. @tormnator can you test and confirm?

The issue also affects the package for Umbraco 13, so a new release will be out soon for U13 as well.

[abjerner]

Bah. I tested a bunch of different cases, and the query string forwarding seemed to work like it should, so I pushed the v4.0.21 release. Then I went along with updating the U13 package, and ended up testing with a case that didn't work and I hadn't tested for in the U10 package 🙃

So now there is a v4.0.22 release as well 😉

And v13.0.3 for Umbraco 13 😎

[tormnator]

I'm testing the latest release, but it looks like redirects are still broken. The following code should be improved:

// To support query string forwarding, we should only return a redirect that match either of the two criteria listed below:
// - query string forwarding isn't enabled and the query string is an exact match
// - query string forwarding is enabled and the query string is part of the query string of the inbound URI
string query1 = query.Length == 0 ? string.Empty : $"&{query}&";
RedirectDto? dto = dtos.FirstOrDefault(x => (!x.ForwardQueryString && query == x.QueryString) || (query1.Contains($"&{x.QueryString}&") && x.ForwardQueryString));

Use case 1: x.ForwardQueryString == true; and the original URL does not have a querystring. In this case query.Length == 0 and hence query1 == string.Empty. What happens now is dto is set to null because the query1.Contains($"&{x.QueryString}&") fails because it assumes query1 contains the wrapper ampersands.

Use case 2: x.ForwardQueryString == true; the registered redirect does not have questrying specified (because we accept any querystring or too many combinations of a given set); and the original URL contains a querystring, but the contents of the querystring are unknown (and that okay). Let's assume the querystring is "value=1". In this case the query1 variable == "&value=1&" and since the test becomes query1.Contains("&&"), dto is set to null.

The expected outcome is in both use cases the dto should be assigned.

[tormnator]

Hi @abjerner, have you had a chance to look this issue again after my previous comment?

[abjerner]

@tormnator I've been caught up in too many other things, so I haven't had the time to look further into this. I had a feeling I was missing some cases (and I should probably have considered empty query strings), so it was always my intention to come back and have another look.

v4.0.22 passed the test cases that I had set up, and even if it didn't cover all cases, it was better releasing a partial fix instead of no fix.

Anyways - I had a quick look, and and think I have a fix for the additional cases. But before I push a new release, could you help sharing some example URLs that I can add to my test setup?

There is also the case when an inbound URL matches two or more redirects with query string forwarding. This scenario is hard to handle, so it has always been somewhat random which redirect is then picked. So far my conclusion has been that this is an acceptable result.

[tormnator]

On our site we don't have any cases where we want the query string, full or partial, part of the inbound URL registered as the Original URL. What we have are inbound URL's which may or may not have query strings. These query strings can have a combination of keys and the full query string should always be forwarded. Here is an example (these aren't real, but conceptually similar to real ones on https://www.langlo.no):

1. /drawing --> /planner/drawing
2. /drawing?custNo=123 --> /planner/drawing?custNo=123
3. /drawing?custGroupId=AA --> planner/drawing?custGroupId=AA
4. /drawing?custGroupId=AA&catalog=Oct23 --> planner/drawing?custGroupId=AA&catalog=Oct23

If you provide a "requirement spec" or a more full up-to-date documentation on the expected behavior, including backwards compatibility, then it would it easier to help test and implement the code (hint, hint :-)).

[abjerner]

Thanks 👍

I think 18469f43a241ddc636ee9cf65592114a6d7126b2 fixes the last issues - at least your examples now work as expected. I will try to follow up tomorrow and hopefully push a new release 😉

[DanielToxic]

Hi @abjerner.

Just tried with 13.0.4 and it looks like everything's smooth when ForwardQueryString is set to true. 🥳

But looks like it doesnt find the Redirect with ForwardQueryString set to false. For example, if we redirect from /oldpage to /newpage, and someone hits /oldpage?param=value, they should end up at /newpage without the querystring if ForwardQueryString is off, right? Think it's been like this before? 🤔

[abjerner]

Forgot to post a new comment here yesterday when I pushed new releases for U10 and U13. @tormnator does the v4.0.23 release solve your remaining issues?

@DanielToxic I'm fairly certain that your case wouldn't have worked in earlier releases either. If query string forwarding isn't enabled, it has to be an exact match. I can see your use case, and others have reported something similar before, but allowing this behavior would add some ambiguity since a request could potentially match multiple redirects (which is already the case when query string forwarding is enabled).

[tormnator]

Sorry for late follow up here, but I was finally able to update and test v4.0.23, and my redirects work as expected now.

[abjerner]

Awesome. I'm closing the issue then 😄