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

Fix map handling with allOf/oneOf + upgrade pb33f/libopenapi #101

Merged
merged 7 commits into from
Dec 13, 2023
Merged

Fix map handling with allOf/oneOf + upgrade pb33f/libopenapi #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec58ce2
initial behavior
austinvalle Dec 12, 2023
84e0135
remove validation for provider ref, as it doesn't exist upstream anymore
austinvalle Dec 12, 2023
597e1c2
add context from signature change
austinvalle Dec 12, 2023
adcc600
initial fix
austinvalle Dec 13, 2023
aa02667
add a nested test
austinvalle Dec 13, 2023
6f3aedf
update comment
austinvalle Dec 13, 2023
584b878
add changelog
austinvalle Dec 13, 2023
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
7 changes: 7 additions & 0 deletions .changes/unreleased/BUG FIXES-20231213-145019.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
6 changes: 3 additions & 3 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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
)
12 changes: 6 additions & 6 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down Expand Up @@ -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=
Expand All @@ -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=
Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
Expand Down
35 changes: 35 additions & 0 deletions internal/cmd/testdata/edgecase/openapi_spec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
35 changes: 35 additions & 0 deletions internal/cmd/testdata/edgecase/provider_code_spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
45 changes: 36 additions & 9 deletions internal/cmd/testdata/kubernetes/provider_code_spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/"
}
}
Expand Down Expand Up @@ -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/"
}
}
Expand Down Expand Up @@ -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/"
}
}
Expand Down Expand Up @@ -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"
}
},
Expand Down Expand Up @@ -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/"
}
}
Expand Down
6 changes: 0 additions & 6 deletions internal/config/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"fmt"
"regexp"

"github.com/pb33f/libopenapi/index"
"gopkg.in/yaml.v3"
)

Expand Down Expand Up @@ -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"))
}

Comment on lines -142 to -146
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only removed functionality from upgrading. The index package in pb33f/libopenapi has been significantly refactored and I can't find an equivalent function for this.

In reality, this validation wasn't super useful anyways, since it never actually checked the spec for the existence of a reference, it just validates that the reference "could" be valid via the syntax.

If we want to replace this validation in the future, we should probably just load the OpenAPI spec provided and check if the reference exists in the spec.

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))
Expand Down
13 changes: 0 additions & 13 deletions internal/config/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion internal/explorer/config_explorer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package explorer

import (
"context"
"errors"
"fmt"
"strings"
Expand Down Expand Up @@ -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)
}
Expand Down
26 changes: 14 additions & 12 deletions internal/mapper/datasource_mapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]{
Copy link
Member Author

@austinvalle austinvalle Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The major breaking change was the structure of the AdditionalProperties field, which has been changed from an any to a strongly typed value! No more type assertions 🥳

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!",
}),
},
}),
},
}),
},
}),
Expand Down
Loading