feat: Add blob scalar type #2091

nasdf · 2023-11-28T19:48:37Z

Relevant issue(s)

Resolves #2090

Description

This PR adds a blob scalar type to the schema system.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Added integration tests.

Specify the platform(s) on which this was tested:

MacOS

fredcarle · 2023-11-28T20:10:52Z

request/graphql/schema/descriptions.go

@@ -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"


thought: Seeing this make me wonder if the type should be added to the graphql-go package.

I'm open to that. Should we move the other types there as well?

This makes me nervous, I think we added DateTime to that package, but graphql-go is supposed to be a general purpose gql package, not a defra package. And we may/do want to get rid of it/replace it at somepoint

I think we added DateTime to that package

No, DateTime was already defined as a scalar in the graphql-go package, but there were some problems with it as it relates to Defra integration, there was PR that got merged on the graphql-go package related to the DateTime stuff, but it wasn't the implementation/definition of the DateTime scalar. (reference: #931 and sourcenetwork/graphql-go#8)

I do agree though that we shouldn't have to define the scalar in the graphql-go package, it exposes the necessary types/functions for us to define our own scalars outside that package, exactly as they're being used here. Moreover, the fact that we have our own defined custom scalars implies that w.e package we may replace the graphql-go package with, should implement the "base" scalars as defined in the GQL spec, and all we have to worry about is the specific "custom" scalars that are neatly organized in a single place.

codecov · 2023-11-28T20:20:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (51b20bb) 73.96% compared to head (438894b) 74.00%.

❗ Current head 438894b differs from pull request most recent head a46fad6. Consider uploading reports for the commit a46fad6 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2091      +/-   ##
===========================================
+ Coverage    73.96%   74.00%   +0.04%     
===========================================
  Files          248      249       +1     
  Lines        24820    24859      +39     
===========================================
+ Hits         18357    18396      +39     
  Misses        5206     5206              
  Partials      1257     1257

