Skip to content

ext/node: assert.deepStrictEqual({}, Object.create(null)) should throw #27565

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

Open
kt3k opened this issue Jan 6, 2025 · 5 comments · May be fixed by #29428
Open

ext/node: assert.deepStrictEqual({}, Object.create(null)) should throw #27565

kt3k opened this issue Jan 6, 2025 · 5 comments · May be fixed by #29428
Labels
bug Something isn't working correctly node compat

Comments

@kt3k
Copy link
Member

kt3k commented Jan 6, 2025

The following script throws in Node, but not in Deno:

import assert from "node:assert"
assert.deepStrictEqual({}, Object.create(null))
@kt3k kt3k added bug Something isn't working correctly node compat labels Jan 6, 2025
@edilson258
Copy link

Hello, @kt3k

Please assign me this one.

@bartlomieju
Copy link
Member

@edilson258 feel free to work on this. We only assign issues to Deno team members.

@edilson258
Copy link

There are some tests failing because they expect the old behavior where deepStrictEqual({}, Object.create(null)) is true, like:

t.assert.deepStrictEqual(
db.prepare('BEGIN').run(),
{ changes: 0, lastInsertRowid: 0 },
);
t.assert.deepStrictEqual(
db.prepare('INSERT INTO data (key) VALUES (100)').run(),
{ changes: 1, lastInsertRowid: 100 },
);
t.assert.deepStrictEqual(
db.prepare('COMMIT').run(),
{ changes: 1, lastInsertRowid: 100 },
);

All the above assertions now fails because db.prepare('BEGIN').run() returns an object whose prototype is null __proto__: null, whereas the expected object { changes: 0, lastInsertRowid: 0 } has the default Object.prototype. Since assert.deepStrictEqual NOW checks both property values and the prototype chain, the difference in prototypes causes the assertion to fail.

I think that setting __proto__: null in the expected object is the best solution because it ensures that the object has no prototype and is an approach that is already being used in similar cases, like:

t.assert.deepStrictEqual(
db.prepare('SELECT * FROM data').all(),
[{ __proto__: null, key: 100 }],
);

There are lots of assertions in the code base that rely on that old behavior, this patch will touch lots of files in the source tree.

Is that proposed change acceptable?

@kt3k
@bartlomieju

@kt3k
Copy link
Member Author

kt3k commented May 26, 2025

@edilson258 Thanks for working on deepStrictEqual. The change in deepStrictEqual looks good to me.

The test files under tests/node_compat/test/parallel are copied from Node.js repository. Those tests are passing with Node.js. I think the issue here is the implementation of node:sqlite in Deno

@edilson258
Copy link

I think the issue here is the implementation of node:sqlite in Deno

True, but the issue isn't limited to node:sqlite, as the patch has caused multiple deepStrictEqual assertions to fail across a range of tests, including many that are unrelated to node:sqlite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants