UoE/WP2 bugfixes - #5
Merged
dspeed2 merged 24 commits intoJul 1, 2026
Merged
Conversation
The repository-wide "Total visits" usage report (shown on the site statistics page) was capped at the top 10 items via `usage-statistics.topItemsLimit`, so users could never see statistics for more than the first 10 datasets. Treat a non-positive `topItemsLimit` as "no limit" so the report returns every item, allowing the UI to paginate through all datasets. Integer.MAX_VALUE is used as the effective Solr facet limit because SolrLogger only applies a facet limit for positive values (a literal -1 falls back to Solr's default of 100). - UsageReportUtils#resolveGlobalUsageReport: map limit <= 0 to "all items". - usage-statistics.cfg: default topItemsLimit to -1 (all items) and document it. - StatisticsRestRepositoryIT: add a test asserting all visited items are returned when the limit is not positive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "download all" dataset zip names each entry after its bitstream's dc.title, so renaming a file in a zip bundle (ORIGINAL/CC-LICENSE/LICENSE) makes the on-disk zip stale even though the item's availability is unchanged. This was not handled: the dispatcher did not deliver Bitstream+Modify_Metadata to the DatashareConsumer, and a REST rename (replace /metadata/dc.title/0/value) edits the value object in place, so the MODIFY_METADATA event carries a null detail. - dspace.cfg: add Bitstream+Modify_Metadata to the datashare consumer filter. - DatashareConsumer: on a title change to a zip-bundle bitstream of an archived item, drop and regenerate the zip. Treat it as a rename when the event detail is absent (in-place value edit) or names the title field; ignore details naming only other fields (e.g. dc_description). - DatashareDatasetUploadIT (new): real event-dispatch regression tests for upload (DSpace#669), remove, rename, in-place rename (REST value-replace) and move-between-collections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- DatashareConsumer: match the changed metadata field exactly (split the comma-separated MODIFY_METADATA detail and compare tokens) so a field like dc_title_alternative no longer triggers an unnecessary zip regeneration. - DatashareDatasetUploadIT: wrap @after cleanup in try/finally so the shared authorization state and datasets.path are always restored, even if a dataset delete throws, avoiding order-dependent test leakage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the Javadoc of resolveItemWhoseZipFileRenamed accurate: an in-place metadata value replace (the REST replace-single-value path) flags the bitstream metadata modified without recording a detail for any field, so a null detail can occur for non-title edits too. Document that a null detail is conservatively treated as a possible rename (a real rename is never missed), at the cost of an occasional redundant - and, when no name changed, identical - regeneration for the rare in-place edit of a non-title field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removing a file left the zip stale: the REST delete (BitstreamRestRepository -> bitstreamService.delete) removes the bitstream from its bundle silently (no Bundle REMOVE event) and severs the bundle link, so the consumer could not see the fileset change nor resolve the owning item from the deleted bitstream. Also, the consumer only deleted (never regenerated) the zip on a fileset change, so an add appeared to "regenerate" (via the rename side effect) while a remove did not. - BitstreamRestRepository#delete: remove the bitstream from each owning bundle via bundleService.removeBitstream (fires Bundle REMOVE, updates the item), falling back to bitstreamService.delete for bundle-less bitstreams (logos). - DatashareConsumer: on a fileset add/remove of an archived item, delete AND regenerate (reconcile) the zip so it stays consistent for both adds and removes, instead of waiting for the ds-datasets batch. - Tests: DatashareDatasetUploadIT now asserts a removal regenerates the zip without the removed file; the two synthetic fileset tests in DatashareDatasetConsumerIT now assert regeneration (not just deletion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-up) The dc.date.embargo removal fix already lives on this branch (commit eef1748), but its test was committed directly without review. This hardens that test: - restore the auth system in finally blocks (init, destroy, both tests) so a failure can't leak auth-disabled state into other tests - always run super.destroy() in finally (fail() in @after raised an AssertionError that previously skipped it, leaking the Context) - capture the expected 'today' value immediately before liftEmbargo() to avoid a rare midnight-boundary flake on the dc.date.available assertion No production code changed. Addresses GitHub Copilot review feedback from PR #22. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…elete) The Angular UI deletes files via a bulk PATCH /api/core/bitstreams (remove ops) handled by BitstreamRemoveOperation, which called bitstreamService.delete directly - removing the bitstream from its bundle silently (no Bundle REMOVE event). So the DataShare consumer never saw the fileset change and the "download all" zip was left stale. The earlier fix only covered the single-resource DELETE endpoint (BitstreamRestRepository#delete), which the UI does not use. - BitstreamRemoveOperation: remove the bitstream from each owning bundle via bundleService.removeBitstream (fires Bundle REMOVE), falling back to direct delete for bundle-less bitstreams - mirroring BitstreamRestRepository#delete. - DatashareBitstreamDeleteIT (new webapp IT): a real bulk PATCH delete regenerates the zip without the removed file. Verified live in Docker: bulk PATCH remove of a file regenerates DS_*.zip without that file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Correct the comment on the topItemsLimit handling: SolrLogger only skips the Solr facet limit when the value is exactly -1 (falling back to Solr's default of 100), which is why "no limit" is mapped to Integer.MAX_VALUE; a large facet limit is safe because Solr only returns facet values that actually exist. - Document the performance/response-size tradeoff of non-positive values in usage-statistics.cfg. - Strengthen the IT to assert the report contains every expected item point (id/label/views) via UsageReportMatcher, not just the number of points. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "download all" zip names each entry after the bitstream's dc.title. When two bitstreams in an item share a name (e.g. the same file uploaded twice), ZipOutputStream.putNextEntry throws "duplicate entry", which aborted the whole zip - so the item ended up with NO zip after any add/remove/regenerate. This is what made it look like "adding a file doesn't generate the ZIP". - DatashareItemDataset.createZip: track used entry names across all zip bundles and disambiguate duplicates as "name (1).ext", "name (2).ext", ... (and fall back to "bitstream" for a null/blank name). - DatashareDatasetUploadIT: new test - an item with two files of the same name still produces a zip containing both (one disambiguated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hange Addresses review feedback: routing the REST bitstream-delete through bundleService.removeBitstream made a generic "delete one bitstream" operation look like it could cascade. Revert both REST delete paths (BitstreamRestRepository#delete and BitstreamRemoveOperation - the bulk PATCH the UI uses) to the standard bitstreamService.delete (deletes exactly that one bitstream, identical to upstream), and afterwards fire a single Bundle REMOVE event per former bundle so the DataShare consumer still regenerates the "download all" zip. The event only signals the fileset change - nothing else is deleted - and it is scoped to these two REST endpoints (not item/bundle deletion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gacy dataset row (DSpace#741) findLatestDatashareDatasetByItem ranked datasets by MAX(ddset.id), but ddset.id is the inherited DSpaceObject UUID (the numeric auto-increment "id" column is mapped to the legacyId property). So it selected the highest UUID, not the latest row. On a DSpace 6 -> 8 upgrade the 'dataset' table keeps legacy rows that have no matching 'dspaceobject' entry (the CREATE TABLE IF NOT EXISTS migration never adds the FK to the pre-existing table). When such a legacy UUID sorted highest, the JOINED-inheritance entity could not be materialized and the query threw NoResultException - swallowed upstream, so the item's "download all" zip link silently disappeared. Whether an item was affected depended purely on UUID byte-ordering, so it looked random. Rank valid datasets instead: the entity query already excludes the un-materializable legacy rows, and we pick the highest numeric legacy id (NULL on fresh installs, handled via nullsFirst). Adds DatashareDatasetLatestLookupIT, which reproduces the exact NoResultException via a dangling legacy row before the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y/finally in test - findLatestDatashareDatasetByItem: add UUID tiebreaker after legacyId so the selection is deterministic when datasets share a legacyId (NULL on fresh installs) instead of depending on DB row order. - DatashareDatasetLatestLookupIT: restore H2 referential integrity in a finally block so a failed insert can't leak a non-default state into later tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…invariant Critical-review follow-up: switch the legacyId comparator from nullsFirst to nullsLast so that, in the unlikely event an item has both an app-created dataset (legacyId NULL on fresh installs) and an older legacy row that carries a numeric id, the app-created row still wins. No behaviour change in real deployments (there is a single materializable dataset per item, and on upgraded DBs every materializable row has a non-null id), but it removes a theoretical foot-gun and documents the single-valid-row invariant the comparator relies on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The zip-file-link endpoint authorized the dataset download per bitstream, and each AuthorizeService.authorizeActionBoolean call ran several DB queries (parent-object lookup, isAnyItemInstalled workspace+workflow probes, and a per-policy group lookup). Cost was O(bitstreams) x several queries, recomputed on every dataset page view with no cache, so a many-file dataset produced hundreds-thousands of queries per view and saturated the backend under a handful of concurrent users (blank page in prod). Rework DatashareDatasetServiceImpl.isUserAuthorizedToDownloadZip to keep auth behaviour identical while removing the storm: - short-circuit at item level for administrators (they bypass policies); - resolve the user's full group membership once (allMemberGroupsSet, cached on the Context) and evaluate each bitstream's READ policies in memory, honouring embargo via isDateValid - no per-file workflow/workspace/parent probing; - cache the decision per (item, eperson) in a bounded, short-TTL Guava cache so repeated page views are O(1); the cache is bypassed when the request carries special (IP-based) groups so a session-specific result is never shared. Extend the unit tests (TDD) to cover the admin short-circuit, the per-(item, user) caching, single group-membership resolution regardless of file count and embargoed-policy denial. The existing DatashareDatasetRestControllerIT (real DB, end-to-end) is unchanged and still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A jstack of the zip-file-link endpoint under load (concurrency 15, a 200-file item) showed the per-user authorization storm was gone, but every thread was now stuck in the *other* per-file loop: findDatashareDatasetByItem -> areAllItemBitstreamsAvailable -> isZipContentAnonymouslyReadable -> getAuthorizedGroups, run (twice) per request and uncached. The endpoint still returned ~2,400 DB rows per request (p50 ~500ms). areAllItemBitstreamsAvailable is user-independent, so cache its result per item (same bounded, short-TTL Guava cache approach as the authorization decision). The whole endpoint is now O(1) on a warm cache. Both caches are invalidated for an item when its dataset zip is (re)generated or removed, so a fileset/policy change is reflected immediately instead of only after the TTL. Add a unit test (mockStatic) asserting the availability walk runs once per item across repeated downloadable checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on from cache loaders Guava's Cache.get(key, loader) wraps a runtime exception thrown by the loader in UncheckedExecutionException (only checked causes surface as ExecutionException). Catch both at the two cache sites so the loader's original cause is always re-thrown unchanged (SQLException as-is, the original RuntimeException as-is) instead of an unexpected wrapper escaping on an error path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer findings on the zip-file-link fix: - (major) For a non-installed (workspace/workflow/draft) item, AuthorizeService ignores custom bitstream policies (DS-2614); the new in-memory evaluation would instead honour them. Since the endpoint can be hit for such an item and the dataset zip only ever exists for archived items, deny early on !isArchived() (after the admin short-circuit, which the platform also honours regardless of install state). Keeps behaviour equivalent to the original for installed items. - (tests) Add coverage for the non-archived denial, the special-groups cache bypass, and cache invalidation on deleteDatasetForItem. - (nit) Rename the shared cache constants AUTHZ_CACHE_* -> ZIP_CACHE_* (they bound both caches) and document the <=TTL staleness bound for policy/group changes that don't flow through create/deleteDatasetForItem. - (nit) Log the wrapping exception (not its possibly-null cause) on the availability cache error path. Unit 16/16, DatashareDatasetRestControllerIT 5/5, checkstyle clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Halve the staleness window for access-policy / group-membership changes that don't flow through create/deleteDatasetForItem (e.g. a user removed from an authorized group, or any node other than the one that handled the change in a multi-node deployment). The extra cost is one O(files) recompute per item per 30s per node instead of per 60s - negligible, and Guava's get(key, loader) still collapses concurrent misses to a single computation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prefer freshness: cap staleness for access changes that don't flow through create/deleteDatasetForItem (e.g. group-membership changes, or other nodes in a multi-node deployment) at 10s. Still a rounding error vs the no-cache original (the O(files) authorization/availability recompute now runs at most once per item per 10s per node instead of on every page view); Guava get(key, loader) collapses concurrent misses to a single computation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fetchDatashareDatasetZipFileLink resolved the dataset twice per request: once via isDatashareDatasetZipFileDownloadable and again directly. Inline the authorization check and resolve findDatashareDatasetByItem a single time, halving the per-request DAO lookups (now ~1 cheap indexed query for the dataset on top of the item load). Also resolve the download URL once and drop noisy info logging. Behaviour unchanged (authorized + dataset present + file on disk -> link). Add a unit test pinning the single DAO resolution; unit 17/17, DatashareDatasetRestControllerIT 5/5, checkstyle clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…blank issued) Two edge cases produced schema-invalid OpenAIRE output (empty mandatory elements) that would fail OpenAIRE validation: - BUG 1: a virtual:: authority entity originating from dc.creator yielded an empty <datacite:creatorName>, because entity_creator matched the name only under dc.contributor.author*. buildEntityNode names the rebuilt field after its source path (dc.creator.* vs dc.contributor.author.*), so match both. - BUG 2: a present-but-blank dc.date.issued (node exists, value empty) made hasIssued true, suppressing the Issued fallback while the per-name issued template still emitted empty Accepted/Issued dates. Detect dates by first NON-EMPTY value (normalize-space) and only emit dates with a non-empty value; the parent template's fallback then derives Issued from available/accessioned. Also guards the generic date template against emitting empty <datacite:date>. Verified via offline Saxon-HE transforms: existing fixtures unchanged (no regression); both edge cases now produce valid, populated output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pins the DataShare customisations of oai_openaire.xsl so they cannot regress: - creators built from dc.creator as well as dc.contributor.author - Issued date derived from dc.date.available when dc.date.issued is absent - ISO timestamps trimmed to YYYY-MM-DD - BUG 1: dc.creator virtual:: entity yields a non-empty datacite:creatorName - BUG 2: a present-but-blank dc.date.issued no longer emits empty mandatory dates and the available-derived Issued fallback still fires Mirrors the existing AbstractXSLTest/RioxxXslTest harness (Saxon transform of an xoai-intermediate fixture, asserted via XPath). Four new fixtures under dspace-oai/src/test/resources. Run with: mvn -pl dspace-oai test -DskipUnitTests=false -Dtest=OpenaireXslTest Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Unit Tests CI job runs license:check (check-headers); the four new XOAI fixtures were missing the standard DSpace license header and failed the build. Header added via license:format. No functional change to the fixtures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix OpenAIRE 4.0 crosswalk: support dc.creator and dc.date.available/…
dspeed2
merged commit Jul 1, 2026
50550be
into
UoEMainLibrary:datashare-UoEMainLibrary-dspace-8_x
10 checks passed
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
2 participants
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.
This PR includes:
New feature:
Bug fixes: