Skip to content

Commit b7e9b86

Browse files
milanmajchrakclaude
andcommitted
fix(self-link): say what happened when the API reduces a page size
ensureSelfLink() logs one wording for every self link that differs from the url it was requested with, and that wording ends with "This could mean there's an issue with the REST endpoint". For a page size the API reduced, that points at the wrong place: the API did exactly what its contract says, the caller asked for a bigger page than it will ever serve. Whoever reads the console goes looking at the backend for a frontend mistake. Give that case its own message: The request for '.../bundles?size=9999' asked for a page of 9999 elements, but the REST API served 1000. Ask for at most MAX_PAGE_SIZE elements It applies only when the two urls are otherwise identical, so an accepted reduction cannot mask a wrong page or sort. Anything else keeps the generic message, including a page that came back larger than requested. This is the remaining half of the fix on dtq-dev. #1415 landed the rest - embed params and percent encoding normalized on both sides, MAX_PAGE_SIZE next to FindListOptions, and every oversized call site brought within it - so a reduced page size now genuinely means a caller asked for something the API was never going to serve, and the message can say so. - src/app/core/data/dspace-rest-response-parsing.service.ts - replace isUnexpectedSelfLink() with selfLinkWarning(), which returns the message to log or undefined when there is nothing to report, and add the getPageSize()/withoutPageSize() helpers and the PAGE_SIZE_PARAM they match on. The self link is normalized either way, as before, so the url a response is cached under does not change - only whether and how we warn. - src/app/core/data/dspace-rest-response-parsing.service.spec.ts - 17 cases to 19. The reduced page size case now expects the specific wording; one new case pins that a reduced size is recognised alongside other params, another that a page differing as well falls back to the generic message. Adapted to 7.6 from DSpace/dspace-angular PR 6083, commit 887ddec. The 9.x-only parts of that commit's file - the @dspace/* path aliases and the inject(APP_CONFIG) refactor - are deliberately left out, which is also why the spec keeps its plain constructor harness instead of upstream's TestBed. Same change as #1448, which went to customer/jcu only. Fixes dataquest-dev/dspace-customers#934 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 914a688 commit b7e9b86

2 files changed

Lines changed: 64 additions & 23 deletions

File tree

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ describe('DspaceRestResponseParsingService', () => {
1919
let objectCache: ObjectCacheService;
2020

2121
const MISMATCH = jasmine.stringMatching(/These don't match/);
22+
const REDUCED_PAGE = jasmine.stringMatching(/asked for a page of 9999 elements, but the REST API served 1000/);
2223
const NO_SELF_LINK = jasmine.stringMatching(/doesn't have a self link/);
2324

2425
const requestFor = (href: string): RestRequest =>
@@ -64,7 +65,6 @@ describe('DspaceRestResponseParsingService', () => {
6465
});
6566

6667
it('should not warn when the self link echoes embed params and the request has no other params', () => {
67-
// observed in the browser against a DSpace 9.1 backend
6868
const href = 'https://rest.api/core/items/eba1c085/bundles?embed=primaryBitstream&embed=bitstreams/format&embed.size=bitstreams=5';
6969
const response = service.callEnsureSelfLink(requestFor(href), responseWithSelfLink(href));
7070

@@ -97,19 +97,35 @@ describe('DspaceRestResponseParsingService', () => {
9797

9898
describe('differences that point at a problem with the endpoint', () => {
9999

100-
it('should warn when the REST API reduced the requested page size', () => {
101-
// callers are expected to stay within MAX_PAGE_SIZE, so a reduced size means a caller asked
102-
// for a page the API was never going to serve
100+
it('should say so when the REST API reduced the requested page size', () => {
103101
const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=9999');
104102
service.callEnsureSelfLink(request, responseWithSelfLink(
105103
'https://rest.api/core/items/eba1c085/bundles?size=1000',
106104
{ number: 0, size: 1000, totalPages: 1, totalElements: 2 }));
107105

106+
expect(console.warn).toHaveBeenCalledTimes(1);
107+
expect(console.warn).toHaveBeenCalledWith(REDUCED_PAGE);
108+
});
109+
110+
it('should report a reduced page size alongside other params without confusing the two', () => {
111+
const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=9999&sort=name,ASC');
112+
service.callEnsureSelfLink(request, responseWithSelfLink(
113+
'https://rest.api/core/items/eba1c085/bundles?page=0&size=1000&sort=name,ASC'));
114+
115+
expect(console.warn).toHaveBeenCalledTimes(1);
116+
expect(console.warn).toHaveBeenCalledWith(REDUCED_PAGE);
117+
});
118+
119+
it('should fall back to the generic warning when more than the page size differs', () => {
120+
const request = requestFor('https://rest.api/core/items/eba1c085/bundles?page=0&size=9999');
121+
service.callEnsureSelfLink(request, responseWithSelfLink(
122+
'https://rest.api/core/items/eba1c085/bundles?page=3&size=1000'));
123+
108124
expect(console.warn).toHaveBeenCalledTimes(1);
109125
expect(console.warn).toHaveBeenCalledWith(MISMATCH);
110126
});
111127

112-
it('should warn when the returned page size is larger than the requested one', () => {
128+
it('should use the generic warning when the returned page size is larger than requested', () => {
113129
const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5');
114130
service.callEnsureSelfLink(request, responseWithSelfLink(
115131
'https://rest.api/core/items/eba1c085/bundles?size=50',

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

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ export function isRestPaginatedList(halObj: any): boolean {
4343
hasValue(halObj.page.number);
4444
}
4545

46+
/**
47+
* The url param holding the page size
48+
*/
49+
const PAGE_SIZE_PARAM = 'size=';
50+
4651
/**
4752
* Split a url into parts
4853
*
@@ -63,8 +68,8 @@ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => {
6368
};
6469

6570
/**
66-
* Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Parts are
67-
* decoded one by one, after the url was split, so a decoded `&` can't merge two params.
71+
* Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Decoding
72+
* after the split keeps a decoded `&` from merging two params.
6873
*/
6974
const decodeUrlParts = (parts: string[]): string[] => {
7075
return parts.map((part: string) => {
@@ -77,20 +82,39 @@ const decodeUrlParts = (parts: string[]): string[] => {
7782
};
7883

7984
/**
80-
* Return true if the self link differs from the requested url in a way that isn't just a different
81-
* way of writing the same request. Takes the requested url already split, since the caller has it.
82-
*
83-
* Both sides are brought to the same form first: `embed`/`embed.size` params are stripped, because
84-
* the frontend treats them as not part of a resource's identity and indexes without them, and both
85-
* are percent decoded. Anything still differing is a real difference between what was asked for and
86-
* what came back, including a page size the API reduced — callers are expected to stay within
87-
* `MAX_PAGE_SIZE` rather than have that reported difference filtered out here.
85+
* The page size a url asks for, or undefined when it doesn't ask for a usable one
86+
*/
87+
const getPageSize = (parts: string[]): number | undefined => {
88+
return parts.filter((part: string) => part.startsWith(PAGE_SIZE_PARAM))
89+
.map((part: string) => Number(part.substring(PAGE_SIZE_PARAM.length)))
90+
.find((size: number) => Number.isInteger(size) && size > 0);
91+
};
92+
93+
/**
94+
* Return the parts without the one holding the page size
8895
*/
89-
const isUnexpectedSelfLink = (requestedUrlParts: string[], selfLink: string): boolean => {
90-
return urlPartsDiffer(
91-
decodeUrlParts(requestedUrlParts),
92-
decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink))),
93-
);
96+
const withoutPageSize = (parts: string[]): string[] => {
97+
return parts.filter((part: string) => !part.startsWith(PAGE_SIZE_PARAM));
98+
};
99+
100+
/**
101+
* Return the warning to log for a self link, or undefined when it describes the same request as the
102+
* url it was requested with. A reduced page size gets its own message, since the generic one blames
103+
* the endpoint for something the caller did.
104+
*/
105+
const selfLinkWarning = (requestedUrl: string, requestedUrlParts: string[], selfLink: string): string | undefined => {
106+
const expected = decodeUrlParts(requestedUrlParts);
107+
const actual = decodeUrlParts(splitUrlInParts(getUrlWithoutEmbedParams(selfLink)));
108+
if (!urlPartsDiffer(expected, actual)) {
109+
return undefined;
110+
}
111+
const requestedSize = getPageSize(expected);
112+
const servedSize = getPageSize(actual);
113+
if (hasValue(requestedSize) && hasValue(servedSize) && servedSize < requestedSize
114+
&& !urlPartsDiffer(withoutPageSize(expected), withoutPageSize(actual))) {
115+
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`;
116+
}
117+
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`;
94118
};
95119

96120
@Injectable({ providedIn: 'root' })
@@ -199,9 +223,10 @@ export class DspaceRestResponseParsingService implements ResponseParsingService
199223
const expected = splitUrlInParts(urlWithoutEmbedParams);
200224
const actual = splitUrlInParts(selfLink);
201225
if (expected[0] === actual[0] && urlPartsDiffer(expected, actual)) {
202-
// the self link is normalized either way, only the warning is filtered
203-
if (isUnexpectedSelfLink(expected, selfLink)) {
204-
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`);
226+
// the self link is normalized either way, only the warning is conditional
227+
const warning = selfLinkWarning(urlWithoutEmbedParams, expected, selfLink);
228+
if (hasValue(warning)) {
229+
console.warn(warning);
205230
}
206231
response.payload._links = Object.assign({}, response.payload._links, {
207232
self: {

0 commit comments

Comments
 (0)