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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

BUG SearchIssue id can be bigger than Integer.MAX #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d9322f
BUG SearchIssue id can be bigger than Integer.MAX
vootelerotov Feb 22, 2024
04df7c1
Merge branch 'master' into BUG-search-issues-id-can-be-bigger-than-int32
Abhi347 Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/spotify/github/v3/issues/Issue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public interface Issue extends CloseTracking {

/** ID. */
@Nullable
Integer id();
Long id();

/** URL. */
@Nullable
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/spotify/github/v3/clients/SearchClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,6 @@ public void testSearchIssue() throws Exception {
when(github.request(ISSUES_URI + "?q=bogus-q", SearchIssues.class)).thenReturn(fixture);
final SearchIssues search =
searchClient.issues(ImmutableSearchParameters.builder().q("bogus-q").build()).get();
assertSearchIssues(search);
assertSearchIssues(search, 35802);
}
}
15 changes: 12 additions & 3 deletions src/test/java/com/spotify/github/v3/search/SearchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@

public class SearchTest {

public static final void assertSearchIssues(final SearchIssues search) {
public static final void assertSearchIssues(final SearchIssues search, long id) {
final Issue issues = search.items().get(0);
assertThat(search.totalCount(), is(280));
assertThat(search.incompleteResults(), is(false));
assertThat(
issues.url(),
is(URI.create("https://api.github.com/repos/batterseapower/pinyin-toolkit/issues/132")));
assertThat(issues.number(), is(132));
assertThat(issues.id(), is(35802));
assertThat(issues.id(), is(id));
assertThat(issues.title(), is("Line Number Indexes Beyond 20 Not Displayed"));
}

Expand All @@ -52,6 +52,15 @@ public void testDeserialization() throws IOException {
Resources.toString(getResource(this.getClass(), "issues.json"), defaultCharset());

final SearchIssues search = Json.create().fromJson(fixture, SearchIssues.class);
assertSearchIssues(search);
assertSearchIssues(search, 35802);
}

@Test
public void testDeserializationWithLargeIssueId() throws IOException {
final String fixture =
Resources.toString(getResource(this.getClass(), "issues-with-large-id.json"), defaultCharset());
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 :)

Copy link
Member

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.


final SearchIssues search = Json.create().fromJson(fixture, SearchIssues.class);
assertSearchIssues(search, 2147534768L);
}
}
Loading