Enhance Metadata Export Subsystem, Part 1 by poikilotherm · Pull Request #12688 · IQSS/dataverse

poikilotherm · 2026-09-10T13:56:56Z

What this PR does / why we need it:
This PR will moves the needle in making the exporting subsystem follow our internal code conventions and good practices.
By splitting the existing "service" into distinctive parts (service, registry, pipeline, invalidators), the architecture is more modular, easier to comprehend and most important: testable.

Which issue(s) this PR closes:

Closes Feature Request: Refactor the export subsystem to support per-version exports, better comprehension, and testability. #12686

Special notes for your reviewer:
This is only the first part of a series of pull requests around making the export subsystem more efficient, testable and improve the code quality. It also prepares for adding features @johannes-darms pays me for (bulk export).

Should the InternalExportDataProvider read the DataCite export from the cache? This PR adds dependency resolution and ordering. We could make the Datacite export format a dependency for all exporters, thus making sure we'd always read it fresh. This would also allow the export to receive Datacite export for the correct version. The current mechanism merges data from the version and the last released, which may cause inconsistencies.

I beg to differ about "only" 70% coverage, failing to meet the Sonarcloud quality gate. Most of ye olde' code I had to touch to make use of the refactored services never had checks in the first place or are only covered by our API tests, which is not reflected in the percentage. IMHO it's not my job to also cover all the classes aside from the exporter subsystem (which is covered now with extensive new unit tests). Within the export subsystem some code was only moved around and isn't new (e. g. the InternalExportDataProvider)

Suggestions on how to test this:
There are extensive new tests added, which will be picked up automatically. If you have more suggestions what else should be tested (integration or API test), let me know.

(For exampe, there never were any API tests around the DDI format & embargoes...)

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.

Is there a release notes update needed for this change?:
🔋 included

Additional documentation:
None so far

…istryBean #12686 - Moved exporter management logic into a dedicated `ExporterRegistryBean` singleton for improved modularity and maintainability. - Simplified `ExportService` to delegate exporter logic to the new registry.

- Enable injectingthe registry and other components - The export process itself is stateless. State is involved in potential write locks, the loaded plugins, etc. - A stateless coordinator bean scales better for multiple export requests coming in.

…alidator, and storage abstraction #12686 The goal is removing the caching logic from the ExportService. At the same time, a distinct caching subsystem shall have policies about what gets cached, when it expires etc, all independent of a coordinating service like ExportService. This make cognitive loader smaller and allows extension without using more code branches.

…e into FileEmbargoExpiryInvalidator #12686

…eIOCache class #12686 - Reorganized export cache handling into a dedicated `StorageIOCache` service, improving modularity and reducing cognitive load in `ExportService`. - Streamlined caching operations with a unified approach across all storage drivers. - Deprecated legacy unversioned cache keys; introduced versioned aux tag schema for better cache qualification. - Enhanced write atomicity and cache eviction logic. - Remove stale code for size of exports

…OCache #12686 The legacy reading of cached exports is prone to produce bugs in production. When we rely on reading cached exports as prerequisites for other metadata formats, we might end up with stale data. Any export has no knowledge about whether and when an export of another format happened. We keep no provenance per format. Assuming there is a cached "latest" with the legacy file format, it would be read as a prerequisite format, but our invalidation mechanisms would not be able to tell if it's actually stale, because it was not yet re-exported. Any released version is immutable, thus if we rely in lookups on cached objects with the version present in the aux tag, we can be sure we get the latest data.

…constructor #12686 - Added null and blank checks for dataset, version, and formatName to ensure robust usage. - Introduced a convenience constructor for creating cache keys directly from a dataset version and format.

…rvice` package and rename `ExportService` to `ExportServiceBean` #12686 - "ExportServiceBean" is more aligned with the codebase style where EJBs mostly have a "Bean" name suffix. - Also move test classes into the same package (under the test source tree)

- Documented `tryRead`, `deleteQuietly`, and `storageFor` with proper Javadoc. - Clarified the stream-closing intent in `write` to make the leak-avoidance pattern explicit.

…to ExportServiceBean #12686 - Relocated the `invalidators` collection from the sealed interface to the service bean, where it logically belongs as a runtime dependency rather than a static on the contract. - Added a section marker for export data retrieval methods in `ExportServiceBean`. - Noted future plan to replace the static list with a registry pattern once plugins can supply their own invalidation logic.

…tServiceBean 12686 Making it simpler to read inline.

Added `ExportCache` as an CDI (not EJB) injected dependency in the service bean.

- Introduced `clearCachedFormats(DatasetVersion, List<String>)` as the version-specific clearing entry point, with the dataset-level overload delegating via a new `defaultVersion()` helper. - Added `clearCachedFormat(DatasetVersion, String)` to evict a single cache entry by key. - Added `requireExists` and `requireAllExist` validation methods to `ExporterRegistryBean` so format names are checked before eviction.

…12686 Align the related methods into one block, not divided by the cache handling stuff.

…istryBean #12686 - Added `buildFormatRequiredByMap` to build a read-only map of prerequisite format names to the exporters that depend on them. - Added `buildAndVerifyRequirements` to validate registry integrity: all prerequisite formats must have a registered exporter, and no cyclic prerequisite chains may exist. - Integrated the check into initialization as Step 4, failing fast with `ExportException` on any integrity violation (missing prerequisite or cycle).

Added `formatRequiredBy` field to store the prerequisite format dependency map alongside the exporters map, populated during registry initialization. Will be reused during cascaded cache eviction or exporting of formats depending on a certain format.

- Added `buildPrerequisitesChainDepth` to compute the prerequisite chain depth for each format (0 = no prerequisite, N = N levels deep). - Added `buildTopologicalComparator` to create an immutable comparator ordering exporters by depth, with format name as tiebreaker for deterministic results. - Exposed via `getTopologicalComparator()` so callers can sort the exporter list in a dependency-safe order. - Integrated as Step 5 in initialization, stored alongside the existing `formatRequiredBy` map.

…#12686 - Added `SecureTempFiles` utility that creates temp files with `0600` permissions on POSIX systems; on Windows it relies on the per-user `%TEMP%` ACLs. - Replaced raw `Files.createTempFile` in `StorageIOCache.write` with `SecureTempFiles.createOwnerOnlyTempFile` so other local users can no longer read or tamper with export temp files.

The cache key should not be responsible to carry the information about the "where" of an export, just about the "what". Changing dependent methods accordingly. Also, fixed ambiguity with the cache invalidator implementations: the invalidator should look for stale *versions* of dataset, not for the dataset as a whole being stale. The cache is treating versions individually, so they shall get stale individually, too. - Reduced `ExportCacheKey` to a single `auxTag` string, removing `Dataset`/`DatasetVersion` references for thread-safety and GC-friendliness. - Moved `TAG_PREFIX`/`TAG_SUFFIX` into `ExportCacheKey` as public constants. - Added explicit `Dataset` parameter to all `ExportCache` methods (`read`, `write`, `evict`) since the key no longer carries storage context. - Added explicit `DatasetVersion` parameter to `ExportCacheInvalidator.isStale`; updated `FileEmbargoExpiryInvalidator` with null-checks and released/archived status guard. - Updated `StorageIOCache` logging to use `dataset.getId()` instead of the version string.

…ndents set #12686 - Renamed `formatRequiredBy` to `transitiveDependents`, changing the value type from `List<String>` to `Set<String>` to capture all direct and transitive dependents per format. - Replaced `buildPrerequisitesChainDepth` with `buildTransitiveDependents`, which walks each exporter's prerequisite chain and registers it as a dependent of every ancestor format. - Updated `buildTopologicalComparator` to sort by new dependent-set - Merged `buildFormatRequiredByMap` into `verifyRequirements` as the former map is no longer stored for reuse - Moved `getFormatsDependingOn` to `getTransitiveDependents` to reflect the new semantics

…12686 - Introduced sealed `Details` interface exposing `localizedDisplayName`, `formatName`, `mediaType`, `isHarvestable`, and `isAvailableToUsers`, thus avoiding having to retrieve these details from the exporter, saving a roundtrip. - Made `ExporterDetails` record package-private to prevent external instantiation while allowing consumers to read via the interface. - Renamed `getLabels()` to `getDetails()`, returning `List<Details>` with the expanded field set. - Added `get(Details)` lookup method to resolve an exporter by its details object. These can only be created and handed out by the registry, thus we can be sure a matching exporter exists. - Removed unused `Collections` import.

…ability #12686

…tCacheKey components #12686 - Replaced the single `auxTag` field with `formatName` and `friendlyVersion` so the key exposes its meaningful parts directly. - Moved `auxTag()` from a static factory into an instance method derived from the record's fields. - Split validation into `checkFormatName` and `checkVersion` private helpers for clearer intent (and compatibility with the constructor needing to be called first thing).

…ServiceBean #12686 These methods (`getExporter`, `isXMLFormat`, `getMediaType`) directly exposed the internal `exporterMap` and are no longer needed now that format details are resolved via the `Details` interface in the registry.

…#12686 Added null check in `get(String formatName)` to return `Optional.empty()` instead of throwing NPE when the underlying Map implementation does not permit null keys.

…ation - Tracks consecutive failures via an `AtomicInteger` streak; escalates from `FINE` to `WARNING` once the streak reaches the configured threshold. - A success resets the streak; a threshold of zero or negative deactivates escalation entirely. - Thread-safe and suitable for sharing across concurrent callers or use in `ConcurrentHashMap` contexts. - Warnings will not be flooding the log once threshold is reached via configurable repeat cycle. - To enable "all clear" messages once the threshold was met, the success recording may then return the number of failures. Using OptionalInt, the logging statement is a one-liner.

…vadoc #12686 - Clarified that the legacy unqualified name is ignored for read/write cycles and only purged via `evictAll`, rather than being a read fallback. - Fix typos

…2686 - Replace static `FINE`-level logging in `tryRead` and `deleteQuietly` with threshold-based escalation via `FailureEscalation` instances (threshold: 256). - Log a recovery warning once consecutive failures drop below the threshold after previously exceeding it. - Include the current failure streak in the read-path log message for operational context.

…EJB #12686 - Introduces a `@Stateless` EJB that funnels all export data production (draft, cached, bulk) through a single path for uniform staleness validation, prerequisite resolution, and error wrapping. - Cached reads consult registered `ExportCacheInvalidator` instances; stale entries are evicted and reported as a miss. - Prerequisite formats are resolved recursively with circular-chain detection via an in-flight `LinkedHashSet`. - Non-cacheable (draft) versions are produced to `SecureTempFiles` with `DELETE_ON_CLOSE` to avoid in-memory retention of large exports. - `IllegalStateException` from exporters is wrapped in `ExportException` with dataset context for consistent reporting across all production paths.

…12686 - Injecting `ExportPipelineBean` as an `@EJB` - Removed the static `invalidators` list and its associated Javadoc from `ExportServiceBean` - they are now owned by the pipeline.

johannes-darms · 2026-09-18T07:13:53Z

+
+    /** The one canonical, version-qualified aux tag. */
+    public String auxTag() {
+        return TAG_PREFIX + formatName + "_" + friendlyVersion + TAG_SUFFIX;


We might add a discriminator to the filenames for datasets and data files. This would make bulk S3 operations on exports easier.

Right. But for now, we only support exporting dataset versions (and not even fully with proper tracking and upfront-processing optimizations). For file metadata, we are probably gonna rely on using StorageIO support to upload auxfiles for files, not dataset aux files. Thus, let's revisit this when we get there.

johannes-darms · 2026-09-18T07:19:29Z

-            }
-        }
-        return retList;
+        String urlTemplate = getDataverseSiteUrl() + "/api/datasets/export?exporter=%s&persistentId=%s";


URLs are generated at multiple locations, would be nice to unify that or at least use a constant to create the URLS consistently. Location one.

I've been thinking about possibilities to construct URLs from REST API Java method references, but so far I haven't invested more research. I completely agree that this kind of hard coded URL is brittle.

johannes-darms · 2026-09-18T07:19:39Z

-        }
-        return retList;  
+    public List<String[]> getExporters(){
+        String urlTemplate = systemConfig.getDataverseSiteUrl() + "/api/datasets/export?exporter=%s&persistentId=%s";


URLs are generated at multiple locations, would be nice to unify that or at least use a constant to create the URLS consistently. Location two.

I've been thinking about possibilities to construct URLs from REST API Java method references, but so far I haven't invested more research. I completely agree that this kind of hard coded URL is brittle.

johannes-darms · 2026-09-18T07:21:05Z

+        String describedByTemplate = "<%s>;rel=\"describedby\";type=\"%s\"";
+
+        StringBuilder describedBy = new StringBuilder();
+        describedBy.append(describedByTemplate.formatted(ds.getGlobalId().asURL(), "application/vnd.citationstyles.csl+json"));


Shouldn't be part of this PR, but for another follow up one. The CSL Export feature should be moved to the Exporter package and make use of the available infrastructure.

johannes-darms · 2026-09-18T07:26:33Z

Hey Oliver, this one looks good to me. I added some comments but those are rather minor suggestions to improve it. However, I've just scrolled through code...

sonarqubecloud · 2026-09-24T20:34:01Z

Quality Gate failed

Failed conditions
70.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

github-actions · 2026-09-24T20:44:38Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:12686-enhance-export-subsys

ghcr.io/gdcc/configbaker:12686-enhance-export-subsys

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

poikilotherm · 2026-09-28T09:19:48Z

@landreev @vera @johannes-darms I added a question for the reviewer in the description. I thought about this while looking into exporting as CSL and noticed there is room for error and improvement (with a rather easy fix).

vera

Overall, looks good to me 👍 Tested the export API with both internal exporters and an external exporter, and didn't notice any major issues. Left some comments in the code

vera · 2026-09-28T11:37:37Z

+        loader.forEach(exp -> {
+            String formatName = exp.getFormatName();
+            // If no entry for this providerName yet or if it is an external exporter
+            if (!exporters.containsKey(formatName) || exp.getClass().getClassLoader().equals(this.exporterClassLoader)) {


Should this check loadedExporters instead of exporters to avoid an internal exporter overwriting an already loaded external exporter?

vera · 2026-09-28T11:56:17Z

+        try {
+            ExportCacheKey key = new ExportCacheKey(datasetVersion, formatName);
+            return pipeline.readFreshCachedExport(datasetVersion, key)
+                           .orElse(pipeline.readFreshExport(datasetVersion, formatName));


Use orElseGet to ensure readFreshExport is only called when no cached export is available?

vera · 2026-09-28T12:11:44Z

+                return error(BAD_REQUEST, "Export failed: no exporter registered for format name " + exporter);
+            }
+
+            InputStream is = exportSvc.getExport(datasetVersion, exporter);


Previously, calling this API for an uncached export format would create the export, write it to cache, then return it. The "write to cache" step is no longer done. Is this intentional?

There's also one exception: prerequisite formats are still written to cache, e.g., when I have no cached export files, then I request the html export via API, the prerequisite ddi format is written to cache, while html is not.

vera · 2026-09-28T12:20:17Z

+        StringBuilder describedBy = new StringBuilder();
+        describedBy.append(describedByTemplate.formatted(ds.getGlobalId().asURL(), "application/vnd.citationstyles.csl+json"));
+        exporterRegistry.getDetails()
+            .forEach(detail -> describedBy.append(


Missing separating commas between the export formats:

vera · 2026-09-28T12:26:26Z

@@ -285,10 +287,13 @@ public Response exportDataset(@Context ContainerRequestContext crc, @Parameter(d
                    return error(BAD_REQUEST, "Non-draft version requested (" + versionId + ") but for published versions only the latest (" + DS_VERSION_LATEST_PUBLISHED + ") is supported.");


Just for my understanding, it would now be possible to remove this restriction and request an export for any published version?

vera · 2026-09-28T12:32:07Z

+                build();
        } catch (Exception wr) {
            logger.warning(wr.getMessage());
            return error(Response.Status.FORBIDDEN, "Export Failed");


With the new ExportSystemException setup, we could return more specific error messages instead of 403 "Export Failed", and distinguish 4xx and 5xx errors for other export-related error cases too, right?

(By "other error cases", I mean everything besides the "format doesn't exist" case, which is handled separately above)

poikilotherm added 30 commits September 10, 2026 15:47

refactor(export): extract file embargo expiry logic from ExportServic…

8a7a5e4

…e into FileEmbargoExpiryInvalidator #12686

docs(export): add Javadoc to private helpers in StorageIOCache #12686

206c07e

- Documented `tryRead`, `deleteQuietly`, and `storageFor` with proper Javadoc. - Clarified the stream-closing intent in `write` to make the leak-avoidance pattern explicit.

style(export): rename exporterRegistry field to registry in Expor…

60d953f

…tServiceBean 12686 Making it simpler to read inline.

feat(export): make ExportCache instance available in service #12686

9bbec86

Added `ExportCache` as an CDI (not EJB) injected dependency in the service bean.

style(export): move export trigger service methods next to each other #…

beefa7b

…12686 Align the related methods into one block, not divided by the cache handling stuff.

style(export): convert field comments to block comment style for read…

b59f4af

…ability #12686

fix(export): guard against null formatName in ExporterRegistryBean#get …

cfc60d9

…#12686 Added null check in `get(String formatName)` to return `Optional.empty()` instead of throwing NPE when the underlying Map implementation does not permit null keys.

docs(export): correct legacy cache name behavior in StorageIOCache Ja…

a4446ec

…vadoc #12686 - Clarified that the legacy unqualified name is ignored for read/write cycles and only purged via `evictAll`, rather than being a read fallback. - Fix typos

refactor(export): introduce ExportPipelineBean to ExportServiceBean #…

8a49f74

…12686 - Injecting `ExportPipelineBean` as an `@EJB` - Removed the static `invalidators` list and its associated Javadoc from `ExportServiceBean` - they are now owned by the pipeline.

poikilotherm added 2 commits September 17, 2026 18:22

doc(export): add release notes snippet for #12686

d25615a

Merge branch 'develop' into 12686-enhance-export-subsys

4ec9683

poikilotherm marked this pull request as ready for review September 17, 2026 16:25

poikilotherm requested review from landreev, pdurbin, qqmyers and vera September 17, 2026 16:30

poikilotherm moved this from In Progress 💻 to Ready for Review ⏩ in IQSS Dataverse Project Sep 17, 2026

This comment has been minimized.

Sign in to view

johannes-darms reviewed Sep 18, 2026

View reviewed changes

Merge branch 'develop' into 12686-enhance-export-subsys

18139d4

This comment has been minimized.

Sign in to view

landreev moved this from Ready for Review ⏩ to In Review 🔎 in IQSS Dataverse Project Sep 21, 2026

poikilotherm mentioned this pull request Sep 22, 2026

Feature: Bulk Metadata Exports (Export Subsystem Enhancement Part 2) #12737

Draft

Merge branch 'develop' into 12686-enhance-export-subsys

417b33c

This comment has been minimized.

Sign in to view

cmbz added FY27 Sprint 6 FY27 Sprint 6 (2026-09-09 - 2026-09-23) FY27 Sprint 7 FY27 Sprint 7 (2026-09-23 - 2026-10-07) labels Sep 23, 2026

Merge branch 'develop' into 12686-enhance-export-subsys

7474428

vera reviewed Sep 28, 2026

View reviewed changes

landreev mentioned this pull request Sep 28, 2026

A local issue for tracking development issues in the main queue that are crucial for the operations of HDV IQSS/dataverse.harvard.edu#401

Open

23 tasks

		@@ -285,10 +287,13 @@ public Response exportDataset(@Context ContainerRequestContext crc, @Parameter(d
		return error(BAD_REQUEST, "Non-draft version requested (" + versionId + ") but for published versions only the latest (" + DS_VERSION_LATEST_PUBLISHED + ") is supported.");

Conversation

poikilotherm commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poikilotherm Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johannes-darms commented Sep 18, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

sonarqubecloud Bot commented Sep 24, 2026

Quality Gate failed

Uh oh!

github-actions Bot commented Sep 24, 2026

Uh oh!

poikilotherm commented Sep 28, 2026

Uh oh!

vera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vera Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

poikilotherm commented Sep 10, 2026 •

edited

Loading

poikilotherm Sep 18, 2026 •

edited

Loading

vera Sep 28, 2026 •

edited

Loading