microsoft/kiota-abstractions-go

Updating a slice or a map from the in-memory backing store results in a panic

ffawkes opened this issue · 2 comments

ffawkes commented

While working with the Microsoft Graph SDK, I've noticed updating a slice or a map from the in-memory backing store results in a panic.

The issue should be easily reproducible with the following test case:

func TestReplaceSlice(t *testing.T) {
	memoryStore := NewInMemoryBackingStore()
	err := memoryStore.Set("key", []string{"a", "b"})
	assert.Nil(t, err)
	err = memoryStore.Set("key", []string{"b", "c"})
	assert.Nil(t, err)
}
=== RUN   TestReplaceSlice
--- FAIL: TestReplaceSlice (0.00s)
panic: runtime error: comparing uncomparable type []string [recovered]
	panic: runtime error: comparing uncomparable type []string

goroutine 17 [running]:
testing.tRunner.func1.2({0x5e3360, 0xc000184d30})
	C:/Program Files/Go/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	C:/Program Files/Go/src/testing/testing.go:1399 +0x39f
panic({0x5e3360, 0xc000184d30})
	C:/Program Files/Go/src/runtime/panic.go:884 +0x212
github.com/microsoft/kiota-abstractions-go/store.(*InMemoryBackingStore).Set(0xc00008dc38, {0x615457?, 0x2?}, {0x5d58a0?, 0xc00018a258})
	D:/git/kiota-abstractions-go/store/inmemory_backing_store.go:59 +0xc6
github.com/microsoft/kiota-abstractions-go/store.TestReplaceSlice(0x0?)
	D:/git/kiota-abstractions-go/store/inmemory_backing_store_test.go:14 +0x26b
testing.tRunner(0xc0001a6ea0, 0x640820)
	C:/Program Files/Go/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	C:/Program Files/Go/src/testing/testing.go:1493 +0x35f

The simplest fix I can think of is replacing the direct comparison with a call to reflect.DeepEqual, but performance-wise it might be better to implement a custom-written function.

baywet commented

Hi @ffawkes
Thanks for reporting this.
The idea was to implement a reference comparison. i.e. If the backing store contains a reference to a different slice, then track it as a changed value. We never intended to implement a value comparison for slices since it'd be costly. Is this something you'd be willing to submit a pull request to correct?

ffawkes commented

Sure. I'll open a PR soon.

  • 👍1