Skip to content

Commit

Permalink
Tuned circular reference handling
Browse files Browse the repository at this point in the history
stopped rolodex looking up root file, looking in components schemas for a reference also. Dropped stripe circular refs down by one. seems to be catching the duplicate now.

Signed-off-by: quobix <[email protected]>
  • Loading branch information
daveshanley committed Dec 3, 2023
1 parent 1e9b42a commit 7a5a4bc
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 27 deletions.
2 changes: 1 addition & 1 deletion datamodel/high/v3/document_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ func TestStripeAsDoc(t *testing.T) {
info, _ := datamodel.ExtractSpecInfo(data)
var err error
lowDoc, err = lowv3.CreateDocumentFromConfig(info, datamodel.NewDocumentConfiguration())
assert.Len(t, utils.UnwrapErrors(err), 2)
assert.Len(t, utils.UnwrapErrors(err), 1)
d := NewDocument(lowDoc)
assert.NotNil(t, d)
}
Expand Down
2 changes: 1 addition & 1 deletion datamodel/low/v3/create_document_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func TestCreateDocumentStripe(t *testing.T) {
data, _ := os.ReadFile("../../../test_specs/stripe.yaml")
info, _ := datamodel.ExtractSpecInfo(data)
d, err := CreateDocumentFromConfig(info, &datamodel.DocumentConfiguration{})
assert.Len(t, utils.UnwrapErrors(err), 2)
assert.Len(t, utils.UnwrapErrors(err), 1)

assert.Equal(t, "3.0.0", d.Version.Value)
assert.Equal(t, "Stripe API", d.Info.Value.Title.Value)
Expand Down
2 changes: 1 addition & 1 deletion document_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func TestDocument_ResolveStripe(t *testing.T) {
rolo := model.Index.GetRolodex()
rolo.Resolve()

assert.Equal(t, 2, len(model.Index.GetRolodex().GetCaughtErrors()))
assert.Equal(t, 1, len(model.Index.GetRolodex().GetCaughtErrors()))

}

Expand Down
31 changes: 17 additions & 14 deletions index/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func (resolver *Resolver) Resolve() []*ResolvingError {
}

resolver.resolvingErrors = append(resolver.resolvingErrors, &ResolvingError{
ErrorRef: fmt.Errorf("infinite circular reference detected: %s", circRef.Start.Name),
ErrorRef: fmt.Errorf("infinite circular reference detected: %s", circRef.Start.Definition),
Node: circRef.LoopPoint.Node,
Path: circRef.GenerateJourneyPath(),
})
Expand Down Expand Up @@ -354,7 +354,8 @@ func (resolver *Resolver) VisitReference(ref *Reference, seen map[string]bool, j
} else {
resolver.circularReferences = append(resolver.circularReferences, circRef)
}

r.Seen = true
r.Circular = true
foundDup.Seen = true
foundDup.Circular = true
}
Expand Down Expand Up @@ -429,30 +430,26 @@ func (resolver *Resolver) extractRelatives(ref *Reference, node, parent *yaml.No
}

