Fix map handling with schema composition + upgrade pb33f/libopenapi (…

…#101) * initial behavior * remove validation for provider ref, as it doesn't exist upstream anymore * add context from signature change * initial fix * add a nested test * update comment * add changelog
hashicorp · Dec 13, 2023 · d3b0da1 · d3b0da1
1 parent 18203e9
commit d3b0da1
Show file tree

Hide file tree

Showing 17 changed files with 474 additions and 347 deletions.
diff --git a/.changes/unreleased/BUG FIXES-20231213-145019.yaml b/.changes/unreleased/BUG FIXES-20231213-145019.yaml
@@ -0,0 +1,7 @@
+kind: BUG FIXES
+body: Fixed a bug where schemas that used `additionalProperties` with schema composition
+  (allOf/anyOf/oneOf) would return an empty single nested attribute. Will now return
+  map or map nested attribute.
+time: 2023-12-13T14:50:19.121128-05:00
+custom:
+  Issue: "100"
diff --git a/go.mod b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/hashicorp/terraform-plugin-codegen-spec v0.1.0
 	github.com/mattn/go-colorable v0.1.13
 	github.com/mitchellh/cli v1.1.5
-	github.com/pb33f/libopenapi v0.11.0
+	github.com/pb33f/libopenapi v0.13.22
 	gopkg.in/yaml.v3 v3.0.1
 )
 
@@ -35,7 +35,7 @@ require (
 	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
 	github.com/xeipuuv/gojsonschema v1.2.0 // indirect
 	golang.org/x/crypto v0.7.0 // indirect
-	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
-	golang.org/x/sync v0.3.0 // indirect
+	golang.org/x/exp v0.0.0-20231206192017-f3f8817b8deb // indirect
+	golang.org/x/sync v0.5.0 // indirect
 	golang.org/x/sys v0.12.0 // indirect
 )
diff --git a/go.sum b/go.sum
@@ -104,8 +104,8 @@ github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1y
 github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY=
 github.com/onsi/gomega v1.19.0 h1:4ieX6qQjPP/BfC3mpsAtIGGlxTWPeA3Inl/7DtXw1tw=
 github.com/onsi/gomega v1.19.0/go.mod h1:LY+I3pBVzYsTBU1AnDwOSxaYi9WoWiqgwooUqq9yPro=
-github.com/pb33f/libopenapi v0.11.0 h1:xhFWajHaTVXD5+hh7LHY7kVBDVEVMSO509NFs6SOiu4=
-github.com/pb33f/libopenapi v0.11.0/go.mod h1:s8uj6S0DjWrwZVj20ianJBz+MMjHAbeeRYNyo9ird74=
+github.com/pb33f/libopenapi v0.13.22 h1:QivxHLf+ZaYl2mFivUFkKZ7315mtYMipaQP5zp0U1H4=
+github.com/pb33f/libopenapi v0.13.22/go.mod h1:Lv2eEtsAtbRFlF8hjH82L8SIGoUNgemMVoKoB6A9THk=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/posener/complete v1.1.1/go.mod h1:em0nMJCgc9GFtwrmVmEMR/ZL6WyhyjMBndrE9hABlRI=
@@ -148,8 +148,8 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5y
 golang.org/x/crypto v0.3.0/go.mod h1:hebNnKkNXi2UzZN1eVRvBB7co0a+JxK6XbPiWVs/3J4=
 golang.org/x/crypto v0.7.0 h1:AvwMYaRytfdeVt3u6mLaxYtErKYjxA2OXjJ1HHq6t3A=
 golang.org/x/crypto v0.7.0/go.mod h1:pYwdfH91IfpZVANVyUOhSIPZaFoJGxTFbZhFTx+dXZU=
-golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g=
-golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k=
+golang.org/x/exp v0.0.0-20231206192017-f3f8817b8deb h1:c0vyKkb6yr3KR7jEfJaOSv4lG7xPkbN6r52aJz1d8a8=
+golang.org/x/exp v0.0.0-20231206192017-f3f8817b8deb/go.mod h1:iRJReGqOEeBhDZGkGbynYwcHlctCvnjTYIamk7uXpHI=
 golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -168,8 +168,8 @@ golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
-golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E=
-golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
+golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE=
+golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=

diff --git a/internal/cmd/generate.go b/internal/cmd/generate.go
@@ -20,7 +20,7 @@ import (
 
 	"github.com/mitchellh/cli"
 	"github.com/pb33f/libopenapi"
-	"github.com/pb33f/libopenapi/resolver"
+	"github.com/pb33f/libopenapi/index"
 )
 
 type GenerateCommand struct {
@@ -133,7 +133,7 @@ func (cmd *GenerateCommand) runInternal(logger *slog.Logger) error {
 	// 4. Log circular references as warnings and fail on any other model building errors
 	var errResult error
 	for _, err := range errs {
-		if rslvErr, ok := err.(*resolver.ResolvingError); ok {
+		if rslvErr, ok := err.(*index.ResolvingError); ok {
 			logger.Warn(
 				"circular reference found in OpenAPI spec",
 				"circular_ref", rslvErr.CircularReference.GenerateJourneyPath())

diff --git a/internal/cmd/testdata/edgecase/openapi_spec.yml b/internal/cmd/testdata/edgecase/openapi_spec.yml
@@ -73,11 +73,16 @@ paths:
               properties:
                 mapnested_prop:
                   $ref: "#/components/schemas/mapnested_schema"
+                mapnested_nullable:
+                  $ref: "#/components/schemas/mapnested_allof_oneof_nullable"
                 map_prop:
                   description: This is a map of strings
                   type: object
                   additionalProperties:
                     type: string
+                map_stringified:
+                  $ref: "#/components/schemas/map_allof_oneof_stringified"
+
   /set_test:
     get:
       summary: Test for SetNested attributes and Set attributes in a data source
@@ -213,3 +218,33 @@ components:
           bool_prop:
             description: Bool inside a map!
             type: boolean
+    mapnested_allof_oneof_nullable:
+      description: This is a map with a nullable object
+      type: object
+      additionalProperties:
+        allOf:
+        - $ref: "#/components/schemas/nullable_object"
+    map_allof_oneof_stringified:
+      description: This is a map with a stringifed value
+      type: object
+      additionalProperties:
+        allOf:
+        - $ref: "#/components/schemas/stringable_number"
+    stringable_number:
+      oneOf:
+      - type: string
+      - type: number
+    nullable_object:
+      oneOf:
+      - type: object
+        required:
+        - string_prop
+        - bool_prop
+        properties:
+          string_prop:
+            description: String inside a map!
+            type: string
+          bool_prop:
+            description: Bool inside a map!
+            type: boolean
+      - type: "null"
diff --git a/internal/cmd/testdata/edgecase/provider_code_spec.json b/internal/cmd/testdata/edgecase/provider_code_spec.json
@@ -279,6 +279,41 @@
 							"description": "This is a map of strings"
 						}
 					},
+					{
+						"name": "map_stringified",
+						"map": {
+							"computed_optional_required": "computed_optional",
+							"element_type": {
+								"string": {}
+							},
+							"description": "This is a map with a stringifed value"
+						}
+					},
+					{
+						"name": "mapnested_nullable",
+						"map_nested": {
+							"computed_optional_required": "computed_optional",
+							"nested_object": {
+								"attributes": [
+									{
+										"name": "bool_prop",
+										"bool": {
+											"computed_optional_required": "required",
+											"description": "Bool inside a map!"
+										}
+									},
+									{
+										"name": "string_prop",
+										"string": {
+											"computed_optional_required": "required",
+											"description": "String inside a map!"
+										}
+									}
+								]
+							},
+							"description": "This is a map with a nullable object"
+						}
+					},
 					{
 						"name": "mapnested_prop",
 						"map_nested": {

diff --git a/internal/cmd/testdata/kubernetes/provider_code_spec.json b/internal/cmd/testdata/kubernetes/provider_code_spec.json
@@ -2540,15 +2540,21 @@
 																					},
 																					{
 																						"name": "limits",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					},
 																					{
 																						"name": "requests",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					}
@@ -4146,15 +4152,21 @@
 																					},
 																					{
 																						"name": "limits",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					},
 																					{
 																						"name": "requests",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					}
@@ -5773,15 +5785,21 @@
 																					},
 																					{
 																						"name": "limits",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					},
 																					{
 																						"name": "requests",
-																						"single_nested": {
+																						"map": {
 																							"computed_optional_required": "computed_optional",
+																							"element_type": {
+																								"string": {}
+																							},
 																							"description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																						}
 																					}
@@ -6345,8 +6363,11 @@
 														},
 														{
 															"name": "overhead",
-															"single_nested": {
+															"map": {
 																"computed_optional_required": "computed_optional",
+																"element_type": {
+																	"string": {}
+																},
 																"description": "Overhead represents the resource overhead associated with running a pod for a given RuntimeClass. This field will be autopopulated at admission time by the RuntimeClass admission controller. If the RuntimeClass admission controller is enabled, overhead must not be set in Pod create requests. The RuntimeClass admission controller will reject Pod create requests which have the overhead already set. If RuntimeClass is configured and selected in the PodSpec, Overhead will be set to the value defined in the corresponding RuntimeClass, otherwise it will remain unset and treated as zero. More info: https://git.k8s.io/enhancements/keps/sig-node/688-pod-overhead/README.md"
 															}
 														},
@@ -7729,15 +7750,21 @@
 																													"attributes": [
 																														{
 																															"name": "limits",
-																															"single_nested": {
+																															"map": {
 																																"computed_optional_required": "computed_optional",
+																																"element_type": {
+																																	"string": {}
+																																},
 																																"description": "Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																															}
 																														},
 																														{
 																															"name": "requests",
-																															"single_nested": {
+																															"map": {
 																																"computed_optional_required": "computed_optional",
+																																"element_type": {
+																																	"string": {}
+																																},
 																																"description": "Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. Requests cannot exceed Limits. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"
 																															}
 																														}

diff --git a/internal/config/parse.go b/internal/config/parse.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"regexp"
 
-	"github.com/pb33f/libopenapi/index"
 	"gopkg.in/yaml.v3"
 )
 
@@ -139,11 +138,6 @@ func (p Provider) Validate() error {
 		result = errors.Join(result, errors.New("must have a 'name' property"))
 	}
 
-	// All schema refs must be a local, file, or http resolve type
-	if p.SchemaRef != "" && index.DetermineReferenceResolveType(p.SchemaRef) < 0 {
-		result = errors.Join(result, errors.New("'schema_ref' must be a valid JSON schema reference"))
-	}
-
 	for _, ignore := range p.Ignores {
 		if !attributeLocationRegex.MatchString(ignore) {
 			result = errors.Join(result, fmt.Errorf("invalid item for ignores: %q - must be dot-separated string", ignore))

diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go
@@ -213,19 +213,6 @@ func TestParseConfig_Invalid(t *testing.T) {
 			input:            ``,
 			expectedErrRegex: `provider must have a 'name' property`,
 		},
-		"provider - invalid schema_ref - not resolvable": {
-			input: `
-provider:
-  name: example
-  schema_ref: thisaintvalid
-
-data_sources:
-  thing_one:
-    read:
-      path: /example/path/to/thing/{id}
-      method: GET`,
-			expectedErrRegex: `provider 'schema_ref' must be a valid JSON schema reference`,
-		},
 		"provider - invalid ignore item": {
 			input: `
 provider:

diff --git a/internal/explorer/config_explorer.go b/internal/explorer/config_explorer.go
@@ -4,6 +4,7 @@
 package explorer
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"strings"
@@ -157,7 +158,7 @@ func extractSchemaProxy(document high.Document, componentRef string) (*highbase.
 	}
 
 	// populate low-level schema, using root document.Index for resolving
-	err = lowSchema.Build(indexRef.Node, document.Index)
+	err = lowSchema.Build(context.Background(), indexRef.Node, document.Index)
 	if err != nil {
 		return nil, fmt.Errorf("error populating low-level schema: %w", err)
 	}

diff --git a/internal/mapper/datasource_mapper_test.go b/internal/mapper/datasource_mapper_test.go
@@ -722,18 +722,20 @@ func TestDataSourceMapper_basic_merges(t *testing.T) {
 					"nested_map": base.CreateSchemaProxy(&base.Schema{
 						Type:        []string{"object"},
 						Description: "hey this is a map!",
-						AdditionalProperties: base.CreateSchemaProxy(&base.Schema{
-							Type: []string{"object"},
-							Properties: map[string]*base.SchemaProxy{
-								"deep_nested_bool": base.CreateSchemaProxy(&base.Schema{
-									Type: []string{"boolean"},
-								}),
-								"deep_nested_int64": base.CreateSchemaProxy(&base.Schema{
-									Type:        []string{"integer"},
-									Description: "hey this is an int64!",
-								}),
-							},
-						}),
+						AdditionalProperties: &base.DynamicValue[*base.SchemaProxy, bool]{
+							A: base.CreateSchemaProxy(&base.Schema{
+								Type: []string{"object"},
+								Properties: map[string]*base.SchemaProxy{
+									"deep_nested_bool": base.CreateSchemaProxy(&base.Schema{
+										Type: []string{"boolean"},
+									}),
+									"deep_nested_int64": base.CreateSchemaProxy(&base.Schema{
+										Type:        []string{"integer"},
+										Description: "hey this is an int64!",
+									}),
+								},
+							}),
+						},
 					}),
 				},
 			}),