disneystreaming / alloy

Other
35 stars 9 forks source link

`JsonName` ignored in examples when generating swagger docs #148

Open ShahOdin opened 4 months ago

ShahOdin commented 4 months ago

If a structure member has a custom @JsonName:

$version: "2.0"
namespace example.hello
operation Hello {
    output: Greeting
}
structure Greeting {
    @required
    @jsonName("msg")
    message: String
}
apply Hello @examples([
    {
        title: "My name is Jeff"
        output: {
            message: "Hello Jeff"
        }
    }
])

the generated swagger doc for the above service would ignore the @JsonName and use message instead of msg:

Screenshot 2024-03-08 at 01 46 56

I think this is wrong. Smithy docs mention useJsonName which might be of relevance but I couldn't find a way of setting that in the interface provided by smithy4s.http4s.swagger.docs.

Also note that in my example, I had to provide the output example as a smithy structure rather than a json. ie {message: "Hello Jeff"} rather than msg, which I'd argue, is a little counter intuitive for the reader. I understand this is perhaps one for smithy but do we know if it Would be possible to specify examples in json rather than smithy structures? 🤔

Baccata commented 4 months ago

@ShahOdin, thank you for transferring the issue. The smithy to openapi conversion is done at built time, by code present in this repository (which fortunately we control), specifically by this class : https://github.com/disneystreaming/alloy/blob/main/modules/openapi/src/software/amazon/smithy/openapi/fromsmithy/protocols/ExampleNode.scala#L48

The bad news is that, though it may not seem like it, this issue is extremely complex. The problem is sort of tied to your end note :

Also note that in my example, I had to provide the output example as a smithy structure rather than a json. ie {message: "Hello Jeff"} rather than msg, which I'd argue, is a little counter intuitive for the reader.

It is important to remember that Smithy is a protocol agnostic IDL. Therefore, the goal of the examples trait provided by the smithy stand library is to define examples regardless of the protocol and serialisation, which is valuable for a number of reasons (such as documentation for generated SDKs). In particular, the fact that its use expects the user to define the inputs/outputs in terms of the canonical json-like representation in smithy means that the data it holds can be validated against the protocol agnostic definition of the data (which disregards protocol-specific traits).

It was not designed to be used in openapi, and it's us (the maintainers of smithy4s and alloy) who were overzealous in trying to make use of it in the context of openapi. I think we were wrong, because taking the protocol-agnostic representation and turning it into a protocol-specific representation (like simpleRestJson) that abides by all the traits relevant to that protocol (like jsonName) is a really really hard problem to solve generically at build time, because protocols can have differences in semantics based on what traits they support. For instance, our simpleRestJson protocol allows for several encodings for unions, which is not something that is supported by the AWS-issued tooling. These custom encodings are also disregarded in openapi examples.

It would be possible for us to solve it at run-time (with the smithy4s constructs), and although this may be desirable in some aspects, it'd take us a fair amount of work to transition from a build-time solution to a run-time solution, and we don't currently have the bandwidth to do so.

Where does that leave us ?

I think the best course of action would be for us to :

The trade-off being that :

  1. The examples would no longer be validated against their models, and could come out of date
  2. Quotes would have to be escaped in the payloads

Regarding your question :

I understand this is perhaps one for smithy but do we know if it Would be possible to specify examples in json rather than smithy structures?

Well my reply hint at it, but considering smithy is a protocol-agnostic IDL, it is extremely unlikely that AWS will change the examples trait.

ShahOdin commented 4 months ago

Thank you so much for the in depth response!

Where does that leave us ?

define a alloy#httpPayloadExamples trait that would take the examples not in the form of a smithy node, but rather in the form of a string.

The trade-off being that

purely from a user's point of view, I'd prefer a structure/Json format as input. I understand that internally you'd probably not want to parse it as a structure.

fwiw, I have decided to drop the discrepency between message and msg for now.

What I would suggest is making this issue more explicit in the guides etc. perhaps here? https://disneystreaming.github.io/smithy4s/docs/protocols/simple-rest-json/overview

Baccata commented 4 months ago

purely from a user's point of view, I'd prefer a structure/Json format as input

Well the thing is that if we put something in place, we'd likely want to re-use it for other serialisations than json, so even if the smithy json-looking "node" (that's what they're called btw) seems like a good idea, I don't think it actually is ...

Anyway, we'll give it a think when we find the time (it may not be for a while though, we're pretty swamped with more urgent stuff at work). If you want to take a go at it, I'm happy to give some guidance.

What I would suggest is making this issue more explicit in the guides etc. perhaps here?

Good point, I'll give it a think. Thanks for the suggestion.