-
Notifications
You must be signed in to change notification settings - Fork 68
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.
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
Limit recursive struct scan level #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for deciding to tackle this!
See comments.
visitedTypes := make(map[reflect.Type]int) | ||
for len(queue) > 0 { | ||
traversal := queue[0] | ||
queue = queue[1:] | ||
structType := traversal.Type | ||
|
||
// remember visited types and number of visits. | ||
if structType.Kind() == reflect.Struct { | ||
if visitCount, visited := visitedTypes[structType]; !visited { | ||
visitedTypes[structType] = 1 | ||
} else if visitCount < MaxStructRecursionLevel { | ||
visitedTypes[structType] = visitCount + 1 | ||
} else { | ||
continue | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visitedTypes
variable is global for the whole traversal, it counts all type encounters even is separate branches. We should judge recursion only on types that were visited on the path to the current type, i.e. only the current branch of the traversal.
I think we should add visitedTypes
as a field to the toTraverse
struct, so would every type we iterate over had it's own set of previously visited types.
for childType.Kind() == reflect.Ptr { | ||
childType = childType.Elem() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
I found two more places there we should do the same thing:
Line 98 in 2eb5f0f
if field.Kind() == reflect.Ptr && field.Type().Elem().Kind() == reflect.Struct && field.IsNil() { |
Lines 136 to 142 in d03fac1
if elementBaseType.Kind() == reflect.Ptr { | |
elementBaseTypeElem := elementBaseType.Elem() | |
if elementBaseTypeElem.Kind() == reflect.Struct { | |
elementBaseType = elementBaseTypeElem | |
elementByPtr = true | |
} | |
} |
I think we should move that change in a separate PR and cover it with tests properly
Closing due to inactvity. Feel free to reopen. |
An attempt to fix #57.