Skip to content

Treat @skip/@include fields as nullable (#424) - #425

Open
raunak2007 wants to merge 1 commit into
Khan:mainfrom
raunak2007:skip-include-nullable
Open

raunak2007 wants to merge 1 commit into
Khan:mainfrom
raunak2007:skip-include-nullable

Conversation

@raunak2007

Copy link
Copy Markdown

Fixes #424.

genqlient generated a selected field's Go type purely from its schema nullability, ignoring any @skip/@include on the selection. Those directives can make a non-null field absent from the response, so the generated type couldn't represent that absence: a skipped field decoded to the zero value with no error.

This treats a field with @skip/@include as nullable (as if id: ID rather than id: ID!) and lets the existing optional-handling decide the representation, per the discussion on #424:

  • optional: value (default): unchanged, still the zero value.
  • optional: pointer / optional: generic: the field becomes a pointer / Option and can represent an absent value.

Technically a breaking change for non-null fields with @skip/@include under optional: pointer or optional: generic. That seems right, and such users are probably rare and may already set @genqlient(pointer: true) by hand.

Tests: SkipInclude.graphql covers the default config (output unchanged), and it's added to the OptionalPointer case, where id now generates as *string instead of string.

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

@csilvers csilvers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A good catch! -- the rationale makes sense to me. And the fix is nice and self-contained. But I'll let @benjaminjkraft give the final OK, especially in regards to how we want to handle the fact this is technically a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@skip / @include fields are generated as non-optional, so a conditionally-absent value silently unmarshals to the zero value

2 participants