search

sangria-graphql / sangria

Scala GraphQL implementation
https://sangria-graphql.github.io
Apache License 2.0
1.96k stars 226 forks source link

Schema Validation Change in 1.4.1 #374

Closed alyssaBiasi closed 6 years ago

alyssaBiasi commented 6 years ago

When upgrading from 1.4.0 to 1.4.1, we found that there is a breaking change which causes our schema to fail validation on startup with the following error:

Type name 'X' is used for several conflicting GraphQL ObjectTypes based on different classes.

Our schema contains a number of union types. Some of the fields on the types share field names and type names. While we belatedly noticed this causes issues using tools like GraphiQL and type definitions being dropped from the /schema endpoint output, requests have been working as expected.

As discussed on gitter, we believe that PR #345 is the cause, despite us explicitly defining our object types instead of deriving from case classes.

I've tried to come up with a stripped back example using images and videos (note the required case classes and types are in the details blocks to make this easier to absorb - not sure if that's helpful I can pull them out if you prefer).

val MediaType: UnionType[Unit] = UnionType(
  name = "Media",
  types = List(VideoType, ImageType)
)

val QueryType = ObjectType(
  name = "Query",
  fields = fields[Unit, Unit](
    Field(
      name = "media",
      fieldType = ListType(MediaType),
      resolve = _ => List(
        Image("Building", MetadataImage("Spain")),
        Video("Cat video", MetadataVideo(5))
      )
    )
  )
)

val schema = Schema(QueryType)

val query = graphql"{ media { __typename ... on Video { title data { length } } ... on Image { data { location } } } }"

val result = Executor.execute(schema, query)
Case Classes ``` trait Media case class MetadataVideo(length: Int) case class Video(title: String, data: MetadataVideo) extends Media case class MetadataImage(location: String) case class Image(title: String, data: MetadataImage) extends Media ``` It's worth noting that these case classes live in different packages in our codebase so `MetadataVideo` and `MetadataImage` would just be called `Metadata`. I've named them differently here to avoid conflicts when working in a Scala worksheet
Schema for video `{ data { length } }` ``` val VideoDataType = ObjectType( name = "Metadata", fields = fields[Unit, MetadataVideo]( Field( name = "length", fieldType = IntType, resolve = _.value.length ) ) ) val VideoType = ObjectType( name = "Video", fields = fields[Unit, Video]( Field( name = "title", fieldType = StringType, resolve = _.value.title ), Field( name = "data", fieldType = VideoDataType, resolve = _.value.data ) ) ) ```
Schema for Image `{ data { location } }` ``` val ImageDataType = ObjectType( name = "Metadata", fields = fields[Unit, MetadataImage]( Field( name = "location", fieldType = StringType, resolve = _.value.location ) ) ) val ImageType = ObjectType( name = "Image", fields = fields[Unit, Image]( Field( name = "title", fieldType = StringType, resolve = _.value.title ), Field( name = "data", fieldType = ImageDataType, resolve = _.value.data ) ) ) ```

With version 1.4.0, this executes as expected. With version 1.4.1, this results in the following validation error:

Type name 'Metadata' is used for several conflicting GraphQL ObjectTypes based on different classes. ``` sangria.schema.SchemaValidationException: Schema does not pass validation. Violations: Type name 'Metadata' is used for several conflicting GraphQL ObjectTypes based on different classes. Conflict found in a field 'data' of 'Image' type. One possible fix is to use ObjectTypeName like this: deriveObjectType[Foo, Bar](ObjectTypeName("OtherBar")) to avoid that two ObjectTypes have the same name. at sangria.schema.Schema.typeConflict$1(Schema.scala:763) at sangria.schema.Schema.collectTypes$1(Schema.scala:790) at sangria.schema.Schema.$anonfun$types$3(Schema.scala:810) at scala.collection.TraversableOnce.$anonfun$foldLeft$1(TraversableOnce.scala:157) at scala.collection.TraversableOnce.$anonfun$foldLeft$1$adapted(TraversableOnce.scala:157) at scala.collection.Iterator.foreach(Iterator.scala:944) at scala.collection.Iterator.foreach$(Iterator.scala:944) at scala.collection.AbstractIterator.foreach(Iterator.scala:1432) at scala.collection.IterableLike.foreach(IterableLike.scala:71) at scala.collection.IterableLike.foreach$(IterableLike.scala:70) at scala.collection.AbstractIterable.foreach(Iterable.scala:54) at scala.collection.TraversableOnce.foldLeft(TraversableOnce.scala:157) at scala.collection.TraversableOnce.foldLeft$(TraversableOnce.scala:155) at scala.collection.AbstractTraversable.foldLeft(Traversable.scala:104) at sangria.schema.Schema.collectTypes$1(Schema.scala:808) at sangria.schema.Schema.$anonfun$types$8(Schema.scala:831) at scala.collection.LinearSeqOptimized.foldLeft(LinearSeqOptimized.scala:122) at scala.collection.LinearSeqOptimized.foldLeft$(LinearSeqOptimized.scala:118) at scala.collection.immutable.List.foldLeft(List.scala:86) at sangria.schema.Schema.collectTypes$1(Schema.scala:831) at sangria.schema.Schema.$anonfun$types$3(Schema.scala:810) at scala.collection.TraversableOnce.$anonfun$foldLeft$1(TraversableOnce.scala:157) at scala.collection.TraversableOnce.$anonfun$foldLeft$1$adapted(TraversableOnce.scala:157) at scala.collection.Iterator.foreach(Iterator.scala:944) at scala.collection.Iterator.foreach$(Iterator.scala:944) at scala.collection.AbstractIterator.foreach(Iterator.scala:1432) at scala.collection.IterableLike.foreach(IterableLike.scala:71) at scala.collection.IterableLike.foreach$(IterableLike.scala:70) at scala.collection.AbstractIterable.foreach(Iterable.scala:54) at scala.collection.TraversableOnce.foldLeft(TraversableOnce.scala:157) at scala.collection.TraversableOnce.foldLeft$(TraversableOnce.scala:155) at scala.collection.AbstractTraversable.foldLeft(Traversable.scala:104) at sangria.schema.Schema.collectTypes$1(Schema.scala:808) at sangria.schema.Schema.types$lzycompute(Schema.scala:836) at sangria.schema.Schema.types(Schema.scala:750) at sangria.schema.Schema.inputTypes$lzycompute(Schema.scala:848) at sangria.schema.Schema.inputTypes(Schema.scala:848) at sangria.schema.DefaultValuesValidationRule$.validate(SchemaValidationRule.scala:61) at sangria.schema.SchemaValidationRule$.$anonfun$validate$1(SchemaValidationRule.scala:37) at scala.collection.immutable.List.flatMap(List.scala:335) at sangria.schema.SchemaValidationRule$.validate(SchemaValidationRule.scala:37) at sangria.schema.SchemaValidationRule$.validateWithException(SchemaValidationRule.scala:40) at sangria.schema.Schema.(Schema.scala:901) ... 40 elided ```
OlegIlyenko commented 6 years ago

Thank you very much for an excellent issue description! Description makes it easy to see and verify the problem. The actual issue is that you have different 2 types with the same name Metadata. If you would rename them to, let's say ImageMetadata and VideoMetadata it will work with both versions of sangria. I would say that lack of validation for this specific scenario was an oversight in 1.4.0.

The reason for this is because GraphQL type system has a global namespace for all types in the schema. This means that on an introspection level there can be only 1 type with the name Metadata. You mentioned that you experienced some issues with GraphiQL before. I think this was the reason for it as well - GraphiQL discoveries the schema via an introspection API, so it is only aware of one Metadata type. Although it was somehow working in past, I believe it was due to the specific of the runtime execution. Since in this form the schema is inconsistent (2 different types with the same name), it might cause all sorts of unexpected behaviour (which not always will manifest itself as an explicit error). This is why it is important to validate it at the schema definition time and ensure the schema consistency.

Let me know if you have any followup questions or concerns.

alyssaBiasi commented 6 years ago

The reason for this is because GraphQL type system has a global namespace for all types in the schema.

That is very interesting. We thought this problem was related to the union type not the schema as a whole. So if videos and images are returned separately without a MediaType union the schema is still invalid.

val QueryType = ObjectType(
  name = "Query",
  fields = fields[Unit, Unit](
    Field(
      name = "videos",
      fieldType = ListType(VideoType),
      resolve = _ => List(Video("Cat video", MetadataVideo(5)))
    ),
    Field(
      name = "images",
      fieldType = ListType(ImageType),
      resolve = _ => List(Image("Building", MetadataImage("Spain")))
    )
  )
)

This feels like a strange constraint as I'm sure that there are many domains out there that contain entities with overlapping (but different) subtypes. Having to explicitly name them and be aware of types elsewhere in large apps seems hard. It makes sense though given there's no real way to have some sort of package style encapsulation.

Ah well! If it's a part of the specification, then I guess we'll start renaming things. Thanks so much for your help @OlegIlyenko 😄

OlegIlyenko commented 6 years ago

As a proponent of DDD myself, I definitely can see the appeal of having types with the same name coming from different bounded contexts. I also like idea of namespaces and a while back made a proposal for adding this feature in the GraphQL:

https://github.com/facebook/graphql/issues/163

Look like it is one of the most popular proposals to spec, but unfortunately there is still no clear consensus on this one.