-
-
Notifications
You must be signed in to change notification settings - Fork 72
fix: validate paginated GitHub responses before spreading #168
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
Open
AmrendraTheCoder
wants to merge
4
commits into
AOSSIE-Org:main
Choose a base branch
from
AmrendraTheCoder:fix/103-paginated-response-validation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+219
−37
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
cd4ea0f
fix: validate paginated GitHub responses before spreading
AmrendraTheCoder 51cc602
fix: return null when a response body is not valid JSON
AmrendraTheCoder 4267e2b
test: assert the bad page was actually requested
AmrendraTheCoder 8290d31
test: cover the maximum page ceiling
AmrendraTheCoder File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' | ||
| import { fetchContributors, fetchIssues, fetchPulls, fetchRepos } from './github' | ||
|
|
||
| // The service writes through to IndexedDB, which jsdom does not implement. | ||
| // Every cache helper already swallows its own errors, so a stub that always | ||
| // rejects exercises the real cache-miss path without touching storage. | ||
| const failingIndexedDB = { | ||
| open: () => { | ||
| const req = {} | ||
| queueMicrotask(() => req.onerror?.()) | ||
| return req | ||
| }, | ||
| } | ||
|
|
||
| function jsonResponse(body, { status = 200, headers = {} } = {}) { | ||
| return { | ||
| ok: status >= 200 && status < 300, | ||
| status, | ||
| headers: { get: k => headers[k] ?? null }, | ||
| text: async () => JSON.stringify(body), | ||
| } | ||
| } | ||
|
|
||
| /** A body that is not JSON at all, e.g. a proxy error page or a truncated response. */ | ||
| function textResponse(body, { status = 200 } = {}) { | ||
| return { | ||
| ok: status >= 200 && status < 300, | ||
| status, | ||
| headers: { get: () => null }, | ||
| text: async () => body, | ||
| } | ||
| } | ||
|
|
||
| /** GitHub answers 204 No Content for repos with no contributors; the body is empty. */ | ||
| function noContentResponse() { | ||
| return { | ||
| ok: true, | ||
| status: 204, | ||
| headers: { get: () => null }, | ||
| text: async () => '', | ||
| } | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.stubGlobal('indexedDB', failingIndexedDB) | ||
| vi.stubGlobal('fetch', vi.fn()) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| describe('paginated fetchers: response validation', () => { | ||
| it('returns an empty list when GitHub answers 204 No Content', async () => { | ||
| // A repo with no commits yet returns 204 from /contributors. Before the | ||
| // guard, res.json() threw and the whole fetch rejected, so explore()'s | ||
| // Promise.allSettled dropped the repo from the contributor map silently. | ||
| fetch.mockResolvedValue(noContentResponse()) | ||
|
|
||
| await expect(fetchContributors('AOSSIE-Org', 'EmptyRepo', 'pat')).resolves.toEqual([]) | ||
| }) | ||
|
|
||
| it('returns an empty list when the payload is an object rather than an array', async () => { | ||
| // Any non-array body used to reach `all.push(...data)` and throw | ||
| // "TypeError: data is not iterable". | ||
| fetch.mockResolvedValue(jsonResponse({ message: 'Moved Permanently' })) | ||
|
|
||
| await expect(fetchIssues('AOSSIE-Org', 'Renamed', 'pat')).resolves.toEqual([]) | ||
| }) | ||
|
|
||
| it('returns an empty list when the payload is null', async () => { | ||
| fetch.mockResolvedValue(jsonResponse(null)) | ||
|
|
||
| await expect(fetchPulls('AOSSIE-Org', 'Whatever', 'pat')).resolves.toEqual([]) | ||
| }) | ||
|
|
||
| it('returns an empty list when the body is not valid JSON', async () => { | ||
| // A proxy error page or a truncated response reaches JSON.parse, which | ||
| // threw before the try/catch and rejected the whole fetch. | ||
| fetch.mockResolvedValue(textResponse('<html>502 Bad Gateway</html>')) | ||
|
|
||
| await expect(fetchIssues('AOSSIE-Org', 'Proxied', 'pat')).resolves.toEqual([]) | ||
| }) | ||
|
|
||
| it('keeps the pages collected before an unparseable page appears', async () => { | ||
| // Same guarantee as the malformed-object case, but for a body that cannot | ||
| // be parsed at all rather than one that parses to a non-array. | ||
| const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
|
|
||
| fetch | ||
| .mockResolvedValueOnce(jsonResponse(fullPage)) | ||
| .mockResolvedValueOnce(textResponse('{ truncated')) | ||
|
|
||
| await expect(fetchContributors('AOSSIE-Org', 'Cut', 'pat')).resolves.toHaveLength(100) | ||
| // Without this the test would also pass if pagination stopped after page 1 | ||
| // and the unparseable page was never requested at all. | ||
| expect(fetch).toHaveBeenCalledTimes(2) | ||
| }) | ||
|
|
||
| it('keeps the pages collected before a malformed page appears', async () => { | ||
| // A full first page must still count even if page 2 comes back malformed. | ||
| const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
|
|
||
| fetch | ||
| .mockResolvedValueOnce(jsonResponse(fullPage)) | ||
| .mockResolvedValueOnce(jsonResponse({ message: 'Server Error' })) | ||
|
|
||
| await expect(fetchContributors('AOSSIE-Org', 'Big', 'pat')).resolves.toHaveLength(100) | ||
| expect(fetch).toHaveBeenCalledTimes(2) | ||
| }) | ||
|
|
||
| it('stops paginating as soon as a malformed page is returned', async () => { | ||
| fetch.mockResolvedValue(jsonResponse({ message: 'Server Error' })) | ||
|
|
||
| await fetchIssues('AOSSIE-Org', 'Broken', 'pat') | ||
|
|
||
| // maxPages is 10 for PAT users; without the break it would burn all ten. | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
| }) | ||
|
|
||
| describe('paginated fetchers: happy path', () => { | ||
| it('stops at the first partial page', async () => { | ||
| fetch.mockResolvedValueOnce(jsonResponse([{ id: 1 }, { id: 2 }])) | ||
|
|
||
| await expect(fetchContributors('AOSSIE-Org', 'Small', 'pat')).resolves.toHaveLength(2) | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('stops at the ten page ceiling for a PAT request', async () => { | ||
| // Every page comes back full, so only maxPages can end the walk. This is | ||
| // the bound issue #103 asks for: without it the loop would follow GitHub's | ||
| // pagination indefinitely. | ||
| const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
| fetch.mockResolvedValue(jsonResponse(fullPage)) | ||
|
|
||
| await expect(fetchIssues('AOSSIE-Org', 'Huge', 'pat')).resolves.toHaveLength(1000) | ||
| expect(fetch).toHaveBeenCalledTimes(10) | ||
| }) | ||
|
|
||
| it('stops after a single page when no PAT is supplied', async () => { | ||
| // maxPages is `pat ? 10 : 1`, so an anonymous caller must not walk on. | ||
| const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
| fetch.mockResolvedValue(jsonResponse(fullPage)) | ||
|
|
||
| await expect(fetchIssues('AOSSIE-Org', 'Huge')).resolves.toHaveLength(100) | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('follows pagination while pages come back full', async () => { | ||
| const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) | ||
|
|
||
| fetch | ||
| .mockResolvedValueOnce(jsonResponse(fullPage)) | ||
| .mockResolvedValueOnce(jsonResponse([{ id: 100 }])) | ||
|
|
||
| await expect(fetchRepos('AOSSIE-Org', 150, 'pat')).resolves.toHaveLength(101) | ||
| expect(fetch).toHaveBeenCalledTimes(2) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.