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

yaml unmarshal for OpenAPIv2 types #279

Closed
wants to merge 1 commit into from
Closed

yaml unmarshal for OpenAPIv2 types #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions pkg/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ limitations under the License.
package util

import (
"fmt"
"reflect"
"strings"

"gopkg.in/yaml.v3"
)

// [DEPRECATED] ToCanonicalName converts Golang package/type canonical name into REST friendly OpenAPI name.
Expand Down Expand Up @@ -108,3 +111,21 @@ func GetCanonicalTypeName(model interface{}) string {
}
return path + "." + t.Name()
}

// Provides a fast path for decoding YAML scalar node as a string
// If the node's value can be simply returned directly, then it is. Otherwise,
// the yaml.v3.Node.Decode slow path is taken
func DecodeYAMLString(n *yaml.Node, s *string) error {
Copy link
Member

Choose a reason for hiding this comment

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

What's the performance difference with this change? I'm curious if this might be better committed in the YAML library rather than here since there isn't any k8s specific logic.

/cc @apelisse

Copy link
Member

Choose a reason for hiding this comment

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

+1 on not putting general-purpose yaml decoding in this lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that it is not desirable long-term. I will prepare a PR for upstream. However, I do not see an alternative in the short term? It will likely take a while for the change to be merged and appear in a tagged release.

Copy link
Contributor Author

@alexzielenski alexzielenski Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Jefftree The difference with this change is significant. Below are benchmark results for openapi v2 with this optimization removed:

goos: darwin
goarch: amd64
pkg: github.com/alexzielenski/parsebench
cpu: Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
BenchmarkFastConversion
BenchmarkFastConversion/json->swagger
BenchmarkFastConversion/json->swagger-8         	       2	 503133747 ns/op	95739432 B/op	 1381637 allocs/op
BenchmarkFastConversion/swagger->json
BenchmarkFastConversion/swagger->json-8         	       9	 122539288 ns/op	66432463 B/op	  274050 allocs/op
BenchmarkFastConversion/json->gnostic
BenchmarkFastConversion/json->gnostic-8         	       5	 225056310 ns/op	81462545 B/op	 1248402 allocs/op
BenchmarkFastConversion/gnostic->pb
BenchmarkFastConversion/gnostic->pb-8           	     100	  11495301 ns/op	 2899970 B/op	       1 allocs/op
BenchmarkFastConversion/pb->gnostic
BenchmarkFastConversion/pb->gnostic-8           	     100	  13719325 ns/op	 9480698 B/op	  123829 allocs/op
BenchmarkFastConversion/gnostic->yaml
BenchmarkFastConversion/gnostic->yaml-8         	      40	  30027913 ns/op	32855169 B/op	  264562 allocs/op
BenchmarkFastConversion/yaml->swagger
BenchmarkFastConversion/yaml->swagger-8         	      13	  92594648 ns/op	33392264 B/op	  675799 allocs/op

~93ms in this run on my machine. Over a number of runs i saw a range of 80-100ms for yaml->swagger.

This is compared to ~60ms with the optimization enabled.

if n.Kind != yaml.ScalarNode {
return fmt.Errorf("expected scalarnode, not %v", n.Kind)
}

if (n.Tag == "!!str" || n.Tag == "tag:yaml.org,2002:!!string") ||
(n.Tag == "" || n.Tag == "!") && n.Style&(yaml.SingleQuotedStyle|yaml.DoubleQuotedStyle|yaml.LiteralStyle|yaml.FoldedStyle) != 0 {
Comment on lines +123 to +124
Copy link
Member

Choose a reason for hiding this comment

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

is this copied from something in the yaml library? I'm not familiar with what this means or if it is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from yaml decoding logic for string path:

https://github.com/go-yaml/yaml/blob/496545a6307b2a7d7a710fd516e5e16e8ab62dbc/yaml.go#L463
https://github.com/go-yaml/yaml/blob/496545a6307b2a7d7a710fd516e5e16e8ab62dbc/decode.go#L565

*s = n.Value
return nil
}

// Use slow path if it is not a basic string
return n.Decode(&s)
}
6 changes: 3 additions & 3 deletions pkg/validation/spec/contact_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ package spec
//
// For more information: http://goo.gl/8us55a#contactObject
type ContactInfo struct {
Name string `json:"name,omitempty"`
URL string `json:"url,omitempty"`
Email string `json:"email,omitempty"`
Name string `json:"name,omitempty" yaml:"name,omitempty"`
URL string `json:"url,omitempty" yaml:"url,omitempty"`
Email string `json:"email,omitempty" yaml:"email,omitempty"`
Comment on lines +21 to +23
Copy link
Member

Choose a reason for hiding this comment

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

I really don't think we should start adding yaml tags to our API types. Having benchmarked the yaml decoding in the past, I'm also really surprised it is faster than the json decoding (it was generally significantly slower in the past). Do we know which bits the json decoding is super slow on?

Copy link
Contributor Author

@alexzielenski alexzielenski Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yaml package since v3 released 2019 operates on an AST; did you look before then?

My benchmarks measured unmarshalling from the AST type instead of text. This is sufficient for my use case of providing a path to convert from protobuf: protoubf -> google/gnostic -> kube-openapi. For me it is important that this conversion runs at interactive speeds (for a CLI tool)

I haven't benchmarked it, but I expect going from YAML text -> AST -> Kube-Openapi would be comparable/slower than JSON.

Copy link
Member

@liggitt liggitt Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we want a google/gnostic → kube-openapi path, it would make more sense to me to build that transformation and round-trip test it. Using yaml AST bits is clever, but I don't think we should decorate API types with yaml tags and push consumers in that direction

Copy link

@natasha41575 natasha41575 Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should decorate API types with yaml tags and push consumers in that direction

I'm curious why you're against having yaml tags here? The proto->gnostic->kube-openapi conversion that this facilitates is a critical performance improvement that kpt and kustomize would like to have as soon as possible, and I'm not sure that I understand the disadvantages of adding yaml tags.

Copy link
Member

Choose a reason for hiding this comment

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

We have an alternative solution that is probably going to be even faster, but it will take a few weeks to implement. This is also a blocker for next code-freeze so we're trying to move fast on that!

Copy link

Choose a reason for hiding this comment

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

Thank you so much for the update!

}
4 changes: 2 additions & 2 deletions pkg/validation/spec/external_docs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ package spec
//
// For more information: http://goo.gl/8us55a#externalDocumentationObject
type ExternalDocumentation struct {
Description string `json:"description,omitempty"`
URL string `json:"url,omitempty"`
Description string `json:"description,omitempty" yaml:"description,omitempty"`
URL string `json:"url,omitempty" yaml:"url,omitempty"`
}
17 changes: 16 additions & 1 deletion pkg/validation/spec/header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"

"github.com/go-openapi/swag"
"gopkg.in/yaml.v3"
)

const (
Expand All @@ -26,7 +27,7 @@ const (

// HeaderProps describes a response header
type HeaderProps struct {
Description string `json:"description,omitempty"`
Description string `json:"description,omitempty" yaml:"description,omitempty"`
}

// Header describes a header for a response of the API
Expand Down Expand Up @@ -69,3 +70,17 @@ func (h *Header) UnmarshalJSON(data []byte) error {
}
return json.Unmarshal(data, &h.HeaderProps)
}

// UnmarshalJSON unmarshals this header from JSON
func (h *Header) UnmarshalYAML(value *yaml.Node) error {
if err := value.Decode(&h.CommonValidations); err != nil {
return err
}
if err := value.Decode(&h.SimpleSchema); err != nil {
return err
}
if err := value.Decode(&h.VendorExtensible); err != nil {
return err
}
return value.Decode(&h.HeaderProps)
}
51 changes: 45 additions & 6 deletions pkg/validation/spec/info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ package spec

import (
"encoding/json"
"errors"
"strings"

"github.com/go-openapi/swag"
"gopkg.in/yaml.v3"
"k8s.io/kube-openapi/pkg/util"
)

// Extensions vendor specific extensions
Expand Down Expand Up @@ -133,14 +136,43 @@ func (v *VendorExtensible) UnmarshalJSON(data []byte) error {
return nil
}

func (v *VendorExtensible) UnmarshalYAML(value *yaml.Node) error {
// var d map[string]interface{}
Copy link
Member

Choose a reason for hiding this comment

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

Is this line needed?

if value.Kind != yaml.MappingNode {
return errors.New("expected mappingNode")
} else if len(value.Content)%2 != 0 {
return errors.New("expected even child nodes")
}

for i := 0; i < len(value.Content); i += 2 {
var keyStr string
if err := util.DecodeYAMLString(value.Content[i], &keyStr); err != nil {
return err
}

if strings.HasPrefix(keyStr, "x-") || strings.HasPrefix(keyStr, "X-") {
Copy link
Member

Choose a reason for hiding this comment

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

Is the capitalization check capturing an edge case or do we already have places where X- is passed in?

Copy link
Contributor Author

@alexzielenski alexzielenski Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This extra check for "X-" is here only to mirror the UnmarshalJSON behavior. I think you wrote that ;)

(Unmarshal JSON calls lk := strings.ToLower(k) before making the comparison)

if v.Extensions == nil {
v.Extensions = map[string]interface{}{}
}

var decodedVal interface{}
if err := value.Content[i+1].Decode(&decodedVal); err != nil {
return err
}
v.Extensions[strings.ToLower(keyStr)] = decodedVal
}
}
return nil
}

