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

Add jsonName and json struct tag support #372

Closed
wants to merge 1 commit into from
Closed

Conversation

NukaCody
Copy link

Issue #326, if available:

Description of changes:

If you specify jsonName("somevalue") trait in your smithy model, then assume you want a json struct tag.

I think that's the most intuitive approach to take this but definitely open to other ideas. Something like you have to specify in smithy-build.json some value such as useJsonStructTag: true. But may be more work for not much reward.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@NukaCody
Copy link
Author

Also, I have no clue what the letters in writer.write("$L $P \json:\"$L\"", memberName, memberSymbol, jsonTag); mean lol. I tried $C and kept running into a InvocationTargetException so I went with L and it worked

@jasdel
Copy link
Contributor

jasdel commented Jul 29, 2022

We've been hesitant to add struct tags to the API types. Mainly because the SDK strives to ensure serialization in an implementation detail of the API and SDK. An API could always change it's protocol and that would put the struct tags at risk of breaking. The public types purposely do not expose these.

We're happy to continue this discussion and hear your use case about the need for this feature. Also it's important to note that API protocol serialization isn't Go JSON standard especially in cases such as byte slices and time.

@NukaCody
Copy link
Author

I can understand that. If I capture it right, looking through an example. EC2RunInstances and it's Input make no assumption on protocol. It's not till invokeOperation and I think more specifically HTTP semantics start here

Our use case is a little bit different. We have a folder structure like this:

.
├── model
│   ├── build/smithyprojections/model/source/go-codegen
└── externalLib # Both use model
└── infra # Both use model

Our model is used in two different contexts, one as supplying an actual API in the traditional sense of an SDK talking to an API endpoint in Infra. Another as a sort of "shared type" folder. In externalLib we have to build a custom marshaller/unmarshaller that transforms a specification (that we can't control) into cloudformation. The jsonName tag makes writing the custom marshaller easier.

I think jsonName() is also a two-way door. If we need to backtrack, we can always Find and Replace jsonName(*) to "" then regenerate the model. Although I do see how that would break externalLib's marshalling if we did that. I think that's a risk we are willing to accept though.

I'm a bit smooth brain and didn't follow this:

Also it's important to note that API protocol serialization isn't Go JSON standard especially in cases such as byte slices and time.

Comment on lines +117 to +127
String jsonTag = "";
if (member.hasTrait(JsonNameTrait.ID)) {
jsonTag = member.getMemberTrait(model, JsonNameTrait.class).get().getValue();
}

if (!jsonTag.isEmpty()) {
writer.write("$L $P `json:\"$L\"`", memberName, memberSymbol, jsonTag);
} else {
writer.write("$L $P", memberName, memberSymbol);
}

Copy link

@jvschneid jvschneid Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using optionals here instead of all these conditionals, something like:

writer.writeInline("$L $P", memberName, memberSymbol);

member.getMemberTrait(model, JsonNameTrait.class).map(JsonNameTrait::getValue)
      .ifPresent(jsonName -> writer.writeInline(" `json:\"$L\"`", jsonName));
                        
writer.write("");

@lucix-aws
Copy link
Contributor

I'm going to close this - this should be handled via protocol generation, specifically we'll get json protocol support (including jsonName) by addressing #458.

@lucix-aws lucix-aws closed this Oct 3, 2023
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

Successfully merging this pull request may close these issues.

5 participants