Fix the causes of the REST self link console warnings - #1415
Merged
Conversation
ensureSelfLink() compares the requested url against the self link in the
response, but strips embed params from the requested url only. The REST API
echoes the request's embed params into the self link, so every request that
embeds a subresource looked broken and flooded the console:
The response for '.../bundles/<uuid>/bitstreams?page=0&size=5' has the self
link '.../bitstreams?page=0&embed=accessStatus&size=5'. These don't match.
The same warning also fired when the API clamped an oversized page size
(?size=9999 comes back as ?size=1000), which the REST contract mandates:
a size over the configured maximum is reset to the maximum, no error thrown.
Narrow the warning so it only fires on differences the API isn't expected to
introduce: strip embed params from both sides, and accept a page size that
shrank when the response's own page.size confirms the smaller value. A self
link that contradicts the payload it describes is still reported, as are
differing page/sort params, extra params, a size larger than requested, and a
missing self link.
The self-link normalization itself is left untouched. The response is keyed in
the object cache by that href, so which url it is normalized to stays exactly
as it was; only whether we warn changes.
Adds the first spec for this service - ensureSelfLink was untested.
Closes dataquest-dev/dspace-customers#862
Mutation testing of the new spec turned up one behaviour bug and a set of
assertions that were never actually pinned down.
isReducedPageSize() accepted any smaller page size the response's page block
confirmed, including zero: asking for size=10 and getting a self link claiming
size=0 with "page": {"size": 0} was silently swallowed. A configured maximum is
never zero, so an empty page is not a clamp - require the effective size to be
greater than zero and let that case be reported again.
Also drop a redundant hasValue(payload) - the caller already guarantees it - and
note in the doc comment that running the self link through
getUrlWithoutEmbedParams() drops a fragment and a trailing slash too, so
differences limited to those stop being reported as well.
Tests added for the gaps mutation testing exposed, each verified to fail against
the corresponding mutant:
- the page size rule tested on its own, without embed params also in play
- the exact text of the warning, not just a substring
- sibling _links surviving the self link being normalized
- `size` matched as a whole param, so `pagesize` isn't mistaken for it
- a page block confirming the size only as a string is not confirmation
- an empty page is reported
- a cross-origin self link is passed through untouched
- reordered params are neither reported nor rewritten
When isReducedPageSize() accepts a reduced page size, isUnexpectedSelfLink() re-compares the remaining parts rather than returning early, so a legitimate clamp can't mask a genuinely wrong page/sort. Nothing defended that: dropping the second urlPartsDiffer() call left all 22 tests green while silently swallowing the defect.
The remaining self link warning on the home page came from the usage statistics request: the frontend sends uri=http%3A%2F%2F... and the REST API echoes it back decoded as uri=http://... Same value, different representation - comparing the raw strings compares encodings, not values, which is the same kind of false alarm as the echoed embed params. Percent decode both sides before comparing. Decoding happens per part, after the url was split, so a decoded & can't merge two params, and a malformed sequence falls back to the raw part. A value that differs beyond its encoding still warns. With this, an item page, the home page and a search page all load with zero self link warnings against a DSpace 9.1 backend. Also trims the doc comments on these helpers down to what isn't already obvious from the code.
Seven call sites asked for 9999 or 10000 elements to mean "give me everything". The REST API caps a page at 1000 (Spring Data REST's spring.data.rest.max-page-size, which DSpace leaves at its default) and silently reduces anything larger, so those requests never returned more than 1000 anyway - they just claimed something the API does not honour, and the difference between the requested and the effective size is what showed up in the self link and produced the console warnings of #862: GET /core/items/<uuid>/bundles?embed=primaryBitstream&size=9999 self ...?embed=primaryBitstream&size=1000 Introduce MAX_PAGE_SIZE next to FindListOptions and use it instead. Verified against a DSpace 9.1 backend that this returns the identical page - same totalElements, same contents - while the self link now matches the request exactly: ?size=9999 -> self ?size=1000 page {size: 1000, totalElements: 1} ?size=1000 -> self ?size=1000 page {size: 1000, totalElements: 1} This is upstream issue DSpace#2513, whose fix (DSpace#3694) removed 9999 from seven components but missed BundleDataService.findByItemAndName - still on main, dspace-9_x, dtq-dev and dtq-dev-9-base. The three clarin-* call sites are ours.
The frontend sent uri=http%3A%2F%2F... and the REST API echoed it back decoded as uri=http://..., which made the self link differ from the requested url and produced the self link warning on the home page. RequestParam encodes by default; pass encodeValue=false, exactly as AuthorizationDataService already does for its own uri param (upstream DSpace#3045/DSpace#3046), and carry the same TODO noting this belongs in the backend.
…ible one Filtering the page size out of the self link comparison treated the symptom: the frontend went on asking for 9999, the API went on reducing it to 1000, and the code here just stopped saying so. With every call site now within MAX_PAGE_SIZE, a page size the API reduced means a caller asked for something it was never going to get - which is exactly what this warning exists to surface. Drops isReducedPageSize, getPageSizes and PAGE_SIZE_PARAM, and with them the need to pass the response payload into isUnexpectedSelfLink at all. What stays is the part that is a genuine comparison bug rather than a filter: embed params and percent encoding are two ways of writing the same request, so both sides are brought to the same form before being compared. A difference in an actual value is still reported. Spec goes from 25 cases to 17: the nine that pinned the page size filtering are gone, replaced by one asserting a reduced page size warns.
milanmajchrak
changed the title
Three CLARIN license components already asked for exactly 1000, so they never triggered the warning - but leaving the bare literal next to a newly introduced MAX_PAGE_SIZE just invites the question why one place names the limit and the other repeats it. Same value, no behaviour change; the constant now says where the number comes from.
An earlier commit here made UsageReportDataService skip encoding its uri param,
copying the workaround and the TODO that AuthorizationDataService carries:
// TODO fix encode the uri parameter in the self link in the backend and set
// encodeValue to true afterwards
Measured against a DSpace 9.1 backend, that TODO is based on a misreading - there
is nothing to fix in the backend. It does not decode the self link; it re-encodes
the parameter values minimally, and ':' and '/' are legal in a query component per
RFC 3986, so it has no reason to escape them:
sent probe=a%25b -> self probe=a%25b (not decoded)
sent probe=a%20b -> self probe=a%20b (not decoded)
sent probe=a+b -> self probe=a%20b ('+' is a space in form encoding)
sent uri=http%3A%2F%2Fx -> self uri=http://x
encodeURIComponent on the frontend simply encodes more than it has to. Both urls
are valid and denote the same value.
Since the comparison now decodes both sides, the encoding no longer matters, so
put the param back on the default: encoding is the safer choice for a value that
could contain '&' or '#'. Verified in a browser - the home page still logs no self
link warning with encoding restored.
The same workaround in AuthorizationDataService is left alone: it is upstream code,
it works either way, and it is out of scope here.
9 tasks
Collaborator
Author
There was a problem hiding this comment.
Pull request overview
This PR reduces noisy “self link mismatch” console warnings by aligning “fetch all” pagination requests with the REST API’s actual max page size and by making ensureSelfLink() treat embed/percent-encoding differences as equivalent while still warning on real mismatches. It also adds dedicated unit coverage for the response parsing behavior.
Changes:
- Introduce
MAX_PAGE_SIZE = 1000and replace ad-hoc large page sizes (9999/10000) across call sites. - Update
ensureSelfLink()comparison logic to ignoreembed/embed.sizeparams and percent-encoding differences when deciding whether to warn. - Add a new
dspace-rest-response-parsing.service.spec.tscoveringensureSelfLink()normalization and warning behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/core/data/find-list-options.model.ts | Adds MAX_PAGE_SIZE constant documenting the REST cap and its rationale. |
| src/app/core/data/dspace-rest-response-parsing.service.ts | Filters self-link warnings by normalizing/decoding before deciding if mismatches are meaningful. |
| src/app/core/data/dspace-rest-response-parsing.service.spec.ts | New unit tests covering multiple self-link normalization and warning scenarios. |
| src/app/core/data/bundle-data.service.ts | Uses MAX_PAGE_SIZE as the default “load all bundles” pagination size. |
| src/app/core/registry/registry.service.ts | Uses MAX_PAGE_SIZE for bulk schema retrieval pagination. |
| src/app/core/browse/browse.service.ts | Uses MAX_PAGE_SIZE when fetching browse definitions (temporary “get all” behavior). |
| src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.service.ts | Uses MAX_PAGE_SIZE for initial bundles pagination options. |
| src/app/item-page/edit-item-page/item-license-mapper/item-license-mapper.component.ts | Uses MAX_PAGE_SIZE when loading all licenses. |
| src/app/clarin-licenses/clarin-all-licenses-page/clarin-all-licenses-page.component.ts | Uses MAX_PAGE_SIZE when loading all licenses. |
| src/app/submission/sections/clarin-license-resource/section-license.component.ts | Uses MAX_PAGE_SIZE when loading all licenses in submission flow. |
| src/app/shared/clarin-item-box-view/clarin-item-box-view.component.ts | Uses MAX_PAGE_SIZE for bundle lookup pagination. |
| src/app/item-page/simple/field-components/clarin-item-versions-field/clarin-item-versions-field.component.ts | Aligns maximum fetched versions with MAX_PAGE_SIZE. |
| src/app/bitstream-page/clarin-zip-download-page/clarin-zip-download-page.component.ts | Uses MAX_PAGE_SIZE when fetching bitstreams for ZIP download. |
ensureSelfLink splits the requested url into parts before the comparison, then isUnexpectedSelfLink split the same string a second time. The branch runs on every response whose self link differs, which after this change is every embedded list response, so the duplicate work is not rare. Takes the parts instead. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
section-license.component.ts already imported FindListOptions from find-list-options.model, so adding MAX_PAGE_SIZE as a second import of the same module - under a different specifier - left the file importing one module twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9 tasks
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
Description
ensureSelfLink()floods the console with "…has the self link… These don't match" warnings. Threecauses: once the frontend asks for a page size the API can't serve (fixed at the source), twice the
comparison treats two spellings of the same url as a difference (fixed there).
Instructions for Reviewers
List of changes in this PR:
MAX_PAGE_SIZE(1000) next toFindListOptionsand used it instead of9999/10000in tencall sites. The REST API already capped these at 1000, so the data returned is identical — only the
request stops claiming something the API never honours. This is upstream Components should never send requests with a page size of
9999as this may bypass pagination DSpace/dspace-angular#2513, whose fix (Prevent request with page size of 9999 DSpace/dspace-angular#3694)missed
BundleDataService.findByItemAndName.ensureSelfLinknow stripsembedparams and percent-decodes both sides before comparing.Those are two ways of writing the same request; a difference in a real value still warns. This also
removes the need for the
encodeValue=falseworkaround the frontend carries foruriparams: thebackend does not decode self links, it re-encodes values minimally, and
:and/are legal in aquery per RFC 3986 —
encodeURIComponentsimply encodes more than it has to.dspace-rest-response-parsing.service.spec.ts(17 cases) — this service had no spec at all,here or upstream.
How to test: open an item page, the home page and
/browse/titlewith DevTools open. Ondtq-deveach logs a self-link warning; on this branch none do. Measured on the same item against alocal DSpace 9.1: 3 warnings → 0. Full suite 5484 passing.
Checklist
mainbranch of code — no: fork PR, base isdtq-dev. Apatch for the upstream-only files, rebased on vanilla
main, is prepared separately.yarn lint.madge; thecheck-circ-depsscript itself can't run on Windows — pre-existing quoting bug).MAX_PAGE_SIZEis documented at its definition.