Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Comparing object with "hasOwnProperty" keys fails #58

Closed
foolip opened this issue Oct 6, 2020 · 4 comments · Fixed by #59
Closed

Comparing object with "hasOwnProperty" keys fails #58

foolip opened this issue Oct 6, 2020 · 4 comments · Fixed by #59

Comments

@foolip
Copy link

foolip commented Oct 6, 2020 •
edited
Loading

I'm trying to use deep-object-diff to compare different copies of https://github.com/mdn/browser-compat-data, which is a dataset describing the web platform itself. As such, it has a key called "hasOwnProperty" to describe that method. deep-object-diff uses code like obj.hasOwnProperty(key) a lot, and obj.hasOwnProperty will in this case be an object in the BCD data, not Object.prototype.hasOwnProperty.

This causes deep-object-diff to throw an exception, the first place being here.

deep-object-diff/src/diff/index.js

Line 12 in 45549c8

return r.hasOwnProperty(key) ? acc : { ...acc, [key]: undefined };

Simplified a bit, here's a subset of the data:

{
  "javascript": {
    "builtins": {
      "Object": {
        "hasOwnProperty": {
          "__compat": "an object with more stuff here"
        }
      }
    }
  }
}

Repro script showing the problem:

const { diff } = require("deep-object-diff");
diff({"hasOwnProperty": 1}, {"hasOwnProperty": 2});

This will throw "Uncaught TypeError: r.hasOwnProperty is not a function".

A possible fix for this would be to add a hasOwnProperty wrapper to https://github.com/mattphillips/deep-object-diff/blob/master/src/utils/index.js and always use that, but there may be other ways.
@papb
Copy link

papb commented Oct 6, 2020

https://eslint.org/docs/rules/no-prototype-builtins would catch that.

anko added a commit to anko/deep-object-diff that referenced this issue Oct 9, 2020
@anko
Add failing tests for mattphillips#58
401457d 
mattphillips#58
"Comparing object with "hasOwnProperty" keys fails"
anko added a commit to anko/deep-object-diff that referenced this issue Oct 9, 2020
@anko
Support diffing keys named like Object.prototype properties
102c2f9 
Fixes mattphillips#58.

Previously, the code assumed that input objects have a `hasOwnProperty`
function, or that at least they will acquire one when passed through
`utils.properObject`.

However, this assumption is flawed:  As noted in issue mattphillips#58, when given
input objects have a property on them called `hasOwnProperty`, it
overrides the prototype's function property that the code relied on,
causing any diffing function to error out with

    Uncaught TypeError: r.hasOwnProperty is not a function

The solution taken here is to forget about `utils.properObject`, and
instead introduce `utils.hasOwnProperty` which uses
`Object.prototype.hasOwnProperty.call` instead of assuming anything
about the input object, and replacing all direct
`inputObject.hasOwnProperty` calls with it instead.
anko added a commit to anko/deep-object-diff that referenced this issue Oct 9, 2020
@anko
Add eslint to enforce no-prototype-builtins
e310b60 
The way to prevent issues like mattphillips#58 is not obvious, and I would expect
not all programmers to have memorised the contents of
`Object.prototype`, so I think it would be prudent to check for such
issues automatically.

The no-prototype-builtins rule is enforced automatically by the set of
rules enabled in `.eslintrc.json` by

    "extends": "eslint:recommended"

It's a great sign that all the other enabled rules also pass without
modifications!

I've added the eslint invocation to `package.json`'s `posttest` field.
This should make the check minimally intrusive while developing, as it
will only run when the functional tests (via `jest`) pass first.
@anko anko mentioned this issue Oct 9, 2020
Merged
@anko
Copy link
Contributor

anko commented Oct 9, 2020

@foolip Review #59?

(Posting this because I think you don't get a notification when a PR links to your issue.)

@foolip
Copy link
Author

foolip commented Oct 21, 2020

@anko thanks for posting the fix, I've tried it and can confirm it fixes my problem.

mattphillips pushed a commit to anko/deep-object-diff that referenced this issue Jan 25, 2022
@anko @mattphillips
Add failing tests for mattphillips#58
61af8e7 
mattphillips#58
"Comparing object with "hasOwnProperty" keys fails"
mattphillips pushed a commit to anko/deep-object-diff that referenced this issue Jan 25, 2022
@anko @mattphillips
Support diffing keys named like Object.prototype properties
a26f852 
Fixes mattphillips#58.

Previously, the code assumed that input objects have a `hasOwnProperty`
function, or that at least they will acquire one when passed through
`utils.properObject`.

However, this assumption is flawed:  As noted in issue mattphillips#58, when given
input objects have a property on them called `hasOwnProperty`, it
overrides the prototype's function property that the code relied on,
causing any diffing function to error out with

    Uncaught TypeError: r.hasOwnProperty is not a function

The solution taken here is to forget about `utils.properObject`, and
instead introduce `utils.hasOwnProperty` which uses
`Object.prototype.hasOwnProperty.call` instead of assuming anything
about the input object, and replacing all direct
`inputObject.hasOwnProperty` calls with it instead.
mattphillips pushed a commit to anko/deep-object-diff that referenced this issue Jan 25, 2022
@anko @mattphillips
Add eslint to enforce no-prototype-builtins
02e3984 
The way to prevent issues like mattphillips#58 is not obvious, and I would expect
not all programmers to have memorised the contents of
`Object.prototype`, so I think it would be prudent to check for such
issues automatically.

The no-prototype-builtins rule is enforced automatically by the set of
rules enabled in `.eslintrc.json` by

    "extends": "eslint:recommended"

It's a great sign that all the other enabled rules also pass without
modifications!

I've added the eslint invocation to `package.json`'s `posttest` field.
This should make the check minimally intrusive while developing, as it
will only run when the functional tests (via `jest`) pass first.
mattphillips pushed a commit that referenced this issue Jan 25, 2022
@anko @mattphillips
Add failing tests for #58
ca791b3 
#58
"Comparing object with "hasOwnProperty" keys fails"
mattphillips pushed a commit that referenced this issue Jan 25, 2022
@anko @mattphillips
Support diffing keys named like Object.prototype properties
b281292 
Fixes #58.

Previously, the code assumed that input objects have a `hasOwnProperty`
function, or that at least they will acquire one when passed through
`utils.properObject`.

However, this assumption is flawed:  As noted in issue #58, when given
input objects have a property on them called `hasOwnProperty`, it
overrides the prototype's function property that the code relied on,
causing any diffing function to error out with

    Uncaught TypeError: r.hasOwnProperty is not a function

The solution taken here is to forget about `utils.properObject`, and
instead introduce `utils.hasOwnProperty` which uses
`Object.prototype.hasOwnProperty.call` instead of assuming anything
about the input object, and replacing all direct
`inputObject.hasOwnProperty` calls with it instead.
mattphillips pushed a commit that referenced this issue Jan 25, 2022
@anko @mattphillips
Add eslint to enforce no-prototype-builtins
98d05ee 
The way to prevent issues like #58 is not obvious, and I would expect
not all programmers to have memorised the contents of
`Object.prototype`, so I think it would be prudent to check for such
issues automatically.

The no-prototype-builtins rule is enforced automatically by the set of
rules enabled in `.eslintrc.json` by

    "extends": "eslint:recommended"

It's a great sign that all the other enabled rules also pass without
modifications!

I've added the eslint invocation to `package.json`'s `posttest` field.
This should make the check minimally intrusive while developing, as it
will only run when the functional tests (via `jest`) pass first.
@mattphillips
Copy link
Owner

Available in v1.1.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support diffing keys named like Object.prototype properties anko/deep-object-diff
4 participants
@foolip @anko @mattphillips @papb