[Port to dtq-dev] fix: Keep the requested scope and configuration in the /discover/search self link - #1421
Merged
Merged
Conversation
…scover/search self link /api/discover/search built its self link from a SearchConfigurationRest that never received the scope and configuration the request carried, so the link described a different request than the one it answered. The frontend then logged a self-link mismatch warning for every scoped or configured search. - DiscoverConfigurationConverter: convert() now takes the requested configuration name and scope and sets them on SearchConfigurationRest, the way DiscoverFacetConfigurationConverter already does for /discover/facets. Both fields are @JsonIgnore, so only _links.self changes -- the JSON body is untouched. The class no longer implements DSpaceConverter and getModelClass() is gone, because the 4-arg signature does not fit that interface; nothing on dtq-dev resolves this converter through ConverterService (no toRest() or getConverter() call asks for DiscoveryConfiguration), so dropping the registry entry orphans nothing. - DiscoveryRestRepository.getSearchConfiguration(): pass both values through. - DiscoverConfigurationConverterTest: adapt the existing call sites and add testRequestedConfigurationAndScopeAreKept. Port of DSpace/DSpace PR 12984 (still open upstream), commit 6477dfe. Upstream tracking issue: DSpace/DSpace issue 8576. Adapted, not cherry-picked: upstream is on 11.0-SNAPSHOT and its first hunk rewrites the class header, where dtq-dev keeps an @Autowired ConfigurationService for the local sort.options.filtered handling. That field, the filtering block and the commons-lang v2 imports are left as they are. SearchConfigurationRest and SearchConfigurationResourceHalLinkFactory need no change -- 7.6 already has the @JsonIgnore scope/configuration fields and the link factory already reads them. Fixes dataquest-dev/dspace-customers#934 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…actually runs The upstream patch adds discoverSearchSelfLinkKeepsScopeAndConfigurationTest to DiscoveryRestControllerIT, but that class is @ignore'd at class level on dtq-dev (since eb40d60, unrelated facet-configuration failures), so the test would never execute here. - DiscoveryRestControllerIT: add the upstream test verbatim, at the same spot, so the file stays in step with upstream and future rebases stay clean. The class-level @ignore is left alone; lifting it is a separate job. - ClarinDiscoveryRestControllerIT: mirror the same test, in this file's continuation-indent style. This is the fork's live copy of the discovery ITs and it runs in CI, so this is where the fix is actually verified. The test asserts only on $._links.self.href, so it is independent of the fork's discovery.xml facet/sort customisations. 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 ports an upstream DSpace fix to ensure /api/discover/search returns a _links.self that accurately reflects the request’s scope and configuration query parameters, preventing the UI’s ensureSelfLink warning for scoped searches.
Changes:
- Propagates requested
scopeandconfigurationthroughDiscoveryRestRepositoryintoSearchConfigurationRestso HAL link generation can build an accurate self link. - Refactors
DiscoverConfigurationConverterto accept request context (configurationName,scope) and persist it on the REST model (both fields remain@JsonIgnore). - Adds integration + unit test coverage verifying the self link includes provided parameters and does not invent absent ones.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/DiscoverConfigurationConverter.java | Updates conversion to carry scope/configuration into SearchConfigurationRest for correct self link building. |
| dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/DiscoveryRestRepository.java | Passes request parameters into the converter so the REST model can reflect the actual request. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/converter/DiscoverConfigurationConverterTest.java | Updates converter unit tests for new signature and adds an assertion that scope/config are preserved. |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinDiscoveryRestControllerIT.java | Adds IT validating _links.self keeps the requested scope and configuration (dtq-dev runnable). |
| dspace-server-webapp/src/test/java/org/dspace/app/rest/DiscoveryRestControllerIT.java | Adds the same IT (but this class is currently @Ignore on dtq-dev). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1268
to
+1270
| @Test | ||
| public void discoverSearchSelfLinkKeepsScopeAndConfigurationTest() throws Exception { | ||
| context.turnOffAuthorisationSystem(); |
4 tasks
milanmajchrak
added a commit
that referenced
this pull request
Sep 9, 2026
…guration in the /discover/search self link (#1426) Source: f27d932 (dtq-dev PR #1421) /api/discover/search built its self link from a SearchConfigurationRest that never received the scope and configuration the request carried, so the link described a different request than the one it answered. The frontend then logged a self-link mismatch warning for every scoped or configured search. - DiscoverConfigurationConverter: convert() now takes the requested configuration name and scope and sets them on SearchConfigurationRest, the way DiscoverFacetConfigurationConverter already does for /discover/facets. Both fields are @JsonIgnore, so only _links.self changes -- the JSON body is untouched. The class no longer implements DSpaceConverter and getModelClass() is gone, because the 4-arg signature does not fit that interface; nothing on dtq-dev-9-base resolves this converter through ConverterService (no toRest() or getConverter() call asks for DiscoveryConfiguration), so dropping the registry entry orphans nothing. - DiscoveryRestRepository.getSearchConfiguration(): pass both values through. - DiscoverConfigurationConverterTest: adapt the existing call sites and add testRequestedConfigurationAndScopeAreKept (12 tests). - DiscoveryRestControllerIT: add discoverSearchSelfLinkKeepsScopeAndConfiguration Test. Unlike dtq-dev, this class is not @ignore'd on dtq-dev-9-base, so the test really runs in CI here. Both production files are byte-identical with vanilla 9.3 on this branch, so the two production hunks and the converter test applied verbatim. Deviation from the source commit: its hunk for dspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinDiscoveryRestControllerIT.java is dropped -- that class does not exist on dtq-dev-9-base (card X-01 tracks it). The same test is covered by DiscoveryRestControllerIT above, where it executes. Upstream: DSpace/DSpace PR 12984 (open), commit 6477dfe, issue 8576. Frontend half: cards FE-06 (d0897c2af7) and FE-01 (51796ec39a) -- the UI warning disappears only once those land. Card: BE-02. Fixes dataquest-dev/dspace-customers#934 (cherry picked from commit f27d932) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
/api/discover/searchleft thescopeandconfigurationit was called with out of its own self link, so the link described a different request than the one it answered. In the UI that shows up as anensureSelfLinkconsole warning on every scoped search.Instructions for Reviewers
Changes:
DiscoverConfigurationConverternow takes the requested configuration name and scope and sets them on theSearchConfigurationRest. It was the only converter in this family implementingDSpaceConverter, which passes just a model object and a projection, so there was nowhere for the request parameters to travel and the link factory built the self link out of two nulls.DiscoverFacetConfigurationConverteralready does it this way.DiscoveryRestRepository.getSearchConfigurationpasses both values through.ClarinDiscoveryRestControllerITas well, becauseDiscoveryRestControllerITcarries a class-level@Ignoreon dtq-dev and would never run it.Adapted, not cherry-picked: upstream's first hunk rewrites the class header, where dtq-dev keeps an
@Autowired ConfigurationServicefor the localsort.options.filteredhandling. That field and both sort blocks are preserved.Both fields are
@JsonIgnore, so only_links.selfchanges and the response body stays as it was.How to test:
Before, that returns
.../api/discover/search. After, it returns the URL that was requested. A request without parameters still gets a bare self link.Checklist
dtq-dev(port of an upstream fix onto our 7.6.5 line).DiscoverConfigurationConverterTest12/12 with a new case; the two ITs were not run here (no IT environment), left to CI.Written with some help from Claude Code.