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.

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

napi_get_property_names() coerces strings to numbers if string contains a number #27496

Closed
darknoon opened this issue Apr 30, 2019 · 1 comment
Labels
node-api Issues and PRs related to the Node-API.

Comments

@darknoon
Copy link

Summary

napi_get_property_names is documented to return "A napi_value representing an array of JavaScript values that represent the property names of the object". It's very unexpected that it would convert strings to numbers, since that doesn't match the output of Object.getOwnPropertyNames:

> Object.getOwnPropertyNames({b:123, "44":56})
[ '44', 'b' ]

napi_get_property_names() output is [44,"b"]

Context

I am developing an extension with N-API via node-addon-api, and I have a function that takes arbitrary values. To ensure that it was complete, I have a unit test that passes in an object with string keys that have numbers in them:

const a = { b: 123, "44": 56 };
myFunc(a);

Inside the implementation of myFunc, I have this code

auto ov = info[0].As<Napi::Object>();
Napi::Array props = ov.GetPropertyNames();
for (uint32_t i = 0; i < length; i++) {
  if (key.IsString()) {
    // OK
  } else {
    // Throw exception for non-string key
  }
}

and I'm hitting the exception case!

in the lldb debugger, I print out the list:

> debug_json(props)
"[44,\"b\"]"

Looking at the source of Object::GetPropertyNames(), it just calls N-API napi_get_property_names(), which calls into V8 v8::Object::GetPropertyNames(), at which point it suggests to me that this is a V8 bug.

  • Version: v11.12.0
  • Platform: Darwin 2012MBP13.local 18.5.0 Darwin Kernel Version 18.5.0: Mon Mar 11 20:40:32 PDT 2019; root:xnu-4903.251.3~3/RELEASE_X86_64 x86_64
  • Subsystem: V8 / N-API
@ChALkeR ChALkeR added the node-api Issues and PRs related to the Node-API. label Apr 30, 2019
addaleax added a commit to addaleax/node that referenced this issue May 1, 2019
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.

Fixes: nodejs#27496
@addaleax
Copy link
Member

addaleax commented May 1, 2019

at which point it suggests to me that this is a V8 bug.

V8 leaves this up to the caller :) #27524 should fix this.

@danbev danbev closed this as completed in 5c08481 May 6, 2019
targos pushed a commit that referenced this issue May 6, 2019
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: #27524
Fixes: #27496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants