fix: add null check in deepFindPathToProperty function

[igwejk]

Resolves #58

---

Before the change?

The deepFindPathToProperty function would occasionally fail with TypeError: Cannot read properties of null (reading 'hasOwnProperty').

After the change?

The error has been fixed:

• src/object-helpers.ts: The deepFindPathToProperty function was refactored and its return type updated from string[] to string[] | null. The refactoring includes the introduction of a new type TreeNode and several new helper functions like getDirectPropertyPath, makeTreeNodeChildrenFromData, and findPathToObjectContainingProperty. The findPaginatedResourcePath function was also updated to accommodate these changes.
• test/typescript-validate.ts: The pageInfoBackwaredExported function was renamed to pageInfoBackwardExported for correct spelling.
• test/object-helpers.test.ts: New tests were added for the findPaginatedResourcePath function to validate its behavior in different scenarios.

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[davelosert]

Hey @igwejk, thank you very much for the PR and for trying to fix #58!

Could you elaborate a bit more as to why you chose to entirely reimplement the deepFindPathToProperty-Function with a lot more code than we had previously, given that you were only trying to fix errors with possible null-values of certain fields?

It seems like you are covering other use-cases with this implementation as well and I don't quite understand 'which' and 'why' yet. 🙂

[igwejk]

👋 Hey @davelosert,

I honestly got carried away 🤦‍♂️ when considering other capabilities I would like the plugin to have

• potentially handle queries with multiple pageInfo that can be arbitrarily nested
• do the search without worrying about overflowing the call stack

Those are needs I came across first when querying repository statistics across an enterprise (and how I came about the motivating issue). This change also serves as a first baby step towards those goals.

Let me clearly say that I understand your line of question, and it makes total sense. So, I can offer to trim things down to a simple null check with additional tests if you would prefer that.

[davelosert]

Hey @igwejk,

as already discussed internally, we've chosen to not support multiple (or nested) pagination with this library on purpose, as this can lead to all sort of problems (you can find more info and a link to a more in depth-explanation in the README Section: Unsupported Nested Pagination.

As I also don't see a GraphQL Response that would be so nested that the current, recursive implementation would cause a overflowing call stack, I'd rather keep the current, more lightweight implementation and just use a null-check to avoid the error reported in #58.

Still, thank you again for the contribution and your work here! 👍🏻

[karpikpl]

Hey @igwejk,

as already discussed internally, we've chosen to not support multiple (or nested) pagination with this library on purpose, as this can lead to all sort of problems (you can find more info and a link to a more in depth-explanation in the README Section: Unsupported Nested Pagination.

As I also don't see a GraphQL Response that would be so nested that the current, recursive implementation would cause a overflowing call stack, I'd rather keep the current, more lightweight implementation and just use a null-check to avoid the error reported in #58.

Still, thank you again for the contribution and your work here! 👍🏻

Was that null check ever implemented?

[igwejk]

@karpikpl It was fixed with with an addition of support for deep nested pageInfo which was not required. I've now updated the PR to keep only the null check.

[github-actions]

🎉 This PR is included in version 5.2.3 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[kachick]

Did this line fix #58? The stacktrace and the description looks like it happens in deepFindPathToProperty implementation, not for the returned value.

This causes currentValue.hasOwnProperty(searchProp) to throw TypeError: Cannot read properties of null (reading 'hasOwnProperty') error.

plugin-paginate-graphql.js/src/object-helpers.ts

Lines 26 to 28 in 17e58b6

	if (currentValue.hasOwnProperty(searchProp)) {
	return currentPath;
	}

[kachick]

https://github.com/octokit/plugin-paginate-graphql.js/pull/59/files#diff-be5e0092082abfc624a87b5ab130c1f22c2c68205637088e88268bbcc776e96dR26 looks focusing to there