ASP.NET Core functionality

[StefanKert]

Since we are closing #89 I wanted to open this issue for discussing ASP.Net Core functionality. I don´t know if you already have some ideas for this @NickCraver, but I would suggest adding a separate Middleware for using the Exceptional Endpoints:

app.UseExceptionalEndpoints(route: "errors");

So we´d populate the List of errors via the "errors" endpoint. I´m not sure if the naming is ok. Maybe UseExceptionalHandler would be a better name for this, since there is already an ExceptionalHandler in the other Project.

If this feature makes sense I could send a PR. Got a running version, but need to tweak it a little bit so we can use both ways to implement it (Middleware and controller endpoint).

Another important thing is the configuration. Like @NickCraver mentioned in the #89 the best way to put this would be a basic JsonSerialization. Since you he prepared the Settings class to work with basic serialization this should not be a big deal.

[NickCraver]

I kinda want to avoid this if at all possible, I think the one-line include in the app (making your own route) is a more secure default to go with. For example if developers have a developer/admin/whatever controller that's secured, they get all the security for free. Exceptions are fundamentally risky information as they can expose data, weak points, etc. so making the simple way the insecure way I'm very wary of.

But settings deserialization is another matter, that could be fun as the base settings object has typed things like RegEx collections on it. This means the JSON deserialization may need a whole other class that mirrors Settings (let's say ConfigSettings again) with string versions for RegExes for example, and we loop through and set on Settings.Current anything we find. I really don't want to mess with that quite yet and would more than welcome a PR on it - want to give it a shot? :)

I just changed settings a bit to take an action lambda to be more in line with MVC conventions (minor). I'm going to take a look at replacing the Developer exception page next, where you can set a bool to return Exceptional's single error page instead on-throw locally, for a more consistent experience overall. We can't do this practically for oh so many situations in ASP.NET, but since we're in a pipeline in ASP.NET Core it's totally viable.

I should note: the lambda settings change should be more friendly to the deserialization angle, since we now change things like this:

app.UseExceptional(settings => 
{
    settings.ApplicationName = "Samples.AspNetCore";
});

...so that runs on Settings.Current, which would already be modified by whatever we deserialized before. Yay progress!

[StefanKert]

I kinda want to avoid this if at all possible, I think the one-line include in the app (making your own route) is a more secure default to go with. For example if developers have a developer/admin/whatever controller that's secured, they get all the security for free. Exceptions are fundamentally risky information as they can expose data, weak points, etc. so making the simple way the insecure way I'm very wary of.

Totally agree with this. That's so true. Didn´t take this into consideration.

But settings deserialization is another matter, that could be fun as the base settings object has typed things like RegEx collections on it. This means the JSON deserialization may need a whole other class that mirrors Settings (let's say ConfigSettings again) with string versions for RegExes for example, and we loop through and set on Settings.Current anything we find. I really don't want to mess with that quite yet and would more than welcome a PR on it - want to give it a shot? :)

Sounds good to me :) I will take a look and file a PR if i get to a result that is applicable.

I'm going to take a look at replacing the Developer exception page next, where you can set a bool to return Exceptional's single error page instead on-throw locally

I like this idea. Since the Default Developerexception page is cool but really overwhelming I do like the Exceptional Page more. Especially the "thrown x times" sounds like a really cool idea to me too see immediately how often this error occurred.

The Lambda thing looks good to me. I will take a look at the Settings Deserialization and what would be the best way to use this and get back to you.

[StefanKert]

Since I had time to get into the details for implementing the Settings Deserialization I got some questions on the current Settings object.

Is there any specific reason that we don´t use a Dictionary for Regexes and Types? The current implementation for ConfigSettings in StackExchange.Exceptional parses a Name and the Pattern from the xml config file. I get the point that we don´t need it in the Settings, but it would help finding issues with IgnoreSettings if we add the names for the patterns to the settings object. Also, it would make the deserialization easier.

There are a few other properties I would like to change if this makes sense, most of them from a simple Collection to a Dictionaryor a KeyValuePair.

@NickCraver does this make sense to you? Or what was the reason behind adding the names to the Configuration but not to the Settings class.

[NickCraver]

@StefanKert IMO, nope not really - comments are there for comments (and supported in ASP.NET config JSON), the names aren't actually used and that's why I removed them. Things like the HashSet<string> on types also does things like deduplication inherently.

[StefanKert]

Awesome. Will give it a try and get back if I got a result or have further questions!

[StefanKert]

Got another question :)

Before I change these bits I wanted to make sure that there is no specific reason that I probably overlooked.

The LogFilterSettings contain two properties Cookies and Forms. Is there any specific reason why you are using a Dictionary? The Key value is the name that is obvious, but Value is the Value that should Replace the Cookie/Form with the Key/Name

