Skip to content

Commit 52acd99

Browse files
milanmajchrakclaude
andcommitted
Port #1490 to dtq-dev-9-base: 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. - dspace-rest-response-parsing.service.ts: isUnexpectedSelfLink() is replaced by selfLinkWarning(), which returns the message to log or undefined when there is nothing to report, plus 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. - 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. It also drops a stray note left in the spec by #1415. v9 notes: this is the 9.3-native original (customer/jcu 382a721) applied unchanged. Its two files were byte-identical with this branch after #1415, so the cherry-pick was clean, and its changed lines are identical to those of the dtq-dev commit 51796ec (the two differ only in the untouched import block's formatting). Stacked on ufal/port-1415-9-base (card FE-06 / PR #1415) - #1490 rewrites the helper that #1415 introduces. Card FE-01 (tranche T1). Source: 51796ec (dtq-dev PR #1490) Adapted-from: 382a721 (customer/jcu PR #1448) (cherry picked from commit 382a721) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2233470 commit 52acd99

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
@@ -22,6 +22,7 @@ describe('DspaceRestResponseParsingService', () => {
2222
let objectCache: ObjectCacheService;
2323

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

2728
const requestFor = (href: string): RestRequest =>
@@ -67,7 +68,6 @@ describe('DspaceRestResponseParsingService', () => {
6768
});
6869

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

@@ -100,19 +100,35 @@ describe('DspaceRestResponseParsingService', () => {
100100

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

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

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

115-
it('should warn when the returned page size is larger than the requested one', () => {
131+
it('should use the generic warning when the returned page size is larger than requested', () => {
116132
const request = requestFor('https://rest.api/core/items/eba1c085/bundles?size=5');
117133
service.callEnsureSelfLink(request, responseWithSelfLink(
118134
'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
@@ -54,6 +54,11 @@ export function isRestPaginatedList(halObj: any): boolean {
5454
hasValue(halObj.page.number);
5555
}
5656

57+
/**
58+
* The url param holding the page size
59+
*/
60+
const PAGE_SIZE_PARAM = 'size=';
61+
5762
/**
5863
* Split a url into parts
5964
*
@@ -74,8 +79,8 @@ const urlPartsDiffer = (expected: string[], actual: string[]): boolean => {
7479
};
7580

7681
/**
77-
* Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Parts are
78-
* decoded one by one, after the url was split, so a decoded `&` can't merge two params.
82+
* Percent decode each url part, so `uri=http%3A%2F%2Fx` and `uri=http://x` compare equal. Decoding
83+
* after the split keeps a decoded `&` from merging two params.
7984
*/
8085
const decodeUrlParts = (parts: string[]): string[] => {
8186
return parts.map((part: string) => {
@@ -88,20 +93,39 @@ const decodeUrlParts = (parts: string[]): string[] => {
8893
};
8994

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

107131
@Injectable({ providedIn: 'root' })
@@ -210,9 +234,10 @@ export class DspaceRestResponseParsingService implements ResponseParsingService
210234
const expected = splitUrlInParts(urlWithoutEmbedParams);
211235
const actual = splitUrlInParts(selfLink);
212236
if (expected[0] === actual[0] && urlPartsDiffer(expected, actual)) {
213-
// the self link is normalized either way, only the warning is filtered
214-
if (isUnexpectedSelfLink(expected, selfLink)) {
215-
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`);
237+
// the self link is normalized either way, only the warning is conditional
238+
const warning = selfLinkWarning(urlWithoutEmbedParams, expected, selfLink);
239+
if (hasValue(warning)) {
240+
console.warn(warning);
216241
}
217242
response.payload._links = Object.assign({}, response.payload._links, {
218243
self: {

0 commit comments

Comments
 (0)