apollographql / apollo-rs

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

Schema types to implement `Hash` #792

Closed BrynCooke closed 8 months ago

BrynCooke commented 9 months ago

It looks like the schema types such as ObjectType, InterfaceType etc do not currently implement Hash. It would be super useful if they did so as it is likely that users will want to create lookups from graphql schema types to other datastructures.

ScalarType alone does implement Hash.

SimonSapin commented 8 months ago

These impls can’t be derived because IndexMap and IndexSet don’t implement Hash. Though perhaps the well-defined iteration order would make it easier to define a meaningful hash for IndexMap than HashMap.

In what kind of situation do you want to use a full definition with all of its details as a map key? As opposed to using only the name as a key, in the implicit context of a specific schema.

BrynCooke commented 8 months ago

I can imagine a situation where you would want to attach extra data to schema elements. As this isn't possible right now my first instinct was to use the objects as a key in a map. Are types unique within a schema? If so maybe it would make sense that Hash should not be deep and only use the name. Agree that users can maintain a separate datastructure to allow decoration of schema elements though, so happy to close.

SimonSapin commented 8 months ago

Yes, type definitions have unique names within a high-level Schema (not necessarily in a lower-level AST Document).

I’m not sure about implementing Hash for a definition by only hashing the name, as opposed to having users explicitly use names as keys. Something like composition can manipulate multiple (subgraph) schemas where the same type name can have different definitions.

BrynCooke commented 8 months ago

Let's close this then.