dataquest-dev
diff --git a/‎src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts‎
Lines changed: 2 additions & 1 deletion b/‎src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts‎
Lines changed: 2 additions & 1 deletion
diff --git a/‎src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts‎
Lines changed: 2 additions & 2 deletions b/‎src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts‎
Lines changed: 2 additions & 2 deletions
diff --git a/‎src/app/core/browse/browse.service.ts‎
Lines changed: 2 additions & 1 deletion b/‎src/app/core/browse/browse.service.ts‎
Lines changed: 2 additions & 1 deletion
diff --git a/‎src/app/core/data/bundle-data.service.ts‎
Lines changed: 2 additions & 2 deletions b/‎src/app/core/data/bundle-data.service.ts‎
Lines changed: 2 additions & 2 deletions
diff --git a/‎src/app/core/data/dspace-rest-response-parsing.service.spec.ts‎
Lines changed: 232 additions & 0 deletions b/‎src/app/core/data/dspace-rest-response-parsing.service.spec.ts‎
Lines changed: 232 additions & 0 deletions
diff --git a/‎src/app/core/data/dspace-rest-response-parsing.service.ts‎
Lines changed: 46 additions & 3 deletions b/‎src/app/core/data/dspace-rest-response-parsing.service.ts‎
Lines changed: 46 additions & 3 deletions
diff --git a/‎src/app/core/data/find-list-options.model.ts‎
Lines changed: 10 additions & 0 deletions b/‎src/app/core/data/find-list-options.model.ts‎
Lines changed: 10 additions & 0 deletions
@@ -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<Item>)  => {
       this.bitstreamDataService.findAllByItemAndBundleName(itemRD?.payload, 'ORIGINAL', {
         currentPage: 1,
-        elementsPerPage: 9999
+        elementsPerPage: MAX_PAGE_SIZE
       }).pipe(
         getFirstCompletedRemoteData(),
       ).subscribe((bitstreamsRD: RemoteData<PaginatedList<Bitstream>>) => {
 
@@ -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 => {
 
@@ -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<BrowseEntry | Item>[] {
@@ -69,7 +70,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(),
     );
   }
 
@@ -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<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)) {
 
@@ -0,0 +1,232 @@
+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 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');
+      });
+
+    });
+
+  });
+});
@@ -54,6 +54,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;
@@ -156,10 +195,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
 
@@ -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
  */