parse_field_set: forbid aliases

[duckki]

Added a check for the absence of aliases in field set directive arguments.

Rationale

Federation-next uses the apollo-compiler to parse the field-set argument string as GraphQL code. Similarly, the JS federation uses graphql-js to parse it. Both GraphQL parsers allow aliases in it, while federation does not allow it. That’s why the JS federation looks for aliases after parsing it. I think it makes sense to do the same thing in the Rust implementation so we don’t need to make changes in the apollo-compiler itself.

Side note

I did not attempt to make our error message to match exactly with the JS implementation. First of all, it's not necessary for unblocking QP work, while improving error messages will require further refactoring elsewhere. We might want to revisit the whole error message business later when we get to composition porting.

[SimonSapin]

Since apollo_compiler::executable::FieldSet exists specifically for federation directives and apollo_parser::Parser::parse_selection_set exists specifically for FieldSet, would it make sense to do this directly in parser or compiler?

Also, is there anything else that should not be accepted in a FieldSet? Directives, arguments, inline fragments, fragment spreads?

[duckki]

Since apollo_compiler::executable::FieldSet exists specifically for federation directives and apollo_parser::Parser::parse_selection_set exists specifically for FieldSet, would it make sense to do this directly in parser or compiler?

Also, is there anything else that should not be accepted in a FieldSet? Directives, arguments, inline fragments, fragment spreads?

Good point. The JS implementation only checked aliasing. I'll look into the other cases.

BTW, we tried not to change apollo_parser, leaving it for generic GraphQL. Otherwise, we'll need to add parser options to limit them.

[duckki]

Good point. The JS implementation only checked aliasing. I'll look into the other cases.

I've investigated this and here are my findings:

The spec (https://www.apollographql.com/docs/federation/subgraph-spec) doesn't articulate what's not allowed. So, I think it's up to the implementation.

Our JS federation allows arguments and inline spreads (with type conditions). The parser allows named fragments, but the validation will error out if used (since fragments can't be defined in schemas). On the other hand, it disallows aliases and directives explicltly. Interestingly, aliases are checked in parseFieldSetArgument function (thus, checked every time it's parsed), but the directive part is checked in validateFieldSetSelections function (only called during validation).

For QP porting purposes, I believe we can defer the rest of validation for a later time.

[duckki]

@SimonSapin @lrlna Oh, Simon's right. FieldSet.parse_and_validate is indeed specific for field set arguments. So, it makes sense to check aliases/directives in the validate_field_set function. It took me a long time to get here, but I want to get this right. I'll move the check into validate_field_set and refactor this PR as necessary.

[duckki]

Based on conversation on Slack, we are leaving the alias check in this repo, not moving it into apollo-rs repo.