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: Set id field of PullRequest, Comment and Issue to Long #200

Merged
merged 7 commits into from
Mar 10, 2025

Conversation

sindunuragarp
Copy link
Contributor

@sindunuragarp sindunuragarp commented Nov 25, 2024

We are experiencing JsonMappingException issues due to id being represented as an Integer in this client. We have observed the following data structures experiencing the issue: PullRequest, Comment, and Issue.

e.g.

Caused by: com.fasterxml.jackson.databind.JsonMappingException: Numeric value (2148718294) out of range of int (-2147483648 - 2147483647)
at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 88] (through reference chain: com.spotify.github.v3.prs.ImmutablePullRequest$Json["id"])

As of late October this year, PR ids have exceeded Integer.MAX. This is especially crippling since most Github Endpoints return an updated object as a response, which gets deserialized by the client. In the case of PullRequest, it results in all use cases related to handling PRs becoming unusable.

Official documentation states that PullRequest id is int64. This is probably the case for all entity ids as well.
https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request

Note: This PR extends from #180 which is older and has broken tests.


This PR is technically not backwards compatible (since it changes data types), but without the fix, the client is unusable for all use cases related to recent Pull Requests.

vootelerotov and others added 4 commits February 22, 2024 07:16

Unverified

This user has not yet uploaded their public signing key.
Represent it as Long instead of Integer.

For schema (where id is specified as 'int64') see:

https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#search-issues-and-pull-requests

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Represent the id as Long instead of Integer.

Official Schema for Pull Request -> int64:
https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@sindunuragarp sindunuragarp marked this pull request as ready for review November 25, 2024 20:06

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@sindunuragarp
Copy link
Contributor Author

Tagging @ebk45 and @Abhi347 for review 🙂

Abhi347 added 2 commits March 10, 2025 10:28

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Copy link
Member

@Abhi347 Abhi347 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have fixed a failing test and will merge it now.

Closes #180

@Abhi347 Abhi347 enabled auto-merge (squash) March 10, 2025 09:39
@Abhi347 Abhi347 merged commit 9d6e361 into spotify:master Mar 10, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants