apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
566 stars 45 forks source link

empty field definition list allowed for object type definition #753

Closed qwerdenkerXD closed 5 months ago

qwerdenkerXD commented 10 months ago

Description

As defined in Specification has an object type to define at least one field. This is not satisfied in grammar of apollo-parser.

Steps to reproduce

Validate this very simple schema:

type Query {}

Here a code sample.

Expected result

The parsed syntax tree should have one error with the message: expected Field Definition

Actual result

No errors.

Environment

qwerdenkerXD commented 10 months ago

Maybe just do the same as for enum or input types, so instead of doing this https://github.com/apollographql/apollo-rs/blob/208d2d7960459469891ff2ab5584f23d02a54d6d/crates/apollo-parser/src/parser/grammar/object.rs#L40-L42 just do sth. like

...

if let Some(T!['{']) = p.peek() {
   object_fields_definition(p);
}

...

pub(crate) fn object_fields_definition(p: &mut Parser) {
    let _g = p.start_node(SyntaxKind::FIELDS_DEFINITION);
    p.bump(S!['{']);
    if let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
        field::field_definition(p);
    } else {
        p.err("expected Field Definition");
    }
    while let Some(TokenKind::Name | TokenKind::StringValue) = p.peek() {
        field::field_definition(p);
    }

    p.expect(T!['}'], S!['}']);
}

But at this point I'm wondering if there is any case where a list of definitions defined as e.g. here FieldsDefinitionlist is expected to possibly be empty. Because then it would need to be changed for the field grammar. As you are more familiar with the GraphQL spec, you may know the answer, because I don't read it from the spec.

SimonSapin commented 10 months ago

I think you’re correct. In spec notation the "list" suffix is defined as "one or more": https://spec.graphql.org/October2021/#sec-Grammar-Notation.Optionality-and-Lists

There’s also "opt" for optional. Somethinglist opt is used for "zero or more":

image

qwerdenkerXD commented 10 months ago

Ok then it needs to be changed in field grammar. If I'm right, this issue also exists for extensions and would be fixed in this way.

SimonSapin commented 5 months ago

I believe this is fixed in https://github.com/apollographql/apollo-rs/pull/845 and https://github.com/apollographql/apollo-rs/pull/847.