Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 215
Add libs/structdiff#2928
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
Uh oh!
There was an error while loading. Please reload this page.
Add libs/structdiff #2928
Changes from all commits
8dcbf37140650e059d6fbd011c685b86e3bd40a6a432c7e251365c231a5ecb511bb3cb76e68e3df206c6e9c7977a3c3b29652c06c9ac38afec0cb162fa2a48fa34ce6b4ed6ef9d863cd3d4fb99a2f5d0e50b7bd354fbfaa6dd52858acf2deFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| #!/bin/bash | ||
| set -ex | ||
| exec go test -bench=. -benchmem -run ^x |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package structdiff | ||
| import ( | ||
| "encoding/json" | ||
| "strings" | ||
| "testing" | ||
| "github.com/databricks/databricks-sdk-go/service/jobs" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
| func bench(b *testing.B, job1, job2 string) { | ||
| var x, y jobs.JobSettings | ||
| require.NoError(b, json.Unmarshal([]byte(job1), &x)) | ||
| require.NoError(b, json.Unmarshal([]byte(job2), &y)) | ||
| total := 0 | ||
| b.ResetTimer() | ||
| for range b.N { | ||
| changes, err := GetStructDiff(&x, &y) | ||
| if err != nil { | ||
| b.Fatalf("error: %s", err) | ||
| } | ||
| total += len(changes) | ||
| } | ||
| b.StopTimer() | ||
| b.Logf("Total: %d / %d", total, b.N) | ||
| } | ||
| func BenchmarkEqual(b *testing.B) { | ||
| bench(b, jobExampleResponse, jobExampleResponse) | ||
| } | ||
| func BenchmarkChanges(b *testing.B) { | ||
| job2 := strings.ReplaceAll(jobExampleResponse, "1", "2") | ||
| bench(b, jobExampleResponse, job2) | ||
| } | ||
| func BenchmarkZero(b *testing.B) { | ||
| bench(b, jobExampleResponse, jobExampleResponseZeroes) | ||
| } | ||
| func BenchmarkNils(b *testing.B) { | ||
| bench(b, jobExampleResponse, jobExampleResponseNils) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,214 @@ | ||||||
| package structdiff | ||||||
| import ( | ||||||
| "fmt" | ||||||
| "reflect" | ||||||
| "slices" | ||||||
| "sort" | ||||||
| "strconv" | ||||||
| "strings" | ||||||
| ) | ||||||
| type Change struct { | ||||||
| Field string | ||||||
| Old any | ||||||
| New any | ||||||
| } | ||||||
| type pathNode struct { | ||||||
| Prev *pathNode | ||||||
| Key string | ||||||
| // If Index >= 0, the node specifies a slice/array index in Index. | ||||||
| // If Index == -1, the node specifies a struct attribute in Key | ||||||
| // If Index == -2, the node specifies a map key in Key | ||||||
| Index int | ||||||
| } | ||||||
| func (p *pathNode) String() string { | ||||||
| if p == nil { | ||||||
| return "" | ||||||
| } | ||||||
| if p.Index >= 0 { | ||||||
| return p.Prev.String() + "[" + strconv.Itoa(p.Index) + "]" | ||||||
| } | ||||||
| if p.Index == -1 { | ||||||
| return p.Prev.String() + "." + p.Key | ||||||
| } | ||||||
| return fmt.Sprintf("%s[%q]", p.Prev.String(), p.Key) | ||||||
| } | ||||||
| // GetStructDiff compares two Go structs and returns a list of Changes or an error. | ||||||
| // Respects ForceSendFields if present. | ||||||
| // Types of a and b must match exactly, otherwise returns an error. | ||||||
| func GetStructDiff(a, b any) ([]Change, error) { | ||||||
| v1 := reflect.ValueOf(a) | ||||||
| v2 := reflect.ValueOf(b) | ||||||
| if !v1.IsValid() && !v2.IsValid() { | ||||||
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. Just curious, Do we encounter invalid values in practice? Or are we just being defensive here? ContributorAuthor 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. Yes, if both a and b are nils. | ||||||
| return nil, nil | ||||||
| } | ||||||
| var changes []Change | ||||||
| if !v1.IsValid() || !v2.IsValid() { | ||||||
| changes = append(changes, Change{Field: "", Old: v1.Interface(), New: v2.Interface()}) | ||||||
| return changes, nil | ||||||
| } | ||||||
| if v1.Type() != v2.Type() { | ||||||
| return nil, fmt.Errorf("type mismatch: %v vs %v", v1.Type(), v2.Type()) | ||||||
| } | ||||||
| diffValues(nil, v1, v2, &changes) | ||||||
| return changes, nil | ||||||
| } | ||||||
| // diffValues appends changes between v1 and v2 to the slice. path is the current | ||||||
| // JSON-style path (dot + brackets). At the root path is "". | ||||||
| func diffValues(path *pathNode, v1, v2 reflect.Value, changes *[]Change) { | ||||||
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. (not blocking) Have you also consider having this function return changes? Slightly easier to reason about. Same for other functions.
Suggested change
The callsite then can be ContributorAuthor 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's the benefit? It's more verbose on the caller side. 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. The benefits are, it mirrors how Not blocking from my POV, but seems marginally better. | ||||||
| if !v1.IsValid() { | ||||||
| if !v2.IsValid() { | ||||||
| return | ||||||
| } | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: nil, New: v2.Interface()}) | ||||||
| return | ||||||
| } else if !v2.IsValid() { | ||||||
| // v1 is valid | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: nil}) | ||||||
| return | ||||||
| } | ||||||
| v1Type := v1.Type() | ||||||
| // This should not happen; if it does, record this a full change | ||||||
| if v1Type != v2.Type() { | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: v2.Interface()}) | ||||||
| return | ||||||
| } | ||||||
| kind := v1.Kind() | ||||||
| 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 | ||||||
| } | ||||||
| if v1Nil || v2Nil { | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: v2.Interface()}) | ||||||
| return | ||||||
| } | ||||||
| } | ||||||
| switch kind { | ||||||
| case reflect.Pointer: | ||||||
| diffValues(path, v1.Elem(), v2.Elem(), changes) | ||||||
| case reflect.Struct: | ||||||
| diffStruct(path, v1, v2, changes) | ||||||
| case reflect.Slice, reflect.Array: | ||||||
| if v1.Len() != v2.Len() { | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: v2.Interface()}) | ||||||
| } else { | ||||||
| for i := range v1.Len() { | ||||||
| node := pathNode{Prev: path, Index: i} | ||||||
| diffValues(&node, v1.Index(i), v2.Index(i), changes) | ||||||
| } | ||||||
| } | ||||||
| case reflect.Map: | ||||||
| if v1Type.Key().Kind() == reflect.String { | ||||||
| diffMapStringKey(path, v1, v2, changes) | ||||||
| } else { | ||||||
| deepEqualValues(path, v1, v2, changes) | ||||||
| } | ||||||
| default: | ||||||
| deepEqualValues(path, v1, v2, changes) | ||||||
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. A struct with a function callback as a field will always never be equal to itself due to deepequal semantics. Can that be a problem? If so we might want to ignore functional fields in ContributorAuthor 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. I guess you're right. I don't have a use case involving structs with callback fields though. | ||||||
| } | ||||||
| } | ||||||
| func deepEqualValues(path *pathNode, v1, v2 reflect.Value, changes *[]Change) { | ||||||
| if !reflect.DeepEqual(v1.Interface(), v2.Interface()) { | ||||||
| *changes = append(*changes, Change{Field: path.String(), Old: v1.Interface(), New: v2.Interface()}) | ||||||
| } | ||||||
| } | ||||||
| func diffStruct(path *pathNode, s1, s2 reflect.Value, changes *[]Change) { | ||||||
| t := s1.Type() | ||||||
| forced1 := getForceSendFields(s1) | ||||||
| forced2 := getForceSendFields(s2) | ||||||
| for i := range t.NumField() { | ||||||
| sf := t.Field(i) | ||||||
| if !sf.IsExported() || sf.Name == "ForceSendFields" { | ||||||
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. You can specify ContributorAuthor 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, let's see a few specific use cases first. This can also be done by the caller btw. It's possible that we'd have to change this function to look at json tags more (for example, report json names instead of golang names). In that case, we'd probably want to ignore these. | ||||||
| continue | ||||||
| } | ||||||
| node := pathNode{Prev: path, Key: sf.Name, Index: -1} | ||||||
| v1Field := s1.Field(i) | ||||||
| v2Field := s2.Field(i) | ||||||
| hasOmitEmpty := strings.Contains(sf.Tag.Get("json"), "omitempty") | ||||||
| if hasOmitEmpty { | ||||||
| if v1Field.IsZero() { | ||||||
| if !slices.Contains(forced1, sf.Name) { | ||||||
| v1Field = reflect.ValueOf(nil) | ||||||
| } | ||||||
| } | ||||||
| if v2Field.IsZero() { | ||||||
| if !slices.Contains(forced2, sf.Name) { | ||||||
| v2Field = reflect.ValueOf(nil) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| diffValues(&node, v1Field, v2Field, changes) | ||||||
| } | ||||||
| } | ||||||
| func diffMapStringKey(path *pathNode, m1, m2 reflect.Value, changes *[]Change) { | ||||||
| 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 | ||||||
| } | ||||||
| var keys []string | ||||||
| for s := range keySet { | ||||||
| keys = append(keys, s) | ||||||
| } | ||||||
| sort.Strings(keys) | ||||||
| for _, ks := range keys { | ||||||
| k := keySet[ks] | ||||||
| v1 := m1.MapIndex(k) | ||||||
| v2 := m2.MapIndex(k) | ||||||
| node := pathNode{ | ||||||
| Prev: path, | ||||||
| Key: ks, | ||||||
| Index: -2, | ||||||
| } | ||||||
| diffValues(&node, v1, v2, changes) | ||||||
| } | ||||||
| } | ||||||
| func getForceSendFields(v reflect.Value) []string { | ||||||
| if !v.IsValid() || v.Kind() != reflect.Struct { | ||||||
| return nil | ||||||
| } | ||||||
| fsField := v.FieldByName("ForceSendFields") | ||||||
| if !fsField.IsValid() || fsField.Kind() != reflect.Slice { | ||||||
| return nil | ||||||
| } | ||||||
| result, ok := fsField.Interface().([]string) | ||||||
| if ok { | ||||||
| return result | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
(not blocking) This can be confusing in isolation. Maybe a separate enum type? For array vs struct vs map?
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.
Separate enum type would increase size for no good reason.
We could add methods that hide this details -- that would be good if this was an exported struct. But currently it's a local helper.