deepdiff icon indicating copy to clipboard operation
deepdiff copied to clipboard

Stats return all zeroes when there is a change in arrays

Open christian-roggia opened this issue 4 years ago • 8 comments

The following test code does throws an error as the diff fails to pick up the changes from file A to file B:

import (
	"context"
	"encoding/json"
	"io/ioutil"
	"testing"

	"github.com/qri-io/deepdiff"
	"github.com/stretchr/testify/assert"
)

func TestJsonDiff(t *testing.T) {
	assert := assert.New(t)

	dd := deepdiff.New()

	data00a, err := ioutil.ReadFile("./testdata/regression-00-a.json")
	assert.Nil(err)

	data00b, err := ioutil.ReadFile("./testdata/regression-00-b.json")
	assert.Nil(err)

	var a, b interface{}

	assert.Nil(json.Unmarshal(data00a, &a))
	assert.Nil(json.Unmarshal(data00b, &b))

	stats, err := dd.Stat(context.TODO(), a, b)
	assert.Nil(err)

	assert.False(stats.Deletes == 0 && stats.Inserts == 0 && stats.Updates == 0)
}

testdata/regression-00-a.json:

{
    "X": [
        {
            "A": "A"
        },
        {
            "B": "B"
        }
    ],
    "Y": [
        {
            "C": "C"
        }
    ]
}

testdata/regression-00-b.json:

{
    "X": [
        {
            "A": "A"
        }
    ],
    "Z": [
        {
            "B": "B"
        },
        {
            "C": "C"
        }
    ]
}

christian-roggia avatar Jul 16 '20 22:07 christian-roggia

Please note that Z has been added, X has been updated, and Y has been deleted.

christian-roggia avatar Jul 16 '20 22:07 christian-roggia

/ping @b5 This is a critical issue in our systems and we will be forced to move away from deepdiff if we cannot solve it

christian-roggia avatar Jul 23 '20 10:07 christian-roggia

Hi @christian-roggia, apologies for the delay, we're going to be focusing on fixing this as soon as possible and hope to have it figured out by the end of the week.

dustmop avatar Jul 29 '20 14:07 dustmop

Any update on this issue?

christian-roggia avatar Sep 14 '20 23:09 christian-roggia

@christian-roggia Sorry for dropping off. After looking into this, there's some problems in deepdiff where some results are returned non-deterministically, and some where clearly wrong results are returned for maps (like your example). There's also a general lack of understanding around the codebase. We're going to need to rewrite the core algorithm in order to fix both of these problems, so it's going to take some more time. However, it is something we want to fix soon, since it's causing user-facing bugs in https://github.com/qri-io/qri. Perhaps there'll be better news in a few more weeks, but I understand if you need to switch to a different library.

dustmop avatar Sep 22 '20 00:09 dustmop

@dustmop thank you very much for sharing the current status quo of this library. We will momentarily switch to another one until the refactor is complete.

christian-roggia avatar Sep 22 '20 02:09 christian-roggia

@christian-roggia would you mind sharing the alternative library you've elected for you project. I'm looking for a similar alternative. I would enjoy to hear news from you too @dustmop

Also looking forward to help you with this rewrite once I'm able find a little more time with my side projects.

malikbenkirane avatar Oct 31 '20 10:10 malikbenkirane

@malikbenkirane we switched back to https://github.com/yudai/gojsondiff as in general we are performing relatively small diffs.

christian-roggia avatar Nov 02 '20 15:11 christian-roggia