fix(wordpress): ensure all file links are rewritten #31652

rburgst · 2021-05-30T07:35:04Z

Description

Ensure that all file links are properly resolved even in cases where it spans multiple graphql queries.

Documentation

in the case of either having a small schema.perPage setting
or many many referenced resources on a single page only the first
page of links were properly rewritten
now we ensure that all pages are fully processed until we resolve the
promise

Related Issues

- in the case of either having a small `schema.perPage` setting or many many referenced resources on a single page only the first page of links were properly rewritten - now we ensure that all pages are fully processed until we resolve the promise fixes gatsbyjs#31646

rburgstaller · 2021-06-23T10:41:26Z

anyone home?

wardpeet · 2021-06-25T11:09:43Z

Hey, thank you for creating the PR, and sorry for taking so long to get to you.

I'll test the reproduction and come back to you!

rburgst · 2021-07-02T06:06:21Z

Hey, thank you for creating the PR, and sorry for taking so long to get to you.

I'll test the reproduction and come back to you!

Any news? we need this fix quite urgently

rburgst · 2021-07-08T04:01:11Z

@wardpeet any updates?

wardpeet

Thanks, sorry for keeping you waiting. The function is a bit weird here so I had a hard time wrapping my head around it.

TylerBarnes · 2021-07-16T23:15:40Z

I think we're going to need to revert this. It looks like it's causing the process to freeze now in tests on another branch since I pulled master (with these changes) into that branch. If I manually undo these changes it stops freezing the process

TylerBarnes · 2021-07-16T23:17:53Z

Looks like the tests were failing here too when this was merged 🤔 weird that the gatsbybot merged this when it wasn't green

This reverts commit f970600.

TylerBarnes · 2021-07-16T23:24:50Z

fwiw the solution here wont work because it will only ever resolve for a single request now that resolve is in that if statement but we need it to resolve for every request since it resolving there is part of what adds those nodes to Gatsby.

allResolvedNodes.push(...nodes) is scoped to a single call of fetchMediaItemsBySourceUrl so pushing there will do nothing if there are multiple calls to it and it will never resolve.

rburgst · 2021-07-17T18:25:50Z

Well the initial problem was that it resolved too soon in case there were more than one pages. Imho it's correct to only resolve once all pages have been fetched. @TylerBarnes could you add a test case that shows the problem. I would be happy to fix it once I know what the problem is.

TylerBarnes · 2021-07-19T17:10:13Z

@rburgst I don't think we need to add another test case for it since the integration tests caught this. The problem was the tests didn't run for your PR originally for some reason and then when "merge on green" was added the WP integration tests weren't marked as required so the PR merged with them failing. You can run them by running yarn test in integration-tests/gatsby-source-wordpress.

My guess is your code breaks when there's only 1 page. Somehow the condition you added never gets tripped and the promise never resolves so we wait indefinitely.
Ideally we should create a new promise for each page in the loop that adds pages to the queue here https://github.com/rburgst/gatsby/blob/7904abff5c2d64f8f364c3a3c4763074a3f039aa/packages/gatsby-source-wordpress/src/steps/source-nodes/fetch-nodes/fetch-referenced-media-items.js#L342 and then wait for all pages to resolve before calling resolveFutureNodes() after the loop.

I think not creating a new promise for each page and doing it as you had it originally could potentially result in subtle race conditions. For example you have 2 pages, the first page takes 2 seconds to finish (for some reason) and the second page takes 1 second. Here, we will resolve the nodes for the second page before the first page finishes. And this could get trickier the more pages there are.

rforster-dev · 2021-07-20T09:18:36Z

Sorry to interject, but does this I wonder relate to the bug I raised: #32350 around having low perPage schema settings?

TylerBarnes · 2021-07-20T16:40:34Z

@rforster-dev potentially yes. We should probably run a build in our integration tests with perPage set low since there are multiple places throughout the codebase that that setting effects and for a lot of sites that setting is needed for the build to succeed.

rburgst · 2021-07-21T08:24:07Z

@TylerBarnes I checked out the last commit before the revert (79b7b04) and ran the int tests. The only difference is that the test resolves node interfaces without errors (https://github.com/gatsbyjs/gatsby/blob/master/integration-tests/gatsby-source-wordpress/test-fns/data-resolution.js#L65) returns 31 nodes in the response rather than 30. The extra node is

{
            "id": "cG9zdDoxMTgzMQ==",
            "slug": "11828",
            "__typename": "WpBlockEditorPreview"
          },

I could not find any failed tests where the promise was not resolved.

This is the query I ran via gatsby develop

{
  allWpContentNode {
    totalCount
    nodes {
      id
      slug
      __typename
    }
  }
}

With the PR I get the following result (31 results):

{
    "data": {
      "allWpContentNode": {
        "totalCount": 31,
        "nodes": [
          {
            "id": "cG9zdDoxMTgzMQ==",
            "slug": "11828",
            "__typename": "WpBlockEditorPreview"
          },
          
          {
            "id": "cG9zdDo3NzQ4",
            "slug": "1002-4312x2868",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzYw",
            "slug": "10-2500x1667",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMTgyOA==",
            "slug": "featured-image-too-big",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDo3ODY4",
            "slug": "beautiful_slug",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoxNg==",
            "slug": "activity",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoy",
            "slug": "sample-page",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDozNjI=",
            "slug": "84-1000x1000",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzUy",
            "slug": "1003-1181x1772",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzU2",
            "slug": "1001-5616x3744",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDox",
            "slug": "hello-world",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDo3NjQ2",
            "slug": "acf-field-test",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoxMDQ=",
            "slug": "wordpress-logotype-standard",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDM=",
            "slug": "zoltan-tasi-482489-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDE=",
            "slug": "jordan-steranka-504707-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDI=",
            "slug": "kristopher-roller-110203-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo5Ng==",
            "slug": "brandon-siu-608784-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMjU=",
            "slug": "gutenberg-layout-element-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMjI=",
            "slug": "gutenberg-formatting-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMjg=",
            "slug": "gutenberg-columns",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMzM1",
            "slug": "english-page",
            "__typename": "WpTranslationFilterTest"
          },
          {
            "id": "cG9zdDoxMzEx",
            "slug": "french-page",
            "__typename": "WpTranslationFilterTest"
          },
          {
            "id": "cG9zdDoxMTY3",
            "slug": "11",
            "__typename": "WpTypeLimitTest"
          },
          {
            "id": "cG9zdDoxMDYyMQ==",
            "slug": "mattmullenweg-interview",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDYyNw==",
            "slug": "giphy",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo5NA==",
            "slug": "gutenberg-common-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDo3NzQw",
            "slug": "file-sample_1mb",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ1Mw==",
            "slug": "sample-mov-file",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ0OQ==",
            "slug": "sample-mp4-file",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ0NQ==",
            "slug": "sample-2",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQzOQ==",
            "slug": "sample",
            "__typename": "WpMediaItem"
          }
        ]
      }
    },
    "extensions": {}
  }

without it I get the following result (30 nodes)

{
    "data": {
      "allWpContentNode": {
        "totalCount": 30,
        "nodes": [
          {
            "id": "cG9zdDo3NzQ4",
            "slug": "1002-4312x2868",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzYw",
            "slug": "10-2500x1667",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMTgyOA==",
            "slug": "featured-image-too-big",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDo3ODY4",
            "slug": "beautiful_slug",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoxNg==",
            "slug": "activity",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoy",
            "slug": "sample-page",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDozNjI=",
            "slug": "84-1000x1000",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzUy",
            "slug": "1003-1181x1772",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo3NzU2",
            "slug": "1001-5616x3744",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDox",
            "slug": "hello-world",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDo3NjQ2",
            "slug": "acf-field-test",
            "__typename": "WpPage"
          },
          {
            "id": "cG9zdDoxMDQ=",
            "slug": "wordpress-logotype-standard",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDM=",
            "slug": "zoltan-tasi-482489-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDE=",
            "slug": "jordan-steranka-504707-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDI=",
            "slug": "kristopher-roller-110203-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo5Ng==",
            "slug": "brandon-siu-608784-unsplash",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMjU=",
            "slug": "gutenberg-layout-element-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMjI=",
            "slug": "gutenberg-formatting-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMjg=",
            "slug": "gutenberg-columns",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDoxMzM1",
            "slug": "english-page",
            "__typename": "WpTranslationFilterTest"
          },
          {
            "id": "cG9zdDoxMzEx",
            "slug": "french-page",
            "__typename": "WpTranslationFilterTest"
          },
          {
            "id": "cG9zdDoxMTY3",
            "slug": "11",
            "__typename": "WpTypeLimitTest"
          },
          {
            "id": "cG9zdDoxMDYyMQ==",
            "slug": "mattmullenweg-interview",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDYyNw==",
            "slug": "giphy",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDo5NA==",
            "slug": "gutenberg-common-blocks",
            "__typename": "WpPost"
          },
          {
            "id": "cG9zdDo3NzQw",
            "slug": "file-sample_1mb",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ1Mw==",
            "slug": "sample-mov-file",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ0OQ==",
            "slug": "sample-mp4-file",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQ0NQ==",
            "slug": "sample-2",
            "__typename": "WpMediaItem"
          },
          {
            "id": "cG9zdDoxMDQzOQ==",
            "slug": "sample",
            "__typename": "WpMediaItem"
          }
        ]
      }
    },
    "extensions": {}
  }

TylerBarnes · 2021-07-21T20:32:50Z

There were some changes to the int tests related to media items so you likely need to pull master into your branch. The other thing is I think promises will sometimes resolve and sometimes not since it'll be a race condition. So there might be some cases where the tests still pass 🤔

rburgst · 2021-07-22T04:39:18Z

That's why I checked out 79b7b04. I couldn't see a problem there other than the extra node.

If you can point me to a commit that would be better suited I would be grateful. At any rate I will try and replicate the problem with this commit in the meantime.

TylerBarnes · 2021-07-22T17:21:03Z

@rburgst you should be able to create a branch from your latest commit in this PR and then merge latest master into it 👍

rburgst · 2021-08-03T16:04:53Z

@TylerBarnes I did exactly this and now the tests succeed without error. I am not sure what to do. Could it be that this fix interacted with a different bug which was fixed in the meantime?

- in the case of either having a small `schema.perPage` setting or many many referenced resources on a single page only the first page of links were properly rewritten - now we ensure that all pages are fully processed until we resolve the promise - this is a recommit of gatsbyjs#31652 fixes gatsbyjs#31646

rburgst · 2021-08-03T16:12:09Z

@TylerBarnes since I cannot replicate any problem, I have rebased the changes and created a new PR: #32679

rburgst · 2021-08-03T18:36:20Z

@TylerBarnes note that I tweaked the PR more like you suggested by creating individual per page promises and using Promise.all at the end. I also added unit tests for single page usage.

- in the case of either having a small `schema.perPage` setting or many many referenced resources on a single page only the first page of links were properly rewritten - now we ensure that all pages are fully processed until we resolve the promise - this is an improved recommit of gatsbyjs#31652 fixes gatsbyjs#31646

* fix(wordpress): ensure all file links are rewritten - in the case of either having a small `schema.perPage` setting or many many referenced resources on a single page only the first page of links were properly rewritten - now we ensure that all pages are fully processed until we resolve the promise - this is an improved recommit of #31652 fixes #31646 Co-authored-by: gatsbybot <[email protected]> Co-authored-by: Tyler Barnes <[email protected]>

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 30, 2021

LekoArts added topic: source-wordpress Related to Gatsby's integration with WordPress and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 31, 2021

wardpeet approved these changes Jul 15, 2021

View reviewed changes

Merge branch 'master' into bugfix/wordpress-file-rewriting-bug

7904abf

wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jul 15, 2021

gatsbybot merged commit f970600 into gatsbyjs:master Jul 15, 2021

TylerBarnes added a commit that referenced this pull request Jul 16, 2021

Revert "fix(wordpress): ensure all file links are rewritten (#31652)"

237b1fb

This reverts commit f970600.

TylerBarnes mentioned this pull request Jul 16, 2021

Revert "fix(wordpress): ensure all file links are rewritten" #32398

Merged

rburgst mentioned this pull request Aug 3, 2021

fix(wordpress): ensure all file links are rewritten #32679

Merged

rburgst mentioned this pull request May 1, 2022

Gatsby-source-wordpress not able to fetch image behind basic authenticate WordPress source #35151

Closed

2 tasks

kaocher82 mentioned this pull request Oct 16, 2024

[Snyk] Fix for 4 vulnerabilities Xtuden-com/gatsby#6949

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(wordpress): ensure all file links are rewritten #31652

fix(wordpress): ensure all file links are rewritten #31652

rburgst commented May 30, 2021

rburgstaller commented Jun 23, 2021

wardpeet commented Jun 25, 2021

rburgst commented Jul 2, 2021

rburgst commented Jul 8, 2021

wardpeet left a comment

TylerBarnes commented Jul 16, 2021 •

edited

Loading

TylerBarnes commented Jul 16, 2021

TylerBarnes commented Jul 16, 2021 •

edited

Loading

rburgst commented Jul 17, 2021

TylerBarnes commented Jul 19, 2021 •

edited

Loading

rforster-dev commented Jul 20, 2021

TylerBarnes commented Jul 20, 2021

rburgst commented Jul 21, 2021 •

edited

Loading

TylerBarnes commented Jul 21, 2021

rburgst commented Jul 22, 2021

TylerBarnes commented Jul 22, 2021

rburgst commented Aug 3, 2021

rburgst commented Aug 3, 2021

rburgst commented Aug 3, 2021

fix(wordpress): ensure all file links are rewritten #31652

fix(wordpress): ensure all file links are rewritten #31652

Conversation

rburgst commented May 30, 2021

Description

Documentation

Related Issues

rburgstaller commented Jun 23, 2021

wardpeet commented Jun 25, 2021

rburgst commented Jul 2, 2021

rburgst commented Jul 8, 2021

wardpeet left a comment

Choose a reason for hiding this comment

TylerBarnes commented Jul 16, 2021 • edited Loading

TylerBarnes commented Jul 16, 2021

TylerBarnes commented Jul 16, 2021 • edited Loading

rburgst commented Jul 17, 2021

TylerBarnes commented Jul 19, 2021 • edited Loading

rforster-dev commented Jul 20, 2021

TylerBarnes commented Jul 20, 2021

rburgst commented Jul 21, 2021 • edited Loading

TylerBarnes commented Jul 21, 2021

rburgst commented Jul 22, 2021

TylerBarnes commented Jul 22, 2021

rburgst commented Aug 3, 2021

rburgst commented Aug 3, 2021

rburgst commented Aug 3, 2021

TylerBarnes commented Jul 16, 2021 •

edited

Loading

TylerBarnes commented Jul 16, 2021 •

edited

Loading

TylerBarnes commented Jul 19, 2021 •

edited

Loading

rburgst commented Jul 21, 2021 •

edited

Loading