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

Open
1 task done
stergion opened this issue Feb 12, 2023 · 3 comments · May be fixed by #59 or #137
Open
1 task done

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

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

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
@ghost ghost added this to Bugs in JS Feb 12, 2023
@stergion stergion linked a pull request Feb 12, 2023 that will close this issue
@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
@igwejk igwejk linked a pull request Dec 9, 2023 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
Status: 🏗 In progress
JS
  
Bugs
3 participants