From 7bd4427d48cdd6d0e739238fda4b82d34d042344 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Tue, 5 Oct 2021 15:24:28 -0700 Subject: [PATCH 1/6] Integer array input, as in uber/h3-js#91 --- h3napi.c | 32 ++++++++++++++++++++++++++------ test/index.js | 16 ++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/h3napi.c b/h3napi.c index 229780b..088e968 100644 --- a/h3napi.c +++ b/h3napi.c @@ -70,15 +70,35 @@ if (napiArrayLen ## A ## N != napi_ok) #define napiGetH3IndexArg(I, O) \ + H3Index O;\ char O ## Str[17];\ size_t O ## StrCount;\ \ - if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) != napi_ok) {\ - napi_throw_error(env, "EINVAL", "Expected string h3 index in arg " #I);\ - return NULL;\ - }\ - \ - H3Index O = stringToH3(O ## Str); + if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\ + O = stringToH3(O ## Str);\ + } else {\ + napiGetNapiArg(I, O ## Arr);\ + uint32_t O ## ArrSz;\ + if (napi_get_array_length(env, O ## Arr, &(O ## ArrSz)) != napi_ok) {\ + napi_throw_error(env, "EINVAL", "Expected string or array H3 index in arg " #I);\ + return NULL;\ + }\ + if (O ## ArrSz != 2) {\ + napi_throw_error(env, "EINVAL", "Invalid length array H3 index in arg " #I);\ + return NULL;\ + }\ + napi_value O ## Val0, O ## Val1;\ + if (napi_get_element(env, O ## Arr, 0, &(O ## Val0)) != napi_ok || napi_get_element(env, O ## Arr, 1, &(O ## Val1)) != napi_ok) {\ + napi_throw_error(env, "EINVAL", "Invalid array H3 index in arg " #I);\ + return NULL;\ + }\ + uint32_t O ## Part0, O ## Part1;\ + if (napi_get_value_uint32(env, O ## Val0, &(O ## Part0)) != napi_ok || napi_get_value_uint32(env, O ## Val1, &(O ## Part1)) != napi_ok) {\ + napi_throw_error(env, "EINVAL", "Invalid integer array H3 index in arg " #I);\ + return NULL;\ + }\ + O = ((H3Index)(O ## Part1) << 32) | (O ## Part0);\ + } #define napiGetH3Index(I, O) \ char O ## Str[17];\ diff --git a/test/index.js b/test/index.js index a5920cc..18fd413 100644 --- a/test/index.js +++ b/test/index.js @@ -113,6 +113,22 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') => const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') => exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName) +exports['h3IsValid_array'] = test => { + test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index'); + test.ok( + !h3node.h3IsValid([0x73fffffff, 0xff2834]), + 'Integer with incorrect bits is not considered an index' + ); + // TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing + test.throws(() => h3node.h3IsValid([]), 'Empty array is not valid'); + test.throws(() => h3node.h3IsValid([1]), 'Array with a single element is not valid'); + test.throws(() => + h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), + 'Array with an additional element is not valid' + ); + test.done(); +}; + exportTest('geoToH3', () => [...randCoords(), 9], simpleTest) exportTest('h3ToGeo', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) exportTest('h3ToGeoBoundary', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) From 2ec5868b19e85d1711c30b3be2f2ad57e30d50bd Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Wed, 6 Oct 2021 14:29:29 -0700 Subject: [PATCH 2/6] Change per review --- h3napi.c | 15 ++++++++------- test/index.js | 1 + 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/h3napi.c b/h3napi.c index 088e968..e2f837a 100644 --- a/h3napi.c +++ b/h3napi.c @@ -71,18 +71,16 @@ #define napiGetH3IndexArg(I, O) \ H3Index O;\ + /*For string form input*/\ char O ## Str[17];\ size_t O ## StrCount;\ + /*For array form input*/\ + napiGetNapiArg(I, O ## Arr);\ + uint32_t O ## ArrSz;\ \ if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\ O = stringToH3(O ## Str);\ - } else {\ - napiGetNapiArg(I, O ## Arr);\ - uint32_t O ## ArrSz;\ - if (napi_get_array_length(env, O ## Arr, &(O ## ArrSz)) != napi_ok) {\ - napi_throw_error(env, "EINVAL", "Expected string or array H3 index in arg " #I);\ - return NULL;\ - }\ + } else if (napi_get_array_length(env, O ## Arr, &(O ## ArrSz)) == napi_ok) {\ if (O ## ArrSz != 2) {\ napi_throw_error(env, "EINVAL", "Invalid length array H3 index in arg " #I);\ return NULL;\ @@ -98,6 +96,9 @@ return NULL;\ }\ O = ((H3Index)(O ## Part1) << 32) | (O ## Part0);\ + } else {\ + napi_throw_error(env, "EINVAL", "Expected string or array H3 index in arg " #I);\ + return NULL;\ } #define napiGetH3Index(I, O) \ diff --git a/test/index.js b/test/index.js index 18fd413..7cb08e0 100644 --- a/test/index.js +++ b/test/index.js @@ -122,6 +122,7 @@ exports['h3IsValid_array'] = test => { // TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing test.throws(() => h3node.h3IsValid([]), 'Empty array is not valid'); test.throws(() => h3node.h3IsValid([1]), 'Array with a single element is not valid'); + test.throws(() => h3node.h3IsValid([1, 'a']), 'Array with invalid elements is not valid'); test.throws(() => h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), 'Array with an additional element is not valid' From c79377388c823d7fac1629d09d57777b66b04a7b Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Thu, 7 Oct 2021 18:23:42 -0700 Subject: [PATCH 3/6] Support Uint32Array --- h3napi.c | 16 +++++++++++++--- test/index.js | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/h3napi.c b/h3napi.c index e2f837a..8162b18 100644 --- a/h3napi.c +++ b/h3napi.c @@ -74,24 +74,34 @@ /*For string form input*/\ char O ## Str[17];\ size_t O ## StrCount;\ + /*For typedarray form input*/\ + napi_typedarray_type O ## Type;\ + size_t O ## Length;\ + void* O ## Data;\ /*For array form input*/\ napiGetNapiArg(I, O ## Arr);\ uint32_t O ## ArrSz;\ \ if (napi_get_value_string_utf8(env, argv[I], O ## Str, 17, &O ## StrCount) == napi_ok) {\ O = stringToH3(O ## Str);\ - } else if (napi_get_array_length(env, O ## Arr, &(O ## ArrSz)) == napi_ok) {\ + } else if (napi_get_typedarray_info(env, O ## Arr, &O ## Type, &O ## Length, &O ## Data, NULL, NULL) == napi_ok) {\ + if (O ## Type != napi_uint32_array || O ## Length != 2) {\ + napi_throw_error(env, "EINVAL", "Invalid Uint32Array H3 index in arg " #I);\ + return NULL;\ + }\ + O = ((H3Index)*(((uint32_t*)O ## Data) + 1) << 32) | *((uint32_t*)O ## Data);\ + } else if (napi_get_array_length(env, O ## Arr, &O ## ArrSz) == napi_ok) {\ if (O ## ArrSz != 2) {\ napi_throw_error(env, "EINVAL", "Invalid length array H3 index in arg " #I);\ return NULL;\ }\ napi_value O ## Val0, O ## Val1;\ - if (napi_get_element(env, O ## Arr, 0, &(O ## Val0)) != napi_ok || napi_get_element(env, O ## Arr, 1, &(O ## Val1)) != napi_ok) {\ + if (napi_get_element(env, O ## Arr, 0, &O ## Val0) != napi_ok || napi_get_element(env, O ## Arr, 1, &O ## Val1) != napi_ok) {\ napi_throw_error(env, "EINVAL", "Invalid array H3 index in arg " #I);\ return NULL;\ }\ uint32_t O ## Part0, O ## Part1;\ - if (napi_get_value_uint32(env, O ## Val0, &(O ## Part0)) != napi_ok || napi_get_value_uint32(env, O ## Val1, &(O ## Part1)) != napi_ok) {\ + if (napi_get_value_uint32(env, O ## Val0, &O ## Part0) != napi_ok || napi_get_value_uint32(env, O ## Val1, &O ## Part1) != napi_ok) {\ napi_throw_error(env, "EINVAL", "Invalid integer array H3 index in arg " #I);\ return NULL;\ }\ diff --git a/test/index.js b/test/index.js index 7cb08e0..0f0e999 100644 --- a/test/index.js +++ b/test/index.js @@ -113,6 +113,7 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') => const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') => exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName) + exports['h3IsValid_array'] = test => { test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index'); test.ok( @@ -130,6 +131,22 @@ exports['h3IsValid_array'] = test => { test.done(); }; +exports['h3IsValid_uint32array'] = test => { + test.ok(h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347])), 'Integer H3 index is considered an index'); + test.ok( + !h3node.h3IsValid(new Uint32Array([0x73fffffff, 0xff2834])), + 'Integer with incorrect bits is not considered an index' + ); + // TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing + test.throws(() => h3node.h3IsValid(new Uint32Array([])), 'Empty array is not valid'); + test.throws(() => h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid'); + test.throws( + () => h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])), + 'Array with too many elements is not valid' + ); + test.done(); +}; + exportTest('geoToH3', () => [...randCoords(), 9], simpleTest) exportTest('h3ToGeo', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) exportTest('h3ToGeoBoundary', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) From bf71ea81fdf431d1c982df6871554871d2c2b484 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Thu, 21 Oct 2021 14:17:57 -0700 Subject: [PATCH 4/6] h3IsValid type should not throw --- h3napi.c | 42 +++++++++++++++++++++++++++++++++++++++++- test/index.js | 18 +++++++----------- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/h3napi.c b/h3napi.c index 8162b18..da30e00 100644 --- a/h3napi.c +++ b/h3napi.c @@ -301,7 +301,47 @@ napiFn(h3GetBaseCell) { napiFn(h3IsValid) { napiArgs(1); - napiGetH3IndexArg(0, h3); + // inlined napiGetH3IndexArg in order to return false instead of throwing + H3Index h3; + /*For string form input*/ + char h3Str[17]; + size_t h3StrCount; + /*For typedarray form input*/ + napi_typedarray_type h3Type; + size_t h3Length; + void* h3Data; + /*For array form input*/ + napiGetNapiArg(0, h3Arr); + uint32_t h3ArrSz; + + if (napi_get_value_string_utf8(env, argv[0], h3Str, 17, &h3StrCount) == napi_ok) { + h3 = stringToH3(h3Str); + } else if (napi_get_typedarray_info(env, h3Arr, &h3Type, &h3Length, &h3Data, NULL, NULL) == napi_ok) { + if (h3Type != napi_uint32_array || h3Length != 2) { + napiNapiBool(false, result); + return result; + } + h3 = ((H3Index)*(((uint32_t*)h3Data) + 1) << 32) | *((uint32_t*)h3Data); + } else if (napi_get_array_length(env, h3Arr, &h3ArrSz) == napi_ok) { + if (h3ArrSz != 2) { + napiNapiBool(false, result); + return result; + } + napi_value h3Val0, h3Val1; + if (napi_get_element(env, h3Arr, 0, &h3Val0) != napi_ok || napi_get_element(env, h3Arr, 1, &h3Val1) != napi_ok) { + napiNapiBool(false, result); + return result; + } + uint32_t h3Part0, h3Part1; + if (napi_get_value_uint32(env, h3Val0, &h3Part0) != napi_ok || napi_get_value_uint32(env, h3Val1, &h3Part1) != napi_ok) { + napiNapiBool(false, result); + return result; + } + h3 = ((H3Index)(h3Part1) << 32) | (h3Part0); + } else { + napiNapiBool(false, result); + return result; + } int isValid = h3IsValid(h3); diff --git a/test/index.js b/test/index.js index 0f0e999..bab2413 100644 --- a/test/index.js +++ b/test/index.js @@ -120,12 +120,10 @@ exports['h3IsValid_array'] = test => { !h3node.h3IsValid([0x73fffffff, 0xff2834]), 'Integer with incorrect bits is not considered an index' ); - // TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing - test.throws(() => h3node.h3IsValid([]), 'Empty array is not valid'); - test.throws(() => h3node.h3IsValid([1]), 'Array with a single element is not valid'); - test.throws(() => h3node.h3IsValid([1, 'a']), 'Array with invalid elements is not valid'); - test.throws(() => - h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), + test.ok(!h3node.h3IsValid([]), 'Empty array is not valid'); + test.ok(!h3node.h3IsValid([1]), 'Array with a single element is not valid'); + test.ok(!h3node.h3IsValid([1, 'a']), 'Array with invalid elements is not valid'); + test.ok(!h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), 'Array with an additional element is not valid' ); test.done(); @@ -137,11 +135,9 @@ exports['h3IsValid_uint32array'] = test => { !h3node.h3IsValid(new Uint32Array([0x73fffffff, 0xff2834])), 'Integer with incorrect bits is not considered an index' ); - // TODO: This differs from h3-js, in these cases h3-js would return false rather than throwing - test.throws(() => h3node.h3IsValid(new Uint32Array([])), 'Empty array is not valid'); - test.throws(() => h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid'); - test.throws( - () => h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])), + test.ok(!h3node.h3IsValid(new Uint32Array([])), 'Empty array is not valid'); + test.ok(!h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid'); + test.ok(!h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])), 'Array with too many elements is not valid' ); test.done(); From b001f430e235b892cdf848cc3fccb18bbd9c6263 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Fri, 22 Oct 2021 17:01:55 -0700 Subject: [PATCH 5/6] Add tests for H3Index parsing outside of h3IsValid --- test/index.js | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index bab2413..7610470 100644 --- a/test/index.js +++ b/test/index.js @@ -113,7 +113,7 @@ const exportTest = (methodName, genArgs, testFn, extraName = '') => const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = '') => exports[`${methodName}${extraName}Benchmark`] = benchmarkGen(methodName, genArgs, useTryCatch, extraName) - +// h3IsValid has unique input parsing logic to return false rather than throw on invalid input exports['h3IsValid_array'] = test => { test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index'); test.ok( @@ -143,6 +143,34 @@ exports['h3IsValid_uint32array'] = test => { test.done(); }; +// Exercise the macro used to reading H3 index input from Node +exports['h3ToGeo_array'] = test => { + test.deepEqual(h3node.h3ToGeo([0x3fffffff, 0x8528347]), h3node.h3ToGeo('85283473fffffff'), 'Integer H3 index is considered an index'); + test.deepEqual(h3node.h3ToGeo([0x73fffffff, 0xff2834]), h3node.h3ToGeo('ff28343fffffff'), + 'Integer with incorrect bits handled consistently' + ); + test.throws(() => h3node.h3ToGeo([]), 'Empty array is not valid'); + test.throws(() => h3node.h3ToGeo([1]), 'Array with a single element is not valid'); + test.throws(() => h3node.h3ToGeo([1, 'a']), 'Array with invalid elements is not valid'); + test.throws(() => h3node.h3ToGeo([0x3fffffff, 0x8528347, 0]), + 'Array with an additional element is not valid' + ); + test.done(); +}; + +exports['h3ToGeo_uint32array'] = test => { + test.deepEqual(h3node.h3ToGeo(new Uint32Array([0x3fffffff, 0x8528347])), h3node.h3ToGeo('85283473fffffff'), 'Integer H3 index is considered an index'); + test.deepEqual(h3node.h3ToGeo(new Uint32Array([0x73fffffff, 0xff2834])), h3node.h3ToGeo('ff28343fffffff'), + 'Integer with incorrect bits handled consistently' + ); + test.throws(() => h3node.h3ToGeo(new Uint32Array([])), 'Empty array is not valid'); + test.throws(() => h3node.h3ToGeo(new Uint32Array([1])), 'Array with a single element is not valid'); + test.throws(() => h3node.h3ToGeo(new Uint32Array([0x3fffffff, 0x8528347, 1])), + 'Array with too many elements is not valid' + ); + test.done(); +}; + exportTest('geoToH3', () => [...randCoords(), 9], simpleTest) exportTest('h3ToGeo', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) exportTest('h3ToGeoBoundary', () => [h3node.geoToH3(...randCoords(), 9)], almostEqualTest) From cd1c99d6ee90433c36073b2f8cf22ce30832613e Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Fri, 22 Oct 2021 17:40:45 -0700 Subject: [PATCH 6/6] Change assertion per review --- test/index.js | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/test/index.js b/test/index.js index 7610470..28e316d 100644 --- a/test/index.js +++ b/test/index.js @@ -115,29 +115,29 @@ const exportBenchmark = (methodName, genArgs, useTryCatch = false, extraName = ' // h3IsValid has unique input parsing logic to return false rather than throw on invalid input exports['h3IsValid_array'] = test => { - test.ok(h3node.h3IsValid([0x3fffffff, 0x8528347]), 'Integer H3 index is considered an index'); - test.ok( - !h3node.h3IsValid([0x73fffffff, 0xff2834]), + test.equal(h3node.h3IsValid([0x3fffffff, 0x8528347]), true, 'Integer H3 index is considered an index'); + test.equal( + h3node.h3IsValid([0x73fffffff, 0xff2834]), false, 'Integer with incorrect bits is not considered an index' ); - test.ok(!h3node.h3IsValid([]), 'Empty array is not valid'); - test.ok(!h3node.h3IsValid([1]), 'Array with a single element is not valid'); - test.ok(!h3node.h3IsValid([1, 'a']), 'Array with invalid elements is not valid'); - test.ok(!h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), + test.equal(h3node.h3IsValid([]), false, 'Empty array is not valid'); + test.equal(h3node.h3IsValid([1]), false, 'Array with a single element is not valid'); + test.equal(h3node.h3IsValid([1, 'a']), false, 'Array with invalid elements is not valid'); + test.equal(h3node.h3IsValid([0x3fffffff, 0x8528347, 0]), false, 'Array with an additional element is not valid' ); test.done(); }; exports['h3IsValid_uint32array'] = test => { - test.ok(h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347])), 'Integer H3 index is considered an index'); - test.ok( - !h3node.h3IsValid(new Uint32Array([0x73fffffff, 0xff2834])), + test.equal(h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347])), true, 'Integer H3 index is considered an index'); + test.equal( + h3node.h3IsValid(new Uint32Array([0x73fffffff, 0xff2834])), false, 'Integer with incorrect bits is not considered an index' ); - test.ok(!h3node.h3IsValid(new Uint32Array([])), 'Empty array is not valid'); - test.ok(!h3node.h3IsValid(new Uint32Array([1])), 'Array with a single element is not valid'); - test.ok(!h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])), + test.equal(h3node.h3IsValid(new Uint32Array([])), false, 'Empty array is not valid'); + test.equal(h3node.h3IsValid(new Uint32Array([1])), false, 'Array with a single element is not valid'); + test.equal(h3node.h3IsValid(new Uint32Array([0x3fffffff, 0x8528347, 1])), false, 'Array with too many elements is not valid' ); test.done();