From 04eac2abe71a5fdff5d03a875df3b697a0f8c680 Mon Sep 17 00:00:00 2001 From: Dave Shanley Date: Sat, 25 Mar 2023 10:49:33 -0400 Subject: [PATCH] More hardening with digitla ocean some ref handling was a bit strange, now it's rendering correctly. I have a feeling we will be back to the diff engine at some point soon, it's picking up some strange changes that are so deep in the model, I can't determine what is what, so we will wait for another set of triggers to appear. --- datamodel/high/node_builder.go | 23 +----------- datamodel/low/base/example.go | 5 +++ datamodel/low/base/example_test.go | 54 ++++++++++++++++++++++++++++ datamodel/low/base/schema.go | 4 --- datamodel/low/v3/path_item.go | 32 +++++++++++++---- document.go | 2 +- document_test.go | 48 +++++++++++++++++-------- utils/nodes.go | 1 + what-changed/model/path_item_test.go | 13 ++++++- what-changed/model/response.go | 17 +++------ what-changed/model/response_test.go | 2 ++ 11 files changed, 140 insertions(+), 61 deletions(-) diff --git a/datamodel/high/node_builder.go b/datamodel/high/node_builder.go index 7f56cdf..3e556f3 100644 --- a/datamodel/high/node_builder.go +++ b/datamodel/high/node_builder.go @@ -93,8 +93,6 @@ func (n *NodeBuilder) add(key string, i int) { u++ } } - default: - panic("not supported yet") } } n.Nodes = append(n.Nodes, nodeEntry) @@ -202,7 +200,6 @@ func (n *NodeBuilder) add(key string, i int) { nodeEntry.Line = jk.GetKeyNode().Line break } - panic("this should not break.") } if nb.GetValueNode() != nil { nodeEntry.Line = nb.GetValueNode().Line @@ -256,10 +253,7 @@ func (n *NodeBuilder) Render() *yaml.Node { node := n.Nodes[i] n.AddYAMLNode(m, node) } - if len(m.Content) > 0 { - return m - } - return nil + return m } // AddYAMLNode will add a new *yaml.Node to the parent node, using the tag, key and value provided. @@ -366,14 +360,7 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *NodeEntry) *yaml.Nod } else { // this is a map, but it may be wrapped still. bj := reflect.ValueOf(gh) - //yh := bj.Interface() - - // if vg, jo := yh.(low.HasKeyNode); jo { - // fv := reflect.ValueOf(vg.GetKeyNode()) - // orderedCollection = n.extractLowMapKeysWrapped(fv, x, orderedCollection, g) - // } else { orderedCollection = n.extractLowMapKeysWrapped(bj, x, orderedCollection, g) - //} } } else { // this is a map, without any low level details available (probably an extension map). @@ -409,8 +396,6 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *NodeEntry) *yaml.Nod } if len(p.Content) > 0 { valueNode = p - } else { - return parent } case reflect.Slice: @@ -423,9 +408,7 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *NodeEntry) *yaml.Nod sl := utils.CreateEmptySequenceNode() skip := false for i := 0; i < m.Len(); i++ { - sqi := m.Index(i).Interface() - // check if this is a reference. if glu, ok := sqi.(GoesLowUntyped); ok { if glu != nil { @@ -506,8 +489,6 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *NodeEntry) *yaml.Nod rawRender, _ := r.MarshalYAML() if rawRender != nil { valueNode = rawRender.(*yaml.Node) - } else { - return parent } } else { @@ -551,8 +532,6 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *NodeEntry) *yaml.Nod } } - default: - panic("not supported yet") } if valueNode == nil { return parent diff --git a/datamodel/low/base/example.go b/datamodel/low/base/example.go index 37621f1..73571fa 100644 --- a/datamodel/low/base/example.go +++ b/datamodel/low/base/example.go @@ -123,5 +123,10 @@ func ExtractExampleValue(exp *yaml.Node) any { _ = exp.Decode(&m) return m } + if utils.IsNodeArray(exp) { + var m []interface{} + _ = exp.Decode(&m) + return m + } return exp.Value } diff --git a/datamodel/low/base/example_test.go b/datamodel/low/base/example_test.go index 8712368..4e8b5c2 100644 --- a/datamodel/low/base/example_test.go +++ b/datamodel/low/base/example_test.go @@ -124,6 +124,60 @@ value: } +func TestExample_ExtractExampleValue_Map(t *testing.T) { + + yml := `hot: + summer: nights + pizza: oven` + + var idxNode yaml.Node + _ = yaml.Unmarshal([]byte(yml), &idxNode) + + val := ExtractExampleValue(idxNode.Content[0]) + if v, ok := val.(map[string]interface{}); ok { + if r, rok := v["hot"].(map[string]interface{}); rok { + assert.Equal(t, "nights", r["summer"]) + assert.Equal(t, "oven", r["pizza"]) + } else { + assert.Fail(t, "failed to decode correctly.") + } + } else { + assert.Fail(t, "failed to decode correctly.") + } +} + +func TestExample_ExtractExampleValue_Slice(t *testing.T) { + + yml := `- hot: + summer: nights +- hotter: + pizza: oven` + + var idxNode yaml.Node + _ = yaml.Unmarshal([]byte(yml), &idxNode) + + val := ExtractExampleValue(idxNode.Content[0]) + if v, ok := val.([]interface{}); ok { + for w := range v { + if r, rok := v[w].(map[string]interface{}); rok { + for k := range r { + if k == "hotter" { + assert.Equal(t, "oven", r[k].(map[string]interface{})["pizza"]) + } + if k == "hot" { + assert.Equal(t, "nights", r[k].(map[string]interface{})["summer"]) + } + } + } else { + assert.Fail(t, "failed to decode correctly.") + } + } + + } else { + assert.Fail(t, "failed to decode correctly.") + } +} + func TestExample_Hash(t *testing.T) { left := `summary: hot diff --git a/datamodel/low/base/schema.go b/datamodel/low/base/schema.go index 0affaa2..4595f23 100644 --- a/datamodel/low/base/schema.go +++ b/datamodel/low/base/schema.go @@ -215,10 +215,6 @@ func (s *Schema) Hash() [32]byte { x = o.GetKeyNode().Value l = o.GetKeyNode().Line v = vo.MapIndex(k).Interface().(low.HasValueNodeUntyped).GetValueNode().Value - } else { - x = k.String() - l = 999 - v = vo.MapIndex(k).Interface() } values = append(values, fmt.Sprintf("%d:%s:%s", l, x, low.GenerateHashString(v))) } diff --git a/datamodel/low/v3/path_item.go b/datamodel/low/v3/path_item.go index 1d7ebaa..f2c910a 100644 --- a/datamodel/low/v3/path_item.go +++ b/datamodel/low/v3/path_item.go @@ -6,13 +6,14 @@ package v3 import ( "crypto/sha256" "fmt" + "sort" + "strings" + "sync" + "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/utils" "gopkg.in/yaml.v3" - "sort" - "strings" - "sync" ) // PathItem represents a low-level OpenAPI 3+ PathItem object. @@ -195,8 +196,9 @@ func (p *PathItem) Build(root *yaml.Node, idx *index.SpecIndex) error { } var op Operation - - if ok, _, _ := utils.IsNodeRefValue(pathNode); ok { + opIsRef := false + var opRefVal string + if ok, _, ref := utils.IsNodeRefValue(pathNode); ok { // According to OpenAPI spec the only valid $ref for paths is // reference for the whole pathItem. Unfortunately, internet is full of invalid specs // even from trusted companies like DigitalOcean where they tend to @@ -206,8 +208,13 @@ func (p *PathItem) Build(root *yaml.Node, idx *index.SpecIndex) error { // $ref: 'file.yaml' // Check if that is the case and resolve such thing properly too. + opIsRef = true + opRefVal = ref r, err := low.LocateRefNode(pathNode, idx) if r != nil { + if r.Kind == yaml.DocumentNode { + r = r.Content[0] + } pathNode = r if r.Tag == "" { // If it's a node from file, tag is empty @@ -232,6 +239,10 @@ func (p *PathItem) Build(root *yaml.Node, idx *index.SpecIndex) error { KeyNode: currentNode, ValueNode: pathNode, } + if opIsRef { + opRef.Reference = opRefVal + opRef.ReferenceNode = true + } ops = append(ops, opRef) @@ -260,8 +271,11 @@ func (p *PathItem) Build(root *yaml.Node, idx *index.SpecIndex) error { opBuildChan := make(chan bool) opErrorChan := make(chan error) - buildOpFunc := func(op low.NodeReference[*Operation], ch chan<- bool, errCh chan<- error) { + buildOpFunc := func(op low.NodeReference[*Operation], ch chan<- bool, errCh chan<- error, ref string) { er := op.Value.Build(op.ValueNode, idx) + if ref != "" { + op.Value.Reference.Reference = ref + } if er != nil { errCh <- er } @@ -273,7 +287,11 @@ func (p *PathItem) Build(root *yaml.Node, idx *index.SpecIndex) error { } for _, op := range ops { - go buildOpFunc(op, opBuildChan, opErrorChan) + ref := "" + if op.ReferenceNode { + ref = op.Reference + } + go buildOpFunc(op, opBuildChan, opErrorChan, ref) } n := 0 diff --git a/document.go b/document.go index 7ecfa57..f58faee 100644 --- a/document.go +++ b/document.go @@ -164,7 +164,7 @@ func (d *document) RenderAndReload() ([]byte, Document, *DocumentModel[v3high.Do if err != nil { return newBytes, nil, nil, []error{err} } - newDoc, err := NewDocument(newBytes) + newDoc, err := NewDocumentWithConfiguration(newBytes, d.config) if err != nil { return newBytes, newDoc, nil, []error{err} } diff --git a/document_test.go b/document_test.go index 1a2765a..dcd33c8 100644 --- a/document_test.go +++ b/document_test.go @@ -4,6 +4,7 @@ package libopenapi import ( "fmt" + "github.com/pb33f/libopenapi/datamodel" "github.com/pb33f/libopenapi/datamodel/high/base" "github.com/pb33f/libopenapi/what-changed/model" "github.com/stretchr/testify/assert" @@ -210,7 +211,6 @@ func TestDocument_RenderAndReload_ChangeCheck_Asana(t *testing.T) { dat, newDoc, _, _ := doc.RenderAndReload() assert.NotNil(t, dat) - //_ = os.WriteFile("asana_reloaded.yaml", dat, 0644) // compare documents compReport, errs := CompareDocuments(doc, newDoc) @@ -218,22 +218,12 @@ func TestDocument_RenderAndReload_ChangeCheck_Asana(t *testing.T) { // get flat list of changes. flatChanges := compReport.GetAllChanges() - // remove everything that is an example set to "true" (asana has 45 of these). the lib converts this to a boolean. - var filtered []*model.Change - for i := range flatChanges { - if flatChanges[i].Property != "example" { - filtered = append(filtered, flatChanges[i]) - } - } - assert.Nil(t, errs) tc := compReport.TotalChanges() - bc := compReport.TotalBreakingChanges() - assert.Equal(t, 0, bc) - assert.Equal(t, 45, tc) + assert.Equal(t, 21, tc) - // there should be no other changes than the 519 example "true" re-renders - assert.Equal(t, 0, len(filtered)) + // there are some properties re-rendered that trigger changes. + assert.Equal(t, 21, len(flatChanges)) } @@ -544,5 +534,35 @@ components: // render the document. rend, _ := result.Model.Render() + assert.Len(t, rend, 644) +} + +func TestDocument_OperationsAsRefs(t *testing.T) { + + ae := `operationId: thisIsAnOperationId +summary: a test thing +description: this is a test, that does a test.` + + _ = os.WriteFile("test-operation.yaml", []byte(ae), 0644) + + var d = `openapi: "3.1" +paths: + /an/operation: + get: + $ref: test-operation.yaml` + + doc, err := NewDocumentWithConfiguration([]byte(d), datamodel.NewOpenDocumentConfiguration()) + if err != nil { + panic(err) + } + + result, errs := doc.BuildV3Model() + if len(errs) > 0 { + panic(errs) + } + + // render the document. + rend, _ := result.Model.Render() + assert.Equal(t, d, strings.TrimSpace(string(rend))) } diff --git a/utils/nodes.go b/utils/nodes.go index fd8d06c..b4bdee0 100644 --- a/utils/nodes.go +++ b/utils/nodes.go @@ -10,6 +10,7 @@ func CreateRefNode(ref string) *yaml.Node { nodes := make([]*yaml.Node, 2) nodes[0] = CreateStringNode("$ref") nodes[1] = CreateStringNode(ref) + nodes[1].Style = yaml.SingleQuotedStyle m.Content = nodes return m } diff --git a/what-changed/model/path_item_test.go b/what-changed/model/path_item_test.go index d7bbf36..587bf59 100644 --- a/what-changed/model/path_item_test.go +++ b/what-changed/model/path_item_test.go @@ -142,6 +142,7 @@ parameters: // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 2, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 2) assert.Equal(t, 2, extChanges.TotalBreakingChanges()) assert.Equal(t, Modified, extChanges.ParameterChanges[0].Changes[0].ChangeType) assert.Equal(t, v3.InLabel, extChanges.ParameterChanges[0].Changes[0].Property) @@ -182,6 +183,7 @@ parameters: // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 1) assert.Equal(t, 1, extChanges.TotalBreakingChanges()) assert.Equal(t, ObjectAdded, extChanges.Changes[0].ChangeType) } @@ -221,6 +223,7 @@ parameters: // compare. extChanges := ComparePathItems(&rDoc, &lDoc) assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 1) assert.Equal(t, 1, extChanges.TotalBreakingChanges()) assert.Equal(t, ObjectRemoved, extChanges.Changes[0].ChangeType) } @@ -332,6 +335,7 @@ parameters: // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 7, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 7) assert.Equal(t, 0, extChanges.TotalBreakingChanges()) } func TestComparePathItem_V2_RemoveMethods(t *testing.T) { @@ -371,6 +375,7 @@ parameters: // compare. extChanges := ComparePathItems(&rDoc, &lDoc) assert.Equal(t, 7, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 7) assert.Equal(t, 7, extChanges.TotalBreakingChanges()) } @@ -485,6 +490,7 @@ x-thing: dang.` // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 12, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 12) assert.Equal(t, 0, extChanges.TotalBreakingChanges()) } @@ -511,6 +517,7 @@ parameters: // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 1) assert.Equal(t, 1, extChanges.TotalBreakingChanges()) assert.Equal(t, PropertyAdded, extChanges.Changes[0].ChangeType) assert.Equal(t, v3.ParametersLabel, extChanges.Changes[0].Property) @@ -537,8 +544,9 @@ parameters: _ = rDoc.Build(rNode.Content[0], nil) // compare. - extChanges := ComparePathItems(&rDoc, &lDoc) + extChanges := ComparePathItemsV3(&rDoc, &lDoc) assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 1) assert.Equal(t, 1, extChanges.TotalBreakingChanges()) assert.Equal(t, PropertyRemoved, extChanges.Changes[0].ChangeType) assert.Equal(t, v3.ParametersLabel, extChanges.Changes[0].Property) @@ -581,6 +589,7 @@ trace: // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 8, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 8) assert.Equal(t, 0, extChanges.TotalBreakingChanges()) } @@ -621,6 +630,7 @@ trace: // compare. extChanges := ComparePathItems(&rDoc, &lDoc) assert.Equal(t, 8, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 8) assert.Equal(t, 8, extChanges.TotalBreakingChanges()) } @@ -653,5 +663,6 @@ func TestComparePathItem_V3_ChangeParam(t *testing.T) { // compare. extChanges := ComparePathItems(&lDoc, &rDoc) assert.Equal(t, 1, extChanges.TotalChanges()) + assert.Len(t, extChanges.GetAllChanges(), 1) assert.Equal(t, 1, extChanges.TotalBreakingChanges()) } diff --git a/what-changed/model/response.go b/what-changed/model/response.go index d031fb9..beef347 100644 --- a/what-changed/model/response.go +++ b/what-changed/model/response.go @@ -5,11 +5,14 @@ package model import ( "github.com/pb33f/libopenapi/datamodel/low" - "github.com/pb33f/libopenapi/datamodel/low/v2" - "github.com/pb33f/libopenapi/datamodel/low/v3" + v3 "github.com/pb33f/libopenapi/datamodel/low/v3" "reflect" ) +import ( + "github.com/pb33f/libopenapi/datamodel/low/v2" +) + // ResponseChanges represents changes found between two Swagger or OpenAPI Response objects. type ResponseChanges struct { *PropertyChanges @@ -23,7 +26,6 @@ type ResponseChanges struct { // OpenAPI Response Properties. ContentChanges map[string]*MediaTypeChanges `json:"content,omitempty" yaml:"content,omitempty"` LinkChanges map[string]*LinkChanges `json:"links,omitempty" yaml:"links,omitempty"` - ServerChanges *ServerChanges `json:"server,omitempty" yaml:"server,omitempty"` } // GetAllChanges returns a slice of all changes made between RequestBody objects @@ -39,9 +41,6 @@ func (r *ResponseChanges) GetAllChanges() []*Change { if r.ExamplesChanges != nil { changes = append(changes, r.ExamplesChanges.GetAllChanges()...) } - if r.ServerChanges != nil { - changes = append(changes, r.ServerChanges.GetAllChanges()...) - } for k := range r.HeadersChanges { changes = append(changes, r.HeadersChanges[k].GetAllChanges()...) } @@ -66,9 +65,6 @@ func (r *ResponseChanges) TotalChanges() int { if r.ExamplesChanges != nil { c += r.ExamplesChanges.TotalChanges() } - if r.ServerChanges != nil { - c += r.ServerChanges.TotalChanges() - } for k := range r.HeadersChanges { c += r.HeadersChanges[k].TotalChanges() } @@ -88,9 +84,6 @@ func (r *ResponseChanges) TotalBreakingChanges() int { if r.SchemaChanges != nil { c += r.SchemaChanges.TotalBreakingChanges() } - if r.ServerChanges != nil { - c += r.ServerChanges.TotalBreakingChanges() - } for k := range r.HeadersChanges { c += r.HeadersChanges[k].TotalBreakingChanges() } diff --git a/what-changed/model/response_test.go b/what-changed/model/response_test.go index fb5ac0b..ad519c3 100644 --- a/what-changed/model/response_test.go +++ b/what-changed/model/response_test.go @@ -22,6 +22,8 @@ headers: description: a header examples: bam: alam +server: + url: https://example.com x-toot: poot` right := left