kipusoep / UrlTracker

The Url Tracker for Umbraco
MIT License
54 stars 61 forks source link

NullReferenceException with 7.3 Beta 3 and Multiple root nodes #90

Closed JBoye closed 8 years ago

JBoye commented 9 years ago

I get the following exception on all 404 pages. It can be reproduced by installing a clean 7.3 Beta 3, and create multiple root nodes

[NullReferenceException: Object reference not set to an instance of an object.]
Umbraco.Web.Routing.DefaultUrlProvider.GetUrl(UmbracoContext umbracoContext, Int32 id, Uri current, UrlProviderMode mode) +243
Umbraco.Web.Routing.<>c__DisplayClass3.<GetUrl>b__0(IUrlProvider provider) +49
System.Linq.WhereSelectArrayIterator`2.MoveNext() +110
System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate) +214
Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id, Uri current, UrlProviderMode mode) +235
InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.UrlTrackerDo(String callingEventName, Boolean ignoreHttpStatusCode) in d:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:178
System.Web.SyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +160
System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +164
kipusoep commented 8 years ago

@danimalx This has got nothing to do with me being busy anymore. I don't know what to do to fix this... Also some of the Url Tracker features will be transferred to the core in the near future (scheduled for v7.5) :-) (source: https://onedrive.live.com/view.aspx?resid=4904432BE890F1F8!18961&app=PowerPoint&authkey=!AEm9CThFw42FZNw)

SelectUX commented 8 years ago

I've stepped into the URLTracker code and it seems the Umbraco method UrlName (on the Node) is failing from Umbraco v7.3.2+ due to a threading issue. I.e. when viewing the Umbraco IPublishedContent in VS debugger you see the following message when accessing UrlName: "The function evaluation requires all threads to run."

This message also appears on a number of other properties so clearly the core has changed somehow.

As a workaround I have used the Node "Path" to create the URL structure for the node. This only resolves the issue around redirects not working for moved nodes or manually created redirects failing. That however, for our projects at least, is by far the most critical issue here.

There is a Pull Request for this change if anybody wants to grab the code or if you want to add it to the current version for now @kipusoep.

nul800sebastiaan commented 8 years ago

Okay, it looks like I missed the revert of this PR: https://github.com/kipusoep/UrlTracker/pull/97/files

Have you tried handling BOTH EndRequest and PostRequestHandlerExecute?

I'll repeat myself again here ;-)

So: for non-static files you can use PostRequestHandlerExecute in 7.3.2 and the GetUrl call should succeed. For static files you'd still subscribe to the EndRequest event and do what you did before I guess? I just don't know how you managed to get a Url from this before.

kipusoep commented 8 years ago

For static files you'd still subscribe to the EndRequest event and do what you did before I guess? I just don't know how you managed to get a Url from this before.

This is not going to work, because the NiceUrl won't function in the EndRequest event.

nul800sebastiaan commented 8 years ago

Would there be a way you to that says to EndRequest: "Hey you EndRequest handler, don't bother, I've already handled this in PostRequestHandlerExecute and you don't need to look any further?"

ZNS commented 8 years ago

Just giving my two cents worth here. Due to this issue but also other unadressed issues with this otherwise great plugin. We've decided to write our own. We've got it working in 7.3.2 with ordinary requests hooking up to PostRequestHandlerExecute and to EndRequest for static requests, as Sebastiaan suggests. As he also suggest we do set a HttpContext-Item to make sure we don't run both on PostRequestHandler and EndRequest. However in EndRequest the UmbracoContext is always null so we have to execute EnsureContext for it to work. It's only run on 404 static requests so shouldn't be a big problem. However we do not use any legacy dependencies as it seems Url Tracker does, so not sure of how much use this is. Just trying to help out :)

kipusoep commented 8 years ago

@ZNS

also other unadressed issues with this otherwise great plugin

Which issues do you refer to?

It's only run on 404 static requests so shouldn't be a big problem.

So static requests can't get redirected to a Node's URL, just like the issue we're discussing here?

Ofcourse I can switch to PostRequestHandler for ASP.NET/Umbraco requests, but the issue will still exist for non-ASP.NET/Umbraco requests.
And those are just as important because of redirecting old URL's from a previoud website running Wordpress for example.

ZNS commented 8 years ago

@kipusoep Mainly this issue: https://github.com/kipusoep/UrlTracker/issues/84

Yes, our module works for static requests also. As long as we ensure the umbraco context in EndRequest. We use UmbracoContext.Current.UrlProvider.GetUrl(nodeId) to get url for page.

kipusoep commented 8 years ago

Hmmm that's interesting. That might just work, I'll have a look at it tomorrow hopefully.

Btw, I think it's a bit weird that you'd rather write your own plugin instead of contributing to existing plugins. Debugging the issue you mentioned and fixing the bug shouldn't be too hard, right?

StephenPAdams commented 8 years ago

@kipusoep Checking Force Redirect on any 301 redirects fixes this issue for me on Umbraco 7.3.1 with 3.9 installed. What about checking that allows it to work versus having it unchecked?

kipusoep commented 8 years ago

That's not an option. The Url Tracker will redirect before Umbraco has a chance of showing content.

Op 11 dec. 2015 om 06:09 heeft Stephen Adams notifications@github.com het volgende geschreven:

@kipusoep https://github.com/kipusoep Checking Force Redirect on any 301 redirects fixes this issue for me on Umbraco 7.3.1 with 3.9 installed. What about checking that allows it to work versus having it unchecked?

— Reply to this email directly or view it on GitHub https://github.com/kipusoep/UrlTracker/issues/90#issuecomment-163842660.

kipusoep commented 8 years ago

@ZNS I still get a NullReferenceException:

[NullReferenceException: Object reference not set to an instance of an object.]
   Umbraco.Web.PublishedCache.XmlPublishedCache.PublishedContentCache.DetermineRouteById(UmbracoContext umbracoContext, Boolean preview, Int32 contentId) +61
   Umbraco.Web.PublishedCache.XmlPublishedCache.PublishedContentCache.GetRouteById(UmbracoContext umbracoContext, Boolean preview, Int32 contentId) +86
   Umbraco.Web.Routing.DefaultUrlProvider.GetUrl(UmbracoContext umbracoContext, Int32 id, Uri current, UrlProviderMode mode) +96
   Umbraco.Web.Routing.<>c__DisplayClass3.b__0(IUrlProvider provider) +28
   System.Linq.WhereSelectArrayIterator`2.MoveNext() +58
   System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate) +94
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id, Uri current, UrlProviderMode mode) +169
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id) +15
ZNS commented 8 years ago

