NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
863 stars 171 forks source link

Fix issue where AppName is overwritten by default #116

Closed zbecknell closed 6 years ago

zbecknell commented 6 years ago

I've encountered an issue where my in-code configured ApplicationName is overwritten. I configure exceptional like so in a class library:

Exceptional.Configure(settings =>
{
    settings.DefaultStore = new SQLErrorStore(new ErrorStoreSettings()
    {
        ApplicationName = applicationName,
        ConnectionString = connectionString
    });
});

In this case, a new Error, when initialized, was not taking the DefaultStore's ApplicationName property. This fixes my issue by checking DefaultStore first before falling back.

Edit: I should mention it was defaulting to "My Application".

zbecknell commented 6 years ago

Whoops, checking on that failing test now.

zbecknell commented 6 years ago

Removing the default AppName of "My Application" allowed my changes to work with all tests passing. If there was another way to accomplish my original in-code configuration that I'm missing, let me know.

NickCraver commented 6 years ago

I'm trying to think through this a bit, doesn't it bake in the assumption that there's only one store, given the ordering?

I agree there's a bug for sure, we're hitting it at Stack and I haven't had time to dig, I'm just not sure about the fix path.

NickCraver commented 6 years ago

I thought through this a bit and realized: it also breaks the "they didn't specify one" case...it's just empty. I think we'll need to solve this a different way.

NickCraver commented 6 years ago

I'm going to close this out as the issue should be resolved in 905ebf97bd57dfafc3dcf1e08e668faee7243214. It's a different/bigger solution as the problem was bit deeper but the packages going to MyGet now will have the fix.

Cheers for filing this and helping with some context here, it's a fix we needed at Stack as well :)

zbecknell commented 6 years ago

Thanks Nick, glad to see there's a deeper solution in place!