// InfoProps the properties for an info definition
type InfoProps struct {
Description string `json:"description,omitempty"`
Title string `json:"title,omitempty"`
TermsOfService string `json:"termsOfService,omitempty"`
Contact *ContactInfo `json:"contact,omitempty"`
License *License `json:"license,omitempty"`
Version string `json:"version,omitempty"`
Description string `json:"description,omitempty" yaml:"description,omitempty"`
Title string `json:"title,omitempty" yaml:"title,omitempty"`
TermsOfService string `json:"termsOfService,omitempty" yaml:"termsOfService,omitempty"`
Contact *ContactInfo `json:"contact,omitempty" yaml:"contact,omitempty"`
License *License `json:"license,omitempty" yaml:"license,omitempty"`
Version string `json:"version,omitempty" yaml:"version,omitempty"`
}

// Info object provides metadata about the API.
Expand Down Expand Up @@ -172,3 +204,10 @@ func (i *Info) UnmarshalJSON(data []byte) error {
}
return json.Unmarshal(data, &i.VendorExtensible)
}

func (i *Info) UnmarshalYAML(value *yaml.Node) error {
if err := value.Decode(&i.InfoProps); err != nil {
return err
}
return i.VendorExtensible.UnmarshalYAML(value)
}
58 changes: 39 additions & 19 deletions pkg/validation/spec/items.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"

"github.com/go-openapi/swag"
"gopkg.in/yaml.v3"
)

