Current design of returning fragment definitions by name prevents duplicate detection

[allancalix]

Description

There are currently a number of validations applied to fragment definitions but the current implementation does not detect that each fragment for each name must be unique¹. It's not possible to add this validation to validate_fragment_definitions² because fragment queries all normalize definitions by name.

I propose modifying all_fragments to return a flat Arc<Vec<FragmentDefinition> and rename the current implementation to all_fragments_by_name. The same should apply to the fragments(file_id) version of the methods.

Footnotes

1. "https://spec.graphql.org/draft/#sec-Fragment-Name-Uniqueness.Formal-Specification" ↩

2. "https://github.com/apollographql/apollo-rs/blob/main/crates/apollo-compiler/src/validation/fragment.rs#L218" ↩

[lrlna]

We do check for fragment duplicates here¹. We don't, however, do it specifically for fragments, it's all just kind of done in one go. We should probably split that up between executable validation and type system validation, so executable validation can be run in isolation.

Here is an example with a duplicate fragment:

type Query {
  pet: Pet
}

interface Pet {
  name: String!
}

type Dog implements Pet {
  name: String!
  barkVolume: Int
}

query UseAllFragments {
  pet {
    ... dogFragment
  }
}

fragment dogFragment on Dog {
  ... on Dog {
    barkVolume
  }
}

fragment dogFragment on Dog {
  ... on Dog {
    barkVolume
  }
}

and here is what the diagnostic looks like:

Footnotes

1. https://github.com/apollographql/apollo-rs/blob/main/crates/apollo-compiler/src/validation/validation_db.rs#L261 ↩

[gmac]

Alternatively, I think it would be most useful if there was some access to all_definitions, allowing implementers to see all the raw definitions (executable, type system, repeat names, etc) that were submitted as part of the input document, and allows the implementer to write their own validations without relying on out-of-the-box compiler validations that may be redundant and/or incomplete for their goals.

To that end, simply making the source AST accessible from outside the compiler would be sufficient for addressing the bulk of use cases that I'm referring to.

[SimonSapin]

Depending on "how raw" you want definitions, I think the AST may be more appropriate for some tasks. It can be accessed like this:

use apollo_compiler::database::AstDatabase;

// make a `compiler` and `file_id`

let ast = compiler.db.ast(file_id);
for def in ast.document().definitions() {
    // …
}

[lrlna]

I will close this issue, as we've redesigned the compiler with the 1.0.0-beta.x releases and this shouldn't be an issue anymore. Feel free to reopen if something isn't right still.

[SimonSapin]

To expand a bit more on this, the 1.0 API is centered on high-level document Schema and ExecutableDocument where IndexMap is usually used for things expected to have unique names in valid documents. Duplicates are detected during conversion (from AST) to this representation and then not preserved.

For use cases not satisfied by this, the lower-level AST is also provided. There, every collection is a Vec of items in the order they appear in GraphQL source text.