-
Notifications
You must be signed in to change notification settings - Fork 127
internal: add structdiff.IsEqual() #4203
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| package structdiff | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "slices" | ||
|
|
||
| "github.com/databricks/cli/libs/structs/structtag" | ||
| ) | ||
|
|
||
| // IsEqual compares two Go structs and returns true if they are equal. | ||
| // It uses the same comparison logic as GetStructDiff but is more efficient | ||
| // as it short-circuits on the first difference found. | ||
| // Respects ForceSendFields if present. | ||
| // Types of a and b must match exactly, otherwise returns false. | ||
| // Note, reflect.DeepEqual() does not work for SDK structs, because ForceSendFields can contain different sets for the same value. | ||
| func IsEqual(a, b any) bool { | ||
| v1 := reflect.ValueOf(a) | ||
| v2 := reflect.ValueOf(b) | ||
|
|
||
| if !v1.IsValid() && !v2.IsValid() { | ||
| return true | ||
| } | ||
|
|
||
| if !v1.IsValid() || !v2.IsValid() { | ||
| return false | ||
| } | ||
|
|
||
| if v1.Type() != v2.Type() { | ||
| return false | ||
| } | ||
|
|
||
| return equalValues(v1, v2) | ||
| } | ||
|
|
||
| // equalValues returns true if v1 and v2 are equal. | ||
| func equalValues(v1, v2 reflect.Value) bool { | ||
| if !v1.IsValid() { | ||
| return !v2.IsValid() | ||
| } else if !v2.IsValid() { | ||
| return false | ||
| } | ||
|
|
||
| v1Type := v1.Type() | ||
|
|
||
| if v1Type != v2.Type() { | ||
| return false | ||
| } | ||
|
|
||
| kind := v1.Kind() | ||
|
|
||
| // Perform nil checks for nilable types. | ||
| switch kind { | ||
| case reflect.Pointer, reflect.Map, reflect.Slice, reflect.Interface, reflect.Chan, reflect.Func: | ||
| v1Nil := v1.IsNil() | ||
| v2Nil := v2.IsNil() | ||
| if v1Nil && v2Nil { | ||
| return true | ||
| } | ||
| if v1Nil || v2Nil { | ||
| return false | ||
| } | ||
| default: | ||
| // Not a nilable type. | ||
| // Proceed with direct comparison below. | ||
| } | ||
|
|
||
| switch kind { | ||
| case reflect.Pointer: | ||
| return equalValues(v1.Elem(), v2.Elem()) | ||
| case reflect.Struct: | ||
| return equalStruct(v1, v2) | ||
| case reflect.Slice, reflect.Array: | ||
| if v1.Len() != v2.Len() { | ||
| return false | ||
| } | ||
| for i := range v1.Len() { | ||
| if !equalValues(v1.Index(i), v2.Index(i)) { | ||
| return false | ||
| } | ||
| } | ||
| case reflect.Map: | ||
| if v1Type.Key().Kind() == reflect.String { | ||
| return equalMapStringKey(v1, v2) | ||
| } | ||
| return reflect.DeepEqual(v1.Interface(), v2.Interface()) | ||
| default: | ||
| return reflect.DeepEqual(v1.Interface(), v2.Interface()) | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func equalStruct(s1, s2 reflect.Value) bool { | ||
| t := s1.Type() | ||
| forced1 := getForceSendFields(s1) | ||
| forced2 := getForceSendFields(s2) | ||
|
|
||
| for i := range t.NumField() { | ||
| sf := t.Field(i) | ||
| if !sf.IsExported() || sf.Name == "ForceSendFields" { | ||
| continue | ||
| } | ||
|
|
||
| // Continue traversing embedded structs. | ||
| if sf.Anonymous { | ||
| if !equalValues(s1.Field(i), s2.Field(i)) { | ||
| return false | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| jsonTag := structtag.JSONTag(sf.Tag.Get("json")) | ||
|
|
||
| v1Field := s1.Field(i) | ||
| v2Field := s2.Field(i) | ||
|
|
||
| zero1 := v1Field.IsZero() | ||
| zero2 := v2Field.IsZero() | ||
|
|
||
| if zero1 || zero2 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the other way around? Fields that are nil but are present in ForceSendFields should be set to their zero value.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure what you mean, can you post an example testcase?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider: a and b should be equal in this case, regardless of the omitempty annotation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what type is myfield? it's not a string, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Go SDK ForceSendFields is only meaningful on "basic" types https://github.com/databricks/databricks-sdk-go/blob/main/marshal/types.go#L5 in cli repo libs/structs and libs/dyn we also apply on maps and slices. However, AFAIK it is never applied it to pointers. |
||
| if jsonTag.OmitEmpty() { | ||
| if zero1 { | ||
| if !slices.Contains(forced1, sf.Name) { | ||
| v1Field = reflect.ValueOf(nil) | ||
| } | ||
| } | ||
| if zero2 { | ||
| if !slices.Contains(forced2, sf.Name) { | ||
| v2Field = reflect.ValueOf(nil) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !equalValues(v1Field, v2Field) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func equalMapStringKey(m1, m2 reflect.Value) bool { | ||
| keySet := map[string]reflect.Value{} | ||
| for _, k := range m1.MapKeys() { | ||
| // Key is always string at this point | ||
| ks := k.Interface().(string) | ||
| keySet[ks] = k | ||
| } | ||
| for _, k := range m2.MapKeys() { | ||
| ks := k.Interface().(string) | ||
| keySet[ks] = k | ||
| } | ||
|
|
||
| for _, k := range keySet { | ||
| v1 := m1.MapIndex(k) | ||
| v2 := m2.MapIndex(k) | ||
| if !equalValues(v1, v2) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
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.
Can't we just use reflect.DeepEqual here?
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.
no, see the comment about ForceSendFields
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.
This PR is prompted by a real issue btw where I did use reflect.DeepEqual but it did not do what's expected because ForceSendFields in one case was set but not in the other.
SDK always adds fields to ForceSendFields regardless of whether they are needed there or not. Our code in many places only adds it if the value is zero.