Infocaster / UrlTracker

An Umbraco package that tracks 404 not found results and helps you redirect visitors to the right page
https://infocaster.net/wat-we-doen/umbraco-plugins/301-url-tracker
MIT License
17 stars 12 forks source link

"Url has changed" On all nodes each time I save and publish the home node #161

Closed mrflo closed 10 months ago

mrflo commented 10 months ago

Describe the bug Using version: 12.0.0 Using Umbraco version 12.2.0 Creating an entry "Url has changed" for all the nodes and descendants each time I save and publish the home node. Ending with a huge number of records.

To Reproduce Steps to reproduce the behavior:

Expected behavior It shouldn't create records as nothin was changed.

A work around is to disable notifications and enable the default Umbraco Url redirect management "Backoffice": { "Notifications":{ "Enable": false } },

D-Inventor commented 10 months ago

Hi @mrflo ! Thank you very much for reaching out! I'm going to try out your reproduction steps and see if I can reproduce the issue.

I have a few questions though: Do these steps describe the process on a clean install? Do you have any special properties set up on your home node, like an internal redirect, a url alias or a url name?

mrflo commented 10 months ago

Hi, thanks for your quick answer @D-Inventor It's not a clean install in fact it's a migrated one from V7 but there is no specific properties except an empty umbracoUrlName. Also we don't have domain on descendants nodes. I discovered this issue because Url tracker was giving a SQL error " String or binary data would be truncated" without naming the table name or knowing at first it was coming from url tracker. We have some sub-node generated by user that have long page name. I'm wondering if the length of 255 for url's is big enough (I fixed it by switching those to 400 but was obviously still very slow because it was creating a record for each descendant, it had 300k + useless entries) but that's another subject.

It seems to be an issue of considering the domain name as part of the old url and detecting it as a change of url ?

Also something to note: I though installing UrlTracker was automatically shutting down the default Umbraco Url redirect management. It's not the case anymore it seems

D-Inventor commented 10 months ago

Good news: I have been able to reproduce this issue and I've identified at least one possible cause: On this line in the code, the check for equality on the Umbraco url name fails, because one returns null while the other returns an empty string. Screenshot of debugger for evidence: image

As this comparison fails, the URL Tracker assumes that the URL has changed and inserts redirects for the content type and all it's descendants. I'm going to see if there is a straightforward solution to this issue and come back when I have an update.

D-Inventor commented 10 months ago

Regarding your closing comment: I've considered that feature and I personally did not find it compelling. I figured as a consumer, you should be able to decide for yourself which features you want to use and which you don't, so instead I allow you to disable the notification handling of the URL Tracker if you prefer the Umbraco redirect manager and you're always free to disable the redirect manager of Umbraco if you prefer to have all your redirects inside the URL Tracker. Both work, so the choice is yours. You could even keep both if you want.

mrflo commented 10 months ago

Thanks for finding the cause of the error quickly and glad it's not a config error or other things.

Also please consider making the url fields bigger if you have the time or nice error with the url name if it failded to create the redirect that can surly help because the error receive by the umbraco backoffice was really not clear. I had to play with turning plugins on/off to find where it was coming from.

Thanks again

D-Inventor commented 10 months ago

I understand, it's not helpful to get a generic database error without clear indication what exactly went wrong. I'll see what I can do.

D-Inventor commented 10 months ago

woops, I didn't realize that the issue would immediately close when I push a commit that references this issue

D-Inventor commented 10 months ago

@mrflo I just pushed a new package version to NuGet: 10.3.1-beta.1. This package should contain the fix for your issue. Would you like to give it a try and see if this fixes your issue?

mrflo commented 10 months ago

Hi @D-Inventor I've just installed it and tested it and it works well now. Thanks! I've also tested to rename the home now and got the much nicer field errors:

Screenshot 2023-10-17 at 10 50 56 Screenshot 2023-10-17 at 10 51 40

I'm still thinking the fields should be bigger if you have a lot of sub-nodes and long page names : Maximum URL length for popular browsers Microsoft Internet Explorer: 2,083 characters. Microsoft Edge: 2,083 characters. Google Chrome: 32,779 characters. Mozilla Firefox: more than 64,000 characters. Apple Safari: more than 64,000 characters. Google Android: 8,192 characters.