fix: Enable filtering doc by fields of JSON and Blob types #2841
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2841 +/- ##
===========================================
+ Coverage 79.22% 79.31% +0.09%
===========================================
Files 323 323
Lines 24499 24661 +162
===========================================
+ Hits 19408 19558 +150
- Misses 3695 3701 +6
- Partials 1396 1402 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -33,8 +33,8 @@ var ( | |||
| client.FieldKind_NILLABLE_STRING: gql.String, | |||
| client.FieldKind_STRING_ARRAY: gql.NewList(gql.NewNonNull(gql.String)), | |||
| client.FieldKind_NILLABLE_STRING_ARRAY: gql.NewList(gql.String), | |||
| client.FieldKind_NILLABLE_BLOB: schemaTypes.BlobScalarType(), | |||
There was a problem hiding this comment.
question: Why have you made this change? This was done quite recently as these are mutated by the gql library and making them singletons results in shared mutated state between database instances.
Is this change, and the one at the top of manager.go required for the fix?
There was a problem hiding this comment.
this is done to follow the same style that is used by github.com/sourcenetwork/graphql-go: to have 1 scalar definition instance instead of creating always a new.
There was a problem hiding this comment.
This is wrong (and graphql-go), and we should not do it. As well as being a production bug, it prevents us from using t.Parallel() in our tests.
There was a problem hiding this comment.
If we have issues with t.Paralel() we should change how we use gql.String and others. I just made it consistent.
I tried to roll this part back and see if it works. It doesn't because there is a pointer comparison https://github.com/sourcenetwork/graphql-go/blob/master/schema.go#L295
Seems like the change is required for the fix.
There was a problem hiding this comment.
It is not just about t.Parallel(). graphql-go is very singleton heavy, and it mutates the singleton types we provide it. This means multiple embedded Defra instances leak GQL types between them, instance1 will have bits of instance2's gql types in its GQL schema.
It doesn't because there is a pointer comparison
You need to only call schemaTypes.BlobScalarType() once per database instance, and fetch it from the type map where-ever else you require it. All our other types do this.
where? how? |
| }, | ||
| }, | ||
| testUtils.Request{ | ||
| // the filtered-by JSON has no spaces, because this is now it's stored. |
There was a problem hiding this comment.
thought: if we normalize JSON when parsing we could avoid the space issue and also match out of order properties.
islamaliev
force-pushed
the
fix/filter-on-json-field
branch
from
698caf1
to
2e61ab0
Compare
Relevant issue(s)
Resolves #2840
Description
Fixes the issue not being able to filter by JSON and Blob fields