Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single reference fields that accept multiple content types don't work in GraphQL #10090

Closed
aofolts opened this issue Nov 22, 2018 · 31 comments · Fixed by #19916
Closed

Single reference fields that accept multiple content types don't work in GraphQL #10090

aofolts opened this issue Nov 22, 2018 · 31 comments · Fixed by #19916
Assignees
Labels
status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: GraphQL Related to Gatsby's GraphQL layer type: bug An issue or pull request relating to a bug in Gatsby

Comments

@aofolts
Copy link

aofolts commented Nov 22, 2018

Description

Revisiting: #3535

As stated in the previous issue (closed), single reference fields that accept multiple content types cause GraphQL to error out: "Expected value of type "ContentfulPage" but got: [object Object]."

The workaround is to create a multiple reference field, but this shouldn't be a permanent fix.

Steps to reproduce

Write a query for a content type with a single reference field which accepts multiple types.

GraphQL errors out.

Expected result

The field should support fragments for each content type.

Actual result

GraphQL errors out: "Expected value of type "ContentfulPage" but got: [object Object]."

@pieh
Copy link
Contributor

pieh commented Nov 22, 2018

It would be great to have ready to use reproduction for this

@szokrika
Copy link

szokrika commented Nov 27, 2018

fragment NavLink on ContentfulModelLink {
    label
    target {
      route
    }
    href {
      href
    } 
  }

When running gatsby build this fragment throws:

"Expected value of type "ContentfulPage" but got: [object Object]."

if I remove the target node, it works.

gatsby develop will go thru, but gatsby build will stop the build.

Other example. Basically I'm linking an asset to a link.

Errors:
  Expected value of type "ContentfulPage" but got: [object Object].

  GraphQL request (163:3)
  162:   label
  163:   target {
         ^
  164:     route

@szokrika
Copy link

szokrika commented Nov 28, 2018

So I fixed this by limiting the target query to the ContentfulModelLink like this:

fragment Link on ContentfulModelLink {
    label
    ... on ContentfulModelLink {
      target {
        route
      }
      analyticsEvent
      accessibilityLabel
      additionalLabel
      tooltip
    }
    href: childContentfulModelLinkHrefTextNode {
      href
    }
    isNewWindow
  }

@aofolts
Copy link
Author

aofolts commented Dec 4, 2018

I wish this issue could get fixed, because it's SUPER obnoxious having to remember to unpack layout[0] in multiple places, when it should really be a single entry. Is there a reason why this isn't a priority?

@KyleAMathews
Copy link
Contributor

KyleAMathews commented Jan 1, 2019

@aofolts agree it sounds obnoxious! Would you like help putting together a PR to fix it?

@jevenson
Copy link
Contributor

jevenson commented Jan 1, 2019

So I was running into this issue using gatsby-source-contentful.

My content model had a Page model, which had a reference field that could be a Landing Page Layout, or a Content Page Layout.

I was trying to dynamically render the pages that had Content Page Layouts using the gatsby-node file. I was querying for allContentfulPage and I wanted to filter to only pages that had the Content Page Layout selected in the reference field. This is where I ran into the issue, because the reference field was not a union type of contentfulLandingPageLayout and contentfulContentPageLayout.

To resolve this, I moved the reference from the parent to the child object. So instead of having the reference field on the parent, I have a reference field on each of the Layout models.

Now I can query allContentfulContentPageLayouts and get all the pages that should be rendered dynamically.

This isn't ideal, but should be a good workaround until this gets resolved. The gatsby-source-contentful README.md recommends putting the reference on the child instead of the parent anyway.

@coreyward
Copy link
Contributor

coreyward commented Jan 2, 2019

@jevenson Putting the reference on the child doesn’t work in some situations, but I’m glad it resolved your issue.

  • When a child can belong to multiple parents
  • When the order of children matters (not really applicable for a single ref, but if you use this pattern elsewhere in Contentful it can be confusing to change it up)
  • When you need to impose a restriction on the number of children associated with the parent (this is true of single-assoc references: there’s no way to enforce this in Contentful when the reference is on the child)

@erickteowarang
Copy link

erickteowarang commented Jan 21, 2019

My dirty fix is to make a one to many reference field, and then limit it to 1 entry. For some reason it works then.

@szokrika
Copy link

szokrika commented Jan 21, 2019

{   
  allContentfulProductLandingPage{
    edges {
      node {
        name
        modules { #this is a reference type
          title
             modules { #this is a reference type
                __typename
                #title #I need title and subfields here
          }
        }
      }
    }
  }
}

How can I get more then the __typename from this query?

My content model has a page that has a reference module modules that included a title and a list of other modules. Seems that content cannot be reached with the plugin. Do you know why? and how to fix it?

@jrutz
Copy link

jrutz commented Feb 12, 2019

@szokrika To get more than __typename you create a query for the specific types that can be in that reference field that you're trying to pull back, for example:

{   
  allContentfulProductLandingPage{
    edges {
      node {
        name
        modules { 
          title
             modules { 
                __typename
               ... on ContentfulArticleA {
                     title
               }
              ... on ContentfulArticleB {
                     title
               }
           }
        }
      }
    }
  }
}

@coreyward
Copy link
Contributor

coreyward commented Feb 12, 2019

@jrutz While that's the correct GraphQL syntax, this isn't actually supported, which is what this issue is about. See above.

@jrutz
Copy link

jrutz commented Feb 12, 2019

@coreyward My bad, long time lurker, first time commenter on these Github threads. Meant to answer the response above me specifically given the workaround of allowing multiple references. We've run into the same issue with single references.

@pieh pieh added type: bug An issue or pull request relating to a bug in Gatsby status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. labels Feb 13, 2019
@pieh
Copy link
Contributor

pieh commented Feb 13, 2019

In case anyone will beat me to fix this - here's some places in gatsby code that are causing this:

When we produce example value to infer schema from, we select just single ___NODE value from all the nodes - this step loses information needed to correctly create Union type and we therefore we can infer just single type.

  • In

    return getExampleScalarFromArray(values)

    we would need special case (if (key.includes(`___NODE`)) {) and instead of extracting single value from array of nodes we would need to create some meta object - for example:

    if (isScalar(exampleValue)) {
      if (key.includes(`___NODE`)) {
        return {
          // we want to have access to all possible node ids to correctly use Union (if linked nodes are of multiple types)
          linkedNodes: values
          isArray: false 
        }
      }
      return getExampleScalarFromArray(values)
    }

    and use similar format few lines below where we have special handling for array of ids for ___NODE field.

  • In

    if (Array.isArray(value)) {
    const linkedNodes = value.map(v => findLinkedNode(v, linkedField))
    linkedNodes.forEach(node => validateLinkedNode(node))
    const linkedTypes = [...new Set(linkedNodes.map(n => n.internal.type))]
    const fields = linkedTypes.map(findType)
    fields.forEach((field, i) => validateField(linkedNodes[i], field))
    let type
    // If there's more than one type, we'll create a union type.
    if (fields.length > 1) {
    const typeName = `Union_${key}_${linkedTypes.sort().join(`__`)}`
    if (unionTypes.has(typeName)) {
    type = unionTypes.get(typeName)
    }
    if (!type) {
    type = new GraphQLUnionType({
    name: createTypeName(`Union_${key}`),
    description: `Union interface for the field "${key}" for types [${linkedTypes
    .sort()
    .join(`, `)}]`,
    types: fields.map(f => f.nodeObjectType),
    resolveType: node => node.internal.type,
    })
    unionTypes.set(typeName, type)
    }
    } else {
    type = fields[0].nodeObjectType
    }
    return {
    type: new GraphQLList(type),
    resolve: pageDependencyResolver(node => {
    let fieldValue = node[key]
    if (fieldValue) {
    return fieldValue.map(value => findLinkedNode(value, linkedField))
    } else {
    return null
    }
    }),
    }
    }
    const linkedNode = findLinkedNode(value, linkedField)
    validateLinkedNode(linkedNode)
    const field = findType(linkedNode.internal.type)
    validateField(linkedNode, field)
    return {
    type: field.nodeObjectType,
    resolve: pageDependencyResolver(node =>
    findLinkedNode(node[key], linkedField)
    ),
    }
    }

    we would need to use information from this meta object (i.e. instead of Array.isArray(value) we would want to use value.isArray)
    we would also could move out all the code that is responsible for picking type out of the isArray code path so we could use same type (or union) for both array and single link variant

  • In

    if (_.includes(key, `___NODE`)) {
    // TODO: Union the objects in array
    const isArray = _.isArray(value)
    const nodeToFind = isArray ? value[0] : value
    const linkedNode = findLinkedNode(nodeToFind)

    we would need to make some adjustments again to use value.isArray/value.linkedNodes.
    (this is code for creating filters and we don't really support filtering on unions, so let's keep current handling of just selecting first type)

  • Lastly make sure that all current tests passes (adjust test if needed - i.e. unit test for extracting example values might fail because of the change to returned format) and (ideally) add tests for this fix in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/schema/__tests__/infer-graphql-type-test.js / https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/schema/__tests__/infer-graphql-input-type-test.js to catch any regressions to this in future

@stjepangolemac
Copy link

stjepangolemac commented Feb 21, 2019

I can confirm that using one-to-many reference field, and then limiting it to 1 entry fixes this issue. Nevertheless, this is just a workaround and this issue should have top priority as it completely blocks the development.

Thanks @erickteowarang 🎉

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Mar 14, 2019
@gatsbot
Copy link

gatsbot bot commented Mar 14, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

Thanks for being a part of the Gatsby community! 💪💜

@coreyward coreyward added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Mar 14, 2019
@MantasMikal
Copy link

MantasMikal commented Mar 28, 2019

Hi,
Any news on this?

@vincro
Copy link

vincro commented Jun 4, 2019

Hi,
Any news on this?

Would be great to have this resolved.

@jaredsilver
Copy link
Contributor

jaredsilver commented Jun 28, 2019

We're also running into this, and we can confirm @stjepangolemac's confirmation that @erickteowarang's suggestion of using a one-to-many reference field limited to one reference indeed fixes the issue. However, this workaround makes me sad.

@coreyward
Copy link
Contributor

coreyward commented Jul 1, 2019

@pieh Is it possible the latest round of schema customization updates make this easier?

@lannonbr lannonbr added the topic: GraphQL Related to Gatsby's GraphQL layer label Aug 14, 2019
@dnlmzw
Copy link

dnlmzw commented Nov 13, 2019

My dirty fix is to make a one to many reference field, and then limit it to 1 entry. For some reason it works then.

I can also confirm that this works, although not pretty.

@bsonntag
Copy link
Contributor

bsonntag commented Nov 19, 2019

A lot changed in #19092, but this issue isn't resolved yet.

@vladar
Copy link
Contributor

vladar commented Nov 25, 2019

I noticed this inconsistency too while working on #19092. Will investigate how we can fix this. I guess it won't be a breaking change as having multiple referenced types simply doesn't work now.

@bsonntag
Copy link
Contributor

bsonntag commented Nov 25, 2019

I've been playing around with the new type inference, trying to add a union type based on the listOfUnion type based on @pieh 's idea in #10090 (comment) with some success. I don't have something working completely yet, tho.

@LukasMachetanz
Copy link

LukasMachetanz commented Apr 12, 2020

Hey, should this issue be resolved already? Because I am experiencing the exact same problem in the moment. And the mentioned workaround #10090 (comment) does not work for me either.

Any updates on this? I would appreciate it.

@coreyward
Copy link
Contributor

coreyward commented Apr 13, 2020

@LukasMachetanz No, I don't believe this has been fixed. Unfortunately @gatsbybot just closes everything that doesn't get worked on, which can lead to the false impression that it is no longer an issue.

@LekoArts
Copy link
Contributor

LekoArts commented Apr 14, 2020

I think you’re misunderstanding something here, the linked PR #19916 closed it and hence it should be fixed

@axe312ger
Copy link
Collaborator

axe312ger commented May 27, 2020

I did run into this as well after #19916 was merged and released.

I'll set up a test repository & contentful space to check this again.

@axe312ger axe312ger reopened this May 27, 2020
@axe312ger axe312ger added this to To do in Contentful source plugin improvements via automation May 27, 2020
@vladar
Copy link
Contributor

vladar commented May 27, 2020

@axe312ger Do you have a reproduction for this? Because I suspect that it might be a different issue.

@axe312ger
Copy link
Collaborator

axe312ger commented Jun 3, 2020

This is the structure I had which still caused issues ->

This was the old state, working well:

  • ContentType Page (Just a simple page with title, some text fields and so on)
  • ContentType MenuItem:
  • Title
  • Subitems (Menu items could reference sub menu items to create a menu tree)
  • Target (Single reference for page content type only)

This change broke it:

we need to add a new content type:

  • ContentType App (Like a page with some changes, irrelevant to the issue)

So we had to change the Target Field to allow referencing App content type as well.

The page was not building anymore, I don't know what error was shown, that was a while ago.

If you cant reproduce it with this, ping me, I'll try to recreate it with the original customer project :)

@vladar
Copy link
Contributor

vladar commented Jun 3, 2020

Thank you for clarifications!

Technically what you describe is a breaking change to the schema, so failing the query is expected. But it is unrelated to this issue.

Let me explain... Before adding App type you had:

type Page {
  id: ID!
  slug: String!
}

type MenuItem {
  target: Page
}

After you've added App it becomes:

type Page {
  id: ID!
  slug: String!
}
type App { id: ID! }

union PageAppUnion = Page | App

type MenuItem {
  target: PageAppUnion
}

Breaking changes to the schema like this must be addressed manually in your components anyways (because the structure of your data may have changed).

For example, your component that was written before this schema change could do something as simple as:

// assuming menuItem.target is always a Page type that has field "slug"
menuItem.target.slug.replace("a", "b")

After schema changes, this component will break (because when target is an App, it has no slug field).

But my main point is that it is unrelated to this issue. This issue was about the fact that the union from the last example was never created in the first place. So you couldn't even have PageAppUnion there.

I am going to close this as I am pretty sure it is fixed. And I suggest you open a new issue for your situation instead.

And strictly speaking, this was a bug in Gatsby core not in contentful source plugin.

@vladar vladar closed this as completed Jun 3, 2020
Contentful source plugin improvements automation moved this from To do to Done Jun 3, 2020
@axe312ger
Copy link
Collaborator

axe312ger commented Jun 5, 2020

@vladar Alright! I'll come up with a example repo as soon I come back to this. Thanks for clarification :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: GraphQL Related to Gatsby's GraphQL layer type: bug An issue or pull request relating to a bug in Gatsby
Development

Successfully merging a pull request may close this issue.