From d0811063612c6dc9bc01b9cef514bb1fc9f235f6 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 10 Aug 2026 10:37:20 +0200 Subject: [PATCH 1/2] Fix the causes of the self link mismatch console warnings ensureSelfLink compares the requested url against the self link in the response. Two of the differences it reported were not mismatches: - embed params are removed from the requested url before the comparison, but not from the self link, which echoes them back. - encoding is compared literally. RequestParam encodes with encodeURIComponent while the REST API escapes only what it has to, so uri=http%3A%2F%2Fx returns as uri=http://x. Both sides are now brought to the same form before being compared. Decoding is done per url part after the split, so a decoded '&' cannot merge two params, and is wrapped in try/catch because a malformed escape makes decodeURIComponent throw. The code that rewrites _links.self is unchanged, so caching is unaffected. The third cause is on the frontend side. Spring Data REST caps a page at spring.data.rest.max-page-size, left at its default of 1000, so a request for 9999 returns a self link saying 1000. MAX_PAGE_SIZE replaces the oversized values in the six call sites that used them: bundle-data.service.ts 9999 browse.service.ts 9999 relationship-type-data.service.ts 9999 registry.service.ts 10000 item-bitstreams.service.ts 9999 filtered-items.component.ts 4 x 10000 The API already capped each of these, so the same rows are returned. A reduced page size still warns, so an oversized request added later is still reported. browse-by-geospatial-data.component.ts is left unchanged: its 99999 is a Discovery facet limit, not a page size. Adds a spec for dspace-rest-response-parsing.service.ts, which had none. --- .../registry/registry.service.ts | 7 +- .../filtered-items.component.ts | 9 +- src/app/core/browse/browse.service.ts | 3 +- src/app/core/data/bundle-data.service.ts | 7 +- ...pace-rest-response-parsing.service.spec.ts | 242 ++++++++++++++++++ .../dspace-rest-response-parsing.service.ts | 49 +++- src/app/core/data/find-list-options.model.ts | 10 + .../data/relationship-type-data.service.ts | 3 +- .../item-bitstreams.service.ts | 3 +- 9 files changed, 319 insertions(+), 14 deletions(-) create mode 100644 src/app/core/data/dspace-rest-response-parsing.service.spec.ts diff --git a/src/app/admin/admin-registries/registry/registry.service.ts b/src/app/admin/admin-registries/registry/registry.service.ts index 48b0c4334ba..c9b12d33b62 100644 --- a/src/app/admin/admin-registries/registry/registry.service.ts +++ b/src/app/admin/admin-registries/registry/registry.service.ts @@ -1,6 +1,9 @@ import { Injectable } from '@angular/core'; import { RequestParam } from '@dspace/core/cache/models/request-param.model'; -import { FindListOptions } from '@dspace/core/data/find-list-options.model'; +import { + FindListOptions, + MAX_PAGE_SIZE, +} from '@dspace/core/data/find-list-options.model'; import { MetadataFieldDataService } from '@dspace/core/data/metadata-field-data.service'; import { MetadataSchemaDataService } from '@dspace/core/data/metadata-schema-data.service'; import { PaginatedList } from '@dspace/core/data/paginated-list.model'; @@ -94,7 +97,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/admin/admin-reports/filtered-items/filtered-items.component.ts b/src/app/admin/admin-reports/filtered-items/filtered-items.component.ts index 60e1a7f2459..fa8c2cb0af7 100644 --- a/src/app/admin/admin-reports/filtered-items/filtered-items.component.ts +++ b/src/app/admin/admin-reports/filtered-items/filtered-items.component.ts @@ -15,6 +15,7 @@ import { RestRequestMethod } from '@dspace/config/rest-request-method'; import { CollectionDataService } from '@dspace/core/data/collection-data.service'; import { CommunityDataService } from '@dspace/core/data/community-data.service'; import { AuthorizationDataService } from '@dspace/core/data/feature-authorization/authorization-data.service'; +import { MAX_PAGE_SIZE } from '@dspace/core/data/find-list-options.model'; import { MetadataFieldDataService } from '@dspace/core/data/metadata-field-data.service'; import { MetadataSchemaDataService } from '@dspace/core/data/metadata-schema-data.service'; import { ScriptDataService } from '@dspace/core/data/processes/script-data.service'; @@ -135,7 +136,7 @@ export class FilteredItemsComponent implements OnInit { const wholeRepo$ = this.translateService.stream('admin.reports.items.wholeRepo'); this.collections.push(OptionVO.collectionLoc('', wholeRepo$)); - this.communityService.findAll({ elementsPerPage: 10000, currentPage: 1 }).pipe( + this.communityService.findAll({ elementsPerPage: MAX_PAGE_SIZE, currentPage: 1 }).pipe( getFirstSucceededRemoteListPayload(), ).subscribe( (communitiesRest: Community[]) => { @@ -143,7 +144,7 @@ export class FilteredItemsComponent implements OnInit { const commVO = OptionVO.collection(community.uuid, community.name, true); this.collections.push(commVO); - this.collectionService.findByParent(community.uuid, { elementsPerPage: 10000, currentPage: 1 }).pipe( + this.collectionService.findByParent(community.uuid, { elementsPerPage: MAX_PAGE_SIZE, currentPage: 1 }).pipe( getFirstSucceededRemoteListPayload(), ).subscribe( (collectionsRest: Collection[]) => { @@ -207,12 +208,12 @@ export class FilteredItemsComponent implements OnInit { this.metadataFieldsWithAny = []; const anyField$ = this.translateService.stream('admin.reports.items.anyField'); this.metadataFieldsWithAny.push(OptionVO.itemLoc('*', anyField$)); - this.metadataSchemaService.findAll({ elementsPerPage: 10000, currentPage: 1 }).pipe( + this.metadataSchemaService.findAll({ elementsPerPage: MAX_PAGE_SIZE, currentPage: 1 }).pipe( getFirstSucceededRemoteListPayload(), ).subscribe( (schemasRest: MetadataSchema[]) => { schemasRest.forEach(schema => { - this.metadataFieldService.findBySchema(schema, { elementsPerPage: 10000, currentPage: 1 }).pipe( + this.metadataFieldService.findBySchema(schema, { elementsPerPage: MAX_PAGE_SIZE, currentPage: 1 }).pipe( getFirstSucceededRemoteListPayload(), ).subscribe( (fieldsRest: MetadataField[]) => { diff --git a/src/app/core/browse/browse.service.ts b/src/app/core/browse/browse.service.ts index d9509e49a08..17e0d496ed2 100644 --- a/src/app/core/browse/browse.service.ts +++ b/src/app/core/browse/browse.service.ts @@ -20,6 +20,7 @@ import { } from 'rxjs/operators'; import { SortDirection } from '../cache/models/sort-options.model'; +import { MAX_PAGE_SIZE } from '../data/find-list-options.model'; import { HrefOnlyDataService } from '../data/href-only-data.service'; import { PaginatedList } from '../data/paginated-list.model'; import { RemoteData } from '../data/remote-data'; @@ -88,7 +89,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 adaa6209877..a114d082e58 100644 --- a/src/app/core/data/bundle-data.service.ts +++ b/src/app/core/data/bundle-data.service.ts @@ -25,7 +25,10 @@ import { PatchDataImpl, } from './base/patch-data'; import { DSOChangeAnalyzer } from './dso-change-analyzer.service'; -import { FindListOptions } from './find-list-options.model'; +import { + FindListOptions, + MAX_PAGE_SIZE, +} from './find-list-options.model'; import { PaginatedList } from './paginated-list.model'; import { RemoteData } from './remote-data'; import { GetRequest } from './request.models'; @@ -87,7 +90,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/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..00eb7269c20 --- /dev/null +++ b/src/app/core/data/dspace-rest-response-parsing.service.spec.ts @@ -0,0 +1,242 @@ +import { Injectable } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { APP_CONFIG } from '@dspace/config/app-config.interface'; + +import { ObjectCacheService } from '../cache/object-cache.service'; +import { RawRestResponse } from '../dspace-rest/raw-rest-response.model'; +import { getMockObjectCacheService } from '../testing/object-cache.service.mock'; +import { DspaceRestResponseParsingService } from './dspace-rest-response-parsing.service'; +import { + GetRequest, + PostRequest, +} from './request.models'; +import { RestRequest } from './rest-request.model'; + +/** + * Exposes the protected {@link DspaceRestResponseParsingService#ensureSelfLink} so it can be + * tested in isolation. + */ +@Injectable() +class TestService extends DspaceRestResponseParsingService { + public callEnsureSelfLink(request: RestRequest, response: RawRestResponse): RawRestResponse { + return this.ensureSelfLink(request, response); + } +} + +describe('DspaceRestResponseParsingService', () => { + let service: TestService; + + 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(() => { + TestBed.configureTestingModule({ + providers: [ + { provide: ObjectCacheService, useValue: getMockObjectCacheService() }, + { provide: APP_CONFIG, useValue: { rest: { baseUrl: 'https://rest.api' } } }, + TestService, + ], + }); + service = TestBed.inject(TestService); + 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', () => { + 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', () => { + 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 self link only percent decoded a param value', () => { + 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, + 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'); + }); + + }); + + 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 + 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 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 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 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, + 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 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, + 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 10e9f899198..aa44f317063 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -71,6 +71,45 @@ const splitUrlInParts = (url: string): string[] => { .reduce((combined, current) => [...combined, ...current]); }; +/** + * 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)) + || actual.some((part: string) => !expected.includes(part)); +}; + +/** + * 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 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. + */ +const isUnexpectedSelfLink = (requestedUrlParts: string[], selfLink: string): boolean => { + return urlPartsDiffer( + decodeUrlParts(requestedUrlParts), + decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))), + ); +}; + @Injectable({ providedIn: 'root' }) export class DspaceRestResponseParsingService implements ResponseParsingService { protected serializerConstructor: GenericConstructor> = DSpaceSerializer; @@ -175,10 +214,14 @@ 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)) { + // 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`); + } response.payload._links = Object.assign({}, response.payload._links, { self: { href: urlWithoutEmbedParams, diff --git a/src/app/core/data/find-list-options.model.ts b/src/app/core/data/find-list-options.model.ts index 78fe26fcab9..97e708061ae 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 { RequestParam } from '../cache/models/request-param.model'; import { SortOptions } from '../cache/models/sort-options.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/data/relationship-type-data.service.ts b/src/app/core/data/relationship-type-data.service.ts index b2d8b7066e3..25f835d0120 100644 --- a/src/app/core/data/relationship-type-data.service.ts +++ b/src/app/core/data/relationship-type-data.service.ts @@ -31,6 +31,7 @@ import { import { BaseDataService } from './base/base-data.service'; import { FindAllDataImpl } from './base/find-all-data'; import { SearchDataImpl } from './base/search-data'; +import { MAX_PAGE_SIZE } from './find-list-options.model'; import { PaginatedList } from './paginated-list.model'; import { RemoteData } from './remote-data'; import { RequestService } from './request.service'; @@ -78,7 +79,7 @@ export class RelationshipTypeDataService extends BaseDataService { // Retrieve all relationship types from the server in a single page - return this.findAllData.findAll({ currentPage: 1, elementsPerPage: 9999 }, true, true, followLink('leftType'), followLink('rightType')) + return this.findAllData.findAll({ currentPage: 1, elementsPerPage: MAX_PAGE_SIZE }, true, true, followLink('leftType'), followLink('rightType')) .pipe( getFirstSucceededRemoteData(), // Emit each type in the page array separately 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 d3f3c2add48..f9b79432dc1 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 @@ -2,6 +2,7 @@ import { Injectable } from '@angular/core'; import { DSONameService } from '@dspace/core/breadcrumbs/dso-name.service'; import { BitstreamDataService } from '@dspace/core/data/bitstream-data.service'; import { BundleDataService } from '@dspace/core/data/bundle-data.service'; +import { MAX_PAGE_SIZE } from '@dspace/core/data/find-list-options.model'; import { FieldChangeType } from '@dspace/core/data/object-updates/field-change-type.model'; import { FieldUpdate } from '@dspace/core/data/object-updates/field-update.model'; import { FieldUpdates } from '@dspace/core/data/object-updates/field-updates.model'; @@ -360,7 +361,7 @@ export class ItemBitstreamsService { return Object.assign(new PaginationComponentOptions(), { id: 'bundles-pagination-options', currentPage: 1, - pageSize: 9999, + pageSize: MAX_PAGE_SIZE, }); } From 887ddec4c1586fa329e0eaee7fd65f28e9d61595 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Tue, 11 Aug 2026 08:14:10 +0200 Subject: [PATCH 2/2] 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. --- ...pace-rest-response-parsing.service.spec.ts | 24 +++++++- .../dspace-rest-response-parsing.service.ts | 61 +++++++++++++------ 2 files changed, 64 insertions(+), 21 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 00eb7269c20..1fd1b0dec8d 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 @@ -27,6 +27,7 @@ describe('DspaceRestResponseParsingService', () => { let service: TestService; 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 => @@ -108,18 +109,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 + 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 aa44f317063..a985d944ed5 100644 --- a/src/app/core/data/dspace-rest-response-parsing.service.ts +++ b/src/app/core/data/dspace-rest-response-parsing.service.ts @@ -60,6 +60,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 * @@ -80,8 +85,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) => { @@ -94,20 +99,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' }) @@ -218,9 +242,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: {