const (
Expand All @@ -26,29 +27,29 @@ const (

// SimpleSchema describe swagger simple schemas for parameters and headers
type SimpleSchema struct {
Type string `json:"type,omitempty"`
Nullable bool `json:"nullable,omitempty"`
Format string `json:"format,omitempty"`
Items *Items `json:"items,omitempty"`
CollectionFormat string `json:"collectionFormat,omitempty"`
Default interface{} `json:"default,omitempty"`
Example interface{} `json:"example,omitempty"`
Type string `json:"type,omitempty" yaml:"type,omitempty"`
Nullable bool `json:"nullable,omitempty" yaml:"nullable,omitempty"`
Format string `json:"format,omitempty" yaml:"format,omitempty"`
Items *Items `json:"items,omitempty" yaml:"items,omitempty"`
CollectionFormat string `json:"collectionFormat,omitempty" yaml:"collectionFormat,omitempty"`
Default interface{} `json:"default,omitempty" yaml:"default,omitempty"`
Example interface{} `json:"example,omitempty" yaml:"example,omitempty"`
}

// CommonValidations describe common JSON-schema validations
type CommonValidations struct {
Maximum *float64 `json:"maximum,omitempty"`
ExclusiveMaximum bool `json:"exclusiveMaximum,omitempty"`
Minimum *float64 `json:"minimum,omitempty"`
ExclusiveMinimum bool `json:"exclusiveMinimum,omitempty"`
MaxLength *int64 `json:"maxLength,omitempty"`
MinLength *int64 `json:"minLength,omitempty"`
Pattern string `json:"pattern,omitempty"`
MaxItems *int64 `json:"maxItems,omitempty"`
MinItems *int64 `json:"minItems,omitempty"`
UniqueItems bool `json:"uniqueItems,omitempty"`
MultipleOf *float64 `json:"multipleOf,omitempty"`
Enum []interface{} `json:"enum,omitempty"`
Maximum *float64 `json:"maximum,omitempty" yaml:"maximum,omitempty"`
ExclusiveMaximum bool `json:"exclusiveMaximum,omitempty" yaml:"exclusiveMaximum,omitempty"`
Minimum *float64 `json:"minimum,omitempty" yaml:"minimum,omitempty"`
ExclusiveMinimum bool `json:"exclusiveMinimum,omitempty" yaml:"exclusiveMinimum,omitempty"`
MaxLength *int64 `json:"maxLength,omitempty" yaml:"maxLength,omitempty"`
MinLength *int64 `json:"minLength,omitempty" yaml:"minLength,omitempty"`
Pattern string `json:"pattern,omitempty" yaml:"pattern,omitempty"`
MaxItems *int64 `json:"maxItems,omitempty" yaml:"maxItems,omitempty"`
MinItems *int64 `json:"minItems,omitempty" yaml:"minItems,omitempty"`
UniqueItems bool `json:"uniqueItems,omitempty" yaml:"uniqueItems,omitempty"`
MultipleOf *float64 `json:"multipleOf,omitempty" yaml:"multipleOf,omitempty"`
Enum []interface{} `json:"enum,omitempty" yaml:"enum,omitempty"`
}

// Items a limited subset of JSON-Schema's items object.
Expand All @@ -62,6 +63,25 @@ type Items struct {
VendorExtensible
}

func (i *Items) UnmarshalYAML(value *yaml.Node) error {
if err := value.Decode(&i.CommonValidations); err != nil {
return err
}

if err := value.Decode(&i.Refable); err != nil {
return err
}

if err := value.Decode(&i.SimpleSchema); err != nil {
return err
}

if err := i.VendorExtensible.UnmarshalYAML(value); err != nil {
return err
}
return nil
}

// UnmarshalJSON hydrates this items instance with the data from JSON
func (i *Items) UnmarshalJSON(data []byte) error {
var validations CommonValidations
Expand Down
4 changes: 2 additions & 2 deletions pkg/validation/spec/license.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ package spec
//
// For more information: http://goo.gl/8us55a#licenseObject
type License struct {
Name string `json:"name,omitempty"`
URL string `json:"url,omitempty"`
Name string `json:"name,omitempty" yaml:"name,omitempty"`
URL string `json:"url,omitempty" yaml:"url,omitempty"`
}
36 changes: 22 additions & 14 deletions pkg/validation/spec/operation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"

"github.com/go-openapi/swag"
"gopkg.in/yaml.v3"
)

// OperationProps describes an operation
Expand All @@ -26,18 +27,18 @@ import (
// - schemes, when present must be from [http, https, ws, wss]: see validate
// - Security is handled as a special case: see MarshalJSON function
type OperationProps struct {
Description string `json:"description,omitempty"`
Consumes []string `json:"consumes,omitempty"`
Produces []string `json:"produces,omitempty"`
Schemes []string `json:"schemes,omitempty"`
Tags []string `json:"tags,omitempty"`
Summary string `json:"summary,omitempty"`
ExternalDocs *ExternalDocumentation `json:"externalDocs,omitempty"`
ID string `json:"operationId,omitempty"`
Deprecated bool `json:"deprecated,omitempty"`
Security []map[string][]string `json:"security,omitempty"`
Parameters []Parameter `json:"parameters,omitempty"`
Responses *Responses `json:"responses,omitempty"`
Description string `json:"description,omitempty" yaml:"description,omitempty"`
Consumes []string `json:"consumes,omitempty" yaml:"consumes,omitempty"`
Produces []string `json:"produces,omitempty" yaml:"produces,omitempty"`
Schemes []string `json:"schemes,omitempty" yaml:"schemes,omitempty"`
Tags []string `json:"tags,omitempty" yaml:"tags,omitempty"`
Summary string `json:"summary,omitempty" yaml:"summary,omitempty"`
ExternalDocs *ExternalDocumentation `json:"externalDocs,omitempty" yaml:"externalDocs,omitempty"`
ID string `json:"operationId,omitempty" yaml:"operationId,omitempty"`
Deprecated bool `json:"deprecated,omitempty" yaml:"deprecated,omitempty"`
Security []map[string][]string `json:"security,omitempty" yaml:"security,omitempty"`
Parameters []Parameter `json:"parameters,omitempty" yaml:"parameters,omitempty"`
Responses *Responses `json:"responses,omitempty" yaml:"responses,omitempty"`
}

// MarshalJSON takes care of serializing operation properties to JSON
Expand All @@ -49,15 +50,15 @@ func (op OperationProps) MarshalJSON() ([]byte, error) {
type Alias OperationProps
if op.Security == nil {
return json.Marshal(&struct {
Security []map[string][]string `json:"security,omitempty"`
Security []map[string][]string `json:"security,omitempty" yaml:"security,omitempty"`
*Alias
}{
Security: op.Security,
Alias: (*Alias)(&op),
})
}
return json.Marshal(&struct {
Security []map[string][]string `json:"security"`
Security []map[string][]string `json:"security" yaml:"security"`
*Alias
}{
Security: op.Security,
Expand All @@ -73,6 +74,13 @@ type Operation struct {
OperationProps
}

func (o *Operation) UnmarshalYAML(value *yaml.Node) error {
if err := value.Decode(&o.OperationProps); err != nil {
return err
}
return o.VendorExtensible.UnmarshalYAML(value)
}

// UnmarshalJSON hydrates this items instance with the data from JSON
func (o *Operation) UnmarshalJSON(data []byte) error {
if err := json.Unmarshal(data, &o.OperationProps); err != nil {
Expand Down
29 changes: 23 additions & 6 deletions pkg/validation/spec/parameter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"encoding/json"

"github.com/go-openapi/swag"
"gopkg.in/yaml.v3"
)

// ParamProps describes the specific attributes of an operation parameter
Expand All @@ -26,12 +27,12 @@ import (
// - Schema is defined when "in" == "body": see validate
// - AllowEmptyValue is allowed where "in" == "query" || "formData"
type ParamProps struct {
Description string `json:"description,omitempty"`
Name string `json:"name,omitempty"`
In string `json:"in,omitempty"`
Required bool `json:"required,omitempty"`
Schema *Schema `json:"schema,omitempty"`
AllowEmptyValue bool `json:"allowEmptyValue,omitempty"`
Description string `json:"description,omitempty" yaml:"description,omitempty"`
Name string `json:"name,omitempty" yaml:"name,omitempty"`
In string `json:"in,omitempty" yaml:"in,omitempty"`
Required bool `json:"required,omitempty" yaml:"required,omitempty"`
Schema *Schema `json:"schema,omitempty" yaml:"schema,omitempty"`
AllowEmptyValue bool `json:"allowEmptyValue,omitempty" yaml:"allowEmptyValue,omitempty"`
}

// Parameter a unique parameter is defined by a combination of a [name](#parameterName) and [location](#parameterIn).
Expand Down Expand Up @@ -85,6 +86,22 @@ func (p *Parameter) UnmarshalJSON(data []byte) error {
return json.Unmarshal(data, &p.ParamProps)
}

func (p *Parameter) UnmarshalYAML(value *yaml.Node) error {
if err := value.Decode(&p.CommonValidations); err != nil {
return err
}
if err := value.Decode(&p.Refable); err != nil {
return err
}
if err := value.Decode(&p.SimpleSchema); err != nil {
return err
}
if err := p.VendorExtensible.UnmarshalYAML(value); err != nil {
return err
}
return value.Decode(&p.ParamProps)
}

// MarshalJSON converts this items object to JSON
func (p Parameter) MarshalJSON() ([]byte, error) {
b1, err := json.Marshal(p.CommonValidations)
Expand Down
Loading