Skip to content
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

Changelog for slices inconsistent when using pointer values #59

Open
chbiel opened this issue Jun 21, 2021 · 1 comment
Open

Changelog for slices inconsistent when using pointer values #59

chbiel opened this issue Jun 21, 2021 · 1 comment

Comments

@chbiel
Copy link

chbiel commented Jun 21, 2021

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 chbiel changed the title Changelog for slices unintuitive Changelog for slices inconsistent when using pointer values Jun 21, 2021
@purehyperbole
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants