Fix for issue 101: IPAddress not set in ASPNetcore

[StefanKert]

As described in #101 in AspNetCore there is an issue with setting the remote ip for the error. This should fix this issue. Only thing that needs to be checked is, if we want to save IP Addresses as IPv4, or IPv6.

Did not set IPAddress to a valid value since the REMOTE_ADDR value wasn´t added to the ServerVariables in AspNetCore

Moved fallback initialization for IPAddress (Servervariables) to SetIPAddress method.

[CptRobby]

@NickCraver Actually, I'm pretty sure that this change wouldn't break previously logged errors. IPAddress is stored in both the database tables and in the json serialization. In populating those two points, the getter would be called and so it would have there any value that was available for IPAddress, whether it was from _ipAddress or ServerVariables at the time. So whenever those errors get retrieved, the _ipAddress would have the value that's stored in the database/json. That means that there would never be a case where you're fetching an existing error and _ipAddress == null && ServerVariables?.GetRemoteIP() != null would be true.

Or is there something else that I'm missing here? :smile:

[CptRobby]

Though as I look at SetIPAddress() again, if Settings.Current.GetIPAddress() fails, then IPAddress would just be null and the ServerVariables?.GetRemoteIP() isn't assigned to IPAddress as a fallback. So the else on line 208 could just be changed to if (!IPAddress.HasValue()) and that would handle any fallback cases.

[StefanKert]

@NickCraver I am not sure but I think the only case that this change will break is if Null is stored in the db because SetIpAdress performs nearly the same task as the setter in the old version. In the old version, so the only thing I would add is a null check and a empt string if null, or maybe the change @CptRobby suggests.

[NickCraver]

Fixed in a much more direct way in 471c68b707e8814eeac4c82dc21ab060c3b44d26, but I'm stil debating the settings interface overall. I need to spend some more time on this, the concerns are different from the MiniProfiler refactor I just did to eliminate statics there.

What I want is Func<HttpContext, string> GetIPAddress, but that's fairly complicated :(

[StefanKert]

Ok. I do agree that the fix is more convenient. Maybe we should statt a separate issue for discussing how to do the settings right?

[CptRobby]

@NickCraver What about just storing the context in the Error as an object? It would only be used before storing the Error and wouldn't be serialized or anything. A generic getter function could then be added that would handle type checking and casting. Just add the following to Error.cs:

[JsonIgnore]
public object ContextObject { get; set; }

public T GetContext<T>() { return ContextObject as T; }

Then your GetCustomData function would look something like this:

public void GetCustomData(Error error)
{
    var context = error.GetContext<HttpContext>();
    if (context == null) return; //Can't do anything without the HttpContext
    //do stuff
}

What do you think of this idea?

[NickCraver]

@CptRobby the problem is that's not very flexible and throws away all compile-time checking across the map. It's too easy for a user to make mistakes, something you really don't want with an error logger. This has a dual conjunction problem in that Settings.Current exists at all. I'm going to play a bit and see how we can change things up.

If you want to see what I'm thinking, basically look at MiniProfiler, a base options and a inheritor that adds ASP.NET Framework (Core or non-Core) specific options on top, and where they're consumed is in the respective libraries as well. It's a huge change though, will take a bit to shift it all around...that's why I wanted to prove it out with MiniProfiler. In the end, static settings should go away. Maybe. I think. We'll see.

[CptRobby]

@NickCraver To clarify, I wasn't meaning to suggest that this would be used by end users. I was meaning more for the internal functions like the GetIPAddress you mentioned.

But I do think this could still help for providing a better end user experience. You could provide a subscribe function (or just a static property) that would take a Action<Error, HttpContext>, store that somewhere (static property or collection), and then when an error is being logged, grab the context and call that function (or functions, if it's a collection) passing in the error and the context. Then the user would still have the full benefits of compile-time type checking.

[NickCraver]

@CptRobby the problem is where does it live? At the moment, such a thing can't live on Settings (or any other place in .Shared), due to the primitives diverging between old and new ASP.NET frameworks. That's why we need inheritance in typed settings like MiniProfiler is using now IMO...giving it a go in stages here, let's see what happens.

[CptRobby]

@NickCraver I understand. I guess I was just assuming that you would understand that the changes to Error (which specifically doesn't include references to HttpContext) would be the only things I was talking about in Shared. The rest would have to live in the other two libraries where HttpContext would be specifically for ASP.NET or ASP.NET Core. Sorry if that was causing confusion. :blush:

[NickCraver]

@CptRobby @StefanKert Much more to do, but put in a stop-gap commit for tonight (different branch): https://github.com/NickCraver/StackExchange.Exceptional/commit/e61b5020ca22733bb495874669b01aa00ff55cb9 - need to move GetCustomData, etc. over to add HttpContext back and all that, but you guys can get an idea of what I'm going for there and I'd appreciate thoughts on everything. I think we may be able to greatly simplify the ASP.NET Core settings from the binder much easier with a custom binder now as well...going to look into that.

I'll play more as time allows, just had an hour tonight and wanted to give the split a shot.

[NickCraver]

Opened #108 for the above, so closing this out to cleanup.