@kipusoep Well, pretty sure you get this error because UmbracoContext.Current.Application is null. Here's the code we use to ensure the context if it's null. Note that ensuring the context in EndRequest didn't work for us in 7.3.1 because it got disposed right away. In 7.3.2 this has been working though, haven't tested in 7.3.3 yet.

var contextBase = new HttpContextWrapper(HttpContext.Current);
UmbracoContext.EnsureContext(contextBase,
ApplicationContext.Current,
new WebSecurity(contextBase, ApplicationContext.Current),
UmbracoConfig.For.UmbracoSettings(),
UrlProviderResolver.Current.Providers,
false);
Shazwazza commented 8 years ago

What I'm not understanding is the need for UmbracoContext in the EndRequest?

Anything that needs to check against an ASP.Net handled request (i.e. Umbraco), the logic should exist in the PostRequestHandlerExecute which has the UmbracoContext available.

Whatever logic is actually being done in EndRequest should not have to access the UmbracoContext. For redirects that are tracked to managed ASP.Net requests - use PostRequestHandlerExecute. For redirects that are tracked for non managed ASP.Net requests (i.e. static files - from what I understand, this package tracks those), - you can use EndRequest.

EnsureContext should be a last option, this should not need to be used in this situation. Further more you should always ensure that you dispose of the UmbracoContext that is returned from EnsureContext.

