[Port to dtq-dev] fix(self-link): say what happened when the REST API reduces a page size - #1490
Merged
Merged
Conversation
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>
milanmajchrak
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refines ensureSelfLink()’s console warning behavior in DspaceRestResponseParsingService so that when the REST API reduces a requested page size (per its contract), the warning explains the real cause (caller requested too many elements) instead of suggesting an endpoint bug.
Changes:
- Replace
isUnexpectedSelfLink()withselfLinkWarning()that returns a specific message for “only page size reduced” cases, or a generic mismatch warning otherwise. - Add page-size parsing helpers (
getPageSize(),withoutPageSize()) to detect when size is the only difference. - Extend the spec suite to assert the new reduced-page-size warning behavior (17 → 19 cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/app/core/data/dspace-rest-response-parsing.service.ts | Adds page-size detection and returns a more accurate warning when the API reduces the requested page size, keeping generic warnings for other mismatches. |
| src/app/core/data/dspace-rest-response-parsing.service.spec.ts | Adds/adjusts test cases to verify the new warning message for reduced page sizes and fallback to the generic warning when other differences exist. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References
customer/jcuonlyDescription
ensureSelfLink()uses one wording for every self link that differs from the url it was requested with, and it 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 what its contract says, the caller asked for a bigger page than it will serve.Instructions for Reviewers
Why this comes after #1415: that PR normalized
embedparams and percent encoding on both sides and replaced the oversized page sizes withMAX_PAGE_SIZEat ten call sites. Now that nothing asks for an impossible page, a reduced page size means the caller was wrong, and the message can say so.Changes:
isUnexpectedSelfLink()becomesselfLinkWarning(), returning the message to log orundefined, plus thegetPageSize()/withoutPageSize()helpers._links.selfis unchanged, so caching is unaffected.Left out of the upstream commit: nothing. What differs is the file around it — upstream
mainuses@dspace/*aliases andinject(APP_CONFIG)and a TestBed spec, none of which exist on 7.6.5.How to test: temporarily change
elementsPerPage: MAX_PAGE_SIZEto9999infindByItemAndName()inbundle-data.service.ts(line 84) and open an item page with the console open. Before the change it blames the REST endpoint, after it names the requested and the served size. Revert afterwards.Checklist
dtq-dev(port of an upstream fix onto our 7.6.5 line).package.json); madge run directly reports none over 2995 files.Written with some help from Claude Code.