Skip to content

Commit

Permalink
always ensure operation overrides work (#375)
Browse files Browse the repository at this point in the history
There were two bugs in the code generator's construction of the OperationMap during API inference.

The first bug was a non-deterministic behaviour that showed itself when an API had multiple Operations of a single operation type (say, READ_ONE). The code that was responsible for determining which Operation mapped to which operation type for a particular resource was looping over the `aws-sdk-go` `private/model/api:API`'s `Operations` field, which is a map. Depending on the parsing of the underlying api-2.json files, same-typed Operations with different IDs -- e.g. `DescribeStreamSummary` and `DescribeStream` -- were being mapped to a single operation type -- OpTypeGet. Depending on which Operation ID came last, that is the operation that was being mapped, regardless of which one appeared in the OperationConfig configuration option that handles overrides.

The second bug was that the `OpTypeFromString` method was too strict in its comparison logic. The strings "get", "READ_ONE", "read_one" and "Get" should all lead to OpTypeGet, but only "Get" was being matched.

Fixes Issue aws-controllers-k8s/community#1555

Signed-off-by: Jay Pipes <jaypipes@gmail.com>

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
jaypipes authored Nov 16, 2022
1 parent f6dd767 commit 62c73f2
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 14 deletions.
28 changes: 14 additions & 14 deletions pkg/model/op.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,32 +150,32 @@ func GetOpTypeAndResourceNameFromOpID(
}

func OpTypeFromString(s string) OpType {
switch s {
case "Create":
switch strings.ToLower(s) {
case "create":
return OpTypeCreate
case "CreateBatch":
case "createbatch":
return OpTypeCreateBatch
case "Delete":
case "delete":
return OpTypeDelete
case "Replace":
case "replace":
return OpTypeReplace
case "Update":
case "update":
return OpTypeUpdate
case "AddChild":
case "addchild":
return OpTypeAddChild
case "AddChildren":
case "addchildren":
return OpTypeAddChildren
case "RemoveChild":
case "removechild":
return OpTypeRemoveChild
case "RemoveChildren":
case "removechildren":
return OpTypeRemoveChildren
case "Get":
case "get", "readone", "read_one":
return OpTypeGet
case "List":
case "list", "readmany", "read_many":
return OpTypeList
case "GetAttributes":
case "getattributes":
return OpTypeGetAttributes
case "SetAttributes":
case "setattributes":
return OpTypeSetAttributes
}

Expand Down
24 changes: 24 additions & 0 deletions pkg/model/sdk_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,30 @@ func (a *SDKAPI) GetOperationMap(cfg *ackgenconfig.Config) *OperationMap {
opMap[opType][resName] = op
}
}

// We need to do a second pass over the operation overrides because some
// APIs have multiple operations of a particular OpType and we need to
// always be sure that the overridden operation is the one that we use
// during inference.
//
// An example of this is the Kinesis API which has two OpTypeGet
// Operations: DescribeStream and DescribeStreamSummary. We want the latter
// only and list that in our `operations:` configuration value.
//
// see: https://github.com/aws-controllers-k8s/community/issues/1555
for opID, opCfg := range cfg.Operations {
if opCfg.ResourceName == "" {
continue
}
op, found := a.API.Operations[opID]
if !found {
panic("operation " + opID + " in generator.yaml 'operations:' object does not exist.")
}
for _, ot := range opCfg.OperationType {
opType := OpTypeFromString(ot)
opMap[opType][opCfg.ResourceName] = op
}
}
a.opMap = &opMap
return &opMap
}
Expand Down

0 comments on commit 62c73f2

Please sign in to comment.