Interceptors content negotiation #1078

kubinio123 · 2021-03-15T08:32:04Z

No description provided.

refactor EndpointToOperationResponse - join all outputs first instead of merging Response later

# Conflicts: # tests/src/main/scala/sttp/tapir/tests/package.scala

…stment) added charset comparing while decoding outputs updated validation of endpoint outputs by comparing charsets

# Conflicts: # core/src/main/scala/sttp/tapir/server/interpreter/ServerInterpreter.scala

adamw · 2021-03-16T08:15:13Z

apispec/openapi-model/src/main/scala/sttp/tapir/openapi/OpenAPI.scala

-    example: Option[ExampleValue],
-    examples: ListMap[String, ReferenceOr[Example]],
-    content: ListMap[String, MediaType]
+    description: Option[String] = None,


do we have default arguments in other places as well? would be keep to stay consistent in the model

no, adjusted

adamw · 2021-03-16T09:54:43Z

client/play-client/src/main/scala/sttp/tapir/client/play/EndpointToPlayClient.scala

+                case Left(_) =>
+                  DecodeResult.Error(statusText, new IllegalArgumentException("Unable to parse Content-Type header"))
+              }
+              .getOrElse(DecodeResult.Error(statusText, new IllegalStateException("Missing Content-Type header")))


what if one of the response variants has no body (e.g. a redirect or some other non-200 status code)?

right, it has to be handled, adjusted

adamw · 2021-03-16T16:01:03Z

core/src/main/scala/sttp/tapir/internal/package.scala

+
+    def matchesContent(content: MediaType): Boolean = {
+      val contentWithCharset = content match {
+        case m @ MediaType(_, _, None) => m.charset(StandardCharsets.UTF_8.name()) // default UTF-8


I don't think we should add UTF-8 as the default. Quite the contrary, most media types except text/* don't support charsets

right, only text has a default one

adamw · 2021-03-16T16:37:18Z

core/src/main/scala/sttp/tapir/server/interpreter/ContentNegotiation.scala

+private[interpreter] class MediaTypeNegotiator(headers: Seq[Header]) {
+
+  private val acceptedMediaTypes: Seq[(MediaType, Float)] =
+    ContentNegotiation.extract(HeaderNames.Accept, headers)(MediaType.unsafeParse).sortBy {


This is a good start, but I think the more natural place for this would be in sttp-model. We already have there a couple of classes which parse specific header values (cache directives, etags). Similarly, we could have a parser for the Accept header value, which would include the q values & the sorting logic.

We could also add some methods to MediaType to check if the main/sub types is a wildcard * to avoid these constants here

I opened a PR in sttp-model softwaremill/sttp-model#76

adamw · 2021-03-16T16:38:19Z

core/src/main/scala/sttp/tapir/server/interpreter/ContentNegotiation.scala

+
+  def indexOf(mediaType: MediaType): Int = acceptedMediaTypes.indexWhere { case (mt, _) => mt.noCharset == mediaType.noCharset }
+
+  private def matches(a: MediaType, b: MediaType): Boolean =


this could also be a method on MediaType, rather than here

adamw · 2021-03-16T16:40:47Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

      case EndpointOutput.MappedPair(wrapped, _) => apply(wrapped, ParamsAsAny(encoded[Any]), ov)
    }
  }

-  private def charset[R](mediaType: MediaType, bodyType: RawBodyType[R]): Option[Charset] = bodyType match {
+  private def charset[R](bodyType: RawBodyType[R]): Option[Charset] = bodyType match {
    // TODO: add to MediaType - setting optional charset if text


I guess the TODO doesn't make sense anymore

…nd with no body output in mappings

adamw · 2021-03-25T09:38:38Z

client/sttp-client/src/main/scala/sttp/tapir/client/sttp/EndpointToSttpClient.scala

-                  meta.statusText,
-                  new IllegalArgumentException(s"Cannot find mapping for status code ${meta.code} in outputs $output")
-                )
+                mappingsForStatus.headOption


shouldn't we give priority to mappings without a body in such case? e.g. for a mapping

oneOf[Either[Unit, X]]( (Ok, jsonBody[X].map(Right(_)), (Ok, emptyOutput.map(Left(_)) )

in case we get back an empty response, the second should be picked

adamw · 2021-03-25T09:45:17Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

+        val bodyMappings: Map[MediaType, StatusMapping[_]] = mappings
+          .filter(_.appliesTo(enc))
+          .collect({
+            case sm @ StatusMapping(_, EndpointIO.Body(bodyType, codec, _), _) =>


the body outputs might be nested, e.g. .output(jsonBody[X].map(...)) or .output(jsonBody[X].and(header(...))), so such a simple pattern match won't work in this case. I think you'll have to traverse the outputs to find the body

yes, I keep forgetting about this nest here, adjusted

adamw · 2021-03-25T09:46:15Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

+
+        if (bodyMappings.nonEmpty) {
+          val ranges = Accepts.unsafeParse(requestHeaders)
+          val mediaTypes = bodyMappings.keys.toSeq.asInstanceOf[scala.collection.immutable.Seq[MediaType]]


why is the cast needed?

adamw · 2021-03-25T09:56:49Z

Will need a rebase/merge unfortunately

adamw · 2021-03-25T09:57:11Z

Looks good, I think there are only the two issues remaining which I wrote about above :)

# Conflicts: # docs/openapi-docs/src/test/scala/sttp/tapir/docs/openapi/VerifyYamlTest.scala # project/Versions.scala

adamw · 2021-03-25T15:53:41Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

+                Vector[(MediaType, StatusMapping[_])](
+                  charset.map(ch => codec.format.mediaType.charset(ch.name())).getOrElse(codec.format.mediaType) -> sm
+                )
+              case EndpointIO.Empty(codec, _) => Vector[(MediaType, StatusMapping[_])](codec.format.mediaType -> sm)


I'm not sure this will work as expected. The media type for an empty output will always be text/plain (by definition). An empty output should match the no-media-type, so we'd get a Map[Option[MediaType], StatusMapping[_]]

And we can't really depend on Empty here. The output might contain headers, but not be empty.

adamw · 2021-03-27T10:14:47Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

+            .bestMatch(mediaTypes, ranges)
+            .flatMap(bodyMappings.get)
+            .map(sm => apply(sm.output, ParamsAsAny(enc), sm.statusCode.map(ov.withStatusCode).getOrElse(ov)))
+            .getOrElse(ov.withStatusCode(StatusCode.NotAcceptable))


I'm not sure if we can do this here. When we get to EncodeOutputs, the server logic has already run - can we return a 4xx response at this point? Shouldn't we either:

perform the validation upfront, when decoding the requests - although we don't know what's the status code going to be, so we can only say if all or none match

when no mapping is found, returning any response instead of "not acceptable"

Would be good to see how other libraries/frameworks behave in this case

adamw · 2021-03-27T10:16:42Z

core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala

        ov.withBody(headers => rawToResponseBody.fromRawValue(encoded[Any], headers, codec.format, rawBodyType))
-          .withDefaultContentType(codec.format, charset(codec.format.mediaType, rawBodyType))
+          .withDefaultContentType(codec.format, maybeCharset)


here the question is the same - should we check if whatever is in the Accept of the request matches what we want to return? Or just return regardless? Either way, behavior should be consistent with that of oneOf

# Conflicts: # core/src/main/scala/sttp/tapir/internal/package.scala # core/src/main/scala/sttp/tapir/server/interpreter/EncodeOutputs.scala # core/src/test/scala/sttp/tapir/EndpointTest.scala # docs/openapi-docs/src/main/scala/sttp/tapir/docs/openapi/EndpointToOperationResponse.scala # docs/openapi-docs/src/main/scala/sttp/tapir/docs/openapi/ExampleConverter.scala

adamw · 2021-03-29T14:16:25Z

client/play-client/src/main/scala/sttp/tapir/client/play/EndpointToPlayClient.scala

-                )
+                val firstNonBodyMapping = mappingsForStatus.find(_.output.traverseOutputs {
+                  case _ @(EndpointIO.Body(_, _, _) | EndpointIO.StreamBodyWrapper(_)) => Vector(false)
+                  case _                                                               => Vector(true)


this won't recurse - see the traverse outputs impl. I think matching on _: EndpointOuput.Basic should do the trick here

Or better:

_.output.traverseOutputs { case Body => Vector(()) }.isEmpty

adamw · 2021-03-29T14:29:27Z

core/src/main/scala/sttp/tapir/model/ServerRequest.scala


 trait ServerRequest extends RequestMetadata {
+  lazy val ranges: Either[String, immutable.Seq[ContentTypeRange]] = Accepts.parse(headers)


I'd like to keep this class as simple as possible, without helper methods

(I know there are some from HasHeaders, but that's shared among sttp client/tapir :) )

# Conflicts: # server/tests/src/main/scala/sttp/tapir/server/tests/ServerBasicTests.scala

kubinio123 added 19 commits March 8, 2021 09:00

validation of status code mappings

d4d00e6

test for open api multiple media types to single status code

334ac9e

test case mapping one status to multiple schemas in multiple formats

69a711a

refactor EndpointToOperationResponse - join all outputs first instead of merging Response later

select output based on content-type in client interpreters

e45f6d4

xml module added

92aee61

test fixing

f4e3755

refactor

bdfd456

filter.find -> collectFirst

9cbdd63

improvement

b905b1d

initial ContentNegotiation impl

4ceacad

refactor

6d202f4

removed validate from mapOut methods

2392a87

content negotiation introduced in logic + tests

3893a79

Merge branch 'feature/248' into interceptors-content-negotiation

5eba867

# Conflicts: # tests/src/main/scala/sttp/tapir/tests/package.scala

merged changes from feature/248 (client side content negotiation adju…

f739155

…stment) added charset comparing while decoding outputs updated validation of endpoint outputs by comparing charsets

Merge branch 'interceptors' into interceptors-content-negotiation

ff07b9f

# Conflicts: # core/src/main/scala/sttp/tapir/server/interpreter/ServerInterpreter.scala

described extract method

e6aed63

charset extraction from StreamBody before picking best content

bbec3a3

test case added, static imports

a7b545a

kubinio123 marked this pull request as ready for review March 15, 2021 09:53

kubinio123 requested a review from adamw March 15, 2021 09:53

adamw reviewed Mar 16, 2021

View reviewed changes

kubinio123 added 3 commits March 18, 2021 10:38

open api model adjustement

b7750ca

decoding response adjustment, handle case with no content specified a…

c73b232

…nd with no body output in mappings

default charset only for text type

615f4c1

kubinio123 added 2 commits March 24, 2021 12:44

compilation fix

2f05eba

handled case when there's no body mappings

d8aaa3d

kubinio123 requested a review from adamw March 25, 2021 08:55

adamw reviewed Mar 25, 2021

View reviewed changes

kubinio123 added 3 commits March 25, 2021 14:15

traverse outputs, handle no content case with body and non body mappings

0d89ce3

Merge branch 'master' into interceptors-content-negotiation

44befae

# Conflicts: # docs/openapi-docs/src/test/scala/sttp/tapir/docs/openapi/VerifyYamlTest.scala # project/Versions.scala

compilation fix + merge

28396e5

adamw reviewed Mar 25, 2021

View reviewed changes

adamw reviewed Mar 27, 2021

View reviewed changes

kubinio123 marked this pull request as draft March 29, 2021 07:42

kubinio123 self-assigned this Mar 29, 2021

kubinio123 added 2 commits March 29, 2021 15:01

ContentTypeInterceptor added

cf19df4

kubinio123 requested a review from adamw March 29, 2021 13:48

adamw reviewed Mar 29, 2021

View reviewed changes

kubinio123 added 3 commits March 30, 2021 11:23

decoding client result unified

a6afdc1

Merge branch 'master' into interceptors-content-negotiation

cc244b2

# Conflicts: # server/tests/src/main/scala/sttp/tapir/server/tests/ServerBasicTests.scala

removed commented code

987ddde

kubinio123 requested a review from adamw March 30, 2021 10:19

adamw added 2 commits March 30, 2021 15:50

Bring back ranges to ServerRequest

cd725aa

Simplify

8c531b7

adamw marked this pull request as ready for review March 30, 2021 14:09

adamw merged commit 0e091f5 into master Mar 30, 2021

mergify bot deleted the interceptors-content-negotiation branch March 30, 2021 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interceptors content negotiation #1078

Interceptors content negotiation #1078

kubinio123 commented Mar 15, 2021

adamw Mar 16, 2021

kubinio123 Mar 18, 2021

adamw Mar 16, 2021

kubinio123 Mar 18, 2021

adamw Mar 16, 2021

kubinio123 Mar 18, 2021

adamw Mar 16, 2021

kubinio123 Mar 17, 2021

adamw Mar 16, 2021

adamw Mar 16, 2021

adamw Mar 25, 2021

adamw Mar 25, 2021

kubinio123 Mar 25, 2021

adamw Mar 25, 2021

adamw Mar 25, 2021

adamw commented Mar 25, 2021

adamw commented Mar 25, 2021

adamw Mar 25, 2021

adamw Mar 25, 2021

adamw Mar 27, 2021

adamw Mar 27, 2021

adamw Mar 29, 2021

adamw Mar 29, 2021

adamw Mar 29, 2021

adamw Mar 29, 2021


		def indexOf(mediaType: MediaType): Int = acceptedMediaTypes.indexWhere { case (mt, _) => mt.noCharset == mediaType.noCharset }

		private def matches(a: MediaType, b: MediaType): Boolean =


		trait ServerRequest extends RequestMetadata {
		lazy val ranges: Either[String, immutable.Seq[ContentTypeRange]] = Accepts.parse(headers)

Interceptors content negotiation #1078

Interceptors content negotiation #1078

Conversation

kubinio123 commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Mar 25, 2021

adamw commented Mar 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment