Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support MethodNotAllowed via new EndpointResult #927

Merged
merged 1 commit into from Mar 18, 2018

Conversation

vkostyukov
Copy link
Collaborator

@vkostyukov vkostyukov commented Mar 14, 2018

This is essentially an iteration on #883 that instead of capturing a matched method as part of Input, introduces a new EndpointResult case. I like this approach better because:

  • It's compliant with https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html (10.4.6)
  • It's more efficient (Input remains skinner)
  • It keeps a reasonable semantic for Endpoint.apply(Input) for endpoint-mappers so REPL/test environments will return "not-matched" when HTTP verb is different.

Example:

scala> val a = get("foo") { Ok("foo") }
a: io.finch.Endpoint[String] = GET /foo

scala> val b = post("foo") { Ok("post foo") }
b: io.finch.Endpoint[String] = POST /foo

scala> val c = put("foo") { Ok("put foo") }
c: io.finch.Endpoint[String] = PUT /foo

scala> val s = Bootstrap.configure(enabledMethodNotAllowed = true).serve[Text.Plain](a :+: b).serve[Text.Plain](c).toService
s: com.twitter.finagle.Service[com.twitter.finagle.http.Request,com.twitter.finagle.http.Response] = <function1>

scala> Http.server.serve(":8081", s)

And then:

[tw-mbp-vkostyukov finch (vk/match-wo-method)]$ http :8081/bar
HTTP/1.1 404 Not Found
Content-Length: 0
Date: Fri, 16 Mar 2018 20:59:30 GMT
Server: Finch



[tw-mbp-vkostyukov finch (vk/match-wo-method)]$ http POST :8081/foo
HTTP/1.1 200 OK
Content-Type: text/plain
Date: Fri, 16 Mar 2018 20:59:37 GMT
Server: Finch
content-encoding: gzip
content-length: 34

post foo

[tw-mbp-vkostyukov finch (vk/match-wo-method)]$ http PUT :8081/foo
HTTP/1.1 200 OK
Content-Type: text/plain
Date: Fri, 16 Mar 2018 20:59:48 GMT
Server: Finch
content-encoding: gzip
content-length: 33

put foo

[tw-mbp-vkostyukov finch (vk/match-wo-method)]$ http DELETE :8081/foo
HTTP/1.1 405 Method Not Allowed
Allow: PUT,GET,POST
Content-Length: 0
Date: Fri, 16 Mar 2018 20:59:56 GMT
Server: Finch

def isMatched: Boolean = true
}

case object NotMatched extends EndpointResult[Nothing] {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to look into this to see if it could be modeled better so we can avoid asInstanceOf casts in Endpoint.

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #927 into master will increase coverage by 0.15%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
+ Coverage   82.65%   82.81%   +0.15%     
==========================================
  Files          51       51              
  Lines         767      768       +1     
  Branches       36       39       +3     
==========================================
+ Hits          634      636       +2     
+ Misses        133      132       -1
Impacted Files Coverage Δ
core/src/main/scala/io/finch/endpoint/param.scala 78.12% <0%> (ø) ⬆️
core/src/main/scala/io/finch/endpoint/header.scala 62.5% <0%> (ø) ⬆️
...e/src/main/scala/io/finch/endpoint/multipart.scala 72.22% <100%> (ø) ⬆️
core/src/main/scala/io/finch/ToService.scala 100% <100%> (ø) ⬆️
core/src/main/scala/io/finch/EndpointResult.scala 81.81% <100%> (+9.09%) ⬆️
core/src/main/scala/io/finch/Bootstrap.scala 90% <100%> (+6.66%) ⬆️
...rc/main/scala/io/finch/syntax/EndpointMapper.scala 100% <100%> (ø) ⬆️
...tee/src/main/scala/io/finch/iteratee/package.scala 95.23% <100%> (-0.22%) ⬇️
core/src/main/scala/io/finch/endpoint/path.scala 95% <100%> (ø) ⬆️
core/src/main/scala/io/finch/Endpoint.scala 75.78% <100%> (-1.22%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4661b23...956eaf8. Read the comment docs.

@vkostyukov vkostyukov changed the title Support MethodNotAllowed via new EndpointResult [WIP] Support MethodNotAllowed via new EndpointResult Mar 15, 2018
@vkostyukov vkostyukov force-pushed the vk/match-wo-method branch 2 times, most recently from 10d8738 to d5b849b Compare March 15, 2018 22:43
@vkostyukov
Copy link
Collaborator Author

@sergeykolbasov Do you mind taking a look?

@vkostyukov vkostyukov force-pushed the vk/match-wo-method branch 2 times, most recently from efa8aac to 5f95c28 Compare March 16, 2018 17:05
@vkostyukov
Copy link
Collaborator Author

See 10.4.6 here https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html.

Looks like we also need to add Allow header. This means changing the implementation a bit.

@vkostyukov vkostyukov changed the title Support MethodNotAllowed via new EndpointResult Support MethodNotAllowed via new EndpointResult [WIP] Mar 16, 2018
@vkostyukov vkostyukov changed the title Support MethodNotAllowed via new EndpointResult [WIP] Support MethodNotAllowed via new EndpointResult Mar 17, 2018
@sergeykolbasov
Copy link
Collaborator

Sounds like a proper solution to me 👍

@vkostyukov vkostyukov merged commit bbc23f4 into master Mar 18, 2018
@vkostyukov vkostyukov deleted the vk/match-wo-method branch March 18, 2018 20:50
@vkostyukov vkostyukov mentioned this pull request Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants