markhuot / craftql

A drop-in GraphQL server for Craft CMS
Other
319 stars 53 forks source link

Type Name Collisions between Matrix block types and Entry Types #125

Open timkelty opened 6 years ago

timkelty commented 6 years ago

It is possible to have type name collisions with similarly named sections and fields.

Admittedly this is coming from a super complex/convoluted content model, but it happened:

Will result in:

  "error": "Schema must contain unique named types but contains multiple types named \"SectionsSubsections\" (see http://webonyx.github.io/graphql-php/type-system/#type-registry)."
markhuot commented 6 years ago

Phew, that's definitely an edge case. Still… I'm thinking of the best way to solve this,

  1. when CraftQL bootstraps it loads all the entry types so I can ensure name uniqueness without any additional DB queries. I could tack Matrix on to the end of the matrix type if I see a conflict…
  2. I could provide some sort of way to override the naming scheme for certain types. I'm thinking this would be some sort of mapping in the config/craftql.php. I don't love this though because it means someone needs to go in to the config just to un-break their schema if they're working primarily through the Craft UI…
timkelty commented 6 years ago

Phew, that's definitely an edge case.

HOW DARE YOU insult convoluted my content model! 😁

But in all seriousness…yeah I think i'd prefer something closer to option 1…I'm wondering if this sort of thing could continue to crop up as more plugins support CraftQL schema (we can hope, right 😸). Considering that, you could maybe tack on something referring more specifically to the fact that its an EntryType or MatrixBlockType, etc...

markhuot commented 6 years ago

I like {fieldHandle}{blockHandle}Matrix ideally, but that'd be a breaking change so I'm worried about introducing it so late in the game. Adding it in only when needed feels safer because anyone afflicted by this, currently, has an entirely broken schema anyway so I can't imagine them complaining.

timkelty commented 6 years ago

A more foreseeable clash could happen in the near future…consider this:

Same thing as what I'm dealing with, but maybe in a more realistic, however hypothetical, scenario. Especially if the future brings more "types" to more elements.

timkelty commented 6 years ago

I like {fieldHandle}{blockHandle}Matrix ideally, but that'd be a breaking change so I'm worried about introducing it so late in the game. Adding it in only when needed feels safer because anyone afflicted by this, currently, has an entirely broken schema anyway so I can't imagine them complaining.

Totally. I even think at this point its fine just to do nothing, other than maybe a heads up in the docs. Just wanted to make sure you were aware it was possible.

markhuot commented 6 years ago

Sure, thanks @timkelty!

Categories are automatically suffixed with Category so a Category Group named Types would have a Twig handle of types and a CraftQL name of TypesCategory.

I expected that would be a conflict and added the suffix early. I wish I had done the same for Matrix fields. I think I'll take the first approach above for any matrix field that conflicts. In a later release, probably 2.0 I think I'd like to tack Matrix on to all the matrix block types.

timkelty commented 6 years ago

Sounds good! 👍

In all honesty the error let me know how effed up things were in our schema, and I just went and changed it 😆

joshangell commented 5 years ago

Hellloooo chaps ... so, I think I’m hitting a related issue, in that if I have a Matrix sub-field with the same handle as an existing field then the Matrix sub-field config is attached to the top-level field.

To replicate create a dropdown field with the handle dropdown and a bunch of options. Then create a Matrix field with whatever block types, make sure there is a dropdown field on there with the handle dropdown and some different options from the top-level one. Go to the GraphiQL tool and load the schema explorer and note that the top-level dropdown field has the options of the Matrix one.

Phew.

Is this the same thing do you reckon or a different issue? I don’t get any errors other than:

Error: GraphQL error: Variable "$dropdown" got invalid value "optionThree".
Expected type "DropdownEnum", found "optionThree".

Which is just because its validating my submission agains the wrong schema for that field.

— I’m using vue-apollo here to submit stuff.

jedcrowther commented 5 years ago

Hey! Unfortunately I have run into the same issue as @joshangell

I have an existing dropdown field with handle imageSize and a Matrix dropdown subfield with the handle imageSize. Both imageSize dropdown fields have different values.

When I go to query the Matrix imageSize dropdown field I receive the error: { "error": "Expected a value of type \"ImageSizeEnum\" but received: \"large\"" }

Of course if I update the handle name this is resolved, but that is a less than ideal solution