Flag	Coverage Δ
all-tests	`74.00% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
client/descriptions.go	`76.47% <100.00%> (+0.57%)`	⬆️
db/collection_update.go	`73.44% <100.00%> (+0.14%)`	⬆️
db/index.go	`87.60% <100.00%> (+0.76%)`	⬆️
request/graphql/schema/collection.go	`91.38% <100.00%> (+0.08%)`	⬆️
request/graphql/schema/manager.go	`97.46% <100.00%> (+0.07%)`	⬆️
request/graphql/schema/types/scalars.go	`100.00% <100.00%> (ø)`

Continue to review full report in Codecov by Sentry.

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

jsimnz · 2023-11-28T22:29:43Z

request/graphql/schema/descriptions.go

+		gql.String:                  client.FieldKind_STRING,
+		&gql.Object{}:               client.FieldKind_FOREIGN_OBJECT,
+		&gql.List{}:                 client.FieldKind_FOREIGN_OBJECT_ARRAY,
+		schemaTypes.BytesScalarType: client.FieldKind_BYTES,


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 👍

AndrewSisley · 2023-11-30T18:00:18Z

client/descriptions.go

@@ -204,6 +204,7 @@ var FieldKindStringToEnumMapping = map[string]FieldKind{
 	"String":     FieldKind_STRING,
 	"[String]":   FieldKind_NILLABLE_STRING_ARRAY,
 	"[String!]":  FieldKind_STRING_ARRAY,
+	"Bytes":      FieldKind_BYTES,


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 future Byte, [Byte!] or Byte! types.

Updated to Blob as we discussed in the standup.

Thanks Keenan :)

AndrewSisley · 2023-11-30T18:03:30Z

request/graphql/schema/types/scalars.go

+			return nil
+		}
+	},
+	ParseValue: func(value any) any {


thought: Thank you for the reminder as to how much I dislike the graphql library and it's totally rubbish way of (not) handling errors...

AndrewSisley · 2023-11-30T18:05:09Z

request/graphql/schema/types/scalars_test.go

+func TestBytesScalarTypeParseValue(t *testing.T) {
+	input := "00ff"
+	output := []byte{0, 255}
+	invalid := "invalid"


question: Why is this invalid?

todo: Please document why this is invalid, if it remains invalid.

AndrewSisley · 2023-11-30T18:06:16Z

request/graphql/schema/types/scalars.go

+			}
+			return data
+		default:
+			return nil


question: Why does this return nil instead of []byte{}?

EDIT:
question: Is this a silent failure? Like the errors above:

if err != nil { return nil }

todo: If so, please document

I'm just following how the other types work in the graphql-go library. It seems like nil is the return value when the type cannot be parsed.

AndrewSisley · 2023-11-30T18:09:18Z

request/graphql/schema/types/scalars.go

+			}
+			return data
+		default:
+			return nil


todo: Similar to another comment, please document this - it looks like a silent error, but I only know that by guessing given the:

if err != nil { return nil }

stuff in other code blocks

AndrewSisley · 2023-11-30T18:11:46Z

tests/integration/schema/updates/add/field/kind/invalid_test.go

@@ -64,30 +64,6 @@ func TestSchemaUpdatesAddFieldKind9(t *testing.T) {
 	testUtils.ExecuteTestCase(t, test)
 }

-func TestSchemaUpdatesAddFieldKind13(t *testing.T) {


todo: Please add a tests (one by enum number, and one by String-name) in .../schema/updates/add/field/kind/foo.go that tests that this field works with PatchSchema

AndrewSisley · 2023-11-30T18:18:54Z

request/graphql/schema/types/scalars.go

+
+var BytesScalarType = graphql.NewScalar(graphql.ScalarConfig{
+	Name:        "Bytes",
+	Description: "The `Bytes` scalar type represents an array of bytes.",


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:

testUtils.CreateDoc{ Doc: `{ "name": "John", "data": [0,1,2,3...] }`, },

I believe this is a bug in theclient.Document API. You can also do this to a string field:

testUtils.CreateDoc{ Doc: `{ "name": 12345 }`, } testUtils.CreateDoc{ Doc: `{ "name": false }`, }

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 [0,1,2,3...] fail? Can we please document the behaviour, as it is a more likely question than assigning 1234 to a boolean :)

AndrewSisley · 2023-11-30T18:20:57Z

request/graphql/schema/types/scalars.go

+		case *[]byte:
+			return hex.EncodeToString(*value)
+		default:
+			return nil


todo: Please document why you return nil here

AndrewSisley

I have a handful of localized questions/todos that I would like resolving before merging, sorry about the hassle :)

AndrewSisley · 2023-11-30T19:16:36Z

request/graphql/schema/types/scalars.go

+	Serialize: func(value any) any {
+		switch value := value.(type) {
+		case []byte:
+			return hex.EncodeToString(value)


question: Why are you encoding/decoding at all here (including in ParseValue)?

todo: The graphql library lacks any documentation here, but we can add some to our code to document why Serialize and ParseValue exist.

AndrewSisley · 2023-12-04T17:03:50Z

request/graphql/schema/types/scalars.go

+var BlobScalarType = graphql.NewScalar(graphql.ScalarConfig{
+	Name:        "Blob",
+	Description: "The `Blob` scalar type represents a binary large object.",
+	// Serialize converts the value to the serialized hex representation


question: Why is it converted to hex?

todo: If we preserve the serialization/deserialization to hex, please document why it is converted.

AndrewSisley

Looks good to me, thanks for this, and the extra tweaks Keenan - I'm approving now with one open/pending discussion RE why you are converting to hex and back that should be resolved before merge :)

nasdf · 2023-12-04T18:48:41Z

Looks good to me, thanks for this, and the extra tweaks Keenan - I'm approving now with one open/pending discussion RE why you are converting to hex and back that should be resolved before merge :)

Thanks! I refactored the parsing logic so that the value will always be a hex string.

fredcarle · 2023-12-04T23:09:16Z

request/graphql/schema/types/scalars.go

+	Serialize: coerceBlob,
+	// ParseValue converts the value to a hex string
+	ParseValue: coerceBlob,
+	// ParseLiteral converts the ast value a hex string


nitpick: "to a hex string"?

islamaliev

Looks good, but one test is missing.

islamaliev · 2023-12-06T15:59:57Z

db/index.go

@@ -51,6 +51,8 @@ func getValidateIndexFieldFunc(kind client.FieldKind) func(any) bool {
 		return canConvertIndexFieldValue[float64]
 	case client.FieldKind_BOOL:
 		return canConvertIndexFieldValue[bool]
+	case client.FieldKind_BLOB:
+		return canConvertIndexFieldValue[string]


todo: please add test here TestNonUnique_StoringIndexedFieldValueOfDifferentTypes

islamaliev

good job, Keenan!

## Relevant issue(s) Resolves sourcenetwork#2090 ## Description This PR adds a blob scalar type to the schema system. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? Added integration tests. Specify the platform(s) on which this was tested: - MacOS

nasdf added 2 commits November 28, 2023 11:44

add bytes scalar definition

cc801b5

add bytes scalar tests

d4ebf91

nasdf added feature New feature or request area/schema Related to the schema system labels Nov 28, 2023

nasdf added this to the DefraDB v0.9 milestone Nov 28, 2023

nasdf self-assigned this Nov 28, 2023

remove invalid field type 13 test

70bf6c3

nasdf requested a review from a team November 28, 2023 20:02

fredcarle reviewed Nov 28, 2023

View reviewed changes

add missing field definition to validateFieldSchema

92dce71

jsimnz reviewed Nov 28, 2023

View reviewed changes

nasdf added 2 commits November 29, 2023 09:43

add comment to type descriptions

d03da82

add scalar unit tests

212dfec

AndrewSisley reviewed Nov 30, 2023

View reviewed changes

AndrewSisley requested changes Nov 30, 2023

View reviewed changes

AndrewSisley reviewed Nov 30, 2023

View reviewed changes

nasdf added 4 commits December 1, 2023 10:02

rename bytes type to blob

da25d3c

add comments to scalar functions and tests

cf090ae

add scalar func documentation

9cb51fc

add schema patch test

d67c1be

nasdf requested a review from AndrewSisley December 1, 2023 18:47

AndrewSisley reviewed Dec 4, 2023

View reviewed changes

AndrewSisley approved these changes Dec 4, 2023

View reviewed changes

nasdf added 3 commits December 4, 2023 10:32

add coerceBlob function

a540518

update docs

95c453d

fix blob regex

389ffef

fredcarle reviewed Dec 4, 2023

View reviewed changes

islamaliev reviewed Dec 6, 2023

View reviewed changes

nasdf and others added 4 commits December 6, 2023 09:21

add index test

1aa1a26

fix typo

8089b49

Merge branch 'develop' into nasdf/feat/bytes-scalar

871990c

improve index blob test

438894b

nasdf changed the title ~~feat: Add bytes scalar type~~ feat: Add blob scalar type Dec 6, 2023

islamaliev approved these changes Dec 6, 2023

View reviewed changes

Merge branch 'develop' into nasdf/feat/bytes-scalar

a46fad6

nasdf merged commit 9084218 into sourcenetwork:develop Dec 7, 2023
27 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add blob scalar type #2091

feat: Add blob scalar type #2091

nasdf commented Nov 28, 2023 •

edited

Loading

fredcarle Nov 28, 2023

nasdf Nov 28, 2023

AndrewSisley Nov 28, 2023

jsimnz Nov 28, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

jsimnz Nov 28, 2023

AndrewSisley Nov 30, 2023

nasdf Dec 1, 2023

AndrewSisley Dec 4, 2023

AndrewSisley Nov 30, 2023

AndrewSisley Nov 30, 2023

AndrewSisley Nov 30, 2023 •

edited

Loading

nasdf Dec 1, 2023

AndrewSisley Nov 30, 2023

AndrewSisley Nov 30, 2023 •

edited

Loading

AndrewSisley Nov 30, 2023

nasdf Dec 1, 2023

AndrewSisley Dec 4, 2023

AndrewSisley Nov 30, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Nov 30, 2023

AndrewSisley Dec 4, 2023 •

edited

Loading

AndrewSisley left a comment

nasdf commented Dec 4, 2023

fredcarle Dec 4, 2023

islamaliev left a comment

islamaliev Dec 6, 2023

islamaliev left a comment

feat: Add blob scalar type #2091

feat: Add blob scalar type #2091

Conversation

nasdf commented Nov 28, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

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 Nov 28, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Nov 30, 2023 • edited Loading

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

AndrewSisley Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

nasdf commented Dec 4, 2023

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

nasdf commented Nov 28, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

AndrewSisley Nov 30, 2023 •

edited

Loading

AndrewSisley Nov 30, 2023 •

edited

Loading

AndrewSisley Nov 30, 2023 •

edited

Loading

AndrewSisley Dec 4, 2023 •

edited

Loading