Shazwazza commented 8 years ago

Ok, so Sebastiaan has told me that this package tracks some static mappings, such that someone might want to redirect a non handled ASP.Net page like blah.php -> Umbraco node 1234. In this situation, EndRequest will execute but PostRequestHandlerExecute will not because .php is not a handled request, just like .js or .css or other client side files are not handled requests. In all of these situations, an UmbracoContext will probably not even be available anyways because an UmbracoContext is generally only created for managed requests (there will be some exceptions to this norm).

So this is what needs to happen:

kipusoep commented 8 years ago

Thanks @Shazwazza for clearing things up a bit :-) Though when I use this piece of code:

var contextBase = new HttpContextWrapper(HttpContext.Current);
using (var context = UmbracoContext.EnsureContext(contextBase, ApplicationContext.Current, new WebSecurity(contextBase, ApplicationContext.Current), UmbracoConfig.For.UmbracoSettings(), UrlProviderResolver.Current.Providers, false))
{
    return context.PublishedContentRequest.RoutingContext.UrlProvider.GetUrl(nodeId);
}

I get a NullReference Exception:

[NullReferenceException: Object reference not set to an instance of an object.]
   Umbraco.Web.Routing.DefaultUrlProvider.GetUrl(UmbracoContext umbracoContext, Int32 id, Uri current, UrlProviderMode mode) +208
   Umbraco.Web.Routing.<>c__DisplayClass3.b__0(IUrlProvider provider) +28
   System.Linq.WhereSelectArrayIterator`2.MoveNext() +58
   System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate) +94
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id, Uri current, UrlProviderMode mode) +169
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id) +15
   InfoCaster.Umbraco.UrlTracker.Helpers.UmbracoHelper.GetUrl(Int32 nodeId) in D:\kipusoep\Documents\GitHub\UrlTracker\Helpers\UmbracoHelper.cs:140
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.LoadUrlTrackerMatchesFromDatabase(HttpRequest request, String urlWithoutQueryString, Boolean urlHasQueryString, String shortestUrl, Int32 rootNodeId, String& redirectUrl, Nullable`1& redirectHttpCode, Boolean& redirectPassThroughQueryString) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:412
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.UrlTrackerDo(String callingEventName, Boolean ignoreHttpStatusCode) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:199
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.context_EndRequest(Object sender, EventArgs e) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:81
   System.Web.SyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +141
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +69
JBoye commented 8 years ago

@kipusoep for static files, would a workaround like this work?

"OldFile.js" > 301 redirect to "UrlTracker.aspx?file=OldFile.js" > 301 redirect to "NewFile.js"?

kipusoep commented 8 years ago

No that's not a final solution I can cope with.

kipusoep commented 8 years ago

After some debugging I've found the NullReference to be on this line: var domainHelper = new DomainHelper(umbracoContext.Application.Services.DomainService);

It's on this line: https://github.com/umbraco/Umbraco-CMS/blob/d50e49ad37fd5ca7bad2fd6e8fc994f3408ae70c/src/Umbraco.Web/Routing/DefaultUrlProvider.cs#L63

ZNS commented 8 years ago

@kipusoep This was my conclusion also for 7.3.1 https://our.umbraco.org/projects/developer-tools/301-url-tracker/version-2/44327-Issues-with-URL-Tracker-in-614

If you debug further you can check when the UmbracoContext gets disposed. In 7.3.1 it got disposed in EndRequest even if I tried to ensure it. I haven't had that issue since 7.3.2 though.

Shazwazza commented 8 years ago

@kipusoep When you EnsureContext, this is just creating an UmbracoContext, you are getting a null ref exception because: context.PublishedContentRequest will be null... because this UmbracoContext hasn't had a PublishedContentRequest assigned because it's not a request with published content. You can access the RoutingContext directly on the UmbracoContext: context.RoutingContext

kipusoep commented 8 years ago

context.PublishedContentRequest is not null, but using context.RoutingContext gives another exception:

