feat: support Smithy intEnum shapes#713
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: naclonts The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
@naclonts from the Smithy docs is look like intEnum provides a name for each numeric value. From a usability perspective should we be accepting that human friendly name instead of the integer?
Address review feedback on aws-controllers-k8s#713: instead of surfacing a Smithy intEnum as an opaque integer, surface the human-friendly member name as a named string enum (*string) on the ACK side, bridging name <-> int32 at the SDK boundary. - apiv2 converter: load intEnum as a string enum, capturing each member name and its integer value (smithy.api#enumValue) in Shape.IntEnumValues - set_sdk: map the name to the SDK int32 alias via a switch - set_resource: map the SDK int32 value back to the name, guarding on the integer zero value (not the empty string) - add a mwaaserverless model + generator.yaml fixture and black-box SetSDK/SetResource tests exercising the EngineVersion intEnum
Yeah, that sounds like a better user experience. Just updated to take an approach similar to |
The apiv2 Smithy loader had no handling for `intEnum` shapes. aws-sdk-go-v2
renders an intEnum as a named integer type alias (e.g.
`type EngineVersion = int32`) whose operation-struct field is a non-pointer
value, and the generator's shape converter copied the `intEnum` type through
verbatim. That left `goType` to hit its default case and panic with
"Unsupported shape type: intEnum", so any service model with an intEnum field
could not be generated (it had to be ignored via generator.yaml).
Normalize `intEnum` to `integer` in createApiShape so the field is surfaced
on the ACK side as `*int64` through the existing integer path, and mark the
shape with a DefaultValue ("0") so it reuses the established value-type SDK
field assignment path (HasDefaultValue drives pointer-vs-value handling in
set_sdk.go / set_resource.go) instead of emitting pointer access against a
value field. Also handle "intEnum" defensively in goType.
Adds converter tests (intEnum -> integer + DefaultValue, integer unaffected)
and set_sdk/set_resource whitebox cases proving value (not pointer)
read/write assignment for an intEnum scalar.
Address review feedback on aws-controllers-k8s#713: instead of surfacing a Smithy intEnum as an opaque integer, surface the human-friendly member name as a named string enum (*string) on the ACK side, bridging name <-> int32 at the SDK boundary. - apiv2 converter: load intEnum as a string enum, capturing each member name and its integer value (smithy.api#enumValue) in Shape.IntEnumValues - set_sdk: map the name to the SDK int32 alias via a switch - set_resource: map the SDK int32 value back to the name, guarding on the integer zero value (not the empty string) - add a mwaaserverless model + generator.yaml fixture and black-box SetSDK/SetResource tests exercising the EngineVersion intEnum
…intEnum members Two correctness fixes on top of the intEnum support: 1. IsEnum() now returns false for intEnum shapes, so SDK-boundary code can never accidentally treat an intEnum as a plain string enum. Callers that legitimately want both (CRD type generation: GoCode, EnumConsts, enum tags, GetEnumDefs, validateShapeNames) now test IsEnum() || IsIntEnum() explicitly. IsNonPointerInSDK() returns true for intEnum, matching how aws-sdk-go-v2 generates the field (a non-pointer int32 alias, e.g. `EngineVersion types.EngineVersion`). 2. The read path no longer guards the int->name switch with `!= 0`. Because the SDK field is a non-pointer int32, zero is indistinguishable from "unset", so a `!= 0` guard would silently drop a legitimate zero-valued intEnum member. The switch is now self-contained with a `default: -> nil`. Also adds panic guards in setSDKForSlice/Map and setResourceForSlice/Map: an intEnum nested in a list or map is not yet supported (no AWS model uses one), and previously would have emitted code that does not compile. Failing loudly at codegen time matches the existing panic-on-unsupported pattern.
naclonts
force-pushed
the
intenum-support
branch
from
7796baf to
9ec04ac
Compare
|
/retest |
|
@naclonts: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| case "ONE": | ||
| res.EngineVersion = svcsdktypes.EngineVersion(1) |
There was a problem hiding this comment.
Hmm, this a tough one. From what I can tell this generated code won't be forward compatible when the AWS service adds new enum values since we won't have a mapping for the string name of the new value. Just to double check with the previous approach of using an integer would the aws-sdk-go-v2 library allow us to send an integer value not included in svcsdktypes.EngineVersion?
There was a problem hiding this comment.
That's a really good catch, I think you're right. The original int-based approach should be forwards-compatible.
That makes me lean toward going back to the int-based approach... Especially considering the MWAA controller's enum is literally just "ONE", there's not much devex improvement in seeing "ONE" versus 1, IMO. Thoughts?
2 participants
Issue #, if available:
Description of changes: Adds support for Smithy intEnum shapes, which previously caused the generator to panic with
Unsupported shape type: intEnum.An
intEnumfield is surfaced on the ACK side as a named string enum (*stringusing the member name), and the generated SDK-conversion code maps the name to/from the underlying integer value at the SDK boundary. Includes a mwaaserverless model fixture and tests.MWAA Serverless field EngineVersion that uses intEnum in model definition: https://github.com/aws/aws-sdk-go-v2/blob/main/codegen/sdk-codegen/aws-models/mwaa-serverless.json#L776
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.