enhancement: strict member mode - value types for required output members #598
Description
Proposal
For smithy-go (@awsRestJson1), all output types generated for clients are inherently nillable, regardless of whether a type is optional or marked with the @required trait.
As a result, for required output members, there is no type-level compiler guarantee that a type is non-nil. All fields appear identical, which may confuse developers about which fields require nil checks to avoid nil-pointer dereference panics at runtime and which can be assumed to be non-nil.
This behavior does not fully utilize model knowledge during codegeneration. Since it is known which fields are @required and guaranteed to be present, code generation could encode this information at the type level, making the generated code more developer-friendly and reducing the need for consumers of generated clients to perform excessive nil checks.
The most common approach to model non-nillable types in Go is to use value types instead of pointer types for required output members in generated code.
It's worth to emphasize that other smithy codegenerators such as smithy-typescript, smithy-kotlin and smithy-rust indeed take into account @requiredtraits for output member types to reduce number of nullable types in generated code. Here is a related feature flag RequiredMemberMode from smithy-typescript. A similar flag could be introduced in smithy-go to avoid any breaking changes.
Example
Current Behavior
Given the following smithy definition:
service RequiredMemberService {
operations: [GetFoo]
}
@readonly
@http(method: "GET", uri: "/v1/some-endpoint", code: 200)
operation GetFoo {
input := { }
output := {
@required
baz: NestedOutputType
}
}
structure NestedOutputType {
@required
v: String
}the following types are generated:
./api_op_GetFoo.go
type GetFooOutput struct {
// This member is required.
Baz *types.NestedOutputType
// Metadata pertaining to the operation's result.
ResultMetadata middleware.Metadata
noSmithyDocumentSerde
}./types.go
type NestedOutputType struct {
// This member is required.
V *string
noSmithyDocumentSerde
}Proposed Behavior
Instead of generating all types with pointers (*T), required members are modeled as value types (T).
./api_op_GetFoo.go
type GetFooOutput struct {
// This member is required.
Baz types.NestedOutputType
// Metadata pertaining to the operation's result.
ResultMetadata middleware.Metadata
noSmithyDocumentSerde
}./types.go
type NestedOutputType struct {
// This member is required.
V string
noSmithyDocumentSerde
}Feature Limitation
-
Given the code that current implementation generates, required value types could only be applied to output types. Generating value types for Input types would make it impossible to perform client-side validation whether required fields were set by client consumers - that's because value types always have default zero values and those are non-distinguishable from values set explicitly. Validation that required fields were set for Inputs is an existing feature (
validators.go). -
It won't be possible to apply this behavior to existing services without a breaking change, hence this behavior will have to stay behind a feature flag as a default. However new services onboarding to Smithy could benefit from a more ergonomic developer interface if they choose to.
Proposed Implementation
I've tried to hack together an implementation for this proposal - it works but likely requires feedback from maintainers of this project 🙏
See the following PRs:
- required member mode - initial draft #597 - introduces
requiredMemberModeflag and implements value type generation. - required member mode - initial draft aws-sdk-go-v2#3169 - related adjustments to deserialization code (
JsonMemberDeserVisitor.java)
Deserialization Code Changes
Deserialization Code
It's worth to note that the change will also apply to generated deserialization code - however this is an implementation detail - not leaking into consumers of the API.
Existing Deserialization Implementation
Structures are being recursively deserialized by passing a pointer of a pointer to deser.. function and allocating that struct if a pointer is nil.
Existing deserialization functions assume first argument is **T.
func awsRestjson1_deserializeOpDocumentGetFooOutput(v **GetFooOutput, value interface{}) error {
// ...
for key, value := range shape {
switch key {
case "baz":
if err := awsRestjson1_deserializeDocumentNestedOutputType(&sv.Baz, value); err != nil {
return err
}
default:
_, _ = key, value
}
}
*v = sv
return nil
}Deserialized string is being wrapped in a pointer before assigning to output type: sv.V = ptr.String(jtv)
func awsRestjson1_deserializeDocumentNestedOutputType(v **types.NestedOutputType, value interface{}) error {
// ..
for key, value := range shape {
switch key {
case "v":
if value != nil {
jtv, ok := value.(string)
if !ok {
return fmt.Errorf("expected String to be of type string, got %T instead", value)
}
sv.V = ptr.String(jtv)
}
default:
_, _ = key, value
}
}
*v = sv
return nil
}Proposed Deserialization Implementation
Deserialization of structures, requires to take a pointer of a pointer to be able to pass it deserialize.. functions. Note ptr := &sv.Baz.
func awsRestjson1_deserializeOpDocumentGetFooOutput(v **GetFooOutput, value interface{}) error {
// ..
for key, value := range shape {
switch key {
case "baz":
ptr := &sv.Baz
if err := awsRestjson1_deserializeDocumentNestedOutputType(&ptr, value); err != nil {
return err
}
default:
_, _ = key, value
}
}
*v = sv
return nil
}Primitive types instead of being wrapped into a pointer type are simply assigned to a value type.
sv.V = jtv
func awsRestjson1_deserializeDocumentNestedOutputType(v **types.NestedOutputType, value interface{}) error {
// ...
for key, value := range shape {
switch key {
case "v":
if value != nil {
jtv, ok := value.(string)
if !ok {
return fmt.Errorf("expected String to be of type string, got %T instead", value)
}
sv.V = jtv
}
default:
_, _ = key, value
}
}
*v = sv
return nil
}