Convert byte array to string #266

sharmaashish13 · 2022-01-06T08:30:16Z

While using a base64 polyfill, encountered an issue where passing binary string from Go to JS didnt work as expected.

If binary string has a null character in middle of string, CString() function just terminates the value at that null character resulting into a truncated value.
v8go.cc's NewValueString function uses NewFromUtf8 which returns invalid response.

Code sample can be found here:
kuoruan/v8go-polyfills#4

Hence, added a new API that uses v8::String::NewFromOneByte to solve this scenario.

value.go

genevieve · 2022-01-06T17:35:16Z

value.go

@@ -53,6 +53,15 @@ func Null(iso *Isolate) *Value {
 	return iso.null
 }

+func NewStringFromByteArray(iso *Isolate, val []byte) (*Value, error) {


We will probably want the inverse to be supported as well (from the v8 value in JS back to Go)

As mentioned in another comment, the reason behind implementing byte array to string is to overcome the challenges with null character in binary strings. I'm not sure for which scenario we would want to implement the reverse.

When v8go converts a *Value to a Go string (e.g. with .String()), it truncates the string at the first null character it encounters.

I added a testcase in value_test.go where i am validating the same. If you print val.String(), it doesnt truncate the data even though there is a 0 byte in the input array.
Having said that, do you have any specific test-case? I can look into that scenario.

I guess we could introduce a (v *Value) BinaryString() []byte or (v *Value) BinaryString() BinaryString depending on this outcome.

When v8go converts a *Value to a Go string (e.g. with .String()), it truncates the string at the first null character it encounters.

Note that we're not trying to solve #192 here. We may do that in a separate PR.

value_test.go

dylanahsmith

Although the test could be improved and documentation could be added, the implementation looks good to me.

dylanahsmith · 2022-01-10T16:52:11Z

value_test.go

+	inputString := "CN7ySxWjjWNpbYPB1n/TBR8avujZS2cHdXRR5ZM7Fi6QBjlVzBqPu0CI9pQXcW9fvbMXdqU+57XY/QdGozJT19+gbQDM0ZQVzjwqtpyLorcPHjMqum+7dHD6XF5cXo3NZlKGsxcnvSVyClBDU5M1dUCe8bB9yV0wVM6ge+0WAmTX2GYbncilTjDw0bSJI1Z+71NT8UQCfmimKhVxJiKrnkaTrTw2Ma/1I2w4Dny3cRlFtCtob9cvNOeeIm8HtQoi/7HXoE0uFr1C39OL2hCC1TJsxX94djtNFqd9aUOPYrwT+zErSokSvbNYS5WpEjEpRJze9+TCV9NLmqCnARK4Bw"
+	byts, _ := base64.RawStdEncoding.DecodeString(inputString)
+
+	expected := "ÞòK£cimÁÖÓ¾èÙKgutQå;.9UÌ»@öqo_½³v¥>çµØýF£2S×ß m"


This seems more complicated than it needs to be, which makes it hard to see the actual connection between the input and output if it fails and makes it harder to see what is actually being tested.

It seems like we just need the input to have the following for testing different edge cases:

a null byte ('\x00') doesn't terminate the string

bytes encoding a valid multi-byte UTF-8 encoding for a character should not be combined into a single character (e.g. []byte("ç") should have length 2 in the JS string)

an invalid UTF-8 byte sequence (e.g. '\xff') gets preserved

Suggested change

inputString := "CN7ySxWjjWNpbYPB1n/TBR8avujZS2cHdXRR5ZM7Fi6QBjlVzBqPu0CI9pQXcW9fvbMXdqU+57XY/QdGozJT19+gbQDM0ZQVzjwqtpyLorcPHjMqum+7dHD6XF5cXo3NZlKGsxcnvSVyClBDU5M1dUCe8bB9yV0wVM6ge+0WAmTX2GYbncilTjDw0bSJI1Z+71NT8UQCfmimKhVxJiKrnkaTrTw2Ma/1I2w4Dny3cRlFtCtob9cvNOeeIm8HtQoi/7HXoE0uFr1C39OL2hCC1TJsxX94djtNFqd9aUOPYrwT+zErSokSvbNYS5WpEjEpRJze9+TCV9NLmqCnARK4Bw"

byts, _ := base64.RawStdEncoding.DecodeString(inputString)

expected := "�ÞòK�£�cim�ÁÖ�Ó��¾èÙKg�utQå�;�.��9UÌ��»@�ö��qo_½³�v¥>çµØý�F£2S×ß m"

expected := "a\x00ç\xffe"

input = []byte(expected)

dylanahsmith · 2022-01-10T17:12:42Z

value_test.go

+	if !val.IsString() {
+		t.Errorf("expected string but got %s", reflect.TypeOf(val))
+	}
+	if val.String() != expected {
+		t.Errorf("expected not same as actual")
+	}


Shouldn't this also work for v8.NewValue(expected), in which case this isn't testing the distinguishing behaviour of this new method, which is that the bytes aren't interpreted as UTF-8 in the JS string. Since there isn't a v8go.String type, a workaround would be to test this in JS (e.g. getting the length and making sure it counts each byte in a UTF-8 encoded byte sequence)

dylanahsmith · 2022-01-10T17:14:37Z

value.go

@@ -53,6 +53,15 @@ func Null(iso *Isolate) *Value {
 	return iso.null
 }

+func NewStringFromByteArray(iso *Isolate, val []byte) (*Value, error) {
+	if iso == nil {
+		return nil, errors.New("v8go: failed to create new Value: Isolate cannot be <nil>")


This should be treated as a bug, so should instead panic to make sure the error is quickly noticed during testing..

value.go

…nary strings

codecov · 2022-01-14T19:10:55Z

Codecov Report

Merging #266 (41fcfd3) into master (5e91d3d) will decrease coverage by 0.30%.
The diff coverage is 60.00%.

❗ Current head 41fcfd3 differs from pull request most recent head 184e508. Consider uploading reports for the commit 184e508 to get more accurate results

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
- Coverage   95.86%   95.55%   -0.31%     
==========================================
  Files          17       17              
  Lines         580      585       +5     
==========================================
+ Hits          556      559       +3     
- Misses         15       16       +1     
- Partials        9       10       +1

Impacted Files	Coverage Δ
value.go	`93.85% <60.00%> (-0.98%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e91d3d...184e508. Read the comment docs.

dylanahsmith · 2022-01-13T19:01:34Z

v8go.h

@@ -195,6 +195,7 @@ extern ValuePtr NewValueUndefined(IsolatePtr iso_ptr);
 extern ValuePtr NewValueInteger(IsolatePtr iso_ptr, int32_t v);
 extern ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso_ptr, uint32_t v);
 extern RtnValue NewValueString(IsolatePtr iso_ptr, const char* v, int v_length);
+extern RtnValue NewValueStringFromByteArray(IsolatePtr iso, const uint8_t* v, int len);


The Value in the name doesn't seem necessary and doesn't match the function it is used from.

dylanahsmith · 2022-01-13T19:01:51Z

value_test.go

+		args := info.Args()
+		input := args[0].String()
+		if len(input) == 0 {
+			//String() terminates input at null character. hence hardcoding for 3rd input


A workaround is no longer needed for this.

.String() not working as expected. for input string a\x00\xffe, it should return 4 bytes instead getting 5.

dylanahsmith · 2022-01-14T19:05:04Z

value.go

@@ -53,6 +53,16 @@ func Null(iso *Isolate) *Value {
 	return iso.null
 }

+// NewStringFromByteArray returns V8::string from byte array. Converts each byte into equivalent character.
+func NewStringFromByteArray(iso *Isolate, val []byte) (*Value, error) {


NewStringFromBytes would be a shorter name. Referring to the argument as a ByteArray doesn't make it any clearer and could even seem to refer to a JS array before looking at the parameter type.

The parameter name could be named bytes to be consistent with what it is referred to as in the function name.

Also, I've opened #277 since this should actually be returning a String type. We should add that type before adding a constructor for a string, so we don't need a breaking change to change it from Value to String later.

Suggested change

func NewStringFromByteArray(iso *Isolate, val []byte) (*Value, error) {

func NewStringFromBytes(iso *Isolate, bytes []byte) (*String, error) {

v8go.cc

CHANGELOG.md

noelcarl · 2024-03-25T20:57:46Z

@dylanahsmith @genevieve is this pull request something we can still move forward with? If not is there another known workaround for supporting the passing of a binary string from Go to JavaScript? Thanks.

cc @sharmaashish13

dylanahsmith · 2024-03-25T21:20:41Z

genevieve and I had been exploring using this library while working at Shopify, which didn't end up adopting this for production use and neither of us work at Shopify anymore. I think you will need help from someone else with merge access who has been more recently active on this project.

noelcarl · 2024-03-25T23:57:31Z

Ok, sounds good. Thanks for the update and context @dylanahsmith.

genevieve reviewed Jan 6, 2022

View reviewed changes

value.go Show resolved Hide resolved

genevieve reviewed Jan 6, 2022

View reviewed changes

GustavoCaso reviewed Jan 7, 2022

View reviewed changes

value_test.go Show resolved Hide resolved

sharmaashish13 force-pushed the string-byte-array branch from 0cf95af to cd7a6a1 Compare January 10, 2022 06:05

dylanahsmith reviewed Jan 10, 2022

View reviewed changes

sharmaashish13 requested a review from dylanahsmith January 13, 2022 10:26

sharmaashish13 added 4 commits January 13, 2022 15:59

byte array to string. with issues

be4941d

convert byte array to string as NewValueString is unable to handle bi…

47c6c3d

…nary strings

test fix - compare output string

3951f1e

added changelog, new method documentation and updated test

41fcfd3

sharmaashish13 force-pushed the string-byte-array branch from 0b5c75c to 41fcfd3 Compare January 13, 2022 10:30

dylanahsmith mentioned this pull request Jan 14, 2022

Add String type, constructor functions for it and Value.AsString to cast to it #277

Open

dylanahsmith reviewed Jan 14, 2022

View reviewed changes

sharmaashish13 added 2 commits January 17, 2022 10:52

renaming functions

4dee01c

test case for null isolate panic

184e508

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert byte array to string #266

Convert byte array to string #266

sharmaashish13 commented Jan 6, 2022

genevieve Jan 6, 2022

sharmaashish13 Jan 6, 2022

maxshirshin Jan 7, 2022

sharmaashish13 Jan 7, 2022

backkem Jan 8, 2022

backkem Jan 8, 2022

dylanahsmith left a comment

dylanahsmith Jan 10, 2022

dylanahsmith Jan 10, 2022

dylanahsmith Jan 10, 2022

codecov bot commented Jan 14, 2022 •

edited

Loading

dylanahsmith Jan 13, 2022

dylanahsmith Jan 13, 2022

sharmaashish13 Jan 19, 2022

dylanahsmith Jan 14, 2022

noelcarl commented Mar 25, 2024

dylanahsmith commented Mar 25, 2024

noelcarl commented Mar 25, 2024

-	inputString := "CN7ySxWjjWNpbYPB1n/TBR8avujZS2cHdXRR5ZM7Fi6QBjlVzBqPu0CI9pQXcW9fvbMXdqU+57XY/QdGozJT19+gbQDM0ZQVzjwqtpyLorcPHjMqum+7dHD6XF5cXo3NZlKGsxcnvSVyClBDU5M1dUCe8bB9yV0wVM6ge+0WAmTX2GYbncilTjDw0bSJI1Z+71NT8UQCfmimKhVxJiKrnkaTrTw2Ma/1I2w4Dny3cRlFtCtob9cvNOeeIm8HtQoi/7HXoE0uFr1C39OL2hCC1TJsxX94djtNFqd9aUOPYrwT+zErSokSvbNYS5WpEjEpRJze9+TCV9NLmqCnARK4Bw"
-	byts, _ := base64.RawStdEncoding.DecodeString(inputString)
-	expected := "�ÞòK�£�cim�ÁÖ�Ó���¾èÙKg�utQå�;�.��9UÌ��»@�ö��qo_½³�v¥>çµØý�F£2S×ß m"
+	expected := "a\x00ç\xffe"
+	input = []byte(expected)

	func NewStringFromByteArray(iso Isolate, val []byte) (Value, error) {
	func NewStringFromBytes(iso Isolate, bytes []byte) (String, error) {

Convert byte array to string #266

Are you sure you want to change the base?

Convert byte array to string #266

Conversation

sharmaashish13 commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanahsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelcarl commented Mar 25, 2024

dylanahsmith commented Mar 25, 2024

noelcarl commented Mar 25, 2024

codecov bot commented Jan 14, 2022 •

edited

Loading