Right now the impl looks like this:

public Dictionary<string, string> Form { get; set; } = new Dictionary<string, string>();

Since we are already using a separate LogFilter class in the classic MVC project wouldn´t it make sense to use this Setting as List of LogFilters?

public List<LogFilter> Form { get; set; } = new List<LogFilter>();

The benefits of this would be the simplification when serializing and also the better naming for the values (value -> replacewith). Also it is easier extensible if we wan´t to add further settings for LogFilters in the future.

Same proposal for IgnoreSettings. Since we already have these classes in the StackExchange.Exceptional project we just could move these parts over to the Shared project.

[StefanKert]

So basically the settings (json) would look like this:

{
  "Exceptional": {
    "DataIncludePattern":  "(efg)*",
    "Email": {
      "ToAddress": "test@test.com",
      "FromAddress": "noreply@test.com",
      "FromDisplayName": "exceptional - NoReply",
      "SMTPHost": "smtp.test.com",
      "SMTPPort": 1234,
      "SMTPUsername": "dummy",
      "SMTPPassword": "pwd",
      "SMTPEnableSSL": true,
      "PreventDuplicates": true
    },
    "LogFilters": {
      "Cookie": [
        {
          "name": "firstFilter",
          "replaceWith": "test"
        },
        {
          "name": "secondFilter",
          "replaceWith": "SecondNonLoggedThings"
        }
      ],
      "Form": [
        {
          "name": "firstFilter",
          "replaceWith": "test"
        },
        {
          "name": "secondFilter",
          "replaceWith": "SecondNonLoggedThings"
        }
      ]
    },
    "Ignore": {
      "Regexes": [
        {
          "Name": "Test",
          "Pattern": "String"
        },
        {
          "Name": "Test2",
          "Pattern": "int"
        }
      ],
      "Types": [
        {
          "Name": "Test",
          "Type": "String"
        },
        {
          "Name": "Test2",
          "Type": "int"
        }
      ]
    },
    "Store": {
      "Type": "TypeTest",
      "Path": "PathTest",
      "ConnectionString": "ConnectionStringTest",
      "ConnectionStringName": "ConnectionStringNameTest",
      "Size": 4000,
      "RollupSeconds": 100,
      "BackupQueueSize": 100
    }
  }
}

[NickCraver]

@StefanKert the reason they are dictionaries is 2-fold: They are lookups (performance), and only the first lookup wins, so semantically they are dictionaries. I'd much prefer to keep them that way. There's nothing really to add in terms of primitives here, it's a simple find/replace in every day.

[StefanKert]

Ok. So basically performance is the reason for the Dictionary. Makes sense to me. I will keep the current structure and look if I can come up with a cool solution :)

[NickCraver]

Developer exception page is in, see https://github.com/NickCraver/StackExchange.Exceptional/commit/aa9a414cd54fe1ef493a6319e2471f5e77ab0384 for detials :)

[StefanKert]

Looks great :) will give it a try today. I've come up with a first working version with StefanKert/StackExchange.Exceptional@a39d371

StefanKert/StackExchange.Exceptional@4d836d33b also fixes a bug that's in the classic MVC version. I can send end extra PR for this if you like. Basically, the error occurred when ignore settings are added to the xml file. When deserializing these ignore settings get added to a list that is not instantiated. This caused a nullreferenceexception. Just let me know if you want to have this as a separate PR.

I will try to proceed today but right now I am a little bit busy with getting the release for buildvision out :)

[NickCraver]

@StefanKert on the class MVC: yes please, I'll merge that immediately. I think you have the settings structure right for the object model on the new bits, but we can simplify the interaction much more. If you want to toss up a PR (minus the .csproj file noise please) I can take a look at the branch and see what I can simplify down. For example I think we can inject IConfiguration, etc. but need to play - feel like tossing up 2 PRs when time allows?

I'm at the beach next week and this laptop is hurting bad, so it could be nothing at all gets done there, just a fair warning ahead of time :)

[StefanKert]

@NickCraver try to see what i can come up with. Yep thought about some ways to simplify the process but most of them need some minor changes in original Settings object.

Regarding the 2 PRs, I found another issue, that happens if you a Get Request is performed the Form Property of the error is not initialized, but if we have settings defined it tries to access this property and this leads to a Nullreference. I will send the two issues in a single PR and another PR for settings.

[StefanKert]

First PR arrived #93.

I will take a look now at the thing you mentioned with IConfiguration. First draft was only to get up and running ;) now comes the cleanup :) !!! <3

[NickCraver]

Closing this out to cleanup, we've got all the base functionality it, future items should probably be their own issues :)

[StefanKert]

Agreed :) if you got further tasks to do I'd be glad to help out :)