From 0c8d4b97df35193cb422b7293cb7658ffa91e0c2 Mon Sep 17 00:00:00 2001 From: Cari <49043580+cari-lynn@users.noreply.github.com> Date: Wed, 17 Nov 2021 15:41:30 -0800 Subject: [PATCH] `@schema/desc` adds `description` key to OpenAPI Doc (#545) - Reordered the keys so description shows before map and array items for readability. * Add concept of documentation annotations Co-authored-by: John S. Ryan --- pkg/cmd/template/schema_author_test.go | 107 +++++++++++++++++ pkg/cmd/template/schema_inspect_test.go | 79 +++++++++++++ pkg/schema/annotations.go | 73 ++++++++++++ pkg/schema/openapi.go | 52 +++++++-- pkg/schema/schema.go | 9 ++ pkg/schema/type.go | 145 +++++++++++++++++++----- pkg/yamlmeta/type.go | 2 + 7 files changed, 427 insertions(+), 40 deletions(-) diff --git a/pkg/cmd/template/schema_author_test.go b/pkg/cmd/template/schema_author_test.go index 0312326c..27399736 100644 --- a/pkg/cmd/template/schema_author_test.go +++ b/pkg/cmd/template/schema_author_test.go @@ -475,6 +475,113 @@ schema.yml: }) }) }) + t.Run("when schema/desc annotation value", func(t *testing.T) { + t.Run("is empty", func(t *testing.T) { + schemaYAML := `#@data/values-schema +--- +#@schema/desc +key: val +` + expectedErr := ` +Invalid schema +============== +syntax error in @schema/desc annotation + +schema.yml: + | + 4 | key: val + | + + = found: missing value (in @schema/desc above this item) + = expected: string +` + + filesToProcess := files.NewSortedFiles([]*files.File{ + files.MustNewFileFromSource(files.NewBytesSource("schema.yml", []byte(schemaYAML))), + }) + + assertFails(t, filesToProcess, expectedErr, opts) + }) + t.Run("has more than one arg", func(t *testing.T) { + schemaYAML := `#@data/values-schema +--- +#@schema/desc "two", "strings" +key: val +` + expectedErr := ` +Invalid schema +============== +syntax error in @schema/desc annotation + +schema.yml: + | + 4 | key: val + | + + = found: 2 values (in @schema/desc above this item) + = expected: string +` + + filesToProcess := files.NewSortedFiles([]*files.File{ + files.MustNewFileFromSource(files.NewBytesSource("schema.yml", []byte(schemaYAML))), + }) + + assertFails(t, filesToProcess, expectedErr, opts) + }) + t.Run("is not a string", func(t *testing.T) { + schemaYAML := `#@data/values-schema +--- +#@schema/desc 1 +key: val +` + expectedErr := ` +Invalid schema +============== +syntax error in @schema/desc annotation + +schema.yml: + | + 4 | key: val + | + + = found: Non-string value (in @schema/desc above this item) + = expected: string +` + + filesToProcess := files.NewSortedFiles([]*files.File{ + files.MustNewFileFromSource(files.NewBytesSource("schema.yml", []byte(schemaYAML))), + }) + + assertFails(t, filesToProcess, expectedErr, opts) + }) + t.Run("is key=value pair", func(t *testing.T) { + schemaYAML := `#@data/values-schema +--- +#@schema/desc key=True +key: val +` + expectedErr := ` +Invalid schema +============== +syntax error in @schema/desc annotation + +schema.yml: + | + 4 | key: val + | + + = found: keyword argument (in @schema/desc above this item) + = expected: string + = hint: this annotation only accepts one argument: a string. +` + + filesToProcess := files.NewSortedFiles([]*files.File{ + files.MustNewFileFromSource(files.NewBytesSource("schema.yml", []byte(schemaYAML))), + }) + + assertFails(t, filesToProcess, expectedErr, opts) + }) + }) } func TestSchema_Provides_default_values(t *testing.T) { diff --git a/pkg/cmd/template/schema_inspect_test.go b/pkg/cmd/template/schema_inspect_test.go index e66b1ea9..a9630a22 100644 --- a/pkg/cmd/template/schema_inspect_test.go +++ b/pkg/cmd/template/schema_inspect_test.go @@ -545,6 +545,85 @@ components: } +func TestSchemaInspect_annotation_adds_key(t *testing.T) { + t.Run("when description provided by @schema/desc", func(t *testing.T) { + opts := cmdtpl.NewOptions() + opts.DataValuesFlags.InspectSchema = true + opts.RegularFilesSourceOpts.OutputType.Types = []string{"openapi-v3"} + + schemaYAML := `#@data/values-schema +#@schema/desc "Network configuration values" +--- +#@schema/desc "List of database connections" +db_conn: +#@schema/desc "A network entry" +- + #@schema/desc "The hostname" + hostname: "" + #@schema/desc "Port should be between 49152 through 65535" + port: 0 + #@schema/desc "Timeout in minutes" + timeout: 1.0 + #@schema/desc "Any type is allowed" + #@schema/type any=True + any_key: thing + #@schema/desc "When not provided, the default is null" + #@schema/nullable + null_key: "" +` + expected := `openapi: 3.0.0 +info: + version: 0.1.0 + title: Schema for data values, generated by ytt +paths: {} +components: + schemas: + dataValues: + type: object + additionalProperties: false + description: Network configuration values + properties: + db_conn: + type: array + description: List of database connections + items: + type: object + additionalProperties: false + description: A network entry + properties: + hostname: + type: string + default: "" + description: The hostname + port: + type: integer + default: 0 + description: Port should be between 49152 through 65535 + timeout: + type: number + default: 1 + format: float + description: Timeout in minutes + any_key: + nullable: true + default: thing + description: Any type is allowed + null_key: + type: string + default: null + nullable: true + description: When not provided, the default is null + default: [] +` + + filesToProcess := files.NewSortedFiles([]*files.File{ + files.MustNewFileFromSource(files.NewBytesSource("schema.yml", []byte(schemaYAML))), + }) + + assertSucceedsDocSet(t, filesToProcess, expected, opts) + }) +} + func TestSchemaInspect_errors(t *testing.T) { t.Run("when --output is anything other than 'openapi-v3'", func(t *testing.T) { opts := cmdtpl.NewOptions() diff --git a/pkg/schema/annotations.go b/pkg/schema/annotations.go index 481816a8..f73e9c2f 100644 --- a/pkg/schema/annotations.go +++ b/pkg/schema/annotations.go @@ -15,6 +15,7 @@ const ( AnnotationNullable template.AnnotationName = "schema/nullable" AnnotationType template.AnnotationName = "schema/type" AnnotationDefault template.AnnotationName = "schema/default" + AnnotationDescription template.AnnotationName = "schema/desc" TypeAnnotationKwargAny string = "any" ) @@ -36,6 +37,11 @@ type DefaultAnnotation struct { val interface{} } +// DescriptionAnnotation documents the purpose of a node +type DescriptionAnnotation struct { + description string +} + // NewTypeAnnotation checks the keyword argument provided via @schema/type annotation, and returns wrapper for the annotated node. func NewTypeAnnotation(ann template.NodeAnnotation, node yamlmeta.Node) (*TypeAnnotation, error) { if len(ann.Kwargs) == 0 { @@ -128,6 +134,46 @@ func NewDefaultAnnotation(ann template.NodeAnnotation, effectiveType yamlmeta.Ty return &DefaultAnnotation{yamlmeta.NewASTFromInterfaceWithPosition(val, pos)}, nil } +// NewDescriptionAnnotation validates the value from the AnnotationDescription, and returns the value +func NewDescriptionAnnotation(ann template.NodeAnnotation, pos *filepos.Position) (*DescriptionAnnotation, error) { + if len(ann.Kwargs) != 0 { + return nil, schemaAssertionError{ + position: pos, + description: fmt.Sprintf("syntax error in @%v annotation", AnnotationDescription), + expected: fmt.Sprintf("string"), + found: fmt.Sprintf("keyword argument (in @%v above this item)", AnnotationDescription), + hints: []string{"this annotation only accepts one argument: a string."}, + } + } + switch numArgs := len(ann.Args); { + case numArgs == 0: + return nil, schemaAssertionError{ + position: pos, + description: fmt.Sprintf("syntax error in @%v annotation", AnnotationDescription), + expected: fmt.Sprintf("string"), + found: fmt.Sprintf("missing value (in @%v above this item)", AnnotationDescription), + } + case numArgs > 1: + return nil, schemaAssertionError{ + position: pos, + description: fmt.Sprintf("syntax error in @%v annotation", AnnotationDescription), + expected: fmt.Sprintf("string"), + found: fmt.Sprintf("%v values (in @%v above this item)", numArgs, AnnotationDescription), + } + } + + strVal, err := core.NewStarlarkValue(ann.Args[0]).AsString() + if err != nil { + return nil, schemaAssertionError{ + position: pos, + description: fmt.Sprintf("syntax error in @%v annotation", AnnotationDescription), + expected: fmt.Sprintf("string"), + found: fmt.Sprintf("Non-string value (in @%v above this item)", AnnotationDescription), + } + } + return &DescriptionAnnotation{strVal}, nil +} + // NewTypeFromAnn returns type information given by annotation. func (t *TypeAnnotation) NewTypeFromAnn() (yamlmeta.Type, error) { if t.any { @@ -150,6 +196,11 @@ func (n *DefaultAnnotation) NewTypeFromAnn() (yamlmeta.Type, error) { return nil, nil } +// NewTypeFromAnn returns type information given by annotation. DescriptionAnnotation has no type information. +func (n *DescriptionAnnotation) NewTypeFromAnn() (yamlmeta.Type, error) { + return nil, nil +} + func (t *TypeAnnotation) IsAny() bool { return t.any } @@ -189,6 +240,22 @@ func collectValueAnnotations(node yamlmeta.Node, effectiveType yamlmeta.Type) ([ return anns, nil } +// collectDocumentationAnnotations provides annotations that are used for documentation purposes +func collectDocumentationAnnotations(node yamlmeta.Node) ([]Annotation, error) { + var anns []Annotation + + for _, annotation := range []template.AnnotationName{AnnotationDescription} { + ann, err := processOptionalAnnotation(node, annotation, nil) + if err != nil { + return nil, err + } + if ann != nil { + anns = append(anns, ann) + } + } + return anns, nil +} + func processOptionalAnnotation(node yamlmeta.Node, optionalAnnotation template.AnnotationName, effectiveType yamlmeta.Type) (Annotation, error) { nodeAnnotations := template.NewAnnotations(node) @@ -225,6 +292,12 @@ func processOptionalAnnotation(node yamlmeta.Node, optionalAnnotation template.A return nil, err } return defaultAnn, nil + case AnnotationDescription: + descAnn, err := NewDescriptionAnnotation(ann, node.GetPosition()) + if err != nil { + return nil, err + } + return descAnn, nil } } diff --git a/pkg/schema/openapi.go b/pkg/schema/openapi.go index bc0b9f20..7b1e2ae0 100644 --- a/pkg/schema/openapi.go +++ b/pkg/schema/openapi.go @@ -9,6 +9,14 @@ import ( "github.com/k14s/ytt/pkg/yamlmeta" ) +// keys used when generating an OpenAPI Document +const ( + typeProp = "type" + defaultProp = "default" + nullableProp = "nullable" + descriptionProp = "description" +) + // OpenAPIDocument holds the document type used for creating an OpenAPI document type OpenAPIDocument struct { docType *DocumentType @@ -50,40 +58,62 @@ func (o *OpenAPIDocument) calculateProperties(schemaVal interface{}) *yamlmeta.M properties = append(properties, &mi) } property := yamlmeta.Map{Items: []*yamlmeta.MapItem{ - {Key: "type", Value: "object"}, + {Key: typeProp, Value: "object"}, {Key: "additionalProperties", Value: false}, - {Key: "properties", Value: &yamlmeta.Map{Items: properties}}, }} + + if typedValue.GetDescription() != "" { + property.Items = append(property.Items, &yamlmeta.MapItem{Key: descriptionProp, Value: typedValue.GetDescription()}) + } + property.Items = append(property.Items, &yamlmeta.MapItem{Key: "properties", Value: &yamlmeta.Map{Items: properties}}) return &property case *ArrayType: valueType := typedValue.GetValueType().(*ArrayItemType) properties := o.calculateProperties(valueType.GetValueType()) property := yamlmeta.Map{Items: []*yamlmeta.MapItem{ - {Key: "type", Value: "array"}, - {Key: "items", Value: properties}, - {Key: "default", Value: typedValue.GetDefaultValue()}, + {Key: typeProp, Value: "array"}, }} + + if typedValue.GetDescription() != "" { + property.Items = append(property.Items, &yamlmeta.MapItem{Key: descriptionProp, Value: typedValue.GetDescription()}) + } + items := []*yamlmeta.MapItem{ + {Key: "items", Value: properties}, + {Key: defaultProp, Value: typedValue.GetDefaultValue()}, + } + + property.Items = append(property.Items, items...) return &property case *ScalarType: typeString := o.openAPITypeFor(typedValue) defaultVal := typedValue.GetDefaultValue() property := yamlmeta.Map{Items: []*yamlmeta.MapItem{ - {Key: "type", Value: typeString}, - {Key: "default", Value: defaultVal}, + {Key: typeProp, Value: typeString}, + {Key: defaultProp, Value: defaultVal}, }} if typedValue.String() == "float" { property.Items = append(property.Items, &yamlmeta.MapItem{Key: "format", Value: "float"}) } + if typedValue.GetDescription() != "" { + property.Items = append(property.Items, &yamlmeta.MapItem{Key: descriptionProp, Value: typedValue.GetDescription()}) + } return &property case *NullType: properties := o.calculateProperties(typedValue.GetValueType()) - properties.Items = append(properties.Items, &yamlmeta.MapItem{Key: "nullable", Value: true}) + properties.Items = append(properties.Items, &yamlmeta.MapItem{Key: nullableProp, Value: true}) + if typedValue.GetDescription() != "" { + properties.Items = append(properties.Items, &yamlmeta.MapItem{Key: descriptionProp, Value: typedValue.GetDescription()}) + } return properties case *AnyType: - return &yamlmeta.Map{Items: []*yamlmeta.MapItem{ - {Key: "nullable", Value: true}, - {Key: "default", Value: typedValue.GetDefaultValue()}, + properties := &yamlmeta.Map{Items: []*yamlmeta.MapItem{ + {Key: nullableProp, Value: true}, + {Key: defaultProp, Value: typedValue.GetDefaultValue()}, }} + if typedValue.GetDescription() != "" { + properties.Items = append(properties.Items, &yamlmeta.MapItem{Key: descriptionProp, Value: typedValue.GetDescription()}) + } + return properties default: panic(fmt.Sprintf("Unrecognized type %T", schemaVal)) } diff --git a/pkg/schema/schema.go b/pkg/schema/schema.go index e960f3b2..e0511131 100644 --- a/pkg/schema/schema.go +++ b/pkg/schema/schema.go @@ -167,6 +167,15 @@ func getType(node yamlmeta.Node) (yamlmeta.Type, error) { return nil, err } } + docAnns, err := collectDocumentationAnnotations(node) + if err != nil { + return nil, NewSchemaError("Invalid schema", err) + } + for _, ann := range docAnns { + if desc, ok := ann.(*DescriptionAnnotation); ok { + typeOfValue.SetDescription(desc.description) + } + } err = valueTypeAllowsItemValue(typeOfValue, node.GetValues()[0], node.GetPosition()) if err != nil { diff --git a/pkg/schema/type.go b/pkg/schema/type.go index 5a966cc3..5db98c6d 100644 --- a/pkg/schema/type.go +++ b/pkg/schema/type.go @@ -17,6 +17,7 @@ var _ yamlmeta.Type = (*ArrayType)(nil) var _ yamlmeta.Type = (*ArrayItemType)(nil) var _ yamlmeta.Type = (*AnyType)(nil) var _ yamlmeta.Type = (*NullType)(nil) +var _ yamlmeta.Type = (*ScalarType)(nil) type DocumentType struct { Source *yamlmeta.Document @@ -25,8 +26,9 @@ type DocumentType struct { defaultValue interface{} } type MapType struct { - Items []*MapItemType - Position *filepos.Position + Items []*MapItemType + Position *filepos.Position + description string } type MapItemType struct { Key interface{} // usually a string @@ -38,6 +40,7 @@ type ArrayType struct { ItemsType yamlmeta.Type Position *filepos.Position defaultValue interface{} + description string } type ArrayItemType struct { ValueType yamlmeta.Type @@ -48,14 +51,17 @@ type ScalarType struct { ValueType interface{} Position *filepos.Position defaultValue interface{} + description string } type AnyType struct { defaultValue interface{} Position *filepos.Position + description string } type NullType struct { - ValueType yamlmeta.Type - Position *filepos.Position + ValueType yamlmeta.Type + Position *filepos.Position + description string } // SetDefaultValue sets the default value of the wrapped type to `val` @@ -80,8 +86,8 @@ func (m *MapType) SetDefaultValue(val interface{}) { } // SetDefaultValue sets the default value to `val` -func (i *MapItemType) SetDefaultValue(val interface{}) { - i.defaultValue = val +func (t *MapItemType) SetDefaultValue(val interface{}) { + t.defaultValue = val } // SetDefaultValue sets the default value to `val` @@ -95,8 +101,8 @@ func (a *ArrayItemType) SetDefaultValue(val interface{}) { } // SetDefaultValue sets the default value to `val` -func (a *ScalarType) SetDefaultValue(val interface{}) { - a.defaultValue = val +func (s *ScalarType) SetDefaultValue(val interface{}) { + s.defaultValue = val } func (n NullType) GetDefaultValue() interface{} { @@ -110,8 +116,9 @@ func (a AnyType) GetDefaultValue() interface{} { return a.defaultValue } -func (m ScalarType) GetDefaultValue() interface{} { - return m.defaultValue // scalar values are copied (even through an interface{} reference) +// GetDefaultValue provides the default value +func (s ScalarType) GetDefaultValue() interface{} { + return s.defaultValue // scalar values are copied (even through an interface{} reference) } func (a ArrayItemType) GetDefaultValue() interface{} { @@ -183,7 +190,9 @@ func (a ArrayType) GetValueType() yamlmeta.Type { func (a ArrayItemType) GetValueType() yamlmeta.Type { return a.ValueType } -func (m ScalarType) GetValueType() yamlmeta.Type { + +// GetValueType provides the type of the value +func (s ScalarType) GetValueType() yamlmeta.Type { panic("Not implemented because it is unreachable") } func (a AnyType) GetValueType() yamlmeta.Type { @@ -205,8 +214,10 @@ func (a ArrayType) GetDefinitionPosition() *filepos.Position { func (a ArrayItemType) GetDefinitionPosition() *filepos.Position { return a.Position } -func (m ScalarType) GetDefinitionPosition() *filepos.Position { - return m.Position + +// GetDefinitionPosition provides the file position +func (s ScalarType) GetDefinitionPosition() *filepos.Position { + return s.Position } func (a AnyType) GetDefinitionPosition() *filepos.Position { return a.Position @@ -227,8 +238,8 @@ func (a ArrayType) String() string { func (a ArrayItemType) String() string { return fmt.Sprintf("- %s", a.ValueType.String()) } -func (m ScalarType) String() string { - switch m.ValueType.(type) { +func (s ScalarType) String() string { + switch s.ValueType.(type) { case float64: return "float" case int: @@ -236,7 +247,7 @@ func (m ScalarType) String() string { case bool: return "boolean" default: - return fmt.Sprintf("%T", m.ValueType) + return fmt.Sprintf("%T", s.ValueType) } } func (a AnyType) String() string { @@ -292,34 +303,35 @@ func (a *ArrayItemType) CheckType(node yamlmeta.TypeWithValues) (chk yamlmeta.Ty return } -func (m *ScalarType) CheckType(node yamlmeta.TypeWithValues) (chk yamlmeta.TypeCheck) { +// CheckType validates the type of the node and the type of the value +func (s *ScalarType) CheckType(node yamlmeta.TypeWithValues) (chk yamlmeta.TypeCheck) { value := node.GetValues()[0] switch value.(type) { case string: - if _, ok := m.ValueType.(string); !ok { + if _, ok := s.ValueType.(string); !ok { chk.Violations = append(chk.Violations, - NewMismatchedTypeAssertionError(node, m)) + NewMismatchedTypeAssertionError(node, s)) } case float64: - if _, ok := m.ValueType.(float64); !ok { + if _, ok := s.ValueType.(float64); !ok { chk.Violations = append(chk.Violations, - NewMismatchedTypeAssertionError(node, m)) + NewMismatchedTypeAssertionError(node, s)) } case int, int64, uint64: - if _, ok := m.ValueType.(int); !ok { - if _, ok = m.ValueType.(float64); !ok { + if _, ok := s.ValueType.(int); !ok { + if _, ok = s.ValueType.(float64); !ok { chk.Violations = append(chk.Violations, - NewMismatchedTypeAssertionError(node, m)) + NewMismatchedTypeAssertionError(node, s)) } } case bool: - if _, ok := m.ValueType.(bool); !ok { + if _, ok := s.ValueType.(bool); !ok { chk.Violations = append(chk.Violations, - NewMismatchedTypeAssertionError(node, m)) + NewMismatchedTypeAssertionError(node, s)) } default: chk.Violations = append(chk.Violations, - NewMismatchedTypeAssertionError(node, m)) + NewMismatchedTypeAssertionError(node, s)) } return } @@ -434,14 +446,89 @@ func (a *ArrayItemType) AssignTypeTo(typeable yamlmeta.Typeable) (chk yamlmeta.T return } -func (m *ScalarType) AssignTypeTo(typeable yamlmeta.Typeable) yamlmeta.TypeCheck { - return yamlmeta.TypeCheck{[]error{NewMismatchedTypeAssertionError(typeable, m)}} +// AssignTypeTo validates that the type is compatible and assigns it to the type +func (s *ScalarType) AssignTypeTo(typeable yamlmeta.Typeable) yamlmeta.TypeCheck { + return yamlmeta.TypeCheck{[]error{NewMismatchedTypeAssertionError(typeable, s)}} } func (a AnyType) AssignTypeTo(yamlmeta.Typeable) (chk yamlmeta.TypeCheck) { return } +// GetDescription provides descriptive information +func (t *DocumentType) GetDescription() string { + return "" +} + +// GetDescription provides descriptive information +func (m *MapType) GetDescription() string { + return m.description +} + +// GetDescription provides descriptive information +func (t *MapItemType) GetDescription() string { + return "" +} + +// GetDescription provides descriptive information +func (a *ArrayType) GetDescription() string { + return a.description +} + +// GetDescription provides descriptive information +func (a *ArrayItemType) GetDescription() string { + return "" +} + +// GetDescription provides descriptive information +func (s *ScalarType) GetDescription() string { + return s.description +} + +// GetDescription provides descriptive information +func (a *AnyType) GetDescription() string { + return a.description +} + +// GetDescription provides descriptive information +func (n *NullType) GetDescription() string { + return n.description +} + +// SetDescription sets the description of the type +func (t *DocumentType) SetDescription(desc string) {} + +// SetDescription sets the description of the type +func (m *MapType) SetDescription(desc string) { + m.description = desc +} + +// SetDescription sets the description of the type +func (t *MapItemType) SetDescription(desc string) {} + +// SetDescription sets the description of the type +func (a *ArrayType) SetDescription(desc string) { + a.description = desc +} + +// SetDescription sets the description of the type +func (a *ArrayItemType) SetDescription(desc string) {} + +// SetDescription sets the description of the type +func (s *ScalarType) SetDescription(desc string) { + s.description = desc +} + +// SetDescription sets the description of the type +func (a *AnyType) SetDescription(desc string) { + a.description = desc +} + +// SetDescription sets the description of the type +func (n *NullType) SetDescription(desc string) { + n.description = desc +} + func (m *MapType) AllowsKey(key interface{}) bool { for _, item := range m.Items { if item.Key == key { diff --git a/pkg/yamlmeta/type.go b/pkg/yamlmeta/type.go index 9c6fc518..a05b5bd5 100644 --- a/pkg/yamlmeta/type.go +++ b/pkg/yamlmeta/type.go @@ -15,6 +15,8 @@ type Type interface { CheckType(node TypeWithValues) TypeCheck GetDefinitionPosition() *filepos.Position String() string + GetDescription() string + SetDescription(string) } type TypeWithValues interface {