search

bitemyapp / bloodhound

Haskell Elasticsearch client and query DSL
bitemyapp.com
BSD 3-Clause "New" or "Revised" License
424 stars 118 forks source link

Add Request hook #84

Closed MichaelXavier closed 8 years ago

MichaelXavier commented 9 years ago

This is pretty much exactly as we discussed but with one difference: rather than having the request hook be in MonadBH m, its in IO. While dogfooding my own solution, I tried making the hook forall m. MonadBH m => Request -> m Request and it gave me some really nasty type inference issues. I didn't fret too much about switching to IO because there are two use cases for these hooks that I'm aware of:

  1. HTTP Basic auth for ElasticSearch Shield.
  2. Amazon request signing.

Both can happen in IO, and HTTP Basic doesn't even need that. If a user has to do something fancier, they'll likely be operating in their own ReaderT-based monad and can just ask the state and run their hook in IO.

I've documented things but I don't really see any way to test this because the standard distribution of ES won't expect or care about auth headers so we wouldn't be able to test it without adding a build matrix dimension that sets up shield and user roles. The tests do show that the identity hook doesn't break anything and I have code running in production using bloodhound-amazonka-auth and its working great.

bitemyapp commented 9 years ago

@MichaelXavier It's looking good to me. How did the upgrade to Aeson 0.10 treat you?

Let me know when you want me to merge.

MichaelXavier commented 9 years ago

Aeson wasn't too bad. The change to how .:? works was kind of insidious since there was no type change. As with most upgrades, most of the work was in bumping upper bounds.

Feel free to merge whenever. I inline fixed the comment bug you noted.

MichaelXavier commented 8 years ago

So the test failure was due to quickcheck every once in a long while producing a many-MBs large JSON test and failing. I have size reduction measures in place to mitigate these issues but the trouble is that travis truncates the logs so I can't get the seed and i've yet to repro it locally, even running 100,000 tests per json instance test. Not sure what I can do to troubleshoot this until I can get it to show up locally.

I suspect one thing that would help is better shrinking. I'm using the derive library to implement automated Arbitrary instances and I bet its using the default shrink (which is to say not shrinking at all). Arbitrary supports a Generic-based shrink and may soon support a Generic arbitrary, which means we may be able to keep the boilerplate minimal but actually get a competent shrink out of it as well.

MichaelXavier commented 8 years ago

@bitemyapp anything else needed on this?

bitemyapp commented 8 years ago

@MichaelXavier no, thank you for the PR and apologies for the delay. Gnarly (and still late) book release coming up. Hopefully this'll help anyone else using AWS :smile: