Fix the causes of the REST self link console warnings

src/app/admin/admin-reports/filtered-items/filtered-items.component.ts

@@ -42,6 +42,7 @@ import { isEmpty } from 'src/app/shared/empty.util';
 import { ThemedLoadingComponent } from 'src/app/shared/loading/themed-loading.component';
 import { environment } from 'src/environments/environment';
 
+import { MAX_PAGE_SIZE } from '../../../core/data/find-list-options.model';
 import { BtnDisabledDirective } from '../../../shared/btn-disabled.directive';
 import { FiltersComponent } from '../filters-section/filters-section.component';
 import { FilteredItemsExportCsvComponent } from './filtered-items-export-csv/filtered-items-export-csv.component';
@@ -135,15 +136,15 @@ 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[]) => {
         communitiesRest.forEach(community => {
           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[]) => {

src/app/core/browse/browse.service.ts

@@ -18,6 +18,7 @@ import {
   FollowLinkConfig,
 } from '../../shared/utils/follow-link-config.model';
 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';
@@ -81,7 +82,7 @@ export class BrowseService {
    */
   getBrowseDefinitions(): Observable<RemoteData<PaginatedList<BrowseDefinition>>> {
     // TODO properly support pagination
-    return this.browseDefinitionDataService.findAll({ elementsPerPage: 9999 }).pipe(
+    return this.browseDefinitionDataService.findAll({ elementsPerPage: MAX_PAGE_SIZE }).pipe(
       getFirstSucceededRemoteData(),
     );
   }

src/app/core/data/bundle-data.service.ts

@@ -22,7 +22,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';
@@ -84,7 +87,7 @@ export class BundleDataService extends IdentifiableDataService<Bundle> implement
   findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
     //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<PaginatedList<Bundle>>) => {
         if (hasValue(rd.payload) && hasValue(rd.payload.page)) {

src/app/core/data/dspace-rest-response-parsing.service.spec.ts

@@ -0,0 +1,235 @@
+import { ObjectCacheService } from '../cache/object-cache.service';
+import { RawRestResponse } from '../dspace-rest/raw-rest-response.model';
+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.
+ */
+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 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,
+          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, 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=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');
+      });
+
+    });
+
+  });
+});

src/app/core/data/dspace-rest-response-parsing.service.ts

@@ -65,6 +65,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<Serializer<any>> = DSpaceSerializer;
@@ -167,10 +206,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,