From e5fdc30bd1bbbc009a21266d157fb2bc31554480 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 1 May 2019 23:40:02 +0200
Subject: [PATCH] n-api: make napi_get_property_names return strings

The documentation says that this method returns an array of strings.
Currently, it does not do so for indices. Resolve that by telling
V8 explicitly to convert to string.

PR-URL: https://github.com/nodejs/node/pull/27524
Fixes: https://github.com/nodejs/node/issues/27496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 src/js_native_api_v8.cc                      |  9 +++++++-
 test/js-native-api/test_object/test.js       | 23 ++++++++++++++++++++
 test/js-native-api/test_object/test_object.c | 20 +++++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index e05163c32f8265..befef0af658511 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -870,7 +870,14 @@ napi_status napi_get_property_names(napi_env env,
   v8::Local<v8::Object> obj;
   CHECK_TO_OBJECT(env, context, obj, object);
 
-  auto maybe_propertynames = obj->GetPropertyNames(context);
+  v8::MaybeLocal<v8::Array> maybe_propertynames = obj->GetPropertyNames(
+    context,
+    v8::KeyCollectionMode::kIncludePrototypes,
+    static_cast<v8::PropertyFilter>(
+        v8::PropertyFilter::ONLY_ENUMERABLE |
+        v8::PropertyFilter::SKIP_SYMBOLS),
+    v8::IndexFilter::kIncludeIndices,
+    v8::KeyConversionMode::kConvertToString);
 
   CHECK_MAYBE_EMPTY(env, maybe_propertynames, napi_generic_failure);
 
diff --git a/test/js-native-api/test_object/test.js b/test/js-native-api/test_object/test.js
index 3047b1f58dc602..d6e15dcfcef00b 100644
--- a/test/js-native-api/test_object/test.js
+++ b/test/js-native-api/test_object/test.js
@@ -202,3 +202,26 @@ assert.strictEqual(newObject.test_string, 'test string');
   assert.strictEqual(test_object.Delete(obj, 'foo'), true);
   assert.strictEqual(obj.foo, 'baz');
 }
+
+{
+  // Verify that napi_get_property_names gets the right set of property names,
+  // i.e.: includes prototypes, only enumerable properties, skips symbols,
+  // and includes indices and converts them to strings.
+
+  const object = Object.create({
+    inherited: 1
+  });
+
+  object.normal = 2;
+  object[Symbol('foo')] = 3;
+  Object.defineProperty(object, 'unenumerable', {
+    value: 4,
+    enumerable: false,
+    writable: true,
+    configurable: true
+  });
+  object[5] = 5;
+
+  assert.deepStrictEqual(test_object.GetPropertyNames(object),
+                         ['5', 'normal', 'inherited']);
+}
diff --git a/test/js-native-api/test_object/test_object.c b/test/js-native-api/test_object/test_object.c
index b6dbbd4dd33bb0..30768bdc812619 100644
--- a/test/js-native-api/test_object/test_object.c
+++ b/test/js-native-api/test_object/test_object.c
@@ -63,6 +63,25 @@ static napi_value GetNamed(napi_env env, napi_callback_info info) {
   return output;
 }
 
+static napi_value GetPropertyNames(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value args[1];
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));
+
+  NAPI_ASSERT(env, argc >= 1, "Wrong number of arguments");
+
+  napi_valuetype value_type0;
+  NAPI_CALL(env, napi_typeof(env, args[0], &value_type0));
+
+  NAPI_ASSERT(env, value_type0 == napi_object,
+      "Wrong type of arguments. Expects an object as first argument.");
+
+  napi_value output;
+  NAPI_CALL(env, napi_get_property_names(env, args[0], &output));
+
+  return output;
+}
+
 static napi_value Set(napi_env env, napi_callback_info info) {
   size_t argc = 3;
   napi_value args[3];
@@ -325,6 +344,7 @@ napi_value Init(napi_env env, napi_value exports) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_PROPERTY("Get", Get),
     DECLARE_NAPI_PROPERTY("GetNamed", GetNamed),
+    DECLARE_NAPI_PROPERTY("GetPropertyNames", GetPropertyNames),
     DECLARE_NAPI_PROPERTY("Set", Set),
     DECLARE_NAPI_PROPERTY("SetNamed", SetNamed),
     DECLARE_NAPI_PROPERTY("Has", Has),