connection should be non nullable #2581

SimonCropp · 2021-06-29T00:34:50Z

thoughts on this one?

codecov-commenter · 2021-06-29T00:39:13Z

Codecov Report

Merging #2581 (55173f8) into master (9235617) will increase coverage by 0.03%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #2581      +/-   ##
==========================================
+ Coverage   83.40%   83.44%   +0.03%     
==========================================
  Files         348      348              
  Lines       14024    14031       +7     
  Branches     2124     2125       +1     
==========================================
+ Hits        11697    11708      +11     
+ Misses       1750     1745       -5     
- Partials      577      578       +1

Impacted Files	Coverage Δ
src/GraphQL/Types/Scalars/UriGraphType.cs	`70.00% <71.42%> (-1.43%)`	⬇️
...hQL.SystemTextJson/ExecutionResultJsonConverter.cs	`89.41% <100.00%> (ø)`
...solveFieldContext/ResolveFieldContextExtensions.cs	`85.00% <100.00%> (+0.84%)`	⬆️
...rc/GraphQL/Types/Composite/InputObjectGraphType.cs	`80.00% <100.00%> (ø)`
src/GraphQL/Types/Relay/ConnectionType.cs	`100.00% <100.00%> (ø)`
...hQL/Utilities/Federation/FederatedSchemaPrinter.cs	`85.52% <100.00%> (+0.39%)`	⬆️
...rc/GraphQL/Utilities/Federation/FederatedSchema.cs	`83.33% <0.00%> (+83.33%)`	⬆️

Continue to review full report at Codecov.

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

Shane32 · 2021-06-29T00:56:08Z

Couple thoughts:

Fine with me... all my connection fields are already non-nullable.
This would be a breaking change; it should go in the next major version.
Is this behavior what would be typically expected? Either in general or specifically for this library? Sometimes it seems like schemas are defined with largely nullable fields, but I'm not sure if that's just a lack of knowledge on the part the developer. Defaulting to null does allow for partial results in case of an error, but I'm not sure how valuable that is.

What's your take @SimonCropp ?

SimonCropp · 2021-06-29T01:35:56Z

Fine with me... all my connection fields are already non-nullable.

how are you doing this? the current api doesnt make it easy, or i am missing something

This would be a breaking change; it should go in the next major version.

yep, changing type could be breaking depending on how clients handle null=>notnull

Is this behavior what would be typically expected? Either in general or specifically for this library? Sometimes it seems like schemas are defined with largely nullable fields, but I'm not sure if that's just a lack of knowledge on the part the developer. Defaulting to null does allow for partial results in case of an error, but I'm not sure how valuable that is.

I would prefer most thing defaulting to not null

SimonCropp · 2021-06-29T02:22:48Z

also should

edges: [CommitmentEdge]
items: [Commitment!]

actually be

edges: [CommitmentEdge!]!
items: [Commitment!]!

?

SimonCropp · 2021-06-29T04:31:00Z

looks like the lgtm error is unrelated to my change?

Shane32 · 2021-06-29T05:17:58Z

Fine with me... all my connection fields are already non-nullable.

how are you doing this? the current api doesnt make it easy, or i am missing something

Just checked and although I thought they were non-null, I was wrong. I don't use the connection builder, so I just need to write my own Connection<> and Edge<> classes to fix my API.

I would prefer most thing defaulting to not null

Sounds good to me

also should

edges: [CommitmentEdge]
items: [Commitment!]

actually be

edges: [CommitmentEdge!]!
items: [Commitment!]!

?

I agree. Now whether Commitment is null or non-null is configured through the connection builder, right? I didn't look closely at the connection builder API yet... but guessing it already does this.

looks like the lgtm error is unrelated to my change?

Correct. It's been broken for some time, but I don't have permission to remove the plugin from github, nor the password (or time) to fix the problem on lgtm.

SimonCropp · 2021-06-29T05:27:32Z

Now whether Commitment is null or non-null is configured through the connection builder, right? I didn't look closely at the connection builder API yet... but guessing it already does this.

no, the connection builder api has it hard coded as nullable

Shane32 · 2021-06-29T05:31:57Z

