Skip to content
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

Adds validation rule @lookup should have nullable return type #55

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions spec/Section 4 -- Composition.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,65 @@ run in sequence to produce the composite execution schema.

### Pre Merge Validation

#### `@lookup` Should Have Nullable Return Type

**Error Code**

LOOKUP_SHOULD_HAVE_NULLABLE_RETURN_TYPE

**Severity**

WARNING

**Formal Specification**

- Let {fields} be the set of all field definitions annotated with `@lookup` in the schema.
- For each {field} in {fields}:
- Let {type} be the return type of {field}.
- {type} must be a nullable type.

**Explanatory Text**

Fields annotated with the `@lookup` directive are intended to retrieve a single entity based on provided arguments.
To properly handle cases where the requested entity does not exist, such fields should have a nullable return type.
This allows the field to return `null` when an entity matching the provided criteria is not found, following the standard GraphQL practices for representing missing data.

In a distributed system, it is likely that some entities are not be found on other subgraphs, even when those subgraphs contribute fields to the type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In a distributed system, it is likely that some entities are not be found on other subgraphs, even when those subgraphs contribute fields to the type.
In a distributed system, it is likely that some entities will not be found on other subgraphs, even when those subgraphs contribute fields to the type.

Ensuring that `@lookup` fields have nullable return types also avoids GraphQL errors on subgraphs and prevents result erasure through non-null propagation.
By allowing null to be returned when an entity is not found, the system can gracefully handle missing data without causing exceptions or unexpected behavior.

Ensuring that `@lookup` fields have nullable return types allows gateways to distinguish between cases where an entity is not found (receiving null) and other error conditions that may have to be propagated to the client.

For example, the following usage is recommended:

```graphql example
extend type Query {
userById(id: ID!): User @lookup
}

type User {
id: ID!
name: String
}
```

In this example, `userById` returns a nullable `User` type, aligning with the recommendation.

This counter-example demonstrates a invalid usage:
Copy link
Contributor

@glen-84 glen-84 Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This counter-example demonstrates a invalid usage:
This counter-example demonstrates an invalid usage:


```graphql counter-example
extend type Query {
userById(id: ID!): User! @lookup
}

type User {
id: ID!
name: String
}
```

Here, `userById` returns a non-nullable `User!`, which does not align with the recommendation that a `@lookup` field should have a nullable return type.

### Merge

### Post Merge Validation
Expand Down
Loading