From bdd533a36f5d1bb64f95033d90abfcc1305a93b9 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Tue, 11 Aug 2026 08:47:59 +0200 Subject: [PATCH] Say what actually happened when the REST API reduces a page size The generic wording ends with "This could mean there's an issue with the REST endpoint", which points at the backend. For a reduced page size that is the wrong place to look: the API did nothing wrong, the caller asked for a bigger page than it will serve. The request for '.../bundles?size=9999' asked for a page of 9999 elements, but the REST API served 1000. Ask for at most MAX_PAGE_SIZE elements Anything else keeps the generic message, including a page that came back larger than requested. Also drops a stray note from the spec that was left in by #1440. Co-Authored-By: Claude Opus 5 (1M context) --- ...pace-rest-response-parsing.service.spec.ts | 26 ++++++-- .../dspace-rest-response-parsing.service.ts | 61 +++++++++++++------ 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/src/app/core/data/dspace-rest-response-parsing.service.spec.ts b/src/app/core/data/dspace-rest-response-parsing.service.spec.ts index 726501a4383..13cadf7b132 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.spec.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.spec.ts @@ -22,6 +22,7 @@ describe('DspaceRestResponseParsingService', () => { let objectCache: ObjectCacheService; const MISMATCH = jasmine.stringMatching(/These don't match/); + const REDUCED_PAGE = jasmine.stringMatching(/asked for a page of 9999 elements, but the REST API served 1000/); const NO_SELF_LINK = jasmine.stringMatching(/doesn't have a self link/); const requestFor = (href: string): RestRequest => @@ -67,7 +68,6 @@ describe('DspaceRestResponseParsingService', () => { }); it('should not warn when the self link echoes embed params and the request has no other params', () => { - // observed in the browser against a DSpace 9.1 backend const href = 'https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&embed=bitstreams/format&embed.size=bitstreams=5'; const response = service.callEnsureSelfLink(requestFor(href), responseWithSelfLink(href)); @@ -100,19 +100,35 @@ describe('DspaceRestResponseParsingService', () => { describe('differences that point at a problem with the endpoint', () => { - it('should warn when the REST API reduced the requested page size', () => { - // callers are expected to stay within MAX_PAGE_SIZE, so a reduced size means a caller asked - // for a page the API was never going to serve + it('should say so when the REST API reduced the requested page size', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=9999'); service.callEnsureSelfLink(request, responseWithSelfLink( 'https://rest.api/core/items/eba1c085/bundles?size=1000', { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(REDUCED_PAGE); + }); + + it('should report a reduced page size alongside other params without confusing the two', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=9999&sort=name,ASC'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?page=0&size=1000&sort=name,ASC')); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(REDUCED_PAGE); + }); + + it('should fall back to the generic warning when more than the page size differs', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=9999'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?page=3&size=1000')); + expect(console.warn).toHaveBeenCalledTimes(1); expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); - it('should warn when the returned page size is larger than the requested one', () => { + it('should use the generic warning when the returned page size is larger than requested', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5'); service.callEnsureSelfLink(request, responseWithSelfLink( 'https://rest.api/core/items/eba1c085/bundles?size=50', diff --git a/src/app/core/data/dspace-rest-response-parsing.service.ts b/src/app/core/data/dspace-rest-response-parsing.service.ts index b6be712314d..49412f097c0 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -54,6 +54,11 @@ export function isRestPaginatedList(halObj: any): boolean { hasValue(halObj.page.number); } +/** + * The url param holding the page size + */ +const PAGE_SIZE_PARAM = 'size='; + /** * Split a url into parts * @@ -74,8 +79,8 @@ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => { }; /** - * Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Parts are - * decoded one by one, after the url was split, so a decoded `&` can't merge two params. + * Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Decoding + * after the split keeps a decoded `&` from merging two params. */ const decodeUrlParts = (parts: string[]): string[] => { return parts.map((part: string) => { @@ -88,20 +93,39 @@ const decodeUrlParts = (parts: string[]): string[] => { }; /** - * Return true if the self link differs from the requested url in a way that isn't just a different - * way of writing the same request. Takes the requested url already split, since the caller has it. - * - * Both sides are brought to the same form first: `embed`/`embed.size` params are stripped, because - * the frontend treats them as not part of a resource's identity and indexes without them, and both - * are percent decoded. Anything still differing is a real difference between what was asked for and - * what came back, including a page size the API reduced — callers are expected to stay within - * `MAX_PAGE_SIZE` rather than have that reported difference filtered out here. + * The page size a url asks for, or undefined when it doesn't ask for a usable one + */ +const getPageSize = (parts: string[]): number | undefined => { + return parts.filter((part: string) => part.startsWith(PAGE_SIZE_PARAM)) + .map((part: string) => Number(part.substring(PAGE_SIZE_PARAM.length))) + .find((size: number) => Number.isInteger(size) && size > 0); +}; + +/** + * Return the parts without the one holding the page size */ -const isUnexpectedSelfLink = (requestedUrlParts: string[], selfLink: string): boolean => { - return urlPartsDiffer( - decodeUrlParts(requestedUrlParts), - decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))), - ); +const withoutPageSize = (parts: string[]): string[] => { + return parts.filter((part: string) => !part.startsWith(PAGE_SIZE_PARAM)); +}; + +/** + * Return the warning to log for a self link, or undefined when it describes the same request as the + * url it was requested with. A reduced page size gets its own message, since the generic one blames + * the endpoint for something the caller did. + */ +const selfLinkWarning = (requestedUrl: string, requestedUrlParts: string[], selfLink: string): string | undefined => { + const expected = decodeUrlParts(requestedUrlParts); + const actual = decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))); + if (!urlPartsDiffer(expected, actual)) { + return undefined; + } + const requestedSize = getPageSize(expected); + const servedSize = getPageSize(actual); + if (hasValue(requestedSize) && hasValue(servedSize) && servedSize < requestedSize + && !urlPartsDiffer(withoutPageSize(expected), withoutPageSize(actual))) { + return `The request for '${requestedUrl}' asked for a page of ${requestedSize} elements, but the REST API served ${servedSize}. Ask for at most MAX_PAGE_SIZE elements`; + } + return `The response for '${requestedUrl}' has the self link '${selfLink}'. These don't match. This could mean there's an issue with the REST endpoint`; }; @Injectable({ providedIn: 'root' }) @@ -210,9 +234,10 @@ export class DspaceRestResponseParsingService implements ResponseParsingService const expected = splitUrlInParts(urlWithoutEmbedParams); const actual = splitUrlInParts(selfLink); if (expected[0] === actual[0] && urlPartsDiffer(expected, actual)) { - // the self link is normalized either way, only the warning is filtered - if (isUnexpectedSelfLink(expected, selfLink)) { - console.warn(`The response for '${urlWithoutEmbedParams}' has the self link '${selfLink}'. These don't match. This could mean there's an issue with the REST endpoint`); + // the self link is normalized either way, only the warning is conditional + const warning = selfLinkWarning(urlWithoutEmbedParams, expected, selfLink); + if (hasValue(warning)) { + console.warn(warning); } response.payload._links = Object.assign({}, response.payload._links, { self: {