-
Notifications
You must be signed in to change notification settings - Fork 30k
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
node-api: fix napi_get_all_property_names use of napi_key_configurable filter #42463
Conversation
Review requested:
|
Should we consider a bug fix as a breaking change? I don't think we should introduce a new workaround for bug fixes if the bug fix is not leading to fatal errors for legit use cases. |
In this PR we are introducing a new mechanism based on features that allows each module developer to choose the behavior they want:
Unlike the normal breaking changes that we had before which usually affect behavior of all addons and must be controlled by a node.js flag, this mechanism allows to:
I wonder if it would a better mechanism to adopt our changes in PR #36510 and #42208. |
I mean, why shouldn't we just fix the problem? This is not working as expected and fixing it won't introduce any fatal errors. I think fixing it will be a more straightforward option. |
I agree, but when we discussed this issue a few months ago, the agreement was that it is a behavioral change that may affect existing code. Thus, I thought it would be a good example to introduce the support of features. |
src/js_native_api.h
Outdated
#ifdef NAPI_EXPERIMENTAL | ||
NAPI_EXTERN napi_status node_api_get_features(napi_env env, | ||
node_api_features* features); | ||
NAPI_EXTERN napi_status node_api_add_features(napi_env env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would enable
and disable
be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I will rename them.
Per our discussion today in the Node-API meeting I am going to split up this PR into two parts: the bug fix and the implementation of features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, as although this can affect existing code, the behavior is wrong in a away that we don't believe any working code should be affected (based on discussion in recent Node-API team meeting)
napi_get_all_property_names(env, | ||
args[0], | ||
napi_key_include_prototypes, | ||
napi_key_enumerable | napi_key_writable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nonblocking: I think we can split out the napi_key_enumerable
to a new method to test we can actually get non-enumerable keys. Testing with an or-ed filter is an important use case too, so I think this is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legendecas, it is a good point! I did it originally that way, but the issue was that it returns too many properties such as all method names from Object
. So, adding the napi_key_enumerable
was the simplest way to work around it. Though, I can probably do it for the current object only without including prototypes.
Is that something you'd like to do before we land this or possibly as as follow on. I see @legendecas's comment is not blocking but don't want to land if you want to update before we do that. |
@mhdawson, I have added tests for own non-enumerable properties to test writable and configurable flags in isolation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Commit Queue failed- Loading data for nodejs/node/pull/42463 ✔ Done loading data for nodejs/node/pull/42463 ----------------------------------- PR info ------------------------------------ Title node-api: fix napi_get_all_property_names use of napi_key_configurable filter (#42463) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch vmoroz:PR/FixGetAllPropertyNames -> nodejs:master Labels c++, lib / src, node-api, needs-ci, commit-queue-squash Commits 4 - node-api: fix napi_get_all_property_names - remove support for features - Merge branch 'master' into PR/FixGetAllPropertyNames - add tests for non-enumerable properties Committers 1 - Vladimir Morozov PR-URL: https://github.com/nodejs/node/pull/42463 Reviewed-By: Michael Dawson Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42463 Reviewed-By: Michael Dawson Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 25 Mar 2022 06:12:10 GMT ✔ Approvals: 3 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/42463#pullrequestreview-939948462 ✔ - Chengzhong Wu (@legendecas): https://github.com/nodejs/node/pull/42463#pullrequestreview-928378619 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42463#pullrequestreview-941150971 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-04-12T18:03:32Z: https://ci.nodejs.org/job/node-test-pull-request/43463/ - Querying data for job/node-test-pull-request/43463/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 42463 From https://github.com/nodejs/node * branch refs/pull/42463/merge -> FETCH_HEAD ✔ Fetched commits as 3026ca0bf2d6..1d071f0f1c58 -------------------------------------------------------------------------------- Auto-merging src/node_api.h [master 822ca46330] node-api: fix napi_get_all_property_names Author: Vladimir Morozov Date: Thu Mar 24 22:55:39 2022 -0700 10 files changed, 245 insertions(+), 12 deletions(-) Auto-merging src/node_api.h error: commit da279a1982b27fd85605f7d8a698c94934df1ce0 is a merge but no -m option was given. fatal: cherry-pick failed [master 90936a3715] remove support for features Author: Vladimir Morozov Date: Wed Mar 30 07:39:52 2022 -0700 10 files changed, 11 insertions(+), 156 deletions(-) ✖ Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/2163283381 |
@vmoroz could you rebase and squash the commits? I tried manually and with the commit queue and it won't easily squash. |
Let me try. |
8f9623a
to
6633d34
Compare
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 6b968fd |
@vmoroz thanks for all your work/patience in getting this landed. |
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#42463 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
What is the issue?
Currently
napi_get_all_property_names
has a bug where thenapi_key_configurable
filter is used asnapi_key_writable
filter. As a result, it is not possible to filter property names bynapi_key_configurable
.Since it is a rare scenario, the issue was not observed, and there were no tests for it.
The fix description
The code is fixed to use
v8::PropertyFilter::ONLY_CONFIGURABLE
for thenapi_key_configurable
filter instead ofv8::PropertyFilter::ONLY_WRITABLE
.New test cases were added to check this scenario.