// this is a safety check to prevent a stack overflow.
if depth > 500 {
if depth > 100 {
def := "unknown"
if ref != nil {
def = ref.FullDefinition
}
if resolver.specIndex != nil && resolver.specIndex.logger != nil {
resolver.specIndex.logger.Warn("libopenapi resolver: relative depth exceeded 500 levels, "+
resolver.specIndex.logger.Warn("libopenapi resolver: relative depth exceeded 100 levels, "+
"check for circular references - resolving may be incomplete",
"reference", def)
}

loop := append(journey, ref)
circRef := &CircularReferenceResult{
Journey: loop,
Start: ref,
LoopIndex: depth,
LoopPoint: ref,
Journey: loop,
Start: ref,
LoopIndex: depth,
LoopPoint: ref,
IsInfiniteLoop: true,
}
resolver.circularReferences = append(resolver.circularReferences, circRef)
resolver.resolvingErrors = append(resolver.resolvingErrors, &ResolvingError{
ErrorRef: fmt.Errorf("circular reference detected: %s", circRef.GenerateJourneyPath()),
Node: node,
Path: circRef.GenerateJourneyPath(),
})
ref.Circular = true
return nil
}
Expand All @@ -464,7 +461,13 @@ func (resolver *Resolver) extractRelatives(ref *Reference, node, parent *yaml.No
if utils.IsNodeMap(n) || utils.IsNodeArray(n) {
depth++

found = append(found, resolver.extractRelatives(ref, n, node, foundRelatives, journey, seen, resolve, depth)...)
foundRef, _ := resolver.specIndex.SearchIndexForReferenceByReference(ref)
if foundRef != nil && !foundRef.Circular {
found = append(found, resolver.extractRelatives(ref, n, node, foundRelatives, journey, seen, resolve, depth)...)
}
if foundRef == nil {
found = append(found, resolver.extractRelatives(ref, n, node, foundRelatives, journey, seen, resolve, depth)...)
}

}

Expand Down
19 changes: 12 additions & 7 deletions index/resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ func TestResolver_DeepDepth(t *testing.T) {
found := resolver.extractRelatives(ref, refA, nil, nil, nil, nil, false, 0)

assert.Nil(t, found)
assert.Contains(t, buf.String(), "libopenapi resolver: relative depth exceeded 500 levels")
assert.Contains(t, buf.String(), "libopenapi resolver: relative depth exceeded 100 levels")
}

func TestResolver_ResolveComponents_Stripe_NoRolodex(t *testing.T) {
Expand All @@ -492,7 +492,7 @@ func TestResolver_ResolveComponents_Stripe_NoRolodex(t *testing.T) {
assert.NotNil(t, resolver)

circ := resolver.CheckForCircularReferences()
assert.Len(t, circ, 2)
assert.Len(t, circ, 1)

_, err := yaml.Marshal(resolver.resolvedRoot)
assert.NoError(t, err)
Expand Down Expand Up @@ -521,9 +521,10 @@ func TestResolver_ResolveComponents_Stripe(t *testing.T) {
// after resolving, the rolodex will have errors.
rolo.Resolve()

assert.Len(t, rolo.GetCaughtErrors(), 2)
assert.Len(t, rolo.GetRootIndex().GetResolver().GetNonPolymorphicCircularErrors(), 2)
assert.Len(t, rolo.GetCaughtErrors(), 1)
assert.Len(t, rolo.GetRootIndex().GetResolver().GetNonPolymorphicCircularErrors(), 1)
assert.Len(t, rolo.GetRootIndex().GetResolver().GetPolymorphicCircularErrors(), 0)
assert.Len(t, rolo.GetRootIndex().GetResolver().GetSafeCircularReferences(), 25)

}

Expand Down Expand Up @@ -769,9 +770,13 @@ func ExampleNewResolver() {
// The Stripe API has a bunch of circular reference problems, mainly from polymorphism.
// So let's print them out.
//
fmt.Printf("There are %d circular reference errors, %d of them are polymorphic errors, %d are not",
len(circularErrors), len(resolver.GetPolymorphicCircularErrors()), len(resolver.GetNonPolymorphicCircularErrors()))
// Output: There are 2 circular reference errors, 0 of them are polymorphic errors, 2 are not
fmt.Printf("There is %d circular reference error, %d of them are polymorphic errors, %d are not\n"+
"with a total pf %d safe circular references.\n",
len(circularErrors), len(resolver.GetPolymorphicCircularErrors()), len(resolver.GetNonPolymorphicCircularErrors()),
len(resolver.GetSafeCircularReferences()))
// Output: There is 1 circular reference error, 0 of them are polymorphic errors, 1 are not
// with a total pf 25 safe circular references.

}

func ExampleResolvingError() {
Expand Down
15 changes: 12 additions & 3 deletions index/search_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ func (index *SpecIndex) SearchIndexForReferenceWithContext(ctx context.Context,

func (index *SpecIndex) SearchIndexForReferenceByReferenceWithContext(ctx context.Context, searchRef *Reference) (*Reference, *SpecIndex, context.Context) {

if v, ok := index.cache.Load(searchRef.FullDefinition); ok {
return v.(*Reference), v.(*Reference).Index, context.WithValue(ctx, CurrentPathKey, v.(*Reference).RemoteLocation)
if index.cache != nil {
if v, ok := index.cache.Load(searchRef.FullDefinition); ok {
return v.(*Reference), v.(*Reference).Index, context.WithValue(ctx, CurrentPathKey, v.(*Reference).RemoteLocation)
}
}

ref := searchRef.FullDefinition
Expand Down Expand Up @@ -105,13 +107,20 @@ func (index *SpecIndex) SearchIndexForReferenceByReferenceWithContext(ctx contex
return r, r.Index, context.WithValue(ctx, CurrentPathKey, r.RemoteLocation)
}

if r, ok := index.allComponentSchemaDefinitions[refAlt]; ok {
index.cache.Store(refAlt, r)
return r, r.Index, context.WithValue(ctx, CurrentPathKey, r.RemoteLocation)
}

// check the rolodex for the reference.
if roloLookup != "" {

if strings.Contains(roloLookup, "#") {
roloLookup = strings.Split(roloLookup, "#")[0]
}

if filepath.Base(roloLookup) == "root.yaml" {
return nil, index, ctx
}
rFile, err := index.rolodex.Open(roloLookup)
if err != nil {
return nil, index, ctx
Expand Down

0 comments on commit 7a5a4bc

Please sign in to comment.