Now whether Commitment is null or non-null is configured through the connection builder, right? I didn't look closely at the connection builder API yet... but guessing it already does this.

no, the connection builder api has it hard coded as nullable

Hmm. Maybe look at that. It would seem that either it should follow the pattern of other fields where you need to specify an explicit graph type (e.g. NonNullGraphType<MyType>) or the pattern of the expression fields that don’t specify a type at all but infer it, along with a nullable boolean property to determine the proper graph type. I wouldn’t think it should automatically assume that the item type is non null.

SimonCropp · 2021-06-29T09:21:14Z

also updating cursor to be non null as per spec https://relay.dev/graphql/connections.htm#sec-undefined.PageInfo.Fields

SimonCropp · 2021-06-29T09:27:08Z

i made edge and items not null

Shane32 · 2021-06-29T13:07:58Z

src/GraphQL/Types/Relay/PageInfoType.cs

+            Field<NonNullGraphType<StringGraphType>>("startCursor", "When paginating backwards, the cursor to continue.");
+            Field<NonNullGraphType<StringGraphType>>("endCursor", "When paginating forwards, the cursor to continue.");


I don't think we can make these non-nullable. If there are no results, these would be null. See the reference specification schema. https://relay.dev/docs/guides/graphql-server-specification/

consider me educated. i have reverted this

I don't think we can make these non-nullable. If there are no results, these would be null. See the reference specification schema. https://relay.dev/docs/guides/graphql-server-specification/

There seems to be conflicting advice in the documentation. startCursor and endCursor are marked as nullable in the schema in the link you provided @Shane32 , but following the link to the GraphQL Cursor Connections spec suggests the opposite. Namely:

PageInfo must contain fields hasPreviousPage and hasNextPage, both of which return non-null booleans. It must also contain fields startCursor and endCursor, both of which return non-null opaque strings.

The Introspection of PageInfo in this doco lists all PageInfo properties as "kind": "NON_NULL"

How do we tell which one is correct?

Even within the spec it is confusing:

An “Edge Type” must contain a field called cursor. This field must return a type that serializes as a String; this may be a String, a Non-Null wrapper around a String, a custom scalar that serializes as a String, or a Non-Null wrapper around a custom scalar that serializes as a String.

Whatever type this field returns will be referred to as the cursor type in the rest of this spec.

So there it says that cursor fields may be nullable. But down below as you say, it explicitly says a "non-null string" and does not mention the words "cursor type" as quoted above.

I think that common sense must prevail here and dictate that when there are no records returned, there surely cannot be a starting cursor and ending cursor. I know that if the cursor was an index, then one could use int.MinValue and int.MaxValue -- but it is not. For example, if the cursor on an item is an ID field of type Guid, there is no possible guid that could represent the starting cursor or ending cursor for a connection that returned no results. So I think it is clear that these fields must be nullable.

If we assume that startCursor and endCursor must be nullable, then we have two options before us:

Use common sense and specify the cursor field's type of an edge type to be a non-null string value.

Match the cursor field's type of the edge type to be nullable to match the startCursor and endCursor field types.

Let us also consider one final thing: how does typical executing code use these fields? Well, it would store the startCursor or endCursor fields verbatim within a variable until needed to pass to the 'after' or 'before' property of another connection request. No parsing or any other type of processing should take place. So in this instance, 'after' or 'before' can be nullable, and indeed would always return the first page of a list if a user tried to navigate forward or backward from a connection that returned no results with null startCursor and endCursor fields. This seems to be perfectly logical behavior.

I suggest implementing the changes shown here, where the edge type's cursor field is non-nullable, but the pageinfo's startCursor and endCursor fields are nullable.

src/GraphQL/Builders/ConnectionBuilder.cs

Shane32 · 2021-06-29T15:04:15Z

src/GraphQL/Types/Relay/ConnectionType.cs

@@ -34,11 +34,11 @@ public ConnectionType()
                .Name("pageInfo")
                .Description("Information to aid in pagination.");

-            Field<ListGraphType<TEdgeType>>()
+            Field<NonNullGraphType<ListGraphType<TEdgeType>>>()


