Scope testScanSearch to its own scans instead of a global count - #5
Open
dmaffitt wants to merge 1 commit into
Open
Scope testScanSearch to its own scans instead of a global count#5dmaffitt wants to merge 1 commit into
dmaffitt wants to merge 1 commit into
Conversation
testScanSearch asserted the xnat:mrScanData advanced-search result size equals either getScanCountFromDataApi() (the /experiments session listing) or the test's own scanIds size. That fails on a shared or long-lived server because the two endpoints enforce read permission at different levels: the session listing surfaces a scan whenever the SESSION is readable, while the stored search requires read on the SCAN element (xnat:mrScanData) itself. Leftover projects from custom-permission tests leave the main user in groups that grant session-read but not scan-read, so the listing counts scans the search legitimately excludes (also non-MR scans inside MR sessions). The global counts are thus incommensurable and the assertion is fragile to unrelated pre-existing data. Replace the global count-equality with a check that, restricted to the scans this test created, the search returns exactly them -- robust to any leftover data while still verifying the search finds this test's scans. Verified: testScanSearch passes against a live long-lived server where the old assertion failed. Note: this removes the only caller of getScanCountFromDataApi(); the helper is left in place because a separate PR reworks the data-API count helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dmaffitt
marked this pull request as ready for review
August 10, 2026 19:23
rherrick
approved these changes
Aug 11, 2026
ian-xnatworks
approved these changes
Aug 11, 2026
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.
Problem
testScanSearchfails on a shared / long-lived server (observed against a live instance) at:It compares the
xnat:mrScanDataadvanced-search result size against the/experimentssession listing count — but the two enforce read permission at different levels:/experiments?columns=xnat:mrSessionData/scans/scan/IDsurfaces a scan whenever the session is readable.xnat:mrScanDatastored search requires read on the scan element itself.On a long-lived server, leftover projects from custom-permission test classes leave
mainin groups that grantxnat:mrSessionDataread but notxnat:mrScanDataread. Those scans appear in the session listing but are (correctly) excluded from the scan search. Non-MR scans (e.g.xnat:petScanData) inside MR sessions add to the gap. So the two global counts are incommensurable, and neither equalsscanIds.size()once unrelated data is present.Investigation on a live server: search returned 44 scans, listing 53; the 9-scan gap was 2
petScanData+ 7mrScanDatain 7 residual custom-permission projects (session-read, not scan-read). Not a product bug and not a regression — the endpoints legitimately differ; the assertion just isn't isolated.Fix
Restrict the result set to the scans this test created and assert the search returns exactly them:
This still verifies the search finds this test's scans (and returns each exactly once) while being robust to any unrelated pre-existing data. Verified:
testScanSearchnow passes against the live long-lived server where the old assertion failed.Note on overlap
This removes the only caller of
getScanCountFromDataApi(). The helper is intentionally left in place here because a separate PR (fix/advanced-search-distinct-count) reworks the data-API count helpers; whichever of the two lands second needs only a trivial rebase. No functional overlap between the two diffs.🤖 Generated with Claude Code