node-api: fix napi_get_all_property_names use of napi_key_configurable filter by vmoroz · Pull Request #42463 · nodejs/node

vmoroz · 2022-03-25T06:12:10Z

What is the issue?

Currently napi_get_all_property_names has a bug where the napi_key_configurable filter is used as napi_key_writable filter. As a result, it is not possible to filter property names by napi_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 the napi_key_configurable filter instead of v8::PropertyFilter::ONLY_WRITABLE.
New test cases were added to check this scenario.

nodejs-github-bot · 2022-03-25T06:12:13Z

Review requested:

@nodejs/node-api

legendecas · 2022-03-25T06:21:47Z

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.

vmoroz · 2022-03-25T13:38:34Z

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:

In case if developer did not recompile addon code, then it is going to continue to behave the same way as it was before because the nm_features field in the napi_module is going to be initialized to 0, or whatever value it used to have before (in future).
As soon as the addon is recompiled, it will acquire the new behavior only if it adopts a new version of Node-API.
If a specific new feature is breaking the addon's code in the new Node-API version, developers can choose to exclude that feature just for that addon.
The cool part is that using the node_api_add_features and node_api_remove_features allows developers to control the new behavior for a subset of code. E.g. in the test_object.c we check the old the new behavior by turning node_api_features_use_configurable_key_filter feature "off" and "on".

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:

control the new behavior per addon and be adopted only after it is recompiled;
backport fixes to previous versions of Node.js without causing widespread code breaks because new features are "on" by default only in the newer versions of Node-API, and addons compiled against the old version will not "see" them. The addons that require the new behavior in older versions can force to turn it "on" after it is backported.

I wonder if it would a better mechanism to adopt our changes in PR #36510 and #42208.

legendecas · 2022-03-25T13:47:43Z

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.

vmoroz · 2022-03-25T13:52:50Z

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.
For that specific issue if we agree that we do not need to use the features, then I can remove them from this PR and introduce them in another PR.

mhdawson · 2022-03-25T15:37:38Z

src/js_native_api.h

+#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,


Would enable and disable be better?

Sounds good! I will rename them.

vmoroz · 2022-03-25T18:12:08Z

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.

mhdawson

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)

legendecas · 2022-04-01T02:46:42Z

test/js-native-api/test_object/test_object.c

+      napi_get_all_property_names(env,
+                                  args[0],
+                                  napi_key_include_prototypes,
+                                  napi_key_enumerable | napi_key_writable,


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.

@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.

mhdawson · 2022-04-07T21:13:16Z

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.

vmoroz · 2022-04-12T15:38:00Z

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 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.

mhdawson

LGTM

nodejs-github-bot · 2022-04-12T18:03:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43463/

nodejs-github-bot · 2022-04-13T19:19:31Z

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 patches

https://github.com/nodejs/node/actions/runs/2163283381

mhdawson · 2022-04-13T19:23:51Z

@vmoroz could you rebase and squash the commits? I tried manually and with the commit queue and it won't easily squash.

vmoroz · 2022-04-13T20:31:01Z

@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.

nodejs-github-bot · 2022-04-29T15:40:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43777/

nodejs-github-bot · 2022-05-02T20:09:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43839/

nodejs-github-bot · 2022-05-06T15:40:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43903/

nodejs-github-bot · 2022-05-06T21:31:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43908/

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson · 2022-05-10T19:47:58Z

Landed in 6b968fd

mhdawson · 2022-05-10T19:48:13Z

@vmoroz thanks for all your work/patience in getting this landed.

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node#42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 25, 2022

legendecas added the node-api Issues and PRs related to the Node-API. label Mar 25, 2022

mhdawson reviewed Mar 25, 2022

View reviewed changes

mhdawson approved these changes Mar 31, 2022

View reviewed changes

legendecas approved these changes Apr 1, 2022

View reviewed changes

legendecas reviewed Apr 1, 2022

View reviewed changes

mhdawson approved these changes Apr 12, 2022

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2022

jasnell approved these changes Apr 13, 2022

View reviewed changes

mhdawson added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 13, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 13, 2022

vmoroz force-pushed the PR/FixGetAllPropertyNames branch from 8f9623a to 6633d34 Compare April 13, 2022 21:16

vmoroz closed this Apr 13, 2022

github-actions bot mentioned this pull request Apr 29, 2022

CI Reliability 2022-04-29 nodejs/reliability#266

Open

21 tasks

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2022

This was referenced Apr 30, 2022

CI Reliability 2022-04-30 nodejs/reliability#267

Open

CI Reliability 2022-05-01 nodejs/reliability#268

Open

CI Reliability 2022-05-02 nodejs/reliability#269

Open

This was referenced May 3, 2022

CI Reliability 2022-05-03 nodejs/reliability#270

Open

CI Reliability 2022-05-04 nodejs/reliability#271

Open

mhdawson pushed a commit that referenced this pull request May 10, 2022

node-api: fix napi_get_all_property_names

6b968fd

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson closed this May 10, 2022

This was referenced May 12, 2022

CI Reliability 2022-05-12 nodejs/reliability#273

Open

CI Reliability 2022-05-12 nodejs/reliability#275

Closed

CI Reliability 2022-05-13 nodejs/reliability#276

Open

CI Reliability 2022-05-14 nodejs/reliability#277

Open

vmoroz deleted the PR/FixGetAllPropertyNames branch May 15, 2022 22:59

BethGriggs pushed a commit that referenced this pull request May 16, 2022

node-api: fix napi_get_all_property_names

e2462a2

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

RafaelGSS mentioned this pull request May 16, 2022

v18.2.0 proposal #43036

Merged

juanarbol pushed a commit that referenced this pull request May 31, 2022

node-api: fix napi_get_all_property_names

e6ab565

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

danielleadams pushed a commit that referenced this pull request Jun 27, 2022

node-api: fix napi_get_all_property_names

0db7ac7

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Jul 12, 2022

node-api: fix napi_get_all_property_names

417d019

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

node-api: fix napi_get_all_property_names

49103e3

PR-URL: #42463 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Uh oh!

Conversation

vmoroz commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the issue?

The fix description

Uh oh!

nodejs-github-bot commented Mar 25, 2022

Uh oh!

legendecas commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmoroz commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legendecas commented Mar 25, 2022

Uh oh!

vmoroz commented Mar 25, 2022

Uh oh!

mhdawson Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

vmoroz Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

vmoroz commented Mar 25, 2022

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

legendecas Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 7, 2022

Uh oh!

vmoroz commented Apr 12, 2022

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 12, 2022

Uh oh!

nodejs-github-bot commented Apr 13, 2022

Uh oh!

mhdawson commented Apr 13, 2022

Uh oh!

vmoroz commented Apr 13, 2022

Uh oh!

nodejs-github-bot commented Apr 29, 2022

Uh oh!

nodejs-github-bot commented May 2, 2022

Uh oh!

nodejs-github-bot commented May 6, 2022

Uh oh!

nodejs-github-bot commented May 6, 2022

Uh oh!

mhdawson commented May 10, 2022

Uh oh!

mhdawson commented May 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vmoroz commented Mar 25, 2022 •

edited

Loading

legendecas commented Mar 25, 2022 •

edited

Loading

vmoroz commented Mar 25, 2022 •

edited

Loading

legendecas Apr 1, 2022 •

edited

Loading