smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

adding option to accept media-type

Open MoeQuadrat opened this issue 3 years ago • 18 comments

I am currently facing a problem where I need the correct content type in the generated open api file for the code generation to work correctly for another team. My understanding is that it is currently not possible to set it in smithy-files. So is there any way to accept the media type from smithy-files as content type for the generated open api files?

MoeQuadrat avatar Jul 22 '22 12:07 MoeQuadrat

Can you elaborate on what content-type you're talking about, that the code generator for that other team expects ? In particular, is it a "json" content-type, because the simpleRestJson protocol provided by smithy4s does not support anything but json.

Baccata avatar Jul 22 '22 14:07 Baccata

Furthermore, I believe the Open API generated by smithy4s contains a media-type at the content level. Take this response for example. Here the media-type is application/json:

"responses": {
                    "200": {
                        "description": "Hello 200 response",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/HelloResponseContent"
                                }
                            }
                        }
                    }
                }

daddykotex avatar Jul 22 '22 14:07 daddykotex

Furthermore, I believe the Open API generated by smithy4s contains a media-type at the content level. Take this response for example. Here the media-type is application/json:

"responses": {
                    "200": {
                        "description": "Hello 200 response",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/HelloResponseContent"
                                }
                            }
                        }
                    }
                }

That's exactly what content type I'm talking about. In my case, i'd need the content type to be editable for example by the media-type annotation in smithy files

MoeQuadrat avatar Jul 22 '22 15:07 MoeQuadrat

So I looked it up a little bit: the mediaType annotation in smithy is not intended the way you think it is. My understanding is that it's there signal the media type of an opaque piece of data: blob or string:

/// Describes the contents of a blob shape using a media type as defined by
/// RFC 6838 (e.g., "video/quicktime").
@trait(
    selector: ":is(blob, string)",
    breakingChanges: [{change: "remove"}]
)
string mediaType

In contrast, I would expect something like httpMediaType if available, to do something like you expect. Unfortunately, it does not exist.

But, it seems like the converter we use has an option to globally redefine the mediaType: https://awslabs.github.io/smithy/javadoc/1.22.0/software/amazon/smithy/openapi/OpenApiConfig.html#getJsonContentType()

There may be a way to expose some of these options via the CLI and/or build tool integration, but the changes would be applied to the whole OpenAPI document, not per operation.

Does that make any sense and do you think that's a practical solution @Baccata ?

daddykotex avatar Jul 22 '22 17:07 daddykotex

For example, I'd like to be able to mark a blob as an image/png in a smithy file, and this content type should be displayed in the generated open api.

MoeQuadrat avatar Jul 22 '22 18:07 MoeQuadrat

@LiiMooTBM this is not supported by the simpleRestJson protocol provided by smithy4s, is the only protocol currently supported by smithy4s, and is a protocol that literally only understand json payloads. Blob payloads in this protocol are encoded as base64 Json strings in this protocol, which is inconsistent with what a media type like image/png means.

Smithy => Openapi conversion is protocol specific, it depends on what protocol annotation you have on your service, and what the protocol understands. For instance, AWS has its own RestJson protocol, which has an understanding of the mediaType trait.

When a service is annotated with the aws.protocols#restJson1 protocol, it's likely that the @mediaType trait will be reflected as you'd expect in the corresponding openapi file. BUT the aws.protocols#restJson1 is not supported by smithy4s, which means that smithy4s is not (yet) able to generate route for that protocol. As a matter of fact, we have an open issue for it, but it's far from trivial, and definitely not our top priority (though we'd gladly accept contributions from motivated people who'd like to move towards that).

So I'm sorry to say that you're asking here is not supported by smithy4s at this point in time, and the fact that it's not supported is not a bug, it's just incompatible with the simpleRestJson protocol semantics.

Baccata avatar Jul 22 '22 22:07 Baccata

@Baccata thank you for providing such valuable information! Will the codegen and and open api part of the project still work with the aws.protocols#restJson1 annotation?

MoeQuadrat avatar Jul 23 '22 00:07 MoeQuadrat

the aws.protocols#restJson1 is not supported by smithy4s

We're sorry but not yet.

daddykotex avatar Jul 23 '22 00:07 daddykotex

@Baccata thank you for providing such valuable information! Will the codegen and and open api part of the project still work with the aws.protocols#restJson1 annotation?

@daddykotex you're not quite right.

@LiiMooTBM : the codegeneration is not tied to any protocol, so that will work. I think the open-api code-generation will work too, because we're piggybacking on a plugin mechanism provided by the smithy library from AWS, but I'm not 100% sure. I'm happy to look into if that doesn't work.

To re-iterate, if the openapi-generation works with the restJson1 protocol, the openapi you'd get will not be an accurate representation of what you'd get from using the SimpleRestJson builders, so you'll have to handcraft the routing logic.

Baccata avatar Jul 23 '22 07:07 Baccata

But to be clear: the SimpleRestJson works with service annotated with aws.protocols#restJson1 and not just smithy4s.api#simpleRestJson?

TIL

I understand that codegen will work for the rest (case class for structure, ADT for union, some newtypes here and there) but I was convinced the http4s server thingy was build on top of smithy4s.api#simpleRestJson.

Apologies for that @LiiMooTBM

daddykotex avatar Jul 23 '22 15:07 daddykotex

@daddykotex, you are correct in that the SimpleRestJson will only work if the service is annotated with smithy4s.api#simpleRestJson, and the router you get out of it only respects the semantics of the simpleRestJson protocol. That is not however the question that was asked.

The question that was asked was :

Will the codegen and and open api part of the project still work with the aws.protocols#restJson1 annotation?

The SimpleRestJson router builder is not directly related to code-generation. Therefore, the fact that it will not work is irrelevant to the answer of the question.

Baccata avatar Jul 24 '22 08:07 Baccata

Thanks for your answers once again. I will give a shot tomorrow. tyvm @Baccata @daddykotex !

dont worry about it @daddykotex :)

MoeQuadrat avatar Jul 24 '22 11:07 MoeQuadrat

So, I tried to use the

aws.protocols#restJson1

but unfortunately the codegen doesn't work. I'm not sure if I'm missing the aws package for the protocols or something similar because I'm getting a compile error for the service hints right here:

val hints : smithy4s.Hints = smithy4s.Hints(
    aws.protocols.RestJson1(None, None),
  )

but the openapi is generated like you expected it to be @Baccata .

MoeQuadrat avatar Jul 25 '22 10:07 MoeQuadrat

@LiiMooTBM you can pull in the smithy4s-aws-kernel dependency, which contains the generated code for a few aws-namespaces.

Baccata avatar Jul 25 '22 12:07 Baccata

@LiiMooTBM you can pull in the smithy4s-aws-kernel dependency, which contains the generated code for a few aws-namespaces.

Tyvm that worked! Just for my information is there an easy way to implement a custom protocol? With the restJson1 im not able to use map/set directly as @httpPayload so i'd have to wrap that up in a structure again. And that changes our output structure aswell.

MoeQuadrat avatar Jul 27 '22 07:07 MoeQuadrat

Just for my information is there an easy way to implement a custom protocol?

Depends on the semantics of the protocol, and on your ability to reason in abstract.

We've made a builder you might be able to reuse if the semantics of your protocol roughly align with ours.

The smithy would look like this, modulo the list of traits you'd like your protocol to support

In your case, the Scala code would probably look like this :

import smithy4s.http4s.SimpleProtocolBuilder
import smithy4s.http.CodecAPI
import smithy4s.HintMask
import smithy4s.http.json.{codecs => jsonCodecs}

object MyProtocolBuilder
    extends SimpleProtocolBuilder[foo.MyProtocol](
      CodecAPI.nativeStringsAndBlob( // this actually has an understanding of the `@mediaType` annotation
        smithy4s.http.json
          .codecs(
             foo.MyProtocol.protocol.hintMask ++ jsonCodecs.defaultHintMask
          )
      )
    )

The CodecAPI is what is used to customise the semantics of the body. It implies that the body is fully loaded in memory and doesn't work for protocol that support the streaming of arbitrary large payloads.

Then, the most complex part will be to implement the smithy => openapi conversion. Thankfully AWS provides functions that can be used to do the heavy lifting, but you'll still have to tweak / experiment, etc. See how we did it

To be clear : at this point in time we are not making any guarantees with regards to source/binary compat across minor versions, so if you do end up using the SimpleProtocolBuilder thing, it'll be at your own risk, as we reserve the right to break it.

Baccata avatar Jul 27 '22 08:07 Baccata

Okay, I think I've looked enough into it get a basic understanding. My understanding is that it's pretty hard to extract the @mediatype when using the Smithy4sAbstractRestProtocol class. So is it possible to add a context attribute to the "getDocumentMediaType()" method? This can be used to extract the media type from the context, if I'm not mistaken. This way, when you create your own Protocol, it is possible to use the Smithy4sAbstractRestProtocol class as a base and have access to the Context. Do you have anything against this idea or do you have a better suggestion? @Baccata

MoeQuadrat avatar Aug 02 '22 12:08 MoeQuadrat

@LiiMooTBM, so, our OpenAPI conversion was adapted from the constructs provided by directly by AWS. See here. I think it'd be best to adapt directly from there. I cannot remember exactly what drove us to copy/paste so much of the logic, and we haven't back-ported the possible changes they've made. It may have been that the interface we needed to plug into was private at the time.

In particular : see the fact that they're already exposing this method which is probably what you're looking for.

Additionally, the things to take into consideration in smithy4s is that :

  1. Smithy4s' SimpleRestJson adds support for two union-encodings that Smithy doesn't know about.
  2. Smithy4s default json codecs use "DATE_TIME" as the default timestamp format in Json bodies, whereas AWS deems that EPOCH_SECOND should be the default format in Json bodies. Honestly, I think it'd be best if all timestamp shapes were accompanied with a @timestampFormat annotation which wouldn't require an arbitrary default, but 🤷

Baccata avatar Aug 03 '22 07:08 Baccata