fix: Change to supporting of non-null inline arrays

[AndrewSisley]

Relevant issue(s)

Resolves #607

Description

Change to supporting of non-null inline arrays vs supporting nullable inline arrays, as we did previously.

To do post merge:

• update query spec doc

[codecov]

Codecov Report

Merging #609 (d54f12c) into develop (e2091fc) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #609      +/-   ##
===========================================
+ Coverage    56.41%   56.44%   +0.02%     
===========================================
  Files          121      121              
  Lines        14317    14326       +9     
===========================================
+ Hits          8077     8086       +9     
  Misses        5542     5542              
  Partials       698      698              

Impacted Files	Coverage Δ
query/graphql/schema/descriptions.go	86.84% <100.00%> (+0.35%)	⬆️
query/graphql/schema/generate.go	82.51% <100.00%> (+0.11%)	⬆️

[fredcarle]

Looks good 👍

[jsimnz]

While reviewing this, I surfaced a strange bug.

I wanted to test what happens if you give it a schema with a nullable field, ie numbers: [Int] instead of numbers: [Int!] and see what kind of error response would happen, or if it just silently translated to a non-nullable equivalent.

In doing so I got the error: "Field missing associated relation. FieldName: numbers, SchemaType: Int, ObjectType: user" which I thought was a strange error. So this is potentially related to #574.

Also saw those same errors in the log about "Cannot update a relation that is already finalized" which was also strange since I was using a basic schema with scalar (and scalar arrays) only, so it shouldn't be interacting with the relation system.

Turns out, that all gql.List types are being treated as FieldKind_FOREIGN_OBJECT_ARRAY types, which was designed for arrays of related types (objects). IE friends: [User]. But scalar arrays don't fall into this category.

The tests are passing, because technically both the creation and query respond correctly, but it means internally the structure isn't exactly represented correctly, and it tries to use the relation system for scalar arrays.

All this to be able to say:

1. Should we silently translate [Int] to [Int!] if present in a given schema
   or
2. Should we return an error saying nullable scalar arrays aren't supported at the moment.

Both of these options likely means resolving the above issue. which would likely be handled by addressing the difference between types for gql.List => [Scalar] and gql.List => [Object]

[fredcarle]

I wanted to test what happens if you give it a schema with a nullable field, ie numbers: [Int] instead of numbers: [Int!] and see what kind of error response would happen, or if it just silently translated to a non-nullable equivalent.

Good catch! I should have tried doing that too when reviewing.

1. Should we silently translate [Int] to [Int!] if present in a given schema
   or
2. Should we return an error saying nullable scalar arrays aren't supported at the moment.

Both of these options likely means resolving the above issue. which would likely be handled by addressing the difference between types for gql.List => [Scalar] and gql.List => [Object]

If we plan on supporting [Int] in the future, I think option 1 is a reasonable thing to do as long as we document that we are doing that.

[AndrewSisley]

Should we silently translate [Int] to [Int!] if present in a given schema?

I'm really not a fan of silently altering the types users explicitly give us

Should we return an error saying nullable scalar arrays aren't supported at the moment.

I don't think we need to define this, there are a fair few other gql types that we don't support (such as time) - and that behaviour is undefined as as far as I can tell (we have no tests)

[AndrewSisley]

Merging, as I see John's comment as a separate issue (and assume John does to - no changes requested?) - I have linked to it from #574 as it might be relevant there. The undefined type issue is separate too, and I don't think we have an issue for it, but I'm not sure it needs one atm (feel very free to create if anyone wants).