deepEqual() can fail on subclassed map

[kanongil]

Support plan

• is this issue currently blocking your project? (yes/no): somewhat
• is this issue affecting a production system? (yes/no): no

Context

• node version: 12
• module version with issue: 9.0.4
• last module version without issue:
• environment (e.g. node, browser, native): native
• used with (e.g. hapi application, another framework, standalone, ...): lab / code
• any other relevant information:

What are you trying to achieve or the steps to reproduce?

class MyMap extends Map {

    get(value, formatter) {

        if (!formatter) {
            throw new Error('formatter is missing');
        }

        return super.get(value);
    }
}

const entries = [['a', 1]];
Hoek.deepEqual(new MyMap(entries), new MyMap(entries));

What was the result you got?

Thrown error: Error: formatter is missing.

What result did you expect?

Return: true.

This issue likely also affects Set and the Hoek.clone().

[devinivy]

I followed the line of thinking that a user might legitimately want to create a subclassed Map or Set that intentionally acts as a proxy for viewing its contents. Here's an incomplete example to illustrate what I mean:

class MultiplierMap extends Map {

    constructor(entries, multiplier = 1) {

        super(entries);

        this.multiplier = multiplier;
    }

    get(value) {

        return this.multiplier * super.get(value);
    }
}

I determined that it's a decent amount of work to follow through on this coherently (including iteration, entries, values, etc.) and that your use-case and proposed fix seems more natural, though its a bit unfortunate to not be able to assume the same interface as Map or Set. If you have any thoughts to even further sway me, I'm interested to hear them.

[kanongil]

@devinivy Your example still works with my patch, including any other idempotent sub-classing. Using the base methods, we just compare the raw, hidden stored items instead. If you compare multiple such maps, with similar entries, but different multipliers, it would fail once it checks the multiplier property.

Eg. this fails, both new and old:

Hoek.deepEqual(new MultiplierMap([['a', 1]], 2), new MultiplierMap([['a', 2]], 1);

If the subclass includes non-idempotent methods, or relies on hidden state (eg. in a # property) for the computation, all bets are off, and there are no mechanisms to reliably determine if they are truly equal. It can only ever be based on guesswork.

[kanongil]

@devinivy The primary reason something needs to be done, is that it will currently throw, which is highly unexpected. The solution is to compare the raw values, using the language accessors. This will make most cases work, as expected.

The only alternative, is to catch the error internally and return a new value, eg. undefined.

FYI, doing a deepEqual() on user-defined objects, is always a best guess scenario. Any of its methods could respond using hidden state. Eg:

const counter = 0;

class Car {

    #markup;

    constructor(price) {

        this._price = price;
        this.#markup = counter++;
    }

    get price() {

        return this._price + this.#markup;
    }
}

const carA = new Car(10);
const carB = new Car(10);

Hoek.deepEqual(carA, carB); // => true
Hoek.deepEqual(carA.price, carB.price); // => false

In essence deepEqual() just asserts that all the observable properties match.

[cjihrig]

I think Hoek.deepEqual() is missing some other validation as well:

const entries = [['c', 1]];
const a = new Map(entries);
const b = new Map(entries);
b.get = (key) => { Map.prototype.get.call(b, key); };

Node's assert.deepEqual() properly identifies this case as not equal, while Hoek misses it.

[kanongil]

Node's assert.deepEqual() properly identifies this case as not equal, while Hoek misses it.

Not for me:

Hoek.deepEqual(a, b); // => false
Hoek.deepEqual(b, a); // => false

[cjihrig]

Apologies. You're right. It was a problem in my test case.

[devinivy]

Makes sense, thanks for laying that out @kanongil!