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

fix(gatsby-source-contentful): Prevent TypeError if many-to-one reference already exists #17500

Merged
merged 1 commit into from Sep 10, 2019
Merged

fix(gatsby-source-contentful): Prevent TypeError if many-to-one reference already exists #17500

merged 1 commit into from Sep 10, 2019

Conversation

froddd
Copy link
Contributor

@froddd froddd commented Sep 9, 2019

Description

When creating nodes, linkages to foreign references are created. If a foreign reference is of type many-to-one, the reference is recorded as a string (normalize.js:330). When creating the reverse linkage (normalize.js:345), createContentTypeNodes() attempts to push to this foreign reference. As it is, in this case, a string, this results in a TypeError.

The above can be tested using the following content type schema:

Blog Post:
- title: string
- slug: slug
- author: single Linked entry (Author)
Author:
- name: string
- favouritePost: single Linked entry (Blog Post)

With one Blog post linking to one Author which itself links to the Blog Post, the node creation will fail with the following error:

 TypeError: entryItemFields[foreignReference.name].push is not a function

      343 |           const existingReference = entryItemFields[foreignReference.name]
      344 |           if (existingReference) {
    > 345 |             entryItemFields[foreignReference.name].push(
          |                                                    ^
      346 |               mId(foreignReference.id)
      347 |             )
      348 |           } else {

      at push (packages/gatsby-source-contentful/src/normalize.js:345:52)
          at Array.forEach (<anonymous>)

Fix

I've added the above simple schema to the test data for normalize.js, and fixed by adding a Array.isArray() check (this approach seems favoured earlier: normalize.js:298), for consistency, rather than checking that the reference is not a string.

Snapshots have been updated.

Related Issues

Related to #3064 (possibly?)

In the case of many-to-one references, the foreign reference is a string
rather than an array. In this case, trying to create the reverse linkage
results in a TypeError from attemtping to `push()` to a string. This
checks the `push()` method can indeed be used to prevent such
TypeError(s).
@froddd froddd requested a review from a team as a code owner September 9, 2019 08:00
@froddd froddd changed the title Prevent TypeError if many-to-one reference already exists [gatsby-source-contentful] Prevent TypeError if many-to-one reference already exists Sep 9, 2019
@wardpeet
Copy link
Contributor

wardpeet commented Sep 9, 2019

Hey @froddd!

Thanks for this PR! We really appreciate you taking the time to open it. This PR looks good but to have more confident in moving this forward I would love to know how to test this outside of jest.

In the interest of moving this along most effectively, we strongly encourage our contributors to provide an example project which we can then use to validate the changes in this PR. This will allow us to quickly and easily ensure that this does indeed address the purported feature or fix in this PR!

An example project:

can be run locally or credentials have been provided
Feel free to message on Twitter, Discord, etc.
can be used to validate this functionality
In other words, the necessary functionality has been enabled/disabled in the project for us to easily validate these > changes>
If you're able to provide an example that we can use to test these changes we can more quickly get this merged and released. If you're unable--we will prioritize as best as we can and test when we're able to do so.

Thanks for this PR and thanks for using Gatsby!

💜

@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Sep 9, 2019
@froddd
Copy link
Contributor Author

froddd commented Sep 9, 2019

Sure thing -- example repo to showcase the issue is a straight clone of the Gatsby Contentful Starter project with one migration added: https://github.com/froddd/gatsby-contentful-starter

Install as normal and link to your contentful space:

$ yarn install
$ yarn run setup

Add an Author content type with a favouritePost field which links to a single blogPost content type -- you can run the migration I've provided for this:

$ contentful space migration -s <your space id> --mt <your management token> contentful/migrations/000-create-author-content-type.js

At this point, if you simply run the dev site locally (yarn run dev), everything should be fine.

Add an Author which has one of the posts as favourite and publish it:
Screenshot 2019-09-09 at 11 08 51

Now clear the cache and re-run the local dev environment:

$ rm -rf .cache && rm -rf public && yarn run dev

You will get the following error:

error Plugin gatsby-source-contentful returned an error


  TypeError: entryItemFields[foreignReference.name].push is not a function

  - normalize.js:331 foreignReferences.forEach.foreignReference
    [gatsby-contentful-starter]/[gatsby-source-contentful]/normalize.js:331:52

[stack trace truncated]

@froddd
Copy link
Contributor Author

froddd commented Sep 9, 2019

Is there a way to link to this PR's gatsby-source-contentful package in the example project to see the fixed behaviour? 🤔

Or are you happy with simply cloning my PR and using yarn link locally?

@froddd
Copy link
Contributor Author

froddd commented Sep 9, 2019

When linking locally this now produces another error:

error UNHANDLED REJECTION


  Error: ContentfulAsset.fixed provided incorrect OutputType: 'ContentfulFixed'

  - TypeMapper.js:294 TypeMapper.convertOutputFieldConfig
    [gatsby-contentful-starter]/[graphql-compose]/lib/TypeMapper.js:294:15

  - configAsThunk.js:19 resolveOutputConfigAsThunk
    [gatsby-contentful-starter]/[graphql-compose]/lib/utils/configAsThunk.js:19:41

  - ObjectTypeComposer.js:300 ObjectTypeComposer.getFieldConfig
    [gatsby-contentful-starter]/[graphql-compose]/lib/ObjectTypeComposer.js:300:58

  - toInputObjectType.js:44 fieldNames.forEach.fieldName
    [gatsby-contentful-starter]/[graphql-compose]/lib/utils/toInputObjectType.js:44:19

  - Array.forEach

I'll do more looking into this.

@froddd
Copy link
Contributor Author

froddd commented Sep 9, 2019

Looks like that extra error could be linked to this: #16455

@froddd
Copy link
Contributor Author

froddd commented Sep 10, 2019

I've created a better, more stripped-down example to validate the bugfix: https://github.com/froddd/gatsby-contentful-test

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I've tested it with your script and it works :ok-hand:. Contentful users will be grateful for this!

@wardpeet wardpeet changed the title [gatsby-source-contentful] Prevent TypeError if many-to-one reference already exists gatsby-source-contentful: Prevent TypeError if many-to-one reference already exists Sep 10, 2019
@wardpeet wardpeet changed the title gatsby-source-contentful: Prevent TypeError if many-to-one reference already exists fix(gatsby-source-contentful): Prevent TypeError if many-to-one reference already exists Sep 10, 2019
@wardpeet wardpeet merged commit fde5de6 into gatsbyjs:master Sep 10, 2019
@gatsbot
Copy link

gatsbot bot commented Sep 10, 2019

Holy buckets, @froddd — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@wardpeet
Copy link
Contributor

Published in gatsby-source-contentful@2.1.35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants