From c8fef09b1211c6bb7d44974e0e7f694c7b064758 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 13:34:02 +0200 Subject: [PATCH 01/11] Only warn about REST self links the API isn't expected to change ensureSelfLink() compares the requested url against the self link in the response, but strips embed params from the requested url only. The REST API echoes the request's embed params into the self link, so every request that embeds a subresource looked broken and flooded the console: The response for '.../bundles//bitstreams?page=0&size=5' has the self link '.../bitstreams?page=0&embed=accessStatus&size=5'. These don't match. The same warning also fired when the API clamped an oversized page size (?size=9999 comes back as ?size=1000), which the REST contract mandates: a size over the configured maximum is reset to the maximum, no error thrown. Narrow the warning so it only fires on differences the API isn't expected to introduce: strip embed params from both sides, and accept a page size that shrank when the response's own page.size confirms the smaller value. A self link that contradicts the payload it describes is still reported, as are differing page/sort params, extra params, a size larger than requested, and a missing self link. The self-link normalization itself is left untouched. The response is keyed in the object cache by that href, so which url it is normalized to stays exactly as it was; only whether we warn changes. Adds the first spec for this service - ensureSelfLink was untested. Closes dataquest-dev/dspace-customers#862 --- ...pace-rest-response-parsing.service.spec.ts | 201 ++++++++++++++++++ .../dspace-rest-response-parsing.service.ts | 94 +++++++- 2 files changed, 292 insertions(+), 3 deletions(-) create mode 100644 src/app/core/data/dspace-rest-response-parsing.service.spec.ts 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 new file mode 100644 index 00000000000..223f91c06d4 --- /dev/null +++ b/src/app/core/data/dspace-rest-response-parsing.service.spec.ts @@ -0,0 +1,201 @@ +import { DspaceRestResponseParsingService } from './dspace-rest-response-parsing.service'; +import { RestRequest } from './rest-request.model'; +import { GetRequest, PostRequest } from './request.models'; +import { RawRestResponse } from '../dspace-rest/raw-rest-response.model'; +import { ObjectCacheService } from '../cache/object-cache.service'; + +/** + * Exposes the protected {@link DspaceRestResponseParsingService#ensureSelfLink} so it can be + * tested in isolation. + */ +class TestParsingService extends DspaceRestResponseParsingService { + public callEnsureSelfLink(request: RestRequest, response: RawRestResponse): RawRestResponse { + return this.ensureSelfLink(request, response); + } +} + +describe('DspaceRestResponseParsingService', () => { + let service: TestParsingService; + let objectCache: ObjectCacheService; + + const MISMATCH = jasmine.stringMatching(/These don't match/); + const NO_SELF_LINK = jasmine.stringMatching(/doesn't have a self link/); + + const requestFor = (href: string): RestRequest => + new GetRequest('c4f0b1b7-3ffa-4b1a-9f5f-8bd6b1c4de71', href); + + const responseWithSelfLink = (href: string, page?: any): RawRestResponse => ({ + payload: { + _links: { + self: { href }, + }, + ...(page ? { page } : {}), + }, + statusCode: 200, + statusText: 'OK', + }); + + beforeEach(() => { + objectCache = jasmine.createSpyObj('objectCache', ['add', 'remove']); + service = new TestParsingService(objectCache); + spyOn(console, 'warn'); + }); + + describe('ensureSelfLink', () => { + + describe('differences the REST API is expected to introduce', () => { + + it('should not warn when the self link matches the requested url', () => { + const href = 'https://rest.api/core/bundles/9d18168a/bitstreams?page=0&size=5'; + const response = service.callEnsureSelfLink(requestFor(href), responseWithSelfLink(href)); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe(href); + }); + + it('should not warn when the self link only echoes the embed params of the request', () => { + // https://github.com/dataquest-dev/dspace-customers/issues/862 + const href = 'https://rest.api/core/bundles/9d18168a/bitstreams?page=0&embed=accessStatus&size=5'; + const response = service.callEnsureSelfLink(requestFor(href), responseWithSelfLink(href)); + + expect(console.warn).not.toHaveBeenCalled(); + // the self link is still normalized, because that's the url the response is cached under + expect(response.payload._links.self.href).toBe('https://rest.api/core/bundles/9d18168a/bitstreams?page=0&size=5'); + }); + + 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)); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles'); + }); + + it('should not warn when the REST API reduced the requested page size', () => { + // https://github.com/dataquest-dev/dspace-customers/issues/862 + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&size=9999'); + const response = service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&size=1000', + { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); + }); + + it('should not warn when params are only in a different order', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=5&page=0')); + + expect(console.warn).not.toHaveBeenCalled(); + }); + + }); + + describe('differences that point at a problem with the endpoint', () => { + + it('should warn when the page size shrank but the page block contradicts the self link', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=100'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?size=20', + { number: 0, size: 100, totalPages: 1, totalElements: 2 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn when the page size shrank but the response has no page block to confirm it', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=100'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=20')); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn when the returned page size is larger than the requested one', () => { + 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', + { number: 0, size: 50, totalPages: 1, totalElements: 2 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn when the url is ambiguous about the page size', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5&size=10'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?size=3', + { number: 0, size: 3, totalPages: 1, totalElements: 2 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn when a non-embed param differs', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&embed=primaryBitstream&size=5'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?page=3&embed=primaryBitstream&size=5')); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn when the self link has a param the request did not have', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=5&sort=name,ASC')); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should warn and fill in the requested url when the response has no self link', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&size=5'); + const response = service.callEnsureSelfLink(request, { + payload: { _links: {} }, + statusCode: 200, + statusText: 'OK', + }); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(NO_SELF_LINK); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=5'); + }); + + }); + + describe('normalization of the self link', () => { + + it('should normalize the self link when it differs, so it matches the cache key', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&embed=primaryBitstream&size=5'); + const response = service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?page=3&embed=primaryBitstream&size=5')); + + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); + }); + + it('should not touch a self link that points at a different path', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5'); + const response = service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085?size=5')); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085?size=5'); + }); + + it('should leave non-GET requests alone', () => { + const request = new PostRequest('c4f0b1b7-3ffa-4b1a-9f5f-8bd6b1c4de71', 'https://rest.api/core/items/eba1c085/bundles?size=5'); + const response = service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=1000')); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=1000'); + }); + + }); + + }); +}); 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 c0e1c70cae9..764e31df7a0 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,89 @@ const splitUrlInParts = (url: string): string[] => { .reduce((combined, current) => [...combined, ...current]); }; +/** + * Return true if two lists of url parts don't hold the same set of parts. The comparison is order + * insensitive, and — like {@link splitUrlInParts} — treats the part in front of the query string as + * just another part. + * + * @param expected the parts of the url we expected + * @param actual the parts of the url we got + */ +const urlPartsDiffer = (expected: string[], actual: string[]): boolean => { + return expected.some((part: string) => !actual.includes(part)) + || actual.some((part: string) => !expected.includes(part)); +}; + +/** + * Matches a page size query param, e.g. `size=100` + */ +const PAGE_SIZE_PARAM = /^size=(\d+)$/; + +/** + * Return the page sizes among the given url parts. More than one means the url is ambiguous about + * the page size, and no conclusion can be drawn from it. + * + * @param parts the url parts, as returned by {@link splitUrlInParts} + */ +const getPageSizes = (parts: string[]): number[] => { + return parts + .map((part: string) => part.match(PAGE_SIZE_PARAM)) + .filter((matches) => hasValue(matches)) + .map((matches) => Number(matches[1])); +}; + +/** + * Return true if the requested url and the self link disagree about the page size only because the + * REST API reduced a size it wasn't willing to serve. + * + * The REST contract says a `size` over the configured maximum is "automatically reset to the + * maximum allowed value, no error is thrown", and it is that effective size which ends up in the + * self link. The frontend can't know the server's maximum, so it can't tell such a clamp apart from + * any other reduction — but it can require the response to be consistent with itself: the `page` + * block has to confirm the smaller size the self link advertises. A self link that contradicts the + * payload it describes is still reported. + * + * @param expected the requested url parts, without embed params + * @param actual the self link parts, without embed params + * @param payload the response body the self link belongs to + */ +const isReducedPageSize = (expected: string[], actual: string[], payload: any): boolean => { + const requestedSizes = getPageSizes(expected); + const effectiveSizes = getPageSizes(actual); + return requestedSizes.length === 1 && effectiveSizes.length === 1 + && effectiveSizes[0] < requestedSizes[0] + && hasValue(payload) && hasValue(payload.page) && payload.page.size === effectiveSizes[0]; +}; + +/** + * Determine whether the difference between the url that was requested and the self link the REST + * API returned for it points at an actual problem with the endpoint. + * + * Two kinds of difference are correct REST behaviour and are not reported: + * + * - The REST API echoes the request's `embed`/`embed.size` params in the self link, while the url + * we compare against has had them stripped by {@link getUrlWithoutEmbedParams}. Comparing the two + * as-is makes every request that embeds a subresource look broken, e.g. + * `…/bitstreams?page=0&size=5` vs `…/bitstreams?page=0&embed=accessStatus&size=5`. Stripping both + * sides also means a self link echoing embeds we never asked for goes unreported. + * - The REST API reduced the requested page size, see {@link isReducedPageSize}. + * + * @param requestedUrl the url that was requested, without embed params + * @param selfLink the self link as returned by the REST API + * @param payload the response body the self link belongs to + */ +const isUnexpectedSelfLink = (requestedUrl: string, selfLink: string, payload: any): boolean => { + const expected = splitUrlInParts(requestedUrl); + const actual = splitUrlInParts(getUrlWithoutEmbedParams(selfLink)); + + if (isReducedPageSize(expected, actual, payload)) { + const withoutPageSize = (parts: string[]): string[] => + parts.filter((part: string) => !PAGE_SIZE_PARAM.test(part)); + return urlPartsDiffer(withoutPageSize(expected), withoutPageSize(actual)); + } + return urlPartsDiffer(expected, actual); +}; + @Injectable({ providedIn: 'root' }) export class DspaceRestResponseParsingService implements ResponseParsingService { protected serializerConstructor: GenericConstructor> = DSpaceSerializer; @@ -156,10 +239,15 @@ export class DspaceRestResponseParsingService implements ResponseParsingService }); } else { + const selfLink = response.payload._links.self.href; const expected = splitUrlInParts(urlWithoutEmbedParams); - const actual = splitUrlInParts(response.payload._links.self.href); - if (expected[0] === actual[0] && (expected.some((e) => !actual.includes(e)) || actual.some((e) => !expected.includes(e)))) { - console.warn(`The response for '${urlWithoutEmbedParams}' has the self link '${response.payload._links.self.href}'. These don't match. This could mean there's an issue with the REST endpoint`); + const actual = splitUrlInParts(selfLink); + if (expected[0] === actual[0] && urlPartsDiffer(expected, actual)) { + // Report only the differences the REST API isn't expected to introduce by itself. Which + // url the self link is normalized to is deliberately left unchanged by this check. + if (isUnexpectedSelfLink(urlWithoutEmbedParams, selfLink, response.payload)) { + 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`); + } response.payload._links = Object.assign({}, response.payload._links, { self: { href: urlWithoutEmbedParams From 62b0e950e68e283eab3f99ab21ce574094473925 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 13:47:51 +0200 Subject: [PATCH 02/11] Don't accept an empty page as a clamped page size, and pin the rest Mutation testing of the new spec turned up one behaviour bug and a set of assertions that were never actually pinned down. isReducedPageSize() accepted any smaller page size the response's page block confirmed, including zero: asking for size=10 and getting a self link claiming size=0 with "page": {"size": 0} was silently swallowed. A configured maximum is never zero, so an empty page is not a clamp - require the effective size to be greater than zero and let that case be reported again. Also drop a redundant hasValue(payload) - the caller already guarantees it - and note in the doc comment that running the self link through getUrlWithoutEmbedParams() drops a fragment and a trailing slash too, so differences limited to those stop being reported as well. Tests added for the gaps mutation testing exposed, each verified to fail against the corresponding mutant: - the page size rule tested on its own, without embed params also in play - the exact text of the warning, not just a substring - sibling _links surviving the self link being normalized - `size` matched as a whole param, so `pagesize` isn't mistaken for it - a page block confirming the size only as a string is not confirmation - an empty page is reported - a cross-origin self link is passed through untouched - reordered params are neither reported nor rewritten --- ...pace-rest-response-parsing.service.spec.ts | 83 ++++++++++++++++++- .../dspace-rest-response-parsing.service.ts | 9 +- 2 files changed, 88 insertions(+), 4 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 223f91c06d4..09e2a097947 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 @@ -83,12 +83,24 @@ describe('DspaceRestResponseParsingService', () => { expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); }); - it('should not warn when params are only in a different order', () => { + it('should not warn when the REST API reduced the page size and no embeds are involved', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=9999'); + const response = service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?size=1000', + { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); + }); + + it('should not warn or normalize when params are only in a different order', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); - service.callEnsureSelfLink(request, + const response = service.callEnsureSelfLink(request, responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=5&page=0')); expect(console.warn).not.toHaveBeenCalled(); + // the urls hold the same params, so nothing is rewritten here + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=5&page=0'); }); }); @@ -134,6 +146,47 @@ describe('DspaceRestResponseParsingService', () => { expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); + it('should warn when the self link claims an empty page', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=10'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?size=0', + { number: 0, size: 0, totalPages: 0, totalElements: 0 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should not treat a param that merely ends in `size` as the page size', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?pagesize=9999'); + service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/core/items/eba1c085/bundles?pagesize=1000', + { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + + it('should not accept a page block that confirms the reduced size only as a string', () => { + 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(MISMATCH); + }); + + it('should report the normalized request url and the raw self link in the warning', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&embed=primaryBitstream&size=5'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?page=3&embed=primaryBitstream&size=5')); + + expect(console.warn).toHaveBeenCalledWith( + 'The response for \'https://rest.api/core/items/eba1c085/bundles?page=0&size=5\' has the self link ' + + '\'https://rest.api/core/items/eba1c085/bundles?page=3&embed=primaryBitstream&size=5\'. ' + + 'These don\'t match. This could mean there\'s an issue with the REST endpoint'); + }); + it('should warn when a non-embed param differs', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&embed=primaryBitstream&size=5'); service.callEnsureSelfLink(request, @@ -177,6 +230,32 @@ describe('DspaceRestResponseParsingService', () => { expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); }); + it('should keep the other links when it normalizes the self link', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); + const response = service.callEnsureSelfLink(request, { + payload: { + _links: { + self: { href: 'https://rest.api/core/items/eba1c085/bundles?page=3&size=5' }, + primaryBitstream: { href: 'https://rest.api/core/bitstreams/6a5f' }, + }, + }, + statusCode: 200, + statusText: 'OK', + }); + + expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); + expect(response.payload._links.primaryBitstream.href).toBe('https://rest.api/core/bitstreams/6a5f'); + }); + + it('should not touch a self link on a different host', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5'); + const response = service.callEnsureSelfLink(request, + responseWithSelfLink('https://other.api/core/items/eba1c085/bundles?size=5')); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href).toBe('https://other.api/core/items/eba1c085/bundles?size=5'); + }); + it('should not touch a self link that points at a different path', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5'); const response = service.callEnsureSelfLink(request, 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 764e31df7a0..0adcfbbe507 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -104,8 +104,9 @@ const isReducedPageSize = (expected: string[], actual: string[], payload: any): const requestedSizes = getPageSizes(expected); const effectiveSizes = getPageSizes(actual); return requestedSizes.length === 1 && effectiveSizes.length === 1 - && effectiveSizes[0] < requestedSizes[0] - && hasValue(payload) && hasValue(payload.page) && payload.page.size === effectiveSizes[0]; + // a maximum page size is never zero, so an empty page isn't a clamp and stays reportable + && effectiveSizes[0] > 0 && effectiveSizes[0] < requestedSizes[0] + && hasValue(payload.page) && payload.page.size === effectiveSizes[0]; }; /** @@ -121,6 +122,10 @@ const isReducedPageSize = (expected: string[], actual: string[], payload: any): * sides also means a self link echoing embeds we never asked for goes unreported. * - The REST API reduced the requested page size, see {@link isReducedPageSize}. * + * Note that {@link getUrlWithoutEmbedParams} rebuilds the url it is given, so running the self link + * through it also drops a fragment and a trailing slash on the path. Differences limited to those + * stop being reported too. + * * @param requestedUrl the url that was requested, without embed params * @param selfLink the self link as returned by the REST API * @param payload the response body the self link belongs to From 2ca7fc02f09a77d3168a9fb0ed2fae71ee1a0904 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 14:23:13 +0200 Subject: [PATCH 03/11] Cover the case where an accepted clamp hides another differing param When isReducedPageSize() accepts a reduced page size, isUnexpectedSelfLink() re-compares the remaining parts rather than returning early, so a legitimate clamp can't mask a genuinely wrong page/sort. Nothing defended that: dropping the second urlPartsDiffer() call left all 22 tests green while silently swallowing the defect. --- .../data/dspace-rest-response-parsing.service.spec.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 09e2a097947..5c7672f1a50 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 @@ -146,6 +146,16 @@ describe('DspaceRestResponseParsingService', () => { expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); + it('should warn when a reduced page size hides another param that 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', + { number: 3, size: 1000, totalPages: 4, totalElements: 3200 })); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + it('should warn when the self link claims an empty page', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=10'); service.callEnsureSelfLink(request, responseWithSelfLink( From ea5f577bc2fe31b340c5f6f56fb4d6c5977c4fe7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 14:45:23 +0200 Subject: [PATCH 04/11] Don't report a self link that only percent decoded a param value The remaining self link warning on the home page came from the usage statistics request: the frontend sends uri=http%3A%2F%2F... and the REST API echoes it back decoded as uri=http://... Same value, different representation - comparing the raw strings compares encodings, not values, which is the same kind of false alarm as the echoed embed params. Percent decode both sides before comparing. Decoding happens per part, after the url was split, so a decoded & can't merge two params, and a malformed sequence falls back to the raw part. A value that differs beyond its encoding still warns. With this, an item page, the home page and a search page all load with zero self link warnings against a DSpace 9.1 backend. Also trims the doc comments on these helpers down to what isn't already obvious from the code. --- ...pace-rest-response-parsing.service.spec.ts | 20 +++++ .../dspace-rest-response-parsing.service.ts | 73 +++++++------------ 2 files changed, 47 insertions(+), 46 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 5c7672f1a50..eddf481414d 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 @@ -93,6 +93,17 @@ describe('DspaceRestResponseParsingService', () => { expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); }); + it('should not warn when the self link only percent decoded a param value', () => { + // https://github.com/dataquest-dev/dspace-customers/issues/862 + const request = requestFor('https://rest.api/statistics/usagereports/search/object?page=-1&size=10&uri=https%3A%2F%2Frest.api%2Fcore%2Fsites%2F8f842a80'); + const response = service.callEnsureSelfLink(request, responseWithSelfLink( + 'https://rest.api/statistics/usagereports/search/object?page=-1&size=10&uri=https://rest.api/core/sites/8f842a80')); + + expect(console.warn).not.toHaveBeenCalled(); + expect(response.payload._links.self.href) + .toBe('https://rest.api/statistics/usagereports/search/object?page=-1&size=10&uri=https%3A%2F%2Frest.api%2Fcore%2Fsites%2F8f842a80'); + }); + it('should not warn or normalize when params are only in a different order', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=5'); const response = service.callEnsureSelfLink(request, @@ -146,6 +157,15 @@ describe('DspaceRestResponseParsingService', () => { expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); + it('should still warn when a param value differs beyond its encoding', () => { + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?uri=https%3A%2F%2Frest.api%2Fcore%2Fsites%2Faaa'); + service.callEnsureSelfLink(request, + responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?uri=https://rest.api/core/sites/bbb')); + + expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledWith(MISMATCH); + }); + it('should warn when a reduced page size hides another param that differs', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=9999'); service.callEnsureSelfLink(request, responseWithSelfLink( 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 0adcfbbe507..74641be2837 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -55,12 +55,7 @@ const splitUrlInParts = (url: string): string[] => { }; /** - * Return true if two lists of url parts don't hold the same set of parts. The comparison is order - * insensitive, and — like {@link splitUrlInParts} — treats the part in front of the query string as - * just another part. - * - * @param expected the parts of the url we expected - * @param actual the parts of the url we got + * Return true if two lists of url parts don't hold the same parts, ignoring their order */ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => { return expected.some((part: string) => !actual.includes(part)) @@ -73,10 +68,21 @@ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => { const PAGE_SIZE_PARAM = /^size=(\d+)$/; /** - * Return the page sizes among the given url parts. More than one means the url is ambiguous about - * the page size, and no conclusion can be drawn from it. - * - * @param parts the url parts, as returned by {@link splitUrlInParts} + * 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. + */ +const decodeUrlParts = (parts: string[]): string[] => { + return parts.map((part: string) => { + try { + return decodeURIComponent(part); + } catch (e) { + return part; + } + }); +}; + +/** + * Return the page sizes among the given url parts. More than one means the url is ambiguous */ const getPageSizes = (parts: string[]): number[] => { return parts @@ -86,53 +92,29 @@ const getPageSizes = (parts: string[]): number[] => { }; /** - * Return true if the requested url and the self link disagree about the page size only because the - * REST API reduced a size it wasn't willing to serve. - * - * The REST contract says a `size` over the configured maximum is "automatically reset to the - * maximum allowed value, no error is thrown", and it is that effective size which ends up in the - * self link. The frontend can't know the server's maximum, so it can't tell such a clamp apart from - * any other reduction — but it can require the response to be consistent with itself: the `page` - * block has to confirm the smaller size the self link advertises. A self link that contradicts the - * payload it describes is still reported. + * Return true if the self link only shrank the page size to a value the response confirms itself. * - * @param expected the requested url parts, without embed params - * @param actual the self link parts, without embed params - * @param payload the response body the self link belongs to + * The REST API silently resets a `size` over its configured maximum. The frontend can't know that + * maximum, so it requires the `page` block to corroborate the smaller size, and reports a self link + * that contradicts the payload it describes. Zero is never a maximum, so an empty page isn't a + * clamp either. */ const isReducedPageSize = (expected: string[], actual: string[], payload: any): boolean => { const requestedSizes = getPageSizes(expected); const effectiveSizes = getPageSizes(actual); return requestedSizes.length === 1 && effectiveSizes.length === 1 - // a maximum page size is never zero, so an empty page isn't a clamp and stays reportable && effectiveSizes[0] > 0 && effectiveSizes[0] < requestedSizes[0] && hasValue(payload.page) && payload.page.size === effectiveSizes[0]; }; /** - * Determine whether the difference between the url that was requested and the self link the REST - * API returned for it points at an actual problem with the endpoint. - * - * Two kinds of difference are correct REST behaviour and are not reported: - * - * - The REST API echoes the request's `embed`/`embed.size` params in the self link, while the url - * we compare against has had them stripped by {@link getUrlWithoutEmbedParams}. Comparing the two - * as-is makes every request that embeds a subresource look broken, e.g. - * `…/bitstreams?page=0&size=5` vs `…/bitstreams?page=0&embed=accessStatus&size=5`. Stripping both - * sides also means a self link echoing embeds we never asked for goes unreported. - * - The REST API reduced the requested page size, see {@link isReducedPageSize}. - * - * Note that {@link getUrlWithoutEmbedParams} rebuilds the url it is given, so running the self link - * through it also drops a fragment and a trailing slash on the path. Differences limited to those - * stop being reported too. - * - * @param requestedUrl the url that was requested, without embed params - * @param selfLink the self link as returned by the REST API - * @param payload the response body the self link belongs to + * Return true if the self link differs from the requested url in a way the REST API isn't expected + * to introduce by itself. Not reported: `embed` params the API echoes back (they are stripped from + * the requested url but not from the self link), percent encoding, and a confirmed page size clamp. */ const isUnexpectedSelfLink = (requestedUrl: string, selfLink: string, payload: any): boolean => { - const expected = splitUrlInParts(requestedUrl); - const actual = splitUrlInParts(getUrlWithoutEmbedParams(selfLink)); + const expected = decodeUrlParts(splitUrlInParts(requestedUrl)); + const actual = decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))); if (isReducedPageSize(expected, actual, payload)) { const withoutPageSize = (parts: string[]): string[] => @@ -248,8 +230,7 @@ export class DspaceRestResponseParsingService implements ResponseParsingService const expected = splitUrlInParts(urlWithoutEmbedParams); const actual = splitUrlInParts(selfLink); if (expected[0] === actual[0] && urlPartsDiffer(expected, actual)) { - // Report only the differences the REST API isn't expected to introduce by itself. Which - // url the self link is normalized to is deliberately left unchanged by this check. + // the self link is normalized either way, only the warning is filtered if (isUnexpectedSelfLink(urlWithoutEmbedParams, selfLink, response.payload)) { 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`); } From cf12d7b7ee6434f2628cfb289ce7532f5f7c6edf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:46:02 +0200 Subject: [PATCH 05/11] Stop asking the REST API for pages it will never serve Seven call sites asked for 9999 or 10000 elements to mean "give me everything". The REST API caps a page at 1000 (Spring Data REST's spring.data.rest.max-page-size, which DSpace leaves at its default) and silently reduces anything larger, so those requests never returned more than 1000 anyway - they just claimed something the API does not honour, and the difference between the requested and the effective size is what showed up in the self link and produced the console warnings of #862: GET /core/items//bundles?embed=primaryBitstream&size=9999 self ...?embed=primaryBitstream&size=1000 Introduce MAX_PAGE_SIZE next to FindListOptions and use it instead. Verified against a DSpace 9.1 backend that this returns the identical page - same totalElements, same contents - while the self link now matches the request exactly: ?size=9999 -> self ?size=1000 page {size: 1000, totalElements: 1} ?size=1000 -> self ?size=1000 page {size: 1000, totalElements: 1} This is upstream issue #2513, whose fix (#3694) removed 9999 from seven components but missed BundleDataService.findByItemAndName - still on main, dspace-9_x, dtq-dev and dtq-dev-9-base. The three clarin-* call sites are ours. --- .../clarin-zip-download-page.component.ts | 3 ++- src/app/core/browse/browse.service.ts | 3 ++- src/app/core/data/bundle-data.service.ts | 4 ++-- src/app/core/data/find-list-options.model.ts | 10 ++++++++++ src/app/core/registry/registry.service.ts | 4 ++-- .../item-bitstreams/item-bitstreams.service.ts | 3 ++- .../clarin-item-versions-field.component.ts | 3 ++- .../clarin-item-box-view.component.ts | 4 ++-- 8 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts b/src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts index e0f13d16150..51b66672497 100644 --- a/src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts +++ b/src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts @@ -20,6 +20,7 @@ import { BitstreamDataService } from '../../core/data/bitstream-data.service'; import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils'; import { NotificationsService } from '../../shared/notifications/notifications.service'; import { TranslateService } from '@ngx-translate/core'; +import { MAX_PAGE_SIZE } from '../../core/data/find-list-options.model'; /** * Fetch ZIP file from the server as a single file into `bitstreamRD$` property which is extended and then call @@ -59,7 +60,7 @@ export class ClarinZipDownloadPageComponent extends ClarinBitstreamDownloadPageC this.itemRD$.subscribe((itemRD: RemoteData) => { this.bitstreamDataService.findAllByItemAndBundleName(itemRD?.payload, 'ORIGINAL', { currentPage: 1, - elementsPerPage: 9999 + elementsPerPage: MAX_PAGE_SIZE }).pipe( getFirstCompletedRemoteData(), ).subscribe((bitstreamsRD: RemoteData>) => { diff --git a/src/app/core/browse/browse.service.ts b/src/app/core/browse/browse.service.ts index db1edf8b113..4301858f49e 100644 --- a/src/app/core/browse/browse.service.ts +++ b/src/app/core/browse/browse.service.ts @@ -24,6 +24,7 @@ import { followLink, FollowLinkConfig } from '../../shared/utils/follow-link-con import { BrowseDefinitionDataService } from './browse-definition-data.service'; import { SortDirection } from '../cache/models/sort-options.model'; import { environment } from '../../../environments/environment'; +import { MAX_PAGE_SIZE } from '../data/find-list-options.model'; export function getBrowseLinksToFollow(): FollowLinkConfig[] { @@ -69,7 +70,7 @@ export class BrowseService { */ getBrowseDefinitions(): Observable>> { // TODO properly support pagination - return this.browseDefinitionDataService.findAll({ elementsPerPage: 9999 }).pipe( + return this.browseDefinitionDataService.findAll({ elementsPerPage: MAX_PAGE_SIZE }).pipe( getFirstSucceededRemoteData(), ); } diff --git a/src/app/core/data/bundle-data.service.ts b/src/app/core/data/bundle-data.service.ts index 78ae204fe71..e62c5d85a3b 100644 --- a/src/app/core/data/bundle-data.service.ts +++ b/src/app/core/data/bundle-data.service.ts @@ -16,7 +16,7 @@ import { RequestService } from './request.service'; import { PaginatedSearchOptions } from '../../shared/search/models/paginated-search-options.model'; import { Bitstream } from '../shared/bitstream.model'; import { RequestEntryState } from './request-entry-state.model'; -import { FindListOptions } from './find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from './find-list-options.model'; import { IdentifiableDataService } from './base/identifiable-data.service'; import { PatchData, PatchDataImpl } from './base/patch-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; @@ -81,7 +81,7 @@ export class BundleDataService extends IdentifiableDataService implement findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions, ...linksToFollow: FollowLinkConfig[]): Observable> { //Since we filter by bundleName where the pagination options are not indicated we need to load all the possible bundles. // This is a workaround, in substitution of the previously recursive call with expand - const paginationOptions = options ?? { elementsPerPage: 9999 }; + const paginationOptions = options ?? { elementsPerPage: MAX_PAGE_SIZE }; return this.findAllByItem(item, paginationOptions, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe( map((rd: RemoteData>) => { if (hasValue(rd.payload) && hasValue(rd.payload.page)) { diff --git a/src/app/core/data/find-list-options.model.ts b/src/app/core/data/find-list-options.model.ts index dc567d4b531..04a4c3c417d 100644 --- a/src/app/core/data/find-list-options.model.ts +++ b/src/app/core/data/find-list-options.model.ts @@ -1,6 +1,16 @@ import { SortOptions } from '../cache/models/sort-options.model'; import { RequestParam } from '../cache/models/request-param.model'; +/** + * The largest page the REST API will serve. Asking for more is not an error: the API silently + * reduces the size to this maximum, so a bigger number returns the exact same page while making the + * request claim something the API never honours. + * + * The limit is Spring Data REST's `spring.data.rest.max-page-size`, which DSpace leaves at its + * default. Use this instead of an arbitrary large number when a caller needs "everything". + */ +export const MAX_PAGE_SIZE = 1000; + /** * The options for a find list request */ diff --git a/src/app/core/registry/registry.service.ts b/src/app/core/registry/registry.service.ts index bbdf41d3ea8..79ce8cd8456 100644 --- a/src/app/core/registry/registry.service.ts +++ b/src/app/core/registry/registry.service.ts @@ -30,7 +30,7 @@ import { MetadataBitstreamDataService } from '../data/metadata-bitstream-data.se import { FollowLinkConfig } from '../../shared/utils/follow-link-config.model'; import { RequestParam } from '../cache/models/request-param.model'; import { NoContent } from '../shared/NoContent.model'; -import { FindListOptions } from '../data/find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from '../data/find-list-options.model'; import { MetadataBitstream } from '../metadata/metadata-bitstream.model'; const metadataRegistryStateSelector = (state: AppState) => state.metadataRegistry; @@ -81,7 +81,7 @@ export class RegistryService { public getMetadataSchemaByPrefix(prefix: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, ...linksToFollow: FollowLinkConfig[]): Observable> { // Temporary options to get ALL metadataschemas until there's a rest api endpoint for fetching a specific schema const options: FindListOptions = Object.assign(new FindListOptions(), { - elementsPerPage: 10000 + elementsPerPage: MAX_PAGE_SIZE }); return this.getMetadataSchemas(options).pipe( getFirstSucceededRemoteDataPayload(), diff --git a/src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.service.ts b/src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.service.ts index 8f63e9ab6e4..c222b0f21ac 100644 --- a/src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.service.ts +++ b/src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.service.ts @@ -24,6 +24,7 @@ import { MoveOperation } from 'fast-json-patch'; import { BundleDataService } from '../../../core/data/bundle-data.service'; import { RequestService } from '../../../core/data/request.service'; import { LiveRegionService } from '../../../shared/live-region/live-region.service'; +import { MAX_PAGE_SIZE } from '../../../core/data/find-list-options.model'; export const MOVE_KEY = 'item.edit.bitstreams.notifications.move'; @@ -344,7 +345,7 @@ export class ItemBitstreamsService { return Object.assign(new PaginationComponentOptions(), { id: 'bundles-pagination-options', currentPage: 1, - pageSize: 9999 + pageSize: MAX_PAGE_SIZE }); } diff --git a/src/app/item-page/simple/field-components/clarin-item-versions-field/clarin-item-versions-field.component.ts b/src/app/item-page/simple/field-components/clarin-item-versions-field/clarin-item-versions-field.component.ts index d0507103c12..d2b4216e5b6 100644 --- a/src/app/item-page/simple/field-components/clarin-item-versions-field/clarin-item-versions-field.component.ts +++ b/src/app/item-page/simple/field-components/clarin-item-versions-field/clarin-item-versions-field.component.ts @@ -5,6 +5,7 @@ import { ItemVersionsComponent } from '../../../versions/item-versions.component import { Item } from '../../../../core/shared/item.model'; import { Version } from '../../../../core/shared/version.model'; import { RemoteData } from '../../../../core/data/remote-data'; +import { MAX_PAGE_SIZE } from '../../../../core/data/find-list-options.model'; /** * Local type definition matching the parent component's VersionsDTO structure @@ -43,7 +44,7 @@ export class ClarinItemVersionsFieldComponent extends ItemVersionsComponent impl /** * Maximum number of versions to fetch at once for the dropdown display. */ - private readonly MAX_VERSIONS_TO_DISPLAY = 9999; + private readonly MAX_VERSIONS_TO_DISPLAY = MAX_PAGE_SIZE; /** * Icon name for the clarin field diff --git a/src/app/shared/clarin-item-box-view/clarin-item-box-view.component.ts b/src/app/shared/clarin-item-box-view/clarin-item-box-view.component.ts index e63961ee6ea..448c4f1840f 100644 --- a/src/app/shared/clarin-item-box-view/clarin-item-box-view.component.ts +++ b/src/app/shared/clarin-item-box-view/clarin-item-box-view.component.ts @@ -29,7 +29,7 @@ import { ListableObject } from '../object-collection/shared/listable-object.mode import { ItemSearchResult } from '../object-collection/shared/item-search-result.model'; import { getItemPageRoute } from '../../item-page/item-page-routing-paths'; import { metadataLangToBcp47 } from '../utils/metadata-language.util'; -import { FindListOptions } from '../../core/data/find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from '../../core/data/find-list-options.model'; import { ClarinDateService } from '../clarin-date.service'; import { AUTHOR_METADATA_FIELDS } from '../../core/shared/clarin/constants'; import {RequestParam} from '../../core/cache/models/request-param.model'; @@ -181,7 +181,7 @@ export class ClarinItemBoxViewComponent implements OnInit { return; } const configAllElements: FindListOptions = Object.assign(new FindListOptions(), { - elementsPerPage: 9999 + elementsPerPage: MAX_PAGE_SIZE }); this.bundleService.findByItemAndName(this.item, 'ORIGINAL', true, true, From 8866ec25914d44b362da7ba82029149f25d63a35 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:46:02 +0200 Subject: [PATCH 06/11] Stop percent-encoding the usagereports uri param The frontend sent uri=http%3A%2F%2F... and the REST API echoed it back decoded as uri=http://..., which made the self link differ from the requested url and produced the self link warning on the home page. RequestParam encodes by default; pass encodeValue=false, exactly as AuthorizationDataService already does for its own uri param (upstream #3045/#3046), and carry the same TODO noting this belongs in the backend. --- src/app/core/statistics/usage-report-data.service.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/core/statistics/usage-report-data.service.ts b/src/app/core/statistics/usage-report-data.service.ts index 96961a863d5..958b3a2bb1e 100644 --- a/src/app/core/statistics/usage-report-data.service.ts +++ b/src/app/core/statistics/usage-report-data.service.ts @@ -46,7 +46,8 @@ export class UsageReportDataService extends IdentifiableDataService searchStatistics(uri: string, page: number, size: number): Observable { return this.searchBy('object', { searchParams: [ - new RequestParam('uri', uri), + // TODO fix encode the uri parameter in the self link in the backend and set encodeValue to true afterwards + new RequestParam('uri', uri, false), ], currentPage: page, elementsPerPage: size, From 93038c28aba93c88093d9b3c52ce56237add91ac Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:46:24 +0200 Subject: [PATCH 07/11] Report a reduced page size again, now that nothing asks for an impossible one Filtering the page size out of the self link comparison treated the symptom: the frontend went on asking for 9999, the API went on reducing it to 1000, and the code here just stopped saying so. With every call site now within MAX_PAGE_SIZE, a page size the API reduced means a caller asked for something it was never going to get - which is exactly what this warning exists to surface. Drops isReducedPageSize, getPageSizes and PAGE_SIZE_PARAM, and with them the need to pass the response payload into isUnexpectedSelfLink at all. What stays is the part that is a genuine comparison bug rather than a filter: embed params and percent encoding are two ways of writing the same request, so both sides are brought to the same form before being compared. A difference in an actual value is still reported. Spec goes from 25 cases to 17: the nine that pinned the page size filtering are gone, replaced by one asserting a reduced page size warns. --- ...pace-rest-response-parsing.service.spec.ts | 90 ++----------------- .../dspace-rest-response-parsing.service.ts | 57 +++--------- 2 files changed, 19 insertions(+), 128 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 eddf481414d..f85f068b9c8 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 @@ -72,27 +72,6 @@ describe('DspaceRestResponseParsingService', () => { expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles'); }); - it('should not warn when the REST API reduced the requested page size', () => { - // https://github.com/dataquest-dev/dspace-customers/issues/862 - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&size=9999'); - const response = service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&size=1000', - { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); - - expect(console.warn).not.toHaveBeenCalled(); - expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); - }); - - it('should not warn when the REST API reduced the page size and no embeds are involved', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=9999'); - const response = service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?size=1000', - { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); - - expect(console.warn).not.toHaveBeenCalled(); - expect(response.payload._links.self.href).toBe('https://rest.api/core/items/eba1c085/bundles?size=9999'); - }); - it('should not warn when the self link only percent decoded a param value', () => { // https://github.com/dataquest-dev/dspace-customers/issues/862 const request = requestFor('https://rest.api/statistics/usagereports/search/object?page=-1&size=10&uri=https%3A%2F%2Frest.api%2Fcore%2Fsites%2F8f842a80'); @@ -118,20 +97,13 @@ describe('DspaceRestResponseParsingService', () => { describe('differences that point at a problem with the endpoint', () => { - it('should warn when the page size shrank but the page block contradicts the self link', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=100'); + 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 + const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=9999'); service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?size=20', - { number: 0, size: 100, totalPages: 1, totalElements: 2 })); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith(MISMATCH); - }); - - it('should warn when the page size shrank but the response has no page block to confirm it', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=100'); - service.callEnsureSelfLink(request, - responseWithSelfLink('https://rest.api/core/items/eba1c085/bundles?size=20')); + '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(MISMATCH); @@ -147,16 +119,6 @@ describe('DspaceRestResponseParsingService', () => { expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); - it('should warn when the url is ambiguous about the page size', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5&size=10'); - service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?size=3', - { number: 0, size: 3, totalPages: 1, totalElements: 2 })); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith(MISMATCH); - }); - it('should still warn when a param value differs beyond its encoding', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?uri=https%3A%2F%2Frest.api%2Fcore%2Fsites%2Faaa'); service.callEnsureSelfLink(request, @@ -166,46 +128,6 @@ describe('DspaceRestResponseParsingService', () => { expect(console.warn).toHaveBeenCalledWith(MISMATCH); }); - it('should warn when a reduced page size hides another param that 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', - { number: 3, size: 1000, totalPages: 4, totalElements: 3200 })); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith(MISMATCH); - }); - - it('should warn when the self link claims an empty page', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=10'); - service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?size=0', - { number: 0, size: 0, totalPages: 0, totalElements: 0 })); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith(MISMATCH); - }); - - it('should not treat a param that merely ends in `size` as the page size', () => { - const request = requestFor('https://rest.api/core/items/eba1c085/bundles?pagesize=9999'); - service.callEnsureSelfLink(request, responseWithSelfLink( - 'https://rest.api/core/items/eba1c085/bundles?pagesize=1000', - { number: 0, size: 1000, totalPages: 1, totalElements: 2 })); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith(MISMATCH); - }); - - it('should not accept a page block that confirms the reduced size only as a string', () => { - 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(MISMATCH); - }); - it('should report the normalized request url and the raw self link in the warning', () => { const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&embed=primaryBitstream&size=5'); service.callEnsureSelfLink(request, 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 74641be2837..14bc88dd88d 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -62,11 +62,6 @@ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => { || actual.some((part: string) => !expected.includes(part)); }; -/** - * Matches a page size query param, e.g. `size=100` - */ -const PAGE_SIZE_PARAM = /^size=(\d+)$/; - /** * 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. @@ -82,46 +77,20 @@ const decodeUrlParts = (parts: string[]): string[] => { }; /** - * Return the page sizes among the given url parts. More than one means the url is ambiguous - */ -const getPageSizes = (parts: string[]): number[] => { - return parts - .map((part: string) => part.match(PAGE_SIZE_PARAM)) - .filter((matches) => hasValue(matches)) - .map((matches) => Number(matches[1])); -}; - -/** - * Return true if the self link only shrank the page size to a value the response confirms itself. + * 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. * - * The REST API silently resets a `size` over its configured maximum. The frontend can't know that - * maximum, so it requires the `page` block to corroborate the smaller size, and reports a self link - * that contradicts the payload it describes. Zero is never a maximum, so an empty page isn't a - * clamp either. + * 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. */ -const isReducedPageSize = (expected: string[], actual: string[], payload: any): boolean => { - const requestedSizes = getPageSizes(expected); - const effectiveSizes = getPageSizes(actual); - return requestedSizes.length === 1 && effectiveSizes.length === 1 - && effectiveSizes[0] > 0 && effectiveSizes[0] < requestedSizes[0] - && hasValue(payload.page) && payload.page.size === effectiveSizes[0]; -}; - -/** - * Return true if the self link differs from the requested url in a way the REST API isn't expected - * to introduce by itself. Not reported: `embed` params the API echoes back (they are stripped from - * the requested url but not from the self link), percent encoding, and a confirmed page size clamp. - */ -const isUnexpectedSelfLink = (requestedUrl: string, selfLink: string, payload: any): boolean => { - const expected = decodeUrlParts(splitUrlInParts(requestedUrl)); - const actual = decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))); - - if (isReducedPageSize(expected, actual, payload)) { - const withoutPageSize = (parts: string[]): string[] => - parts.filter((part: string) => !PAGE_SIZE_PARAM.test(part)); - return urlPartsDiffer(withoutPageSize(expected), withoutPageSize(actual)); - } - return urlPartsDiffer(expected, actual); +const isUnexpectedSelfLink = (requestedUrl: string, selfLink: string): boolean => { + return urlPartsDiffer( + decodeUrlParts(splitUrlInParts(requestedUrl)), + decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))), + ); }; @Injectable({ providedIn: 'root' }) @@ -231,7 +200,7 @@ export class DspaceRestResponseParsingService implements ResponseParsingService 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(urlWithoutEmbedParams, selfLink, response.payload)) { + if (isUnexpectedSelfLink(urlWithoutEmbedParams, 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`); } response.payload._links = Object.assign({}, response.payload._links, { From a5f19fb4b184be3b16503557657e9dbb9b485e63 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 08:15:56 +0200 Subject: [PATCH 08/11] Use MAX_PAGE_SIZE for the license lookups too Three CLARIN license components already asked for exactly 1000, so they never triggered the warning - but leaving the bare literal next to a newly introduced MAX_PAGE_SIZE just invites the question why one place names the limit and the other repeats it. Same value, no behaviour change; the constant now says where the number comes from. --- .../clarin-all-licenses-page.component.ts | 4 ++-- .../item-license-mapper/item-license-mapper.component.ts | 4 ++-- .../clarin-license-resource/section-license.component.ts | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts b/src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts index b122dc16f82..1b81ece6473 100644 --- a/src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts +++ b/src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts @@ -3,7 +3,7 @@ import { BehaviorSubject } from 'rxjs'; import { ClarinLicense } from '../../core/shared/clarin/clarin-license.model'; import { ClarinLicenseDataService } from '../../core/data/clarin/clarin-license-data.service'; import { getFirstSucceededRemoteListPayload } from '../../core/shared/operators'; -import { FindListOptions } from '../../core/data/find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from '../../core/data/find-list-options.model'; import { ClarinLicenseRequiredInfo } from '../../core/shared/clarin/clarin-license.resource-type'; import { ClarinLicenseRequiredInfoSerializer } from '../../core/shared/clarin/clarin-license-required-info-serializer'; @@ -36,7 +36,7 @@ export class ClarinAllLicensesPageComponent implements OnInit { const options = new FindListOptions(); options.currentPage = 0; // Load all licenses - options.elementsPerPage = 1000; + options.elementsPerPage = MAX_PAGE_SIZE; return this.clarinLicenseService.findAll(options, false) .pipe(getFirstSucceededRemoteListPayload()) .subscribe(res => { diff --git a/src/app/item-page/edit-item-page/item-license-mapper/item-license-mapper.component.ts b/src/app/item-page/edit-item-page/item-license-mapper/item-license-mapper.component.ts index d999bed8678..d9e06d455cd 100644 --- a/src/app/item-page/edit-item-page/item-license-mapper/item-license-mapper.component.ts +++ b/src/app/item-page/edit-item-page/item-license-mapper/item-license-mapper.component.ts @@ -8,7 +8,7 @@ import { ClarinLicenseDataService } from '../../../core/data/clarin/clarin-licen import { getFirstCompletedRemoteData, getFirstSucceededRemoteListPayload } from '../../../core/shared/operators'; import { PaginatedList } from '../../../core/data/paginated-list.model'; import { ClarinLicense } from '../../../core/shared/clarin/clarin-license.model'; -import { FindListOptions } from '../../../core/data/find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from '../../../core/data/find-list-options.model'; import { PutRequest } from '../../../core/data/request.models'; import { HALEndpointService } from '../../../core/shared/hal-endpoint.service'; import { RequestService } from '../../../core/data/request.service'; @@ -90,7 +90,7 @@ export class ItemLicenseMapperComponent implements OnInit { const options = new FindListOptions(); options.currentPage = 0; // Load all licenses - options.elementsPerPage = 1000; + options.elementsPerPage = MAX_PAGE_SIZE; this.clarinLicenseService.findAll(options, false) .pipe( diff --git a/src/app/submission/sections/clarin-license-resource/section-license.component.ts b/src/app/submission/sections/clarin-license-resource/section-license.component.ts index 374f5ec4170..d7e24ef7e8e 100644 --- a/src/app/submission/sections/clarin-license-resource/section-license.component.ts +++ b/src/app/submission/sections/clarin-license-resource/section-license.component.ts @@ -35,6 +35,7 @@ import { TranslateService } from '@ngx-translate/core'; import { FindListOptions } from 'src/app/core/data/find-list-options.model'; import { hasFailed } from 'src/app/core/data/request-entry-state.model'; import {RequestParam} from '../../../core/cache/models/request-param.model'; +import { MAX_PAGE_SIZE } from '../../../core/data/find-list-options.model'; /** * This component render resource license step in the submission workflow. @@ -551,7 +552,7 @@ export class SubmissionSectionClarinLicenseComponent extends SectionModelCompone const options = new FindListOptions(); options.currentPage = 0; // Load all licenses - options.elementsPerPage = 1000; + options.elementsPerPage = MAX_PAGE_SIZE; return this.clarinLicenseService.findAll(options, false) .pipe(getFirstSucceededRemoteListPayload()) .toPromise(); From 97d4eefa2150b42e2c5408b1e46d95ac15dd8311 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 10:28:29 +0200 Subject: [PATCH 09/11] Drop the uri encoding workaround instead of copying its TODO An earlier commit here made UsageReportDataService skip encoding its uri param, copying the workaround and the TODO that AuthorizationDataService carries: // TODO fix encode the uri parameter in the self link in the backend and set // encodeValue to true afterwards Measured against a DSpace 9.1 backend, that TODO is based on a misreading - there is nothing to fix in the backend. It does not decode the self link; it re-encodes the parameter values minimally, and ':' and '/' are legal in a query component per RFC 3986, so it has no reason to escape them: sent probe=a%25b -> self probe=a%25b (not decoded) sent probe=a%20b -> self probe=a%20b (not decoded) sent probe=a+b -> self probe=a%20b ('+' is a space in form encoding) sent uri=http%3A%2F%2Fx -> self uri=http://x encodeURIComponent on the frontend simply encodes more than it has to. Both urls are valid and denote the same value. Since the comparison now decodes both sides, the encoding no longer matters, so put the param back on the default: encoding is the safer choice for a value that could contain '&' or '#'. Verified in a browser - the home page still logs no self link warning with encoding restored. The same workaround in AuthorizationDataService is left alone: it is upstream code, it works either way, and it is out of scope here. --- src/app/core/statistics/usage-report-data.service.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/core/statistics/usage-report-data.service.ts b/src/app/core/statistics/usage-report-data.service.ts index 958b3a2bb1e..96961a863d5 100644 --- a/src/app/core/statistics/usage-report-data.service.ts +++ b/src/app/core/statistics/usage-report-data.service.ts @@ -46,8 +46,7 @@ export class UsageReportDataService extends IdentifiableDataService searchStatistics(uri: string, page: number, size: number): Observable { return this.searchBy('object', { searchParams: [ - // TODO fix encode the uri parameter in the self link in the backend and set encodeValue to true afterwards - new RequestParam('uri', uri, false), + new RequestParam('uri', uri), ], currentPage: page, elementsPerPage: size, From 941fdea0f32850fb11293fc68f91715b309c1b7b Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Fri, 7 Aug 2026 11:50:08 +0200 Subject: [PATCH 10/11] Pass the already split request url into isUnexpectedSelfLink ensureSelfLink splits the requested url into parts before the comparison, then isUnexpectedSelfLink split the same string a second time. The branch runs on every response whose self link differs, which after this change is every embedded list response, so the duplicate work is not rare. Takes the parts instead. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/core/data/dspace-rest-response-parsing.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 14bc88dd88d..12715b0ff71 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -78,7 +78,7 @@ 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. + * 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 @@ -86,9 +86,9 @@ const decodeUrlParts = (parts: string[]): string[] => { * 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. */ -const isUnexpectedSelfLink = (requestedUrl: string, selfLink: string): boolean => { +const isUnexpectedSelfLink = (requestedUrlParts: string[], selfLink: string): boolean => { return urlPartsDiffer( - decodeUrlParts(splitUrlInParts(requestedUrl)), + decodeUrlParts(requestedUrlParts), decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))), ); }; @@ -200,7 +200,7 @@ export class DspaceRestResponseParsingService implements ResponseParsingService 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(urlWithoutEmbedParams, selfLink)) { + 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`); } response.payload._links = Object.assign({}, response.payload._links, { From 8c40ed2460dbbe557ee747a3d49d529b57519984 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Fri, 7 Aug 2026 11:50:21 +0200 Subject: [PATCH 11/11] Merge the MAX_PAGE_SIZE import into the existing one section-license.component.ts already imported FindListOptions from find-list-options.model, so adding MAX_PAGE_SIZE as a second import of the same module - under a different specifier - left the file importing one module twice. Co-Authored-By: Claude Opus 5 (1M context) --- .../clarin-license-resource/section-license.component.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/submission/sections/clarin-license-resource/section-license.component.ts b/src/app/submission/sections/clarin-license-resource/section-license.component.ts index d7e24ef7e8e..2161cf111ef 100644 --- a/src/app/submission/sections/clarin-license-resource/section-license.component.ts +++ b/src/app/submission/sections/clarin-license-resource/section-license.component.ts @@ -32,10 +32,9 @@ import { ItemDataService } from '../../../core/data/item-data.service'; import { Item } from '../../../core/shared/item.model'; import { MetadataValue } from '../../../core/shared/metadata.models'; import { TranslateService } from '@ngx-translate/core'; -import { FindListOptions } from 'src/app/core/data/find-list-options.model'; +import { FindListOptions, MAX_PAGE_SIZE } from 'src/app/core/data/find-list-options.model'; import { hasFailed } from 'src/app/core/data/request-entry-state.model'; import {RequestParam} from '../../../core/cache/models/request-param.model'; -import { MAX_PAGE_SIZE } from '../../../core/data/find-list-options.model'; /** * This component render resource license step in the submission workflow.