Shane32 · 2021-06-29T15:04:20Z

src/GraphQL/Types/Relay/ConnectionType.cs

                .Name("edges")
                .Description("A list of all of the edges returned in the connection.");

-            Field<ListGraphType<TNodeType>>()
+            Field<NonNullGraphType<ListGraphType<TNodeType>>>()


This reverts commit c4876b5.

SimonCropp · 2021-06-30T00:08:37Z

@Shane32 seems the connection api already supports nullable. i added a test 55173f8

so i think this is good to go?

Shane32 · 2021-07-06T19:19:54Z

@SimonCropp I do think this is ready to go but I might release another v4 before v5, in which case we shouldn't merge into master yet. I'll try to review again this weekend.

JTeeuwissen · 2022-05-12T15:59:59Z

Hi @SimonCropp, why was revert NonNullGraphType<TConnectionType this revert performed? It seems to me that the connection itself should still be non-nullable.

SimonCropp · 2022-05-12T23:07:13Z

@JTeeuwissen this wasnt merged

SimonCropp added 2 commits June 29, 2021 10:34

connection should be non nullable

a3025ed

Update StarWarsIntrospectionTests.cs

5ce2aa1

SimonCropp added a commit to SimonCropp/GraphQL.EntityFramework that referenced this pull request Jun 29, 2021

workaround graphql-dotnet/graphql-dotnet#2581

a45d871

SimonCropp mentioned this pull request Jun 29, 2021

Make connection non nullable SimonCropp/GraphQL.EntityFramework#722

Closed

make cursors non null

c4876b5

make edge and items non nullable

20721d0

Shane32 reviewed Jun 29, 2021

View reviewed changes

SimonCropp added 4 commits June 30, 2021 09:12

Revert "make cursors non null"

ed0b234

This reverts commit c4876b5.

revert NonNullGraphType<TConnectionType

7d1fe98

Update ConnectionBuilderTests.cs

6caa1ea

add test for nullable connection item graph type

55173f8

Shane32 added this to the 5.0 milestone Jul 1, 2021

SimonCropp mentioned this pull request May 9, 2022

connection should be non nullable #3123

Closed

SimonCropp closed this May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connection should be non nullable #2581

connection should be non nullable #2581

SimonCropp commented Jun 29, 2021

codecov-commenter commented Jun 29, 2021 •

edited

Loading

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021 •

edited

Loading

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

Shane32 Jun 29, 2021

SimonCropp Jun 29, 2021

ValdisThomann Jun 30, 2021

Shane32 Jun 30, 2021

Shane32 Jun 29, 2021

Shane32 Jun 29, 2021

SimonCropp commented Jun 30, 2021

Shane32 commented Jul 6, 2021

JTeeuwissen commented May 12, 2022

SimonCropp commented May 12, 2022

		Field<NonNullGraphType<StringGraphType>>("startCursor", "When paginating backwards, the cursor to continue.");
		Field<NonNullGraphType<StringGraphType>>("endCursor", "When paginating forwards, the cursor to continue.");

connection should be non nullable #2581

connection should be non nullable #2581

Conversation

SimonCropp commented Jun 29, 2021

codecov-commenter commented Jun 29, 2021 • edited Loading

Codecov Report

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021 • edited Loading

Shane32 commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

SimonCropp commented Jun 29, 2021

Shane32 Jun 29, 2021

Choose a reason for hiding this comment

SimonCropp Jun 29, 2021

Choose a reason for hiding this comment

ValdisThomann Jun 30, 2021

Choose a reason for hiding this comment

Shane32 Jun 30, 2021

Choose a reason for hiding this comment

Shane32 Jun 29, 2021

Choose a reason for hiding this comment

Shane32 Jun 29, 2021

Choose a reason for hiding this comment

SimonCropp commented Jun 30, 2021

Shane32 commented Jul 6, 2021

JTeeuwissen commented May 12, 2022

SimonCropp commented May 12, 2022

codecov-commenter commented Jun 29, 2021 •

edited

Loading

SimonCropp commented Jun 29, 2021 •

edited

Loading