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

validate does not check that selection sets are non-empty #3790

Closed
berkowitze opened this issue Nov 29, 2022 · 4 comments
Closed

validate does not check that selection sets are non-empty #3790

berkowitze opened this issue Nov 29, 2022 · 4 comments

Comments

@berkowitze
Copy link

The validate function does not find any issues if you pass an AST with an empty selection set for an object/interface field.

Running into this when building a query ast based on a form, and toggling form fields sometimes lead to an empty selection set. I'm going to properly handle "empty" selection sets (by removing the node from the ast), but it being caught by validation would be really nice as well as a backup and for correctness.

From the spec:

If selectionType is an interface, union, or object:
The subselection set of that selection must NOT BE empty.

Repro (sandbox):

import {
  buildSchema,
  DocumentNode,
  Kind,
  OperationTypeNode,
  validate
} from "graphql";

const s = `
type Query {
  dog: Int
}

schema {
  query: Query
}
`;

const schema = buildSchema(s);

const query: DocumentNode = {
  kind: Kind.DOCUMENT,
  definitions: [
    {
      kind: Kind.OPERATION_DEFINITION,
      operation: OperationTypeNode.QUERY,
      selectionSet: {
        kind: Kind.SELECTION_SET,
        selections: []
      }
    }
  ]
};

console.log(validate(schema, query)); // outputs `[]`

Happy to fix this but this would be my first OSS contribution so wanted to ask:

  • Is this the right place to post this?
  • Is this actually an issue?
  • What is the general approach to a fix?
  • What is the right wording for any error messaging?
@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 2, 2024

This seems like a bug!

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 2, 2024

It seems like from the section you are quoting that the ScalarLeafsRule was co-opted in the spec to also say something about non-leaf types and then did not get implemented. We may need another rule, but it might pay to discuss at a wg meeting whether the spec text should be divided as well.

@JoviDeCroock
Copy link
Member

Technically this case is covered by the parser already so we'd have to manually construct the AST. I do think it's worth covering this case though, in case a consumer is using an alternative parser

JoviDeCroock added a commit that referenced this issue Oct 13, 2024
Closes #3790

This follows the spec closer to the letter as we consider empty
selection-sets invalid as well. This however does not cover empty
operation-definitions, which should also be considered invalid.

The parser itself considers empty-selections invalid as it requires you
to specify a name before a closing curly, hence why testing happens a
bit more manual here. Despite our parser not considering this valid, I
think this is good to validate when folks use a separate parser or
construct documents manually as this is valid in our type-system.
@JoviDeCroock
Copy link
Member

Added in v17 on main thank you for reporting! We'll also bring this up at a WG meeting

erikwrede pushed a commit to erikwrede/graphql-js that referenced this issue Oct 17, 2024
Closes graphql/graphql-js#3790

This follows the spec closer to the letter as we consider empty
selection-sets invalid as well. This however does not cover empty
operation-definitions, which should also be considered invalid.

The parser itself considers empty-selections invalid as it requires you
to specify a name before a closing curly, hence why testing happens a
bit more manual here. Despite our parser not considering this valid, I
think this is good to validate when folks use a separate parser or
construct documents manually as this is valid in our type-system.
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

No branches or pull requests

3 participants