-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: Add blob scalar type #2091
Changes from 6 commits
cc801b5
d4ebf91
70bf6c3
92dce71
d03da82
212dfec
da25d3c
cf090ae
9cb51fc
d67c1be
a540518
95c453d
389ffef
1aa1a26
8089b49
871990c
438894b
a46fad6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import ( | |
gql "github.com/sourcenetwork/graphql-go" | ||
|
||
"github.com/sourcenetwork/defradb/client" | ||
schemaTypes "github.com/sourcenetwork/defradb/request/graphql/schema/types" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: Seeing this make me wonder if the type should be added to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to that. Should we move the other types there as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me nervous, I think we added DateTime to that package, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, DateTime was already defined as a scalar in the I do agree though that we shouldn't have to define the scalar in the |
||
) | ||
|
||
var ( | ||
|
@@ -31,9 +32,10 @@ var ( | |
gql.String: client.FieldKind_STRING, | ||
&gql.Object{}: client.FieldKind_FOREIGN_OBJECT, | ||
&gql.List{}: client.FieldKind_FOREIGN_OBJECT_ARRAY, | ||
// Custom scalars | ||
schemaTypes.BytesScalarType: client.FieldKind_BYTES, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Related to the other comment from Fred, I think it should be made clear that these are custom scalars, as the comment on line 36 suggests ("More custom ones to come"). Just a quick comment separating base scalars from custom scalars for clarity 👍 |
||
// More custom ones to come | ||
// - JSON | ||
// - ByteArray | ||
// - Counters | ||
} | ||
|
||
|
@@ -52,6 +54,7 @@ var ( | |
client.FieldKind_STRING: gql.String, | ||
client.FieldKind_STRING_ARRAY: gql.NewList(gql.NewNonNull(gql.String)), | ||
client.FieldKind_NILLABLE_STRING_ARRAY: gql.NewList(gql.String), | ||
client.FieldKind_BYTES: schemaTypes.BytesScalarType, | ||
} | ||
|
||
// This map is fine to use | ||
|
@@ -70,6 +73,7 @@ var ( | |
client.FieldKind_STRING: client.LWW_REGISTER, | ||
client.FieldKind_STRING_ARRAY: client.LWW_REGISTER, | ||
client.FieldKind_NILLABLE_STRING_ARRAY: client.LWW_REGISTER, | ||
client.FieldKind_BYTES: client.LWW_REGISTER, | ||
client.FieldKind_FOREIGN_OBJECT: client.NONE_CRDT, | ||
client.FieldKind_FOREIGN_OBJECT_ARRAY: client.NONE_CRDT, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// Copyright 2023 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package types | ||
|
||
import ( | ||
"encoding/hex" | ||
|
||
"github.com/sourcenetwork/graphql-go" | ||
"github.com/sourcenetwork/graphql-go/language/ast" | ||
) | ||
|
||
var BytesScalarType = graphql.NewScalar(graphql.ScalarConfig{ | ||
Name: "Bytes", | ||
Description: "The `Bytes` scalar type represents an array of bytes.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: If it is an array of bytes, why does it only appear to accept string values? todo: I'm not sure if it does only accept string values, could you add an integration test documenting the beheviour when provided an array of numbers:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is a bug in the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite a bug, we just dont do any checking of the saved value. The read should fail though I think. Does the read of |
||
Serialize: func(value any) any { | ||
switch value := value.(type) { | ||
case []byte: | ||
return hex.EncodeToString(value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Why are you encoding/decoding at all here (including in ParseValue)? todo: The |
||
case *[]byte: | ||
return hex.EncodeToString(*value) | ||
default: | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: Please document why you return nil here |
||
} | ||
}, | ||
ParseValue: func(value any) any { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: Thank you for the reminder as to how much I dislike the |
||
switch value := value.(type) { | ||
case string: | ||
data, err := hex.DecodeString(value) | ||
if err != nil { | ||
return nil | ||
} | ||
return data | ||
case *string: | ||
data, err := hex.DecodeString(*value) | ||
if err != nil { | ||
return nil | ||
} | ||
return data | ||
default: | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Why does this return nil instead of EDIT:
todo: If so, please document There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just following how the other types work in the |
||
} | ||
}, | ||
ParseLiteral: func(valueAST ast.Value) any { | ||
switch valueAST := valueAST.(type) { | ||
case *ast.StringValue: | ||
data, err := hex.DecodeString(valueAST.Value) | ||
if err != nil { | ||
return nil | ||
} | ||
return data | ||
default: | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: Similar to another comment, please document this - it looks like a silent error, but I only know that by guessing given the:
stuff in other code blocks |
||
} | ||
}, | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
// Copyright 2023 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package types | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/sourcenetwork/graphql-go/language/ast" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestBytesScalarTypeSerialize(t *testing.T) { | ||
input := []byte{0, 255} | ||
output := "00ff" | ||
|
||
cases := []struct { | ||
input any | ||
expect any | ||
}{ | ||
{input, output}, | ||
{&input, output}, | ||
{nil, nil}, | ||
{0, nil}, | ||
{false, nil}, | ||
} | ||
for _, c := range cases { | ||
result := BytesScalarType.Serialize(c.input) | ||
assert.Equal(t, c.expect, result) | ||
} | ||
} | ||
|
||
func TestBytesScalarTypeParseValue(t *testing.T) { | ||
input := "00ff" | ||
output := []byte{0, 255} | ||
invalid := "invalid" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Why is this invalid? todo: Please document why this is invalid, if it remains invalid. |
||
|
||
cases := []struct { | ||
input any | ||
expect any | ||
}{ | ||
{input, output}, | ||
{&input, output}, | ||
{invalid, nil}, | ||
{&invalid, nil}, | ||
{nil, nil}, | ||
{0, nil}, | ||
{false, nil}, | ||
} | ||
for _, c := range cases { | ||
result := BytesScalarType.ParseValue(c.input) | ||
assert.Equal(t, c.expect, result) | ||
} | ||
} | ||
|
||
func TestBytesScalarTypeParseLiteral(t *testing.T) { | ||
cases := []struct { | ||
input ast.Value | ||
expect any | ||
}{ | ||
{&ast.StringValue{Value: "00ff"}, []byte{0, 255}}, | ||
{&ast.StringValue{Value: "invalid"}, nil}, | ||
{&ast.IntValue{}, nil}, | ||
{&ast.BooleanValue{}, nil}, | ||
{&ast.NullValue{}, nil}, | ||
{&ast.EnumValue{}, nil}, | ||
{&ast.FloatValue{}, nil}, | ||
{&ast.ListValue{}, nil}, | ||
{&ast.ObjectValue{}, nil}, | ||
} | ||
for _, c := range cases { | ||
result := BytesScalarType.ParseLiteral(c.input) | ||
assert.Equal(t, c.expect, result) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// Copyright 2023 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package field_kinds | ||
|
||
import ( | ||
"testing" | ||
|
||
testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
) | ||
|
||
func TestMutationUpdate_WithBytesField(t *testing.T) { | ||
test := testUtils.TestCase{ | ||
Description: "Simple update of bytes field", | ||
Actions: []any{ | ||
testUtils.SchemaUpdate{ | ||
Schema: ` | ||
type Users { | ||
name: String | ||
data: Bytes | ||
} | ||
`, | ||
}, | ||
testUtils.CreateDoc{ | ||
Doc: `{ | ||
"name": "John", | ||
"data": "00FE" | ||
}`, | ||
}, | ||
testUtils.UpdateDoc{ | ||
Doc: `{ | ||
"data": "00FF" | ||
}`, | ||
}, | ||
testUtils.Request{ | ||
Request: ` | ||
query { | ||
Users { | ||
data | ||
} | ||
} | ||
`, | ||
Results: []map[string]any{ | ||
{ | ||
"data": "00FF", | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
testUtils.ExecuteTestCase(t, test) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,30 +64,6 @@ func TestSchemaUpdatesAddFieldKind9(t *testing.T) { | |
testUtils.ExecuteTestCase(t, test) | ||
} | ||
|
||
func TestSchemaUpdatesAddFieldKind13(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: Please add a tests (one by enum number, and one by String-name) in |
||
test := testUtils.TestCase{ | ||
Description: "Test schema update, add field with kind deprecated (13)", | ||
Actions: []any{ | ||
testUtils.SchemaUpdate{ | ||
Schema: ` | ||
type Users { | ||
name: String | ||
} | ||
`, | ||
}, | ||
testUtils.SchemaPatch{ | ||
Patch: ` | ||
[ | ||
{ "op": "add", "path": "/Users/Fields/-", "value": {"Name": "foo", "Kind": 13} } | ||
] | ||
`, | ||
ExpectedError: "no type found for given name. Type: 13", | ||
}, | ||
}, | ||
} | ||
testUtils.ExecuteTestCase(t, test) | ||
} | ||
|
||
func TestSchemaUpdatesAddFieldKind14(t *testing.T) { | ||
test := testUtils.TestCase{ | ||
Description: "Test schema update, add field with kind deprecated (14)", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: I think this is a strange way of representing this, and that
[Byte]
is more sensible and fits much better with the rest of the types, and any potential futureByte
,[Byte!]
orByte!
types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to
Blob
as we discussed in the standup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Keenan :)