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

node-api: fix napi_get_all_property_names use of napi_key_configurable filter #42463

Closed
wants to merge 1 commit into from
Closed

node-api: fix napi_get_all_property_names use of napi_key_configurable filter #42463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,8 +936,8 @@ napi_status napi_get_all_property_names(napi_env env,
filter | v8::PropertyFilter::ONLY_ENUMERABLE);
}
if (key_filter & napi_key_configurable) {
filter = static_cast<v8::PropertyFilter>(filter |
v8::PropertyFilter::ONLY_WRITABLE);
filter = static_cast<v8::PropertyFilter>(
filter | v8::PropertyFilter::ONLY_CONFIGURABLE);
}
if (key_filter & napi_key_skip_strings) {
filter = static_cast<v8::PropertyFilter>(filter |
Expand Down
50 changes: 47 additions & 3 deletions test/js-native-api/test_object/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const object = {
assert.strictEqual(test_object.Get(object, 'hello'), 'world');
assert.strictEqual(test_object.GetNamed(object, 'hello'), 'world');
assert.deepStrictEqual(test_object.Get(object, 'array'),
[ 1, 94, 'str', 12.321, { test: 'obj in arr' } ]);
[1, 94, 'str', 12.321, { test: 'obj in arr' }]);
assert.deepStrictEqual(test_object.Get(object, 'newObject'),
{ test: 'obj in obj' });

Expand Down Expand Up @@ -54,7 +54,7 @@ assert.strictEqual(newObject.test_string, 'test string');

{
// Verify that napi_has_own_property() fails if property is not a name.
[true, false, null, undefined, {}, [], 0, 1, () => {}].forEach((value) => {
[true, false, null, undefined, {}, [], 0, 1, () => { }].forEach((value) => {
assert.throws(() => {
test_object.HasOwn({}, value);
}, /^Error: A string or symbol was expected$/);
Expand Down Expand Up @@ -240,13 +240,57 @@ assert.strictEqual(newObject.test_string, 'test string');
writable: true,
configurable: true
});
Object.defineProperty(object, 'writable', {
value: 4,
enumerable: true,
writable: true,
configurable: false
});
Object.defineProperty(object, 'configurable', {
value: 4,
enumerable: true,
writable: false,
configurable: true
});
object[5] = 5;

assert.deepStrictEqual(test_object.GetPropertyNames(object),
['5', 'normal', 'inherited']);
['5',
'normal',
'writable',
'configurable',
'inherited']);

assert.deepStrictEqual(test_object.GetSymbolNames(object),
[fooSymbol]);

assert.deepStrictEqual(test_object.GetEnumerableWritableNames(object),
['5',
'normal',
'writable',
fooSymbol,
'inherited']);

assert.deepStrictEqual(test_object.GetOwnWritableNames(object),
['5',
'normal',
'unenumerable',
'writable',
fooSymbol]);

assert.deepStrictEqual(test_object.GetEnumerableConfigurableNames(object),
['5',
'normal',
'configurable',
fooSymbol,
'inherited']);

assert.deepStrictEqual(test_object.GetOwnConfigurableNames(object),
['5',
'normal',
'unenumerable',
'configurable',
fooSymbol]);
}

// Verify that passing NULL to napi_set_property() results in the correct
Expand Down
117 changes: 117 additions & 0 deletions test/js-native-api/test_object/test_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,119 @@ static napi_value GetSymbolNames(napi_env env, napi_callback_info info) {
return output;
}

static napi_value GetEnumerableWritableNames(napi_env env,
napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NODE_API_ASSERT(env, argc >= 1, "Wrong number of arguments");

napi_valuetype value_type0;
NODE_API_CALL(env, napi_typeof(env, args[0], &value_type0));

NODE_API_ASSERT(
env,
value_type0 == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

napi_value output;
NODE_API_CALL(
env,
napi_get_all_property_names(env,
args[0],
napi_key_include_prototypes,
napi_key_enumerable | napi_key_writable,
Copy link
Member

@legendecas legendecas Apr 1, 2022 •
edited
Loading

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.

Copy link
Member Author

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.

napi_key_numbers_to_strings,
&output));

return output;
}

static napi_value GetOwnWritableNames(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NODE_API_ASSERT(env, argc >= 1, "Wrong number of arguments");

napi_valuetype value_type0;
NODE_API_CALL(env, napi_typeof(env, args[0], &value_type0));

NODE_API_ASSERT(
env,
value_type0 == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

napi_value output;
NODE_API_CALL(env,
napi_get_all_property_names(env,
args[0],
napi_key_own_only,
napi_key_writable,
napi_key_numbers_to_strings,
&output));

return output;
}

static napi_value GetEnumerableConfigurableNames(napi_env env,
napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NODE_API_ASSERT(env, argc >= 1, "Wrong number of arguments");

napi_valuetype value_type0;
NODE_API_CALL(env, napi_typeof(env, args[0], &value_type0));

NODE_API_ASSERT(
env,
value_type0 == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

napi_value output;
NODE_API_CALL(
env,
napi_get_all_property_names(env,
args[0],
napi_key_include_prototypes,
napi_key_enumerable | napi_key_configurable,
napi_key_numbers_to_strings,
&output));

return output;
}

static napi_value GetOwnConfigurableNames(napi_env env,
napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NODE_API_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NODE_API_ASSERT(env, argc >= 1, "Wrong number of arguments");

napi_valuetype value_type0;
NODE_API_CALL(env, napi_typeof(env, args[0], &value_type0));

NODE_API_ASSERT(
env,
value_type0 == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

napi_value output;
NODE_API_CALL(env,
napi_get_all_property_names(env,
args[0],
napi_key_own_only,
napi_key_configurable,
napi_key_numbers_to_strings,
&output));

return output;
}

static napi_value Set(napi_env env, napi_callback_info info) {
size_t argc = 3;
napi_value args[3];
Expand Down Expand Up @@ -536,6 +649,10 @@ napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("GetNamed", GetNamed),
DECLARE_NODE_API_PROPERTY("GetPropertyNames", GetPropertyNames),
DECLARE_NODE_API_PROPERTY("GetSymbolNames", GetSymbolNames),
DECLARE_NODE_API_PROPERTY("GetEnumerableWritableNames", GetEnumerableWritableNames),
DECLARE_NODE_API_PROPERTY("GetOwnWritableNames", GetOwnWritableNames),
DECLARE_NODE_API_PROPERTY("GetEnumerableConfigurableNames", GetEnumerableConfigurableNames),
DECLARE_NODE_API_PROPERTY("GetOwnConfigurableNames", GetOwnConfigurableNames),
DECLARE_NODE_API_PROPERTY("Set", Set),
DECLARE_NODE_API_PROPERTY("SetNamed", SetNamed),
DECLARE_NODE_API_PROPERTY("Has", Has),
Expand Down