[NullReferenceException: Object reference not set to an instance of an object.]
   Umbraco.Web.PublishedCache.XmlPublishedCache.PublishedContentCache.DetermineRouteById(UmbracoContext umbracoContext, Boolean preview, Int32 contentId) +61
   Umbraco.Web.PublishedCache.XmlPublishedCache.PublishedContentCache.GetRouteById(UmbracoContext umbracoContext, Boolean preview, Int32 contentId) +86
   Umbraco.Web.Routing.DefaultUrlProvider.GetUrl(UmbracoContext umbracoContext, Int32 id, Uri current, UrlProviderMode mode) +96
   Umbraco.Web.Routing.<>c__DisplayClass3.b__0(IUrlProvider provider) +28
   System.Linq.WhereSelectArrayIterator`2.MoveNext() +58
   System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate) +94
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id, Uri current, UrlProviderMode mode) +169
   Umbraco.Web.Routing.UrlProvider.GetUrl(Int32 id) +15
   InfoCaster.Umbraco.UrlTracker.Helpers.UmbracoHelper.GetUrl(Int32 nodeId) in D:\kipusoep\Documents\GitHub\UrlTracker\Helpers\UmbracoHelper.cs:145
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.LoadUrlTrackerMatchesFromDatabase(HttpRequest request, String urlWithoutQueryString, Boolean urlHasQueryString, String shortestUrl, Int32 rootNodeId, String& redirectUrl, Nullable`1& redirectHttpCode, Boolean& redirectPassThroughQueryString) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:412
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.UrlTrackerDo(String callingEventName, Boolean ignoreHttpStatusCode) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:199
   InfoCaster.Umbraco.UrlTracker.Modules.UrlTrackerModule.context_EndRequest(Object sender, EventArgs e) in D:\kipusoep\Documents\GitHub\UrlTracker\Modules\UrlTrackerModule.cs:81
   System.Web.SyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +141
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +69

Again the exception occurs with this line: var domainHelper = new DomainHelper(umbracoContext.Application.Services.DomainService); This time it's inside Umbraco.Web.PublishedCache.XmlPublishedCache.PublishedContentCache.DetermineRouteById(UmbracoContext umbracoContext, bool preview, int contentId) on line 125. Again umbracoContext.Application is null.

Shazwazza commented 8 years ago

Ah right. Well if there's any conciliation, that won't occur in 7.4. We dispose and set a few referenced items to null on the UmbracoContext and some sub objects because at one point in time there was a fear of a potential memory leak with HttpContext, or objects not being GC'd that had references to old HttpContexts. Some of these null settings will still occur, but not ones where there is a reference to an application wide singleton such as the ApplicationContext.

In the meantime, the exception is coming from here: InfoCaster.Umbraco.UrlTracker.Helpers.UmbracoHelper.GetUrl Which I can see does this: https://github.com/kipusoep/UrlTracker/blob/master/Helpers/UmbracoHelper.cs#L135

which is not good since library is legacy and should only ever be used for XSLT. This will also always use singleton access because you are using statics. As a best practice, please try to shy away from using statics and Singletons whenever possible, that will just cause you issues in the long run.

Instead, do something like this:

//don't set the boolean for replaceContext since you don't need to use the singleton
using (var umbCtx = UmbracoContext.EnsureUmbracoContext(....)) {
     var helper = new UmbracoHelper(umbCtx)
     var url = helper.UrlProvider.GetUrl(...);
     //do stuff ....
}
kipusoep commented 8 years ago

Nice work, just tested with Umbraco 7.4.0 beta and it's working again :-) So my advice is to skip 7.3 or upgrade to 7.4!

Ps. I know it's legacy, but I had to support legacy versions too, so I'm not sure what can be refactored at the moment and what can't. Thanks for your help @Shazwazza

Shazwazza commented 8 years ago

That snippet I posted will work for 7.3 so I'm sure you could support it if you wanted.

I know a lot of devs have a single build to support multiple versions... But I don't really see the point. These are my own personal opinions: Just release a new major version of your project with a dependency on the latest version of Umbraco that you want to use/support. People can still use your older versions, and its easy to release another minor version if you need to, since everything is source controlled (and tagged accordingly). It also leaves you with less headaches, nicer code, less legacy code to support, as well as promoting people to use newer versions of Umbraco. This is what I do with my projects and people have never complained about it. If a new version of Umbraco comes out, and I want to use a new feature or bit of code, release a new major version with a new Umbraco dependency and document breaking changes .

Anyways, those are my opinion ;) have a great xmas!!!

kipusoep commented 8 years ago

Hmm I really don't know why I got that NullReferences then, I tried 7.3 multiple times and got the same Exception over and over again.

Anyway, I agree with you about versioning. Though I don't think it's worth it for the Url Tracker (functionality migrating to the core soonish?). Since Umbraco is much easier to upgrade nowadays (NuGet) it's easier to work as you described.

You have a great Christmas too :-)

Shazwazza commented 8 years ago

Some updates today:

I would still recommend that UrlTracker is versioned correctly since you don't have access to any of the new APIs in Umbraco unless you do this. I would recommend that you create a new major version of UrlTracker, use Nuget to reference Umbraco Core (remove your 'lib' folder and references) and start versioning this product the way you want. It will be much easier for you to manage, you can start cleaning up the usages of old legacy (probably untested and inefficient) APIs and start using new ones.

I would also still recommend that you handle static file and manage requests separately

Shazwazza commented 8 years ago

I would strongly recommend that you get the latest Umbraco 7.3.6 nightly when it's built and verify that it all works as you expect before the actual 7.3.6 release next week.

kipusoep commented 8 years ago

Thanks @Shazwazza, that's great news! Appreciate it! What you're saying sounds like a good plan to me. But how about the plan to integrate the basics of the Url Tracker functionality into the core? That's on the roadmap for v7.soon isn't it? When 7.3.6 nightly is available I'll do some testing.

Shazwazza commented 8 years ago

Yes that is a goal for 7.x. This will be a more basic approach however, it will track all changed Umbraco Urls but as far as static file redirects or other non-managed Ursl, that functionality will most likely remain outside of Umbraco core.

kipusoep commented 8 years ago

Ok, I will follow your advice and create a new major version :-) Thanks again!

kipusoep commented 8 years ago

So I created a v4 branch, just a quick question @Shazwazza; what v7 branch would you require for a new major version of this tool?

Shazwazza commented 8 years ago

Remove all your Umbraco references, delete your lib folder, change your target framework to 4.5.2, then run Install-Package UmbracoCms.Core. Now you are targeting a minimum of 7.3.5 ... So you'll need to wait until 7.3.6 is out to ew target. Or you can build the 7.3.6 nuget from the 7.3.6 branch.

kipusoep commented 8 years ago

I did, but I'm just thinking about targeting 7.0, 7.1, 7.2 or 7.3.6. Isn't 7.3.6 a bit too high? Lots of projects are lower then 7.3.6 or even 7.3 and they won't be able to update Url Tracker anymore.

Shazwazza commented 8 years ago

Then people can upgrade if they want to use new features. It really depends on the lowest APIs you want to use. You can stay stuck using the legacy APIs if you want but it's your choice and I don't recommend that. I keep articulate running on recent versions of Umbraco because I use recent APIs. Nobody complains, they just upgrade to the latest Umbraco version. When people install new versions of Umbraco, they don't install 7.1 or whatever, they install the latest version. I don't understand the reason why developers are so determined to support older versions of Umbraco. You can release a new major version of your product when you put a dependency on a newer Umbraco version... And move forward with your product.

kipusoep commented 8 years ago

Yeah I think you're right, developers tend to support older versions for a long time, including me. I installed Umbraco.Core v7.3.5 just now :+1:

kipusoep commented 8 years ago

Just tested v7.3.6 and the Exception is gone + the redirect works :-)

Shazwazza commented 8 years ago

Good news. Was there ever an Umbraco issue created for this ?

kipusoep commented 8 years ago

No not that I know of...