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

Convert byte array to string #266

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Convert byte array to string #266

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be4941d
byte array to string. with issues
sharmaashish13 Jan 5, 2022
47c6c3d
convert byte array to string as NewValueString is unable to handle bi…
sharmaashish13 Jan 6, 2022
3951f1e
test fix - compare output string
sharmaashish13 Jan 10, 2022
41fcfd3
added changelog, new method documentation and updated test
sharmaashish13 Jan 12, 2022
4dee01c
renaming functions
sharmaashish13 Jan 17, 2022
184e508
test case for null isolate panic
sharmaashish13 Jan 17, 2022
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added
- Support for [binary strings](https://developer.mozilla.org/en-US/docs/Web/API/DOMString/Binary). `NewStringFromByteArray` takes bytes array as input and uses `String::NewFromOneByte` to convert each byte to an equivalent character.

### Fixed
- Use string length to ensure null character-containing strings in Go/JS are not terminated early.
- Object.Set with an empty key string is now supported
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module rogchap.com/v8go

go 1.16
go 1.16
17 changes: 17 additions & 0 deletions v8go.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,23 @@ ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso, uint32_t v) {
return tracked_value(ctx, val);
}

RtnValue NewValueStringFromByteArray(IsolatePtr iso, const uint8_t* v, int len){
ISOLATE_SCOPE_INTERNAL_CONTEXT(iso);
TryCatch try_catch(iso);
RtnValue rtn = {};
Local<String> str;
if (!String::NewFromOneByte(iso, v, NewStringType::kNormal, len).ToLocal(&str)) {
rtn.error = ExceptionError(try_catch, iso, ctx->ptr.Get(iso));
return rtn;
}
m_value* val = new m_value;
val->iso = iso;
val->ctx = ctx;
val->ptr = Persistent<Value, CopyablePersistentTraits<Value>>(iso, str);
rtn.value = tracked_value(ctx, val);
return rtn;
}

RtnValue NewValueString(IsolatePtr iso, const char* v, int v_length) {
ISOLATE_SCOPE_INTERNAL_CONTEXT(iso);
TryCatch try_catch(iso);
Expand Down
1 change: 1 addition & 0 deletions v8go.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

extern ValuePtr NewValueBoolean(IsolatePtr iso_ptr, int v);
extern ValuePtr NewValueNumber(IsolatePtr iso_ptr, double v);
extern ValuePtr NewValueBigInt(IsolatePtr iso_ptr, int64_t v);
Expand Down
10 changes: 10 additions & 0 deletions value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

if iso == nil {
panic(errors.New("v8go: failed to create new Value: Isolate cannot be <nil>"))
}
cUint := (*C.uchar)(unsafe.Pointer(&val[0]))
rtnVal := C.NewValueStringFromByteArray(iso.ptr, cUint, C.int(len(val)))
return valueResult(nil, rtnVal)
}

// NewValue will create a primitive value. Supported values types to create are:
// string -> V8::String
// int32 -> V8::Integer
Expand Down
53 changes: 53 additions & 0 deletions value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,59 @@ func TestValueFunction(t *testing.T) {

}

func TestNewStringFromByteArray(t *testing.T) {
t.Parallel()
iso := v8.NewIsolate()
defer iso.Dispose()
global := v8.NewObjectTemplate(iso)

testFn := v8.NewFunctionTemplate(iso, func(info *v8.FunctionCallbackInfo) *v8.Value {
args := info.Args()
input := args[0].String()
if len(input) == 0 {
//String() terminates input at null character. hence hardcoding for 3rd input
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A workaround is no longer needed for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

input = "a\x00\xffe"
}
val, _ := v8.NewStringFromByteArray(iso, []byte(input))
return val
})
global.Set("foo", testFn, v8.ReadOnly)

ctx := v8.NewContext(iso, global)
defer ctx.Close()
tests := [...]struct {
name string
script string
}{
{"normal string",
` let res = foo("str");
if(res.length != 3){
throw Error("expected length 3")
}
`},
{"multi-byte sequence",
` res = foo("Ò");
if(res.length != 2 ){
throw Error("expected length 2")
}
`},
{"null terminated sequence",
` res = foo("");
if(res.length != 4){
throw Error("expected length 4")
}
`},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if _, err := ctx.RunScript(tt.script, ""); err != nil {
t.Errorf("expected <nil> error, but got error: %v", err)
}
})
}

}

func TestValueSameValue(t *testing.T) {
t.Parallel()
iso := v8.NewIsolate()
Expand Down