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

[BUG]: deepFindPathToProperty() throws TypeError if graphql response field is null #58

Closed
1 task done
stergion opened this issue Feb 12, 2023 · 5 comments · Fixed by #137 · May be fixed by #59
Closed
1 task done

[BUG]: deepFindPathToProperty() throws TypeError if graphql response field is null #58

stergion opened this issue Feb 12, 2023 · 5 comments · Fixed by #137 · May be fixed by #59
Labels
released Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented

Comments

@stergion
Copy link

stergion commented Feb 12, 2023

What happened?

Some times a graphql response fields have value null. This causes currentValue.hasOwnProperty(searchProp) to throw TypeError: Cannot read properties of null (reading 'hasOwnProperty') error.

Minimal Reproducible Example
import nock from 'nock';
import { Octokit } from "octokit";
import { paginateGraphql } from "@octokit/plugin-paginate-graphql";

const MyOctokit = Octokit.plugin(paginateGraphql);
const octokit = new MyOctokit({ auth: "secret123" });


const myQuery = `{ 
    query {
      user(login: "stergion") {
        name
        login
        contributionsCollection {
          earliestRestrictedContributionDate
          latestRestrictedContributionDate
        }
      }
    }
  }`;

nock.disableNetConnect();
nock('https://api.github.com')
  .post('/graphql', '{"query":"{ \\n    query {\\n      user(login: \\"stergion\\") {\\n        name\\n        login\\n        contributionsCollection {\\n          earliestRestrictedContributionDate\\n          latestRestrictedContributionDate\\n        }\\n      }\\n    }\\n  }"}')
  .reply(200, {
    "data": {
      "user": {
        "name": null,
        "login": "stergion",
        "contributionsCollection": {
          "earliestRestrictedContributionDate": "2022-02-13",
          "latestRestrictedContributionDate": "2023-02-14"
        }
      }
    }
  });

// const response = await octokit.graphql.paginate(myQuery);

try {
  const response = await octokit.graphql.paginate(myQuery);
  console.log(response);
} catch (error) {
  console.log(`${error.name}: ${error.status}`);
  console.log(error);
}

Versions

@octokit/plugin-paginate-graphql@2.0.1
octokit@2.0.14
Node v16.17.0

Relevant log output

TypeError: undefined
TypeError: Cannot read properties of null (reading 'hasOwnProperty')
    at deepFindPathToProperty (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:52:22)
    at deepFindPathToProperty (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:57:22)
    at findPaginatedResourcePath (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:38:33)
    at extractPageInfos (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:94:24)
    at Object.next (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:122:35)
    at async Function.paginate (/home/mre/node_modules/@octokit/plugin-paginate-graphql/dist-node/index.js:178:22)
    at async file:///home/rme/index.mjs:41:20

Code of Conduct

  • I agree to follow this project's Code of Conduct
@stergion stergion added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Feb 12, 2023
@stergion
Copy link
Author

Added a minimal reproducible example and the corresponding log output.

@kfcampbell kfcampbell added Priority: Normal Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Feb 17, 2023
@nickfloyd
Copy link
Contributor

I had a quick look at this. Here is a good place to start - the conditional on line 26 could be predicated by a null check on currentValue | this is a common issue in .NET as well around nullable types and hasValue.

However, I’m not 100% clear on what side effects that might cause so well need some coverage on this scenario as well - just to ensure this change is doing what is expected.

@stergion
Copy link
Author

stergion commented Aug 3, 2023

That's what I also thought. My suggested PR #59 adds a null check.

igwejk added a commit to igwejk/plugin-paginate-graphql.js that referenced this issue Dec 9, 2023
Adopt non-recursive path calculation.

Fixes octokit#58
@karpikpl
Copy link

I'm getting same issue when I try to do a query without enough permissions.

    const iterator = await this.graphql.paginate.iterator(query, { ent })
    let page_check_done = false
      for await (const result of iterator) {
        const orgs = result.enterprise.organizations.nodes

it blows up with:

TypeError: Cannot read properties of null (reading 'hasOwnProperty')
at deepFindPathToProperty (file:///home/projects/gh-users-action/node_modules/@octokit/plugin-paginate-graphql/dist-bundle/index.js:49:22)

is there a chance for a fix?

igwejk added a commit to igwejk/plugin-paginate-graphql.js that referenced this issue Sep 20, 2024
Previously the `deepFindPathToProperty` function would occasionally fail with `TypeError: Cannot read properties of null (reading 'hasOwnProperty')`. This change fixes that by adding a null check to the `findPaginatedResourcePath` function.

Resolves octokit#58
Copy link

🎉 This issue has been resolved in version 5.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
Status: Done
4 participants