diff icon indicating copy to clipboard operation
diff copied to clipboard

Changelog for slices inconsistent when using pointer values

Open chbiel opened this issue 4 years ago • 1 comments

We want to compare slices of strings.

Following example code:

type TestStruct struct {
	Slice  []string `json:"slice"`
}

type TestData struct {
	TS *TestStruct
}

func main() {
	s1 := TestData{&TestStruct{Slice: []string{"1", "2", "3"}}}
	s2 := TestData{&TestStruct{Slice: []string{"1", "4", "3"}}}

	d, err := diff.NewDiffer(diff.SliceOrdering(false))
	if err != nil {
		panic(err)
	}
	changes, _ := d.Diff(nil, &s2)
	if changes != nil { // for usage and debugger
		fmt.Sprintf("")
	}
	changes2, _ := d.Diff(&s1, &s2)
	if changes2 != nil { // for usage and debugger
		fmt.Sprintf("")
	}
	changes3, _ := d.Diff(&s1, nil)
	if changes3 != nil { // for usage and debugger
		fmt.Sprintf("")
	}
}

With this example there are three different Types of changes visible: changes contains a create changes2 contains a update changes3 contains a delete

The create and delete take the whole interface as the From/To values. So the path is TS.

The update on the other side lists all individual changes by index and the path is TS, Slice, 1.

My expectation would be, that the lists of changes are consistent, what means

  • either the create and delete also go down to the index level
  • or the update also shows the whole slice diff

maybe an option to make on or the other behaviour optional, would be the best case.

Do I miss something here or is this intended behaviour?

My previous concern is still value: with the above example, the update (example changes2) has one record in the changes, that says

Type: update
path: TS, Slice, 1
From: 2
To: 4

what actually (from my perspective) is wrong, as here we have an create and delete of values, and not an update.

Note: when changing the type of TestData.TS to a value and remove the pointer, the diff is consistent and all items are listed with changes based on indices. I personally prefer the diff as a whole, like given in create and delete.

EDIT: after some investigation, more info added

chbiel avatar Jun 21 '21 10:06 chbiel

Hi @chbiel, thanks for raising the issue and sorry for the late response!

There a few things to note here about the behaviour of Diff and the way it handles types;

Firstly, if you compare a pointer to a concrete type to nil, it will always return a change log that contains the entire contents of the concrete type as a create or delete (depending on the order the values are passed).

This is expected behaviour as we cannot infer any type information from a nil value. We could infer it from the value which has a concrete type by doing something like reflect.New(reflect.ValueOf(concreteType)), but this gets tricky with structs as they need to be walked recursively to setup each of their uninitialised values/types.

As the library is unlikely to support this reflection due to the addition complexity, you may be better off not comparing to nil, but rather a empty, but instantiated value like so:

var emptyData = TestData{&TestStruct{}}

func diff(a, b *TestData) {
    if a == nil {
        a = &emptyData{}
    }
    
    if b == nil {
        b = &emptyData{}
    }
    
    changelog, _ := diff.Diff(a, b)
}

Which when one of the values is nil, will produce:

[
    {
        "type": "delete",
        "path": [
            "TS",
            "Slice",
            "0"
        ],
        "from": "1",
        "to": null
    },
    {
        "type": "delete",
        "path": [
            "TS",
            "Slice",
            "1"
        ],
        "from": "4",
        "to": null
    },
    {
        "type": "delete",
        "path": [
            "TS",
            "Slice",
            "2"
        ],
        "from": "3",
        "to": null
    }
]

As for your second point, when comparing values in a slice, the reason you see changes to an item as an update, is because the path TS.Slice.1 references an index on that slice. If there is an additional item added to the slice, it will appear as a create, as the index is new. Likewise, if an item is removed, it will appear as a delete.

Just a note that the option you are using in your example, diff.SliceOrdering(false), is generally used for omitting change log entries where the contents of a slice may have been re-ordered, but it contains the same items. So with this enabled, this example would produce no changes:

s1 := TestData{&TestStruct{Slice: []string{"1", "2", "3"}}}
s2 := TestData{&TestStruct{Slice: []string{"3", "2", "1"}}}

d, err := diff.NewDiffer(diff.SliceOrdering(false))
if err != nil {
	panic(err)
}

// produces an empty changelog
changes, _ := d.Diff(&s1, &s2)

purehyperbole avatar Jun 28 '21 14:06 purehyperbole