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

JsonName ignored in examples when generating swagger docs #148

Open
ShahOdin opened this issue Mar 8, 2024 · 3 comments
Open

JsonName ignored in examples when generating swagger docs #148

ShahOdin opened this issue Mar 8, 2024 · 3 comments

Comments

@ShahOdin
Copy link

ShahOdin commented Mar 8, 2024

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 Baccata changed the title JsonName ignored when generating swagger docs JsonName ignored in examples when generating swagger docs Mar 8, 2024
@Baccata Baccata transferred this issue from disneystreaming/smithy4s Mar 8, 2024
@Baccata
Copy link
Contributor

Baccata commented Mar 8, 2024

@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 :

  • stop using the official smithy.api#example trait in openapi conversions. It's just unsound, as you've discovered it.
  • 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 :

  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
Copy link
Author

ShahOdin commented Mar 8, 2024

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
Copy link
Contributor

Baccata commented Mar 8, 2024

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.

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

No branches or pull requests

2 participants