elmahio / Elmah.Io.ElasticSearch

Elmah.Io.ElasticSearch is an Elasticsearch storage backend for ELMAH.
https://elmah.io
Apache License 2.0
23 stars 8 forks source link

Upgrade to Nest 1.0.1 #3

Closed jayhilden closed 10 years ago

jayhilden commented 10 years ago

Created for issue #2

I've made the following enhancements:

  1. Upgrade to Nest 1.0.1
  2. upgrade to json.net 6.0.4
  3. add ability to specify the default index within the connection string (legacy way of specifying within the elmah config still supported).
  4. new unit tests for new functionality
  5. let ES define the ID, it will be a bit less consistent but it will be considerably faster
jayhilden commented 10 years ago

@ThomasArdal and @Mpdreamz let me know what you think of my changes. If anything needs tweaking let me know. Also, I'd be happy to be a regular contributor to this project if you'd like any assistance.

ThomasArdal commented 10 years ago

Awesome! Will look at it tomorrow.

jayhilden commented 10 years ago

@ThomasArdal, I just noticed that there is an issue with the web UI, that has been corrected now as well.

jayhilden commented 10 years ago

I just added a new feature to this pull request: Now there is an Elmah MVC controller to go along with the Elmah.axd. This is very nice because you can then decorate the controller with the same [Authorize] attributes (and custom authorize attribute) that you would with all your other MVC sites.

ThomasArdal commented 10 years ago

I think ElmahController is redundant. That's what Elmah.MVC is for, right?

ThomasArdal commented 10 years ago

ElasticSearchErrorLogTest.CanGetErrors fails.

jayhilden commented 10 years ago

As to your comment about the MVC Controller being redundant for my company that was not true. Elmah.MVC provides for allowing/restricting users. We have a custom [Authorization] attribute setup that is privileged based rather than roles based. Ours looks like this:

[VBClaimsAuthorize(PermissionVerb.View, PermissionNoun.ErrorLog)]
public class ElmahVBController : VBControllerBase

I wanted to provide a sample for anyone else who might be looking to do the more fine grain permissions that we did.

ThomasArdal commented 10 years ago

Oh I see. Being an example project and all, having an ElmahController is fine.

jayhilden commented 10 years ago

@ThomasArdal I just fixed the broken unit test and I made it so that any port can be used, I no longer assume port 9200 (or any port for that matter). The update is much cleaner, using the Uri class instead of string manipulation.

The only outstanding issue is that the test CanGetError is still ignored. Can you elaborate on how I might be able to mock that.

ThomasArdal commented 10 years ago

@jayhilden Just change the implementation in the error log class to use Get(Func, GetDescriptor>) instead of Get(string). All that Get(string) does, is call Get(Func, GetDescriptor>) anyway.

I think it would looks something like this:

session.Get(x => x.ById(id))

Can't remember the exact syntax. Then you can mock the Get(Func, GetDescriptor>) call.

jayhilden commented 10 years ago

@ThomasArdal I the code so that it uses the func instead of the static method. I then updated the unit test to mock the func. I think we are good to go.

ThomasArdal commented 10 years ago

Thank you very much @jayhilden

jayhilden commented 10 years ago

happy to help. Are you going to re-package and put the updates on NuGet?

ThomasArdal commented 10 years ago

The project is build on teamcity.codebetter.com and a new package is automatically build, every time someone pushes to master. Your changes are already on NuGet:

http://www.nuget.org/packages/Elmah.ElasticSearch/

jayhilden commented 10 years ago

That's really cool @ThomasArdal, I'll have to take a look at how that works, I've never pushed anything to NuGet before. One thing I noticed on the website is that the dependencies still list the old version of NEST and Newtonsoft. Do those need to be updated manually in the .nuspec file?

ThomasArdal commented 10 years ago

Oh you're right. That's the nuspec files which needs an update. Do you mind doing another PR? :)

jayhilden commented 10 years ago

no problem, I'll create a new issue and see if I can do that this weekend