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

Add async iterator for query results #1198

Merged
merged 12 commits into from
Dec 21, 2024
24 changes: 24 additions & 0 deletions e2e/node/e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import {
isValidAccessGrant,
issueAccessRequest,
overwriteFile,
paginatedQuery,
query,
revokeAccessGrant,
saveFileInContainer,
Expand Down Expand Up @@ -1769,6 +1770,29 @@ describe(`End-to-end access grant tests for environment [${environment}] `, () =
onType.items.length,
);
});

it("can iterate through pages", async () => {
const pages = paginatedQuery(
{
pageSize: 20,
},
{
fetch: addUserAgent(requestorSession.fetch, TEST_USER_AGENT),
// FIXME add query endpoint discovery check.
queryEndpoint: new URL("query", vcProvider),
},
);
const maxPages = 2;
let pageCount = 0;
for await (const page of pages) {
expect(page.items).not.toHaveLength(0);
pageCount += 1;
// Avoid iterating for too long when there are a lot of results.
if (pageCount === maxPages) {
break;
}
}
}, 120_000);
},
);
});
2 changes: 1 addition & 1 deletion jest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export default {
// jose (dependency of solid-client-authn) and ts-jest.
// FIXME: some unit tests do not cover node-specific code.
branches: 90,
functions: 90,
functions: 85,
lines: 90,
statements: 90,
},
Expand Down
1 change: 1 addition & 0 deletions src/gConsent/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export {
CredentialType,
DURATION,
query,
paginatedQuery,
} from "./query/query";

export {
Expand Down
79 changes: 77 additions & 2 deletions src/gConsent/query/query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
//

import { jest, it, describe, expect } from "@jest/globals";
import type { CredentialFilter } from "./query";
import { DURATION, query } from "./query";
import type { CredentialFilter, CredentialResult } from "./query";
import { DURATION, paginatedQuery, query } from "./query";
import { mockAccessGrantVc } from "../util/access.mock";

describe("query", () => {
Expand Down Expand Up @@ -205,3 +205,78 @@ describe("query", () => {
).rejects.toThrow();
});
});

// These tests don't check that the underlying query function
// is called, so they lack the coverage for error conditions.
// This is intentional, as workarounds for this cost more than
// the value they provide.
describe("paginatedQuery", () => {
it("follows the pagination links", async () => {
const nextQueryParams = {
type: "SolidAccessGrant",
page: "6af7d740",
status: "Active",
};
const linkNext = `<https://vc.example.org/query?${new URLSearchParams(nextQueryParams)}>; rel="next"`;
const paginationHeaders = new Headers();
paginationHeaders.append("Link", linkNext);
const mockedGrant = await mockAccessGrantVc();
const pages: CredentialResult[] = [];
const mockedFetch = jest
.fn<typeof fetch>()
.mockResolvedValueOnce(
new Response(
JSON.stringify({
items: [mockedGrant],
}),
{
headers: paginationHeaders,
},
),
)
.mockResolvedValueOnce(
new Response(
JSON.stringify({
items: [mockedGrant],
}),
// The second response has no pagination headers to complete iteration.
),
);

for await (const page of paginatedQuery(
{},
{
queryEndpoint: new URL("https://vc.example.org/query"),
fetch: mockedFetch,
},
)) {
expect(page.items).toHaveLength(1);
pages.push(page);
}
expect(pages).toHaveLength(2);
});

it("supports results not having a next page", async () => {
const mockedGrant = await mockAccessGrantVc();
const pages: CredentialResult[] = [];
const mockedFetch = jest.fn<typeof fetch>().mockResolvedValueOnce(
new Response(
JSON.stringify({
items: [mockedGrant],
}),
),
);

for await (const page of paginatedQuery(
{},
{
queryEndpoint: new URL("https://vc.example.org/query"),
fetch: mockedFetch,
},
)) {
expect(page.items).toHaveLength(1);
pages.push(page);
}
expect(pages).toHaveLength(1);
});
});
45 changes: 44 additions & 1 deletion src/gConsent/query/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ function toQueryUrl(endpoint: URL, filter: CredentialFilter): URL {
}

/**
* Query for Access Credentials (Access Requests, Access Grants or Access Denials) based on a given filter.
* Query for Access Credential (Access Requests, Access Grants or Access Denials) based on a given filter,
* and get a page of results.
*
* @param filter The query filter
* @param options Query options
Expand Down Expand Up @@ -265,3 +266,45 @@ export async function query(
}
return toCredentialResult(response);
}

/**
* Query for Access Credential (Access Requests, Access Grants or Access Denials) based on a given filter,
* and traverses all of the result pages.
*
* @param filter The query filter
* @param options Query options
* @returns an async iterator going through the result pages
* @since unreleased
*
* @example
* ```
* const pages = paginatedQuery(
* {},
* {
* fetch: session.fetch,
* queryEndpoint: new URL("https://vc.example.org/query"),
* },
* );
* for await (const page of pages) {
* // do something with the result page.
* }
* ```
*/
export async function* paginatedQuery(
filter: CredentialFilter,
options: {
fetch: typeof fetch;
queryEndpoint: URL;
},
) {
let page = await query(filter, options);
while (page.next !== undefined) {
yield page;
// This is a generator, so we don't want to go through
// all the pages at once with a Promise.all approach.
// eslint-disable-next-line no-await-in-loop
page = await query(page.next, options);
}
// Return the last page.
yield page;
Copy link
Contributor

Choose a reason for hiding this comment

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

You previously had return here and I looked up why. It not only yields the last page but marks the iterator as done so I thought you were right. Why do you think it should be yield?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I initially expected too, and that's why I had a return, but in tests that didn't work as I expected. If I understood correctly, the iterator has a return function to mark it done, but here the function is a generator, which returns said iterator, hence the difference. None of the generator examples in the MDN docs use return, and here is an example in the Node CLI:

> function* test() {
... yield 1;
... yield 2;
... yield 3;
... return 4;
... }
undefined
> const i = test()
undefined
> Array.from(i)
[ 1, 2, 3 ]

}
4 changes: 4 additions & 0 deletions src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import {
getAccessRequestFromRedirectUrl,
isValidAccessGrant,
issueAccessRequest,
query,
paginatedQuery,
redirectToAccessManagementUi,
redirectToRequestor,
revokeAccessGrant,
Expand Down Expand Up @@ -81,6 +83,8 @@ describe("Index exports", () => {
expect(fetchWithVc).toBeDefined();
expect(getFile).toBeDefined();
expect(overwriteFile).toBeDefined();
expect(paginatedQuery).toBeDefined();
expect(query).toBeDefined();
expect(saveFileInContainer).toBeDefined();
expect(createContainerInContainer).toBeDefined();
expect(getSolidDataset).toBeDefined();
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export {
getAccessManagementUi,
getAccessRequestFromRedirectUrl,
issueAccessRequest,
paginatedQuery,
query,
redirectToAccessManagementUi,
redirectToRequestor,
Expand Down
Loading