-
Notifications
You must be signed in to change notification settings - Fork 93
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 SearchIssue id can be bigger than Integer.MAX #180
BUG SearchIssue id can be bigger than Integer.MAX #180
Conversation
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
Ran into this in the wild. |
Hmm, build is failing with |
It seems that a lot of PR-s now have ID-s that are bigger than |
Comment ID could have the same integer overflow. e.g. It maybe safer to convert all ID of |
Also ran into this. |
Yep, had the same problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vootelerotov The PR is good to go if you add the test json file. Please ping me once you have done so.
Thanks for your contribution
@Test | ||
public void testDeserializationWithLargeIssueId() throws IOException { | ||
final String fixture = | ||
Resources.toString(getResource(this.getClass(), "issues-with-large-id.json"), defaultCharset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you forgot to add the issues-with-large-id.json
file in the test resources. It needs to be added at src/test/resources/com/spotify/github/v3/issues/issues-with-large-id.json
for this PR to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh :(
@Abhi347 I think #200 is a more complete solution with passing tests (thanks @sindunuragarp), happy to close this PR in favour of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abhi347 Would be great if one of the two PRs could be merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge #200 Closing this now. Thanks for your contribution.
Closing this as another PR (#200) fixing this issue is merged |
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