Enable 406 support with additional type

[sergeykolbasov]

• Refactor ToResponse to consume also status and headers instead of overriding it again in outputToResponse. It enables returning kind of final responses from ToResponse type class (there are still cookies).
• Add NotAcceptable406 uninhabited type
• In case if the last element of Content-Type negotiation Coproduct is NotAcceptable406, fail with 406 instead of using the last possible ToResponse on failed negotiation.

Essentially, it allows users to decide on type-level whether they want to enable or disable 406 errors:

Bootstrap.serve[Text.Plain :+: Text.Html :+: CNil] //would pick text/html encoding
Bootstrap.serve[Text.Plain :+: Text.Html :+: NotAcceptable406 :+: CNil] //would fail with 406

[codecov-io]

Codecov Report

Merging #1147 into master will increase coverage by 0.05%. The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
+ Coverage   80.51%   80.56%   +0.05%     
==========================================
  Files          54       54              
  Lines        1016     1019       +3     
  Branches       32       38       +6     
==========================================
+ Hits          818      821       +3     
  Misses        198      198

Impacted Files	Coverage Δ
core/src/main/scala/io/finch/Output.scala	94.73% <100%> (-0.27%)	:arrow_down:
...ore/src/main/scala/io/finch/internal/package.scala	100% <100%> (ø)	:arrow_up:
core/src/main/scala/io/finch/ToResponse.scala	96.66% <92.85%> (-3.34%)	:arrow_down:
core/src/main/scala/io/finch/Endpoint.scala	73.14% <0%> (+0.57%)	:arrow_up:

---

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 51be253...6456411. Read the comment docs.

[vkostyukov]

I'm curious if you considered structuring this as a boolean arugment on Bootstrap, similar to how we do the rest of these features.

Bootstrap.configure(enabledNotAcceptable = true)

[sergeykolbasov]

For that, I would need somehow drag this parameter down to Negotiable through:

• Compile
• Output.outputToResponse
• Negotiable.apply

So I found it more natural to use a type instead, as it's just a matter of new implicit instance of Negotiable.

[sergeykolbasov]

Also, users don't explicitly enable Content-Type negotiation with some boolean parameter but use Coproduct on CT type at the moment. That's another argument why 406 should be enabled through the type.

[joroKr21]

Just wanted to point out that the two options discussed so far also have different granularity. Actually while writing this, I also came up with a third option:

1. The type parameter is per-endpoint (more flexible, but has to be repeated)
2. The boolean option is per-server (less flexible, but no boilerplate)
3. Consider a serveOr406 (name is nor important) method on Bootstrap - this is similar to 1.

It looks like a trade-off that needs to be considered, i.e. is it more likely that users would want to configure this per-endpoint or once per-server?

[sergeykolbasov]

The idea about serveOr406 sounds interesting indeed

[rpless]

I really like this as a type param per-endpoint. re: more flexible, but has to be repeated I'd be curious to hear how common having multiple serves is. My suspicion is that its not all that often, but I could be wrong. If that's the case than I don't think repetition is a huge issue.

Having a serveOr406 could be useful too, but I'd be concerned that it deviates from the current pattern of using boolean flags on BootStrap.

[joroKr21]

I'd be curious to hear how common having multiple serves is.

That's an interesting question. I can only say how we use Finch at work currently and we definitely use multiple serves. We have servers with many APIs and each API has many endpoints. Each API is added to the server via serve[CT]. Usually one API serves the same set of content types from all endpoints, but that doesn't have to be true across APIs. And if an API serves only one content type we can often use coproduct instead of :+: to combine endpoints which simplifies the types a lot.

If that's the case than I don't think repetition is a huge issue.

I also think it's not a big issue.

Having a serveOr406 could be useful too, but I'd be concerned that it deviates from the current pattern of using boolean flags on BootStrap.

Yeah, maybe not such a great idea.

[vkostyukov]

The reason I think it's pretty reasonable to structure this as a Bootstrap.configre param is because there is already a functionality that's modeled very similarly.

Unsupported Media Type (enableUnsupportedMediaType) is a param on a Bootstrap and it's more or less controlled via a Coproduct passed into a decoder: body[Json :+: Text :+: CNil].

Although this is less flexible (can only be enabled/disabled on a per-server basis) it seems to fit nicely into our configuration story.

If people believe the ability to have this option on per endpoint basis (I'm somehow not yet convinced we need to worry about it yet) outweighs consistency, I'm +1 on moving this forward as is.

[sergeykolbasov]

So, what shall be the next step? Do we agree on the idea?

[vkostyukov]

Sorry, I'm not yet sold on the idea of having this per-endpoint. I think just doing this per server not only simplifies things but also allows us to use a more consistent approach for configuration.

Although if @rpless and @sergeykolbasov are +1 on this, I'm willing to change my mind.

[rpless]

allows us to use a more consistent approach for configuration. This is a good point. And thinking more on my suspicion that its not called many times then it doesn't really matter if its used once in the type parameter or once on Bootstrap.

Maybe we start out with it being configured per server to remain consistent and if it becomes enough of an issue we can re-evaluate? Although it does seem like a lot more effort to thread the bootstrap config through everything.