search

trailblazer / trailblazer-endpoint

Generic HTTP endpoints to deal with different results from your operations.
22 stars 19 forks source link

Fix for matchers NonExhaustiveMatchError error and new failure matcher #6

Open andriimosin opened 7 years ago

andriimosin commented 7 years ago

I tried to fix few issues I faced with.

First of all, I added extra check for result.policy.default in unauthenticated matcher, cause it failed in cases when there was no policy at all. Actually, I'm not sure about it, because when you are not unauthenticated, you will not get policy to be initialized. Looks like we need more tests here, but since everything failed because of that, this solution is at least something.

I added failure matcher, because it is possible to have operation without any contracts or policies. In this case, the invalid matcher will not match, because it's require contract to exist. If you have different opinion about it, I'd like to discuss it, but in my case it helped me.

The most interesting part is NonExhaustiveMatchError. Currently, it will raise the error if don't have ALL handlers in your controller. I'm pretty sure that this solution is not final, but it will help to move forward.

All specs except one are passing. The generic handler because user handler doesn't match. test is currently failed, because in current realisation matcher will be called with user block if it was pass. But this test expecting generic handlers to be fired if user's handler doesn't match. I think it would be nice to find a solution how return to generic handlers in future, but looks like this case never worked.

I made a lot of changes here only because build was completely broken, so I was in need to update dependencies and change test_helper little bit.

I would like to discuss the future of this gem and all this changes here 😉

P.S. looks like there are fixes for this 3 issues in this PR: https://github.com/trailblazer/trailblazer-endpoint/issues/4, https://github.com/trailblazer/trailblazer-endpoint/issues/3, https://github.com/trailblazer/trailblazer-endpoint/issues/2

apotonick commented 7 years ago

@andriymosin !!! :heart_eyes: :beers:

I will go through your changes, but regarding the future, I am considering removing the "pattern matching" with a simple switch statement, I was going through the code with Jose Valim and he inspired this change!!! :joy:

andriimosin commented 7 years ago

Hey bro!!! Nice to hear you, at least in such form of conversation 🤣🤣🤣 Thank you, I'd like to participate in the development, cause I'm feeling really exciting about all these TB features. I have few ideas about how to make it without the pattern matching, will try to implement it in the next couple of days. Cheers!!! 🍻🍻🍻

apotonick commented 7 years ago

That would be amazing. The idea here is very simple (as you might have noticed already): The Result object is the only way to communicate with the outer world, or the Endpoint. The endpoint knows certain states on the result object and from there interprets what action to take (e.g. policy error will lead to a 401 with a potential custom error message in the JSON, etc. etc.).

The "knowing" of what has happened must be encoded in some kind of configurable/extendable form. ATM, I use pattern matching because I wanted to play with it, be we can simply use if here and have some hash that contains condition/action tuples in a certain order. This is where you could come and do your magic.

BTW, in TRB 2.1, you can even have different ends and can infer state/outcome from there. I will give you an update once it's presentable.

Thanks and cheers brother :sparkler:

zoggxxx commented 7 years ago

@andriymosin Thanks for doing this! I've actually been playing around with Endpoints all day, along with pattern matching. From what I had been reading, I was aware that this was sort of on the backburner (and not due to lack of importance), as I had been running into similar issues that you had experienced. In particular:

"The most interesting part is NonExhaustiveMatchError. Currently, it will raise the error if don't have ALL handlers in your controller. I'm pretty sure that this solution is not final, but it will help to move forward."

I noticed that too. Anyway, I can certainly get by for now...I just had my heart set on one line per controller action, not two...I guess I'll just have to live with it for now. I've learned that every time I build some utility to slightly improve pieces of my app, TRB will come up with a better solution in another week or two. I can wait (hint hint, Nick ;)).

andriimosin commented 7 years ago

@zoggxxx you right, NonExhaustiveMatchError is a noisy piece and I'm working on removing the whole 'pattern matching' at all, as @apotonick suggested before. I hope to present something soon 😉

zoggxxx commented 7 years ago

Awesome, thanks again :)

rpbaltazar commented 6 years ago

This has been a bit stalled but I discussed with @apotonick earlier this year about it. I like the idea and the more it is in production the more repetitive code we have. I finally took some time and decided to refactor the endpoint. I have not yet merged it in my endpoint repo as it was easier for me test different use cases with a real application.

https://gist.github.com/rpbaltazar/806773e9c0a12e9fbd30e5c9d27a5243

Please guys, take a look and see if this is a usable approach.