From b7e9b868dba2ae09378eb952691fd4d7dae3d607 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 26 Aug 2026 13:36:58 +0200 Subject: [PATCH] fix(self-link): say what happened when the API reduces a page size ensureSelfLink() logs one wording for every self link that differs from the url it was requested with, and that wording ends with "This could mean there's an issue with the REST endpoint". For a page size the API reduced, that points at the wrong place: the API did exactly what its contract says, the caller asked for a bigger page than it will ever serve. Whoever reads the console goes looking at the backend for a frontend mistake. Give that case its own message: 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 It applies only when the two urls are otherwise identical, so an accepted reduction cannot mask a wrong page or sort. Anything else keeps the generic message, including a page that came back larger than requested. This is the remaining half of the fix on dtq-dev. #1415 landed the rest - embed params and percent encoding normalized on both sides, MAX_PAGE_SIZE next to FindListOptions, and every oversized call site brought within it - so a reduced page size now genuinely means a caller asked for something the API was never going to serve, and the message can say so. - src/app/core/data/dspace-rest-response-parsing.service.ts - replace isUnexpectedSelfLink() with selfLinkWarning(), which returns the message to log or undefined when there is nothing to report, and add the getPageSize()/withoutPageSize() helpers and the PAGE_SIZE_PARAM they match on. The self link is normalized either way, as before, so the url a response is cached under does not change - only whether and how we warn. - src/app/core/data/dspace-rest-response-parsing.service.spec.ts - 17 cases to 19. The reduced page size case now expects the specific wording; one new case pins that a reduced size is recognised alongside other params, another that a page differing as well falls back to the generic message. Adapted to 7.6 from DSpace/dspace-angular PR 6083, commit 887ddec4c1. The 9.x-only parts of that commit's file - the @dspace/* path aliases and the inject(APP_CONFIG) refactor - are deliberately left out, which is also why the spec keeps its plain constructor harness instead of upstream's TestBed. Same change as #1448, which went to customer/jcu only. Fixes dataquest-dev/dspace-customers#934 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 f85f068b9c8..4d5b8060e79 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 @@ -19,6 +19,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 => @@ -64,7 +65,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)); @@ -97,19 +97,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 12715b0ff71..7f4125a4a6c 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -43,6 +43,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 * @@ -63,8 +68,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) => { @@ -77,20 +82,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' }) @@ -199,9 +223,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: {