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

gatsby-source-contentful: Maximum call stack size exceeded #11364

Closed
marcneander opened this issue Jan 28, 2019 · 66 comments · Fixed by #19802
Closed

gatsby-source-contentful: Maximum call stack size exceeded #11364

marcneander opened this issue Jan 28, 2019 · 66 comments · Fixed by #19802
Assignees
Labels
type: bug An issue or pull request relating to a bug in Gatsby

Comments

@marcneander
Copy link

Not sure if this is a bug or not. And unfortunately I don't really have much time this week to investigate. But I thought I might as well put this up here.

I'm using the gatsby-source-contentful plugin. In my contentful content model I'm having two content types, both with the new Richtext editor. Problems occurs when I have entry links to each other.

To be more clear. Say I have two pages, home and about. If I link from home -> about and from about -> home inside the Richtext editor I get this error.

Like I said before. Not sure if this is a bug.

Here's a callstack https://pastebin.com/fD676e6j

Environment (if relevant)

  System:
    OS: macOS 10.14.3
    CPU: (4) x64 Intel(R) Core(TM) i5-8210Y CPU @ 1.60GHz
    Shell: 3.0.0 - /usr/local/bin/fish
  Binaries:
    Node: 10.15.0 - /var/folders/vh/cx86q1vs2xz5rxlf8b8cny2r0000gn/T/yarn--1548706327929-0.8647198662198463/node
    Yarn: 1.13.0 - /var/folders/vh/cx86q1vs2xz5rxlf8b8cny2r0000gn/T/yarn--1548706327929-0.8647198662198463/yarn
    npm: 6.4.1 - ~/.nvm/versions/node/v10.15.0/bin/npm
  Languages:
    Python: 2.7.10 - /usr/bin/python
  Browsers:
    Chrome: 71.0.3578.98
    Safari: 12.0.3
  npmPackages:
    gatsby: 2.0.98 => 2.0.98 
    gatsby-plugin-layout: 1.0.11 => 1.0.11 
    gatsby-plugin-offline: 2.0.21 => 2.0.21 
    gatsby-plugin-react-helmet: 3.0.5 => 3.0.5 
    gatsby-plugin-styled-components: 3.0.4 => 3.0.4 
    gatsby-source-contentful: 2.0.26 => 2.0.26 
    gatsby-transformer-remark: 2.2.0 => 2.2.0 

gatsby-config.js:

plugins: [
        {
            resolve: 'gatsby-plugin-styled-components',
            options: {
                displayName: false
            }
        },
        'gatsby-plugin-react-helmet',
        {
            resolve: 'gatsby-plugin-layout',
            options: {
                component: require.resolve('./src/components/Layout')
            }
        },
        {
            resolve: 'gatsby-source-contentful',
            options: {
                spaceId: '<spaceID>',
                accessToken: process.env.CONTENTFUL_ACCESS_TOKEN
            }
        },
        {
            resolve: '@contentful/gatsby-transformer-contentful-richtext',
            options: contentfulRenderOptions
        },
        'gatsby-plugin-offline'
    ]
@karenhou
Copy link
Contributor

When using reference fields, be aware that this source plugin will automatically create the reverse reference. You do not need to create references on both content types. For simplicity, it is easier to put the reference field on the child in child/parent relationships.

https://www.gatsbyjs.org/packages/gatsby-source-contentful/?=contentful

Not sure if this would help, but try to disconnect one of your reference.

@marcneander
Copy link
Author

marcneander commented Jan 29, 2019

@karenhou It's not reference fields that's the problem. It makes sense to have references one way. What I'm talking about is the Rich text field which has Entry hyperlink support. If you want to create a <a href> to another entry you'd do it that way. Problems occurs when two content entries link to each other. Which would be a very valid case.

Say you have a 3 part article that would link to each other in the beginning. That would never work right now if you're not fine hardcoding the links.

@sidharthachatterjee sidharthachatterjee added the type: question or discussion Issue discussing or asking a question about Gatsby label Jan 29, 2019
@sidharthachatterjee
Copy link
Contributor

My best guess is some recursive handling of references is exploding the call stack because of the lack of an exit case that covers this maybe?

@marcneander Can you try to create a minimal reproduction repo for this (with a contentful instance setup to break it) and link to it? That'll help us debug this faster

@sidharthachatterjee sidharthachatterjee added the status: needs reproduction This issue needs a simplified reproduction of the bug for further troubleshooting. label Jan 29, 2019
@simoneb
Copy link

simoneb commented Feb 1, 2019

I've been looking into this issue because we're also experiencing it. I'm not very familiar with the contentful source plugin codebase but the first impression is that the logic in place to avoid circular references only accounts for field types which are not richText. The latter is a lot more complex to analyze because it can contain a "link" to another entry in deeply nested places. I think that the logic to handle this should live in the buildForeignReferenceMap function in the gatsby-node.js module, but I'm not sure about the feasibility of recursively checking the complex data structure of richText fields.

At the current time, two entries cross-referencing each other via hyperlinks inside a richText field lead to infinite recursion.

This is a perfectly valid use case, of a series of articles referencing each other at the top of the article content for instance, and @marcneander repro is a perfect one.

@dkhuntrods
Copy link

@sidharthachatterjee Thanks for your work on this. Would just like to add my upvote to this issue - we've encountered the exact same scenario, and I would second @simoneb's comments about this being a very valid use-case and @marcneander's repro an accurate reduced test-case of our situation. We've got a very urgent situation on our hands at the moment - do you think it likely you'll be looking at this soon? Otherwise we will look at a PR of some sort. Thanks again.

simoneb added a commit to nearform/gatsby that referenced this issue Feb 5, 2019
simoneb added a commit to nearform/gatsby that referenced this issue Feb 8, 2019
@simoneb
Copy link

simoneb commented Feb 9, 2019

Hi @marcneander, given the lack of support around here we've worked around the issue by forking the gastby repo, changing the gatsby-source-contentful plugin and publishing our own version on npm.

Because it was really hard to figure out how to handle rich text nodes with links to other contentful entries, we've come up with a workaround which consists in the plugin accepting an array of whitelist field names to include when processing rich text field nodes. All other fields are omitted. For the lack of a better and generic solution, this basically shifts the responsibility of which fields won't cause circular references on the shoulders of developers.

For instance, we use the gatsby-transformer-contentful-richtext plugin to process rich text fields and transform hyperlinks to entries into plain <a> tags, in a similar way as shown in the docs:

[INLINES.ASSET_HYPERLINK]: node => {
  return `<img src="${
    node.data.target.fields.file['en-US'].url
  }"/>`
}

So, I configure our custom gatsby-source-contentful plugin with the option richTextFieldEntryHyperlinkWhitelist: ['file'], because it's the only field we use and we know it won't cause circular references.

You can find our fork here. The plugin is published as @nearform/gatsby-source-contentful.

Hope it helps!

@snide
Copy link

snide commented Feb 25, 2019

@simoneb TY for your fork. This fixed the issue for me as well. It's pretty easy to get into this kind of reference, so although it's not an optimal fix I really appreciate you publishing your fork.

@simoneb
Copy link

simoneb commented Feb 25, 2019

@snide you're welcome!

@wardpeet wardpeet 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. and removed status: needs reproduction This issue needs a simplified reproduction of the bug for further troubleshooting. type: question or discussion Issue discussing or asking a question about Gatsby labels Mar 1, 2019
@toby-bushell
Copy link

@sidharthachatterjee Thanks for your work on this. Would just like to add my upvote to this issue - we've encountered the exact same scenario, and I would second @simoneb's comments about this being a very valid use-case and @marcneander's repro an accurate reduced test-case of our situation. We've got a very urgent situation on our hands at the moment - do you think it likely you'll be looking at this soon? Otherwise we will look at a PR of some sort. Thanks again.

I'd also like to upvote this, this is currently making internal links impossible to use in the RichText. If the reference contains any link back to the content at any level this causes the stack size explosion, which is impossible to manage or restrict.

@simoneb
Copy link

simoneb commented Mar 6, 2019

@toby-bushell as unappealing a solution as it may sound, have you tried the fork I mentioned here? That's the solution we are applying for now.

@toby-bushell
Copy link

Hi @simoneb, am I right in thinking that the workaround is to skip trying to resolve internal links? And explicitly list the embed types to resolve in Rich Text?

@simoneb
Copy link

simoneb commented Mar 6, 2019

@toby-bushell not entirely. If you list only the fields that you need when processing your rich text, they will be resolved. The rest you will not need can safely not include in the whitelist so it doesn't cause issues.

@toby-bushell
Copy link

@simoneb thanks for this and the fork, It looks like a good failsafe option, that we may well use.

We do however want to be able to link to other references safely within RichText and are keen on a permanent solution, as this is a very big feature of contentful. Even an understanding of the roadmap for this so we can make a call on how much time to dedicate to alternatives.

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

gatsbot bot commented Mar 27, 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! 💪💜

@marcneander
Copy link
Author

Spooky quiet indeed. But not ready to be closed

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

olapersson commented Apr 1, 2019

Having the same issue, also found a mention of the same problem here contentful/rich-text#53 (issue was closed but it seems to me that the problem is still present).

Cross referencing entries is a quite natural thing (inline link from one page to another for example, much better to be able to use a reference rather than a hardcoded string)

As a temporary workaround I used the forked version and queried body { body } on the node and then parsed the json response (richTextRenderer being my wrapper function that maps embedded entries, images etc.)

      {richTextRenderer({json: JSON.parse(data.body.body) })}

Downside is the resulting data has a different structure so once it's fixed I'll have to refactor all field access.

@Khaledgarbaya
Copy link
Contributor

Hey @simoneb,
Any reason you didn't make a pull request since your fork fixed the issue?

@simoneb
Copy link

simoneb commented Apr 24, 2019

@Khaledgarbaya because the fix is not generic enough in my opinion, it's more of a workaround.

@cameron-martin
Copy link
Contributor

cameron-martin commented Dec 9, 2019

There is a repro in the form of an e2e test in this PR: #15358

It's not isolated from contentful, but it should demonstrate the issue still.

@pvdz
Copy link
Contributor

pvdz commented Dec 10, 2019

Ok. Can repro. On it.

@pvdz pvdz removed the status: needs reproduction This issue needs a simplified reproduction of the bug for further troubleshooting. label Dec 10, 2019
@pvdz
Copy link
Contributor

pvdz commented Dec 10, 2019

@cameron-martin Thank you. I was able to add a repro to the tests that caught this stack overflow and able to prevent the stack overflow. Please report back after testing with #20039 .

@pvdz
Copy link
Contributor

pvdz commented Dec 17, 2019

@cameron-martin This repro was fixed in #20039 which has been published. Please check whether this works for you.

I'm going to close this for now. If anyone has related problems feel free to reopen. Or a new issue and tag me.

Thanks!

@pvdz pvdz closed this as completed Dec 17, 2019
@jessepinho
Copy link
Contributor

jessepinho commented Jan 14, 2020

🚨 I've created a minimal reproduction of this issue! 🚨

This appears to still be a problem on the latest version of Gatsby, even after #20039 was merged.

Here is the repo: https://github.com/jessepinho/gatsby-contentful-infinite-loop-repro

Here's the Contentful Discovery app with all the data (note that a bug in the app prevents it from showing article records): https://discovery.contentful.com/entries/by-content-type?delivery_access_token=04QAYaRsr548atM4KwInDCWk8v84agx0uS7r8LrQutk&preview=false&preview_access_token=&space_id=j66qheg22uli

And a demonstration GIF:
screencap

@pvdz could you please reopen this issue? (I don't have rights to.)

@pvdz
Copy link
Contributor

pvdz commented Jan 14, 2020

Great, thank you!

@pvdz
Copy link
Contributor

pvdz commented Jan 17, 2020

Confirmed. Looking into a fix now.

@pvdz
Copy link
Contributor

pvdz commented Jan 17, 2020

The fix (which at least resolves your repro) is in #20674

Follow that to see when it gets merged and to see what version it gets released in.

@jessepinho
Copy link
Contributor

@pvdz WOW that was fast, thank you! When I get a chance, I'll try it with my real-life repo and see if it fixes the issue.

@pvdz
Copy link
Contributor

pvdz commented Jan 21, 2020

With #20674 being merged this repro has been fixed. It should be published on the next patch bump (should be posted on PR).

Please feel free to reopen (with repro..) if you're still having issues after that.

@pvdz pvdz closed this as completed Jan 21, 2020
@ma-pe
Copy link

ma-pe commented Jan 22, 2020

I can confirm that this fix (released with v 2.7.8) solves my problem 👍

Thanks a lot!

edit: I meant version number 2.1.78.

@jessepinho
Copy link
Contributor

I just upgraded and am still experiencing the problem in my own repo. @ma-pe what library do you have v2.7.8 of? There is no gatsby-source-contentful with that version...?

@pvdz
Copy link
Contributor

pvdz commented Feb 12, 2020

@jessepinho are you saying the repro you posted is still having this problem with the above fix in place? I couldn't repro it anymore when it landed. But please let me know if you can repro it when all deps are up to date (it may help to just delete the node_modules entirely and make sure the package.json is up to date).

@vpreponen
Copy link

I assumed he meant 2.1.78. I also continued to experience the problem with v2.1.78 so I continued to use a work-around with legacy versions.

@pvdz
Copy link
Contributor

pvdz commented Feb 12, 2020

@vpreponen The current published version is 2.1.85, can you check if you're still experiencing it with that version? (Is there anything blocking you from upgrading the patch bump?)

@jessepinho
Copy link
Contributor

@pvdz not the repro I posted, but a separate repo that I was trying to reproduce with that minimal repro I posted. Apparently it was a little too minimal 😆 Will look into this further. And yeah I used v2.1.85, and upgrade Gatsby to 2.19.16.

@pvdz
Copy link
Contributor

pvdz commented Feb 12, 2020

@jessepinho ok, if you manage to get another repro repo or test case please share it and I'll have a look and try to fix it :)

@vpreponen
Copy link

Deleting lock files and node_modules plus updating to gatsby v2.19.16 and gatsby-source-contentful v2.1.85 fixes the issue for me.

@inblock-io
Copy link

@jessepinho ok, if you manage to get another repro repo or test case please share it and I'll have a look and try to fix it :)

I have one here: #27515

@StyvekDrtic
Copy link

im still having problems...

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this issue Apr 28, 2022
…#19802)

* fix(gatsby): do not cause stack overflow over circular refs

This allows nodes to have a circular dependency without causing stack overflow exceptions when traversing the graph.

Fixes gatsbyjs#11364 (hopefully). Will have to confirm this after publishing an update.

Causes somewhere between 0 and 1% perf regression for our 25k page benchmark site.

* Use actual `if` for conditionals

* Only add data to the path when actually an object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment