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.

Sign up for GitHub

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

chore: PoC - handle merging computability in nested attributes #2722

Merged
merged 12 commits into from
Oct 23, 2024
Merged

chore: PoC - handle merging computability in nested attributes #2722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c1b30e6
add support for config overrides
maastha Oct 18, 2024
924fe49
Merge branch 'CLOUDP-201096-tf-autogen-schema-conv-main' into CLOUDP-…
maastha Oct 18, 2024
8282070
minor
maastha Oct 18, 2024
35231f5
infer computability for nested
maastha Oct 22, 2024
dc25ae0
minor
maastha Oct 22, 2024
9b232a6
minor
maastha Oct 22, 2024
e94a4db
Merge branch 'CLOUDP-201096-tf-autogen-schema-conv-main' into CLOUDP-…
maastha Oct 22, 2024
1fd92b3
minor
maastha Oct 22, 2024
ce5ec10
minor
maastha Oct 22, 2024
af62b39
update test case for nested partial computed attr
maastha Oct 22, 2024
072e3eb
address PR comments
maastha Oct 23, 2024
88f5fbd
lint
maastha Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions tools/codegen/codespec/api_to_provider_spec_mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//nolint:gocritic
package codespec

import (
Expand Down Expand Up @@ -37,18 +36,18 @@ func ToCodeSpecModel(atlasAdminAPISpecFilePath, configPath string, resourceName
for name, resourceConfig := range resourceConfigsToIterate {
log.Printf("Generating resource: %s", name)
// find resource operations, schemas, etc from OAS
oasResource, err := getAPISpecResource(apiSpec.Model, resourceConfig, SnakeCaseString(name))
oasResource, err := getAPISpecResource(&apiSpec.Model, &resourceConfig, SnakeCaseString(name))
if err != nil {
return nil, fmt.Errorf("unable to get APISpecResource schema: %v", err)
}
// map OAS resource model to CodeSpecModel
results = append(results, *apiSpecResourceToCodeSpecModel(oasResource, resourceConfig, SnakeCaseString(name)))
results = append(results, *apiSpecResourceToCodeSpecModel(oasResource, &resourceConfig, SnakeCaseString(name)))
}

return &Model{Resources: results}, nil
}

func apiSpecResourceToCodeSpecModel(oasResource APISpecResource, resourceConfig config.Resource, name SnakeCaseString) *Resource {
func apiSpecResourceToCodeSpecModel(oasResource APISpecResource, resourceConfig *config.Resource, name SnakeCaseString) *Resource {
createOp := oasResource.CreateOp
readOp := oasResource.ReadOp

Expand All @@ -70,7 +69,7 @@ func apiSpecResourceToCodeSpecModel(oasResource APISpecResource, resourceConfig
Schema: schema,
}

applyConfigSchemaOptions(&resourceConfig, resource)
applyConfigSchemaOptions(resourceConfig, resource)

return resource
}
Expand Down Expand Up @@ -136,7 +135,7 @@ func opResponseToAttributes(op *high.Operation) Attributes {
return responseAttributes
}

func getAPISpecResource(spec high.Document, resourceConfig config.Resource, name SnakeCaseString) (APISpecResource, error) {
func getAPISpecResource(spec *high.Document, resourceConfig *config.Resource, name SnakeCaseString) (APISpecResource, error) {
var errResult error
var resourceDeprecationMsg *string

Expand Down
34 changes: 25 additions & 9 deletions tools/codegen/codespec/api_to_provider_spec_mapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ func TestConvertToProviderSpec_nested(t *testing.T) {
},
Description: conversion.StringPtr(testFieldDesc),
},
{
Name: "list_primitive_string_computed_attr",
ComputedOptionalRequired: codespec.Computed,
List: &codespec.ListAttribute{
ElementType: codespec.String,
},
Description: conversion.StringPtr(testFieldDesc),
},
},
},
},
Expand All @@ -170,7 +178,7 @@ func TestConvertToProviderSpec_nested(t *testing.T) {
Attributes: codespec.Attributes{
{
Name: "attr",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
String: &codespec.StringAttribute{},
},
},
Expand All @@ -185,13 +193,13 @@ func TestConvertToProviderSpec_nested(t *testing.T) {
Attributes: codespec.Attributes{
{
Name: "inner_num_attr",
ComputedOptionalRequired: codespec.Required,
ComputedOptionalRequired: codespec.Computed,
Int64: &codespec.Int64Attribute{},
Description: conversion.StringPtr(testFieldDesc),
},
{
Name: "list_primitive_string_attr",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
List: &codespec.ListAttribute{
ElementType: codespec.String,
},
Expand All @@ -218,13 +226,13 @@ func TestConvertToProviderSpec_nested(t *testing.T) {
Attributes: codespec.Attributes{
{
Name: "inner_int_attr",
ComputedOptionalRequired: codespec.Required,
ComputedOptionalRequired: codespec.Computed,
Int64: &codespec.Int64Attribute{},
Description: conversion.StringPtr(testFieldDesc),
},
{
Name: "inner_str_attr",
ComputedOptionalRequired: codespec.Required,
ComputedOptionalRequired: codespec.Computed,
String: &codespec.StringAttribute{},
Description: conversion.StringPtr(testFieldDesc),
},
Expand All @@ -241,14 +249,14 @@ func TestConvertToProviderSpec_nested(t *testing.T) {
Attributes: codespec.Attributes{
{
Name: "map_attr1",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
Map: &codespec.MapAttribute{
ElementType: codespec.String,
},
},
{
Name: "map_attr2",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
Map: &codespec.MapAttribute{
ElementType: codespec.String,
},
Expand Down Expand Up @@ -306,6 +314,14 @@ func TestConvertToProviderSpec_nested_schemaOverrides(t *testing.T) {
Int64: &codespec.Int64Attribute{},
Description: conversion.StringPtr("Overridden inner_num_attr_alias description"),
},
{
Name: "list_primitive_string_computed_attr",
ComputedOptionalRequired: codespec.Computed,
List: &codespec.ListAttribute{
ElementType: codespec.String,
},
Description: conversion.StringPtr(testFieldDesc),
},
},
},
},
Expand All @@ -319,13 +335,13 @@ func TestConvertToProviderSpec_nested_schemaOverrides(t *testing.T) {
Attributes: codespec.Attributes{
{
Name: "nested_level1",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
SingleNested: &codespec.SingleNestedAttribute{
NestedObject: codespec.NestedAttributeObject{
Attributes: codespec.Attributes{
{
Name: "level_field1_alias",
ComputedOptionalRequired: codespec.Optional,
ComputedOptionalRequired: codespec.Computed,
String: &codespec.StringAttribute{},
Description: conversion.StringPtr("Overridden level_field1_alias description"),
},
Expand Down
140 changes: 109 additions & 31 deletions tools/codegen/codespec/merge_attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,110 @@
package codespec

import "sort"
import (
"sort"
)

// TODO: update to infer computability of inner nested attributes
func mergeAttributes(pathParams, createRequest, createResponse, readResponse Attributes) Attributes {
merged := make(map[string]*Attribute)

addOrUpdate := func(attr *Attribute, computability ComputedOptionalRequired) {
if existingAttr, exists := merged[attr.Name.SnakeCase()]; exists {
if existingAttr.Description == nil || *existingAttr.Description == "" {
existingAttr.Description = attr.Description
}
// mergeNestedAttributes recursively merges nested attributes
func mergeNestedAttributes(existingAttrs *Attributes, newAttrs Attributes, computability ComputedOptionalRequired, isFromResponse bool) {
Copy link
Member

Choose a reason for hiding this comment

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

After debugging over the code I was surprised to see the logic for handling nested attributes is within addOrUpdate, and not during mergeAttributes where you can obtain for each nested attribute what value it has for each operation request/responses. Curious if you thought of this alternative or found limitation for this approach.

Copy link
Collaborator Author

@maastha maastha Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I understand completely. mergeAttributes is intended as an entry point.

can you elaborate what you mean and how that may be better?

Copy link
Member

Choose a reason for hiding this comment

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

I see. So each time an attribute is merged with addOrUpdate all nested attributes are being processed. Leaving some comments on mergeAttributes, addOrUpdate, and mergeNestedAttributes I believe will be very helpful for future reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. We have to process nested attributes on every merge because create/read responses may contain additional attributes under the same parent attribute name that was initially included from the request.

mergedMap := make(map[string]*Attribute)
if existingAttrs != nil {
for i := range *existingAttrs {
mergedMap[(*existingAttrs)[i].Name.SnakeCase()] = &(*existingAttrs)[i]
}
}

if existingAttr.ComputedOptionalRequired == Required {
return
}
// add new attributes and merge when necessary
for i := range newAttrs {
newAttr := &newAttrs[i]

existingAttr.ComputedOptionalRequired = computability
if _, exists := mergedMap[newAttr.Name.SnakeCase()]; exists {
addOrUpdate(newAttr, computability, mergedMap, isFromResponse)
} else {
newAttr := *attr
newAttr.ComputedOptionalRequired = computability
merged[attr.Name.SnakeCase()] = &newAttr
mergedMap[newAttr.Name.SnakeCase()] = newAttr
}
}

// update original existingAttrs with the merged result
*existingAttrs = make(Attributes, 0, len(mergedMap))
for _, attr := range mergedMap {
*existingAttrs = append(*existingAttrs, *attr)
}

sortAttributes(*existingAttrs)
}

// addOrUpdate adds or updates an attribute in the merged map, including nested attributes
func addOrUpdate(attr *Attribute, computability ComputedOptionalRequired, merged map[string]*Attribute, isFromResponse bool) {
if existingAttr, exists := merged[attr.Name.SnakeCase()]; exists {
if existingAttr.Description == nil || *existingAttr.Description == "" {
existingAttr.Description = attr.Description
}

// retain computability if already set from request
if !isFromResponse && existingAttr.ComputedOptionalRequired != Required {
existingAttr.ComputedOptionalRequired = computability
}

// handle nested attributes
if existingAttr.ListNested != nil && attr.ListNested != nil {
mergeNestedAttributes(&existingAttr.ListNested.NestedObject.Attributes, attr.ListNested.NestedObject.Attributes, computability, isFromResponse)
} else if attr.ListNested != nil {
existingAttr.ListNested = attr.ListNested
}

if existingAttr.SingleNested != nil && attr.SingleNested != nil {
mergeNestedAttributes(&existingAttr.SingleNested.NestedObject.Attributes, attr.SingleNested.NestedObject.Attributes, computability, isFromResponse)
} else if attr.SingleNested != nil {
existingAttr.SingleNested = attr.SingleNested
}

if existingAttr.SetNested != nil && attr.SetNested != nil {
mergeNestedAttributes(&existingAttr.SetNested.NestedObject.Attributes, attr.SetNested.NestedObject.Attributes, computability, isFromResponse)
} else if attr.SetNested != nil {
existingAttr.SetNested = attr.SetNested
}

if existingAttr.MapNested != nil && attr.MapNested != nil {
mergeNestedAttributes(&existingAttr.MapNested.NestedObject.Attributes, attr.MapNested.NestedObject.Attributes, computability, isFromResponse)
} else if attr.MapNested != nil {
existingAttr.MapNested = attr.MapNested
}
} else {
// add new attribute with the given computability
newAttr := *attr
newAttr.ComputedOptionalRequired = computability
merged[attr.Name.SnakeCase()] = &newAttr
}
}

func mergeAttributes(pathParams, createRequest, createResponse, readResponse Attributes) Attributes {
merged := make(map[string]*Attribute)

// Path parameters: all attributes will be "required"
for i := range pathParams {
addOrUpdate(&pathParams[i], Required)
addOrUpdate(&pathParams[i], Required, merged, false)
}

// POST request body: optional/required is as defined
for i := range createRequest {
addOrUpdate(&createRequest[i], createRequest[i].ComputedOptionalRequired)
addOrUpdate(&createRequest[i], createRequest[i].ComputedOptionalRequired, merged, false)
}

// POST/GET response body: properties not in the request body are "computed" or "computed_optional" (if a default is present)
for i := range createResponse {
if _, exists := merged[createResponse[i].Name.SnakeCase()]; !exists {
if isOptional(&createResponse[i]) {
addOrUpdate(&createResponse[i], ComputedOptional)
} else {
addOrUpdate(&createResponse[i], Computed)
}
if hasDefault(&createResponse[i]) {
addOrUpdate(&createResponse[i], ComputedOptional, merged, true)
} else {
addOrUpdate(&createResponse[i], Computed, merged, true)
}
}

for i := range readResponse {
if _, exists := merged[readResponse[i].Name.SnakeCase()]; !exists {
if isOptional(&readResponse[i]) {
addOrUpdate(&readResponse[i], ComputedOptional)
} else {
addOrUpdate(&readResponse[i], Computed)
}
if hasDefault(&readResponse[i]) {
addOrUpdate(&readResponse[i], ComputedOptional, merged, true)
} else {
addOrUpdate(&readResponse[i], Computed, merged, true)
}
}

Expand All @@ -62,10 +115,35 @@ func mergeAttributes(pathParams, createRequest, createResponse, readResponse Att

sortAttributes(resourceAttributes)

updateNestedComputability(&resourceAttributes, Optional)

return resourceAttributes
}

func isOptional(attr *Attribute) bool {
func updateNestedComputability(attrs *Attributes, parentComputability ComputedOptionalRequired) {
for i := range *attrs {
attr := &(*attrs)[i]

if parentComputability == Computed {
attr.ComputedOptionalRequired = Computed
}

if attr.ListNested != nil {
updateNestedComputability(&attr.ListNested.NestedObject.Attributes, attr.ComputedOptionalRequired)
}
if attr.SingleNested != nil {
updateNestedComputability(&attr.SingleNested.NestedObject.Attributes, attr.ComputedOptionalRequired)
}
if attr.SetNested != nil {
updateNestedComputability(&attr.SetNested.NestedObject.Attributes, attr.ComputedOptionalRequired)
}
if attr.MapNested != nil {
updateNestedComputability(&attr.MapNested.NestedObject.Attributes, attr.ComputedOptionalRequired)
}
}
}

func hasDefault(attr *Attribute) bool {
return (attr.Bool != nil && attr.Bool.Default != nil) ||
(attr.Int64 != nil && attr.Int64.Default != nil) ||
(attr.String != nil && attr.String.Default != nil) ||
Expand Down
22 changes: 21 additions & 1 deletion tools/codegen/codespec/testdata/api-spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,27 @@ components:
type: array
description: Test field description
items:
$ref: "#/components/schemas/NestedObjectAttr"
type: object
properties:
innerNumAttr:
type: integer
format: int32
description: Test field description
example: 2
maximum: 32
minimum: 2
listPrimitiveStringAttr:
type: array
description: Test field description
items:
type: string
listPrimitiveStringComputedAttr:
type: array
description: Test field description
items:
type: string
required:
- innerNumAttr
readOnly: true
nestedSetArrayAttr:
type: array
Expand Down