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

Allow smithy.api#jsonName trait to target union members #953

Closed
Baccata opened this issue Oct 21, 2021 · 7 comments · Fixed by #1153
Closed

Allow smithy.api#jsonName trait to target union members #953

Baccata opened this issue Oct 21, 2021 · 7 comments · Fixed by #1153
Labels
feature-request A feature should be added or improved.

Comments

@Baccata
Copy link

Baccata commented Oct 21, 2021

It is currently restricted to structure members.

I think it should be possible for it to target union members as well.

@JordonPhillips JordonPhillips added the feature-request A feature should be added or improved. label Oct 22, 2021
@JordonPhillips
Copy link
Contributor

Do you have a particular reason for wanting this? The jsonName trait, as it is, is kind of a concession to the reality of needing to support existing services. IMO a new service shouldn't use traits like that because they make it harder for a human to reason about the serialized data. I'm not super against adding it, I just want to know if there's some context I'm missing.

@Baccata
Copy link
Author

Baccata commented Oct 22, 2021

Here are a few reasons

  • Fundamentally speaking, in the context of data modelling, structure and unions are dual to each other. Allowing one type of customisation on structures but not unions is somewhat ... awkward. And reduces the number of services that can be naturally ported to smithy without breaking their APIs
  • The lack of customisability of the serialisation means that the UX of the generated has to suffer to reduce the size of payloads. For instance, dynamodb's AttributeValue is a union the members of which are named with just one or two letters. It makes sense at a protocol level, but in the generated code, it's not great that users have to mentally keep track of "ss" meaning "stringSet" and "n" meaning "number", etc. Which means it is harder for a human to reason about client code.

@mtdowling
Copy link
Member

Those sound like good reasons to me!

Just for bookkeeping: to implement this, we need to update the spec, the prelude model's trait definition, and add a protocol test for awsRestJson1 to make sure AWS SDK generators know about the change.

@Grundlefleck
Copy link

The jsonName trait, as it is, is kind of a concession to the reality of needing to support existing services.

We are running into similar problems trying to match existing HTTP API style guides where we have standardised on snake_case property names in JSON objects. We do not yet provide SDKs universally, so we still have to take care with JSON shapes in the wire format, as we expect developers to use JSON directly.

We were getting by with @jsonName annotations, but hit this roadblock on our first use of Smithy union types.

I would have expected to be able to do the following (example from the Smithy docs):

union PlayerAction {
    quit: Unit,

    @jsonName("move_about")
    moveAbout: DirectedAction,

    @jsonName("jump_around")
    jumpAround: DirectedAction
}

so that awsRestJson1 protocol would adapt the input and output shapes. For reference we're using smithy-typescript to generate the validation code, and it's all working well.

I'm worried that this approach we're taking is repeatedly going to bump into issues attempting to curate JSON shapes via the Smithy models alone. @JordonPhillips do you think we should be approaching this differently, so that we are more aligned with Smithy and can leverage as much as possible out of the box? Or is enforcing style guides on a serialization format fundamentally misaligned with Smithy?

@Grundlefleck
Copy link

@mtdowling just a note on implementation as I didn't see it mentioned. While the @jsonName trait can differ from the union member names, as a user I'd expect the same constraints of having unique @jsonNames within a union, and failing validation if I have duplicates.

@mtdowling
Copy link
Member

mtdowling commented Mar 23, 2022

jsonName is now supported on union members. This will go out in the next release.

@Baccata
Copy link
Author

Baccata commented Mar 24, 2022

🎉 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants