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 null pointer exception #35244

Merged
merged 11 commits into from
Apr 21, 2022
Merged

fix(gatsby-source-contentful): Prevent null pointer exception #35244

merged 11 commits into from
Apr 21, 2022

Conversation

xavivars
Copy link
Contributor

@xavivars xavivars commented Mar 29, 2022

Description

This MR prevents a null pointer when creating asset nodes that are not configured on some languages.

Related Issues

Fixes #35216

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 29, 2022
@xavivars xavivars changed the title Fix null pointer exception on gatsby-source-contentful Workarounds null pointer exception on gatsby-source-contentful Mar 29, 2022
@xavivars xavivars changed the title Workarounds null pointer exception on gatsby-source-contentful Prevents null pointer exception on gatsby-source-contentful Mar 29, 2022
@wardpeet
Copy link
Contributor

Any chance you can update a test?

@xavivars
Copy link
Contributor Author

@wardpeet , is the solution "good enough"? TBH, I'm not sure creating "broken assets" makes sense.

image

I needed this to (at least) make my build pass. If you think this is good enough, I can try to add a test.

@axe312ger
Copy link
Collaborator

@wardpeet , is the solution "good enough"? TBH, I'm not sure creating "broken assets" makes sense.

Yes, I think it does make sense. It is a situation that can be caused within Contentful, when using Preview API:

When you create a new entry, the auto save can be triggered. That will save entities with required fields that are empty.

Entries in current version of the plugin are not affected by this. This is something I have to address in #31385, as the new schema generation will finally mirror the fields from Contentful instead of guessing them.

@axe312ger
Copy link
Collaborator

As @xavivars gave me write access to his repo, I'll update this PR with a test in a moment :)

@axe312ger
Copy link
Collaborator

My test repo looks like the following. It should mirror all cases of published/unpublished, valid/invalid, translated/untranslated and has no locale fallback enabled.

I'll add tests that emulate a build process using data from this space. One for Delivery API and one for Preview API.

This should cover a lot of use cases, that are not covered by our E2E tests. (They rely on rendering the Delivery API only)

Screenshot 2022-04-07 at 15 04 56

Screenshot 2022-04-07 at 15 05 05

Screenshot 2022-04-07 at 15 05 13

@LekoArts LekoArts added topic: source-contentful Related to Gatsby's integration with Contentful and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Apr 7, 2022
@LekoArts LekoArts changed the title Prevents null pointer exception on gatsby-source-contentful fix(gatsby-source-contentful): Prevent null pointer exception Apr 7, 2022
@axe312ger
Copy link
Collaborator

My tests show that the fix is not enough. I changed the code to properly nullify all fields when the file is missing. (No matter if not uploaded or not translated)

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.

Can't we ignore creating the asset if the file is null? That sounds like a better path forward, no?

@axe312ger
Copy link
Collaborator

@wardpeet i think you are right. There is no use in importing "broken" assets. If we simply ignore empty assets, the implementation on user/project level should be simplified as well.

I'll change it!

@axe312ger
Copy link
Collaborator

I pushed the change, assets without any field will now be skipped.

The good part:

This is a performance improvement! Less nodes!

The bad part:

This could be seen as a breaking change, as data that was existing is suddenly gone.

On the other hand, everyone affected by this potentially breaking change is also affected by the bug and can't build (As long their content has assets with missing files)

@axe312ger
Copy link
Collaborator

@wardpeet

The CI now suffers of Error: MDB_PROBLEM: Unexpected problem - txn should abort (#35055 (comment))

https://app.circleci.com/pipelines/github/gatsbyjs/gatsby/81481/workflows/9efc942e-361d-4d52-a5d7-4c48eec52eb9/jobs/965650

Comment on lines 751 to 752
mimeType: file.contentType ?? null,
filename: file.fileName ?? null,
Copy link
Contributor

Choose a reason for hiding this comment

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

These two props can't be null

Suggested change
mimeType: file.contentType ?? null,
filename: file.fileName ?? null,
mimeType: file.contentType,
filename: file.fileName,

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wardpeet Unfortunately they can in some cases when using the Preview API and a (big) asset is still processed by Contentful

I talked to the API team about it. So I added another check for file.url to exist and more comments to this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can't create an asset when these things are null cause Image CDN needs them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

alright, then lets require them as well! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done!

@LekoArts
Copy link
Contributor

Any documentation we need to update with this change? You mentioned this would be a "somewhat" breaking change.

@axe312ger
Copy link
Collaborator

@LekoArts

The change/commit documentation would be:

Assets without a file, either empty or untranslated without locale fallback, will no more be available within Gatsby.

This ensures the new Image CDN feature is working properly and simplifies the check for the existence of an (translated) asset.

Fixes #35216

You could argue that this is a breaking change, as it might require changes in the code, especially when handling assets without locale fallbacks. On the other hand, everyone benefitting from this fix, won't be able to build their site right now.

This is likely to prevent multiple projects from upgrading to Gatsby v4.

@wardpeet
Copy link
Contributor

@axe312ger can you sketch when it would be breaking change? (Can this happen outside of preview?)

@axe312ger
Copy link
Collaborator

@wardpeet:

The change requires to change code that handles non-existent assets. Before this, assets could exist, and users might checked if asset.file.url did exist. Now the whole asset won't be there anymore.

For users using a space without locale fallback, the page is likely not to build at all as it will fail due to incomplete assets in translation.

The more I think about it, the less it is breaking. It is either working now for you, so you probably don't have to change anything, if its not working for you, this is the patch you need.

I'd say lets publish it to canary/next and see?

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 21, 2022
@wardpeet wardpeet merged commit 7bc7cf6 into gatsbyjs:master Apr 21, 2022
@axe312ger axe312ger deleted the fix/gatsby-source-contentful-assets branch April 22, 2022 08:21
@xavivars xavivars restored the fix/gatsby-source-contentful-assets branch April 22, 2022 11:33
@xavivars xavivars deleted the fix/gatsby-source-contentful-assets branch April 22, 2022 11:34
tyhopp pushed a commit that referenced this pull request Apr 26, 2022
Co-authored-by: axe312ger <[email protected]>
Co-authored-by: Lennart <[email protected]>
Co-authored-by: Ward Peeters <[email protected]>
(cherry picked from commit 7bc7cf6)
tyhopp pushed a commit that referenced this pull request Apr 26, 2022
#35492)

Co-authored-by: axe312ger <[email protected]>
Co-authored-by: Lennart <[email protected]>
Co-authored-by: Ward Peeters <[email protected]>
(cherry picked from commit 7bc7cf6)

Co-authored-by: Xavi Ivars <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby-source-contentful Cannot read properties of null (reading 'url')
4 participants