Skip to content

Commit

Permalink
Support resource types that aren't quite "really" resources (#3525)
Browse files Browse the repository at this point in the history
They have a HEAD rather than a GET and no body parameters
  • Loading branch information
matthchr authored Nov 21, 2023
1 parent 1009d02 commit bd90009
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 7 deletions.
5 changes: 3 additions & 2 deletions v2/tools/generator/internal/codegen/code_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package codegen
import (
"context"
"fmt"
"runtime/debug"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -325,9 +326,9 @@ func (generator *CodeGenerator) executeStage(
defer func() {
if r := recover(); r != nil {
if e, ok := r.(error); ok {
err = e
err = errors.Wrapf(e, "panic: %s", string(debug.Stack()))
} else {
err = errors.Errorf("panic: %s", r)
err = errors.Errorf("panic: %s\n%s", r, string(debug.Stack()))
}
}
}()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ func findEmptyObjectTypes(
) astmodel.TypeNameSet {
result := astmodel.NewTypeNameSet()

// Get all top level spec and status types
specs := astmodel.FindSpecDefinitions(definitions)
statuses := astmodel.FindStatusDefinitions(definitions)

for _, def := range definitions {
ot, ok := astmodel.AsObjectType(def.Type())
if !ok {
Expand All @@ -55,6 +59,11 @@ func findEmptyObjectTypes(
continue
}

// Don't prune top level status or spec types, they are allowed to be empty
if specs.Contains(def.Name()) || statuses.Contains(def.Name()) {
continue
}

log.V(1).Info(
"Removing empty type",
"name", def.Name())
Expand Down
26 changes: 21 additions & 5 deletions v2/tools/generator/internal/jsonast/swagger_type_extractor.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ func (extractor *SwaggerTypeExtractor) ExtractResourceTypes(ctx context.Context,
}

for rawOperationPath, op := range extractor.swagger.Paths.Paths {
// a resource must have both PUT and GET
if op.Put == nil || op.Get == nil {
// a resource must have a PUT and one of GET or HEAD
if op.Put == nil || (op.Get == nil && op.Head == nil) {
continue
}

Expand Down Expand Up @@ -214,7 +214,10 @@ func (extractor *SwaggerTypeExtractor) extractOneResourceType(
var resourceSpec astmodel.Type
if specSchema == nil {
// nil indicates empty body
resourceSpec = astmodel.NewObjectType()
// Fabricate a TypeName here because that's what all other specs are (typename pointing to parameters type)
name := astmodel.MakeInternalTypeName(resourceName.InternalPackageReference(), fmt.Sprintf("%sCreateParameters", resourceName.Name()))
result.OtherDefinitions[name] = astmodel.MakeTypeDefinition(name, astmodel.NewObjectType())
resourceSpec = name
} else {
resourceSpec, err = scanner.RunHandlerForSchema(ctx, *specSchema)
if err != nil {
Expand All @@ -240,7 +243,10 @@ func (extractor *SwaggerTypeExtractor) extractOneResourceType(
var resourceStatus astmodel.Type
if statusSchema == nil {
// nil indicates empty body
resourceStatus = astmodel.NewObjectType()
// Fabricate a TypeName here because that's what all other specs are (typename pointing to parameters type)
name := astmodel.MakeInternalTypeName(resourceName.InternalPackageReference(), fmt.Sprintf("%sStatus", resourceName.Name()))
result.OtherDefinitions[name] = astmodel.MakeTypeDefinition(name, astmodel.NewObjectType())
resourceStatus = name
} else {
resourceStatus, err = scanner.RunHandlerForSchema(ctx, *statusSchema)
if err != nil {
Expand Down Expand Up @@ -337,7 +343,10 @@ func (extractor *SwaggerTypeExtractor) findARMResourceSchema(op spec.PathItem, r
isResource := false

var foundStatus *Schema
if op.Get.Responses != nil {
// The previous assumption that all ARM resources have a GET is not correct, see for example
// https://learn.microsoft.com/en-us/azure/templates/microsoft.apimanagement/service/products/groups?pivots=deployment-language-arm-template
// which has HEAD and not GET
if op.Get != nil && op.Get.Responses != nil {
for statusCode, response := range op.Get.Responses.StatusCodeResponses {
// only check OK and Created (per above linked comment)
// TODO: we should really check that the results are the same in each status result
Expand All @@ -352,6 +361,13 @@ func (extractor *SwaggerTypeExtractor) findARMResourceSchema(op spec.PathItem, r
}
}

if isResource == false && op.Head != nil {
// assume this is a resource. It's possible this is too permissive and classifies some things as resources
// when they really aren't, but that's OK because anyway we only generate the resources we name in the configuration
// and classifying something as a resource here just makes it a candidate for mention in the config.
isResource = true
}

if !isResource {
return nil, nil, false
}
Expand Down

0 comments on commit bd90009

Please sign in to comment.