Fix getCache() internal key filtering

[sdesai]

I didn't get a chance to get the context around this PR from Michael in person before he stepped out on vacation: #640 but it seems to have regressed model.getCache() behavior, by allowing some internal keys to remain in the results of model.getCache() (e.g. refN, ref-index, etc). It seems to be a pretty conscious change, so not sure if it was intentional.

However, in the absence of this context, it seems like the desired behavior is to strip ALL internal keys from the cache, when returning a copy through getCache().

This PR aims to fix the above regression, and also fix what seems to be a gap in the original implementation, where $size would still be left in the results of getCache() at the leaf/sentinel nodes, when cloning boxed values. I don't think this was intentional.

Unit tests added, to make sure:

1. Stripping refN, ref-index doesn't result in references breaking when the copy of the cache is used to re-hydrate a new model
2. Make sure no internal keys are left in the cache-copy returned by getCache()

@jhusain @michaelbpaulson @trxcllnt - eyes appreciated, especially since I'm making assumptions about the original design intention.

[trxcllnt]

@sdesai 😍 thanks! cc: @ekosz

[sdesai]

This was another part of the original implementation I didn't understand the motivation for.

I'm not sure what the cacheNext.value === 'object' check was trying to do. We didn't end up cloning cacheNext.value anyway (that is, it's not like we deep cloned the wrapper if we discovered that the value was an 'object').

It seems to imply we only want to maintain the wrapper, for values which are objects (or for nodes which are pre-wrapped by the user - that makes sense), and I don't understand the motivation for the 'values which are objects' decision.

[ThePrimeagen]

@sdesai All is correct. The motivation for 'values which are objects' decision is a subtle one.

var m = new Model();
var m2 = new Model();

// set a complex object.
m.setValue(['foo'], {bar: 'baz'}).subscribe();

// Set the cache from m to m2
m2.setCache(m.getCache());

Without the check for objectyness the following would happen.

m2.get(['foo']).subscribe(log, log, log.bind(null, 'i completed!'));

This would simply console out i completed! because the underlying cache changed from m to m2.

m and m2's cache would be

// m
{
    foo: {
        $type: 'atom',
        value: {
            bar: 'baz'
        }
    }
}

//m2
{
    foo: {
            bar: {
                $type: 'atom',
                value: 'baz'
            }
    }
}

Hope that helps!

[sdesai]

I see. Thanks. Makes sense. It's to distinguish between branches and atoms, and we don't need to take on the cost of the wrapper/clone for the non-object case, because there's nothing ambiguous to distinguish against.

Gonna merge this.

[trxcllnt]

@sdesai is this going to be merged into master soon?

[sdesai]

Hey Paul, I believe master has the original (prefix) based filter already. Otherwise can merge it in. This one does have the additional fix for sentinel wrappers

[trxcllnt]

@sdesai from what I can tell, master's version of isInternalKey is still checking for specific internal keys.

[sdesai]

Ok. I'm OOO for Xmas, but will merge it in, in between things