Refactor/clean up the SPI for the release 2.1.0

[landreev]

Per discussion with @poikilotherm, opening the first, inaugural issue for some refactoring work prior to the next release being published.
Oliver has already started a branch: main...refactor-for-2.1-release
I will copy-and-paste here the initial discussion on slack. Some potential issues/questions were raised back in the main repo, in the PR where the first iteration of the spi extensions was added: IQSS/dataverse#11767 (comment)

• Make new Context immutable
• Have a complete builder for the Context
• Make the data provider accept a single context object, but keep backward compatibility for using no-arg method from before
• Think about compatibility negotiations for the context with the core: how can an exporter plugin avoid trying to use context options the core doesn't understand? How do we avoid loading an exporter plugin that was compiled against an older version of the SPI than the core uses/supports?
  • Idea: let's have a VERSION constant in each interface (exporter and data provider). We increase on any backward incompatible changes (to any interface or the context). In the exporter, we add a default method to ask for the version compatibility, returning it from the class (lookup by constant). The first time we introduce a breaking change to the API (delete methods), we replace the default by forcing everyone to implement. In the core, we check the compatibility and can safely catch any incompatible plugins that still use the old contract. This way we introduce the version level without actually breaking anything now (just mark it deprecated, tell people to implement). Note: this MUST be a constant (public static final) to be put into bytecode AT COMPILE TIME of the plugin. Must not be a method or retrieving from the plugin will have version creep!

  • public interface Exporter {
        /**
         * Version of the Exporter interface contract.
         * Increment when the Exporter interface changes incompatibly.
         */
        int VERSION = 1;
        
        /**
         * Returns the Exporter interface version this plugin was built against.
         */
        default int getExporterInterfaceVersion() {
            return VERSION;  // Inlined at compile time
        }
        
        /**
         * Returns the minimum ExportDataProvider version required.
         */
        default int getMinimumProviderVersion() {
            return ExportDataProvider.VERSION;  // Inlined at compile time
        }
        
        // ... interface methods
    }
    
    public interface ExportDataProvider {
        /**
         * Version of the ExportDataProvider interface contract.
         * Increment when the provider interface or ExportDataContext changes incompatibly.
         */
        int VERSION = 1;
        
        // ... interface methods
    }

  • Why two versions? The SPI has two distinct contract directions:
    • Exporter interface — What core expects from plugins. Core calls methods on the plugin's Exporter implementation. If this contract changes (methods added, removed, or signatures changed), core and plugin must agree on the same version.
    • ExportDataProvider interface — What plugins expect from core. Plugins call methods on the ExportDataProvider that core provides. If this contract changes, plugins built against an older version may still work (backward compatible), but plugins requiring newer features need a minimum version. The ExportDataContext falls under the same contract (an exporter asks the core within the context), so there is no need for a third version.
  • This will and cannot possibly solve the problem of any releases before 6.10 that try to load a plugin with a breaking change. Maybe that's alright, we will just need to have people document this thoroughly. Also, we probably will not see breaking changes soon, so time might be on our side and everybody is on a new enough release by then.
• Think about communicating feedback from the core back to the plugin code: do we keep the exception? Do we want other ways to interact? More explicit exceptions? Capabilities to ask from the provider? A wrapper or hint objects?
• What about pagination with offset / length? What are we actually trying to do here? Some other pattern may be better if we want to iterate through things in chunks (use streaming or a cursor approach), but for one-off stuff to truncate the existing thing is probably fine.

[landreev]

Combining the following question asked on slack initially:

J) I'm not entirely sure about the pagination implementation in general. It introduces state into the data provider. Who is responsible of keeping track? What happens if the metadata changes between iterations? (This is of course only a potential problem for draft versions) What are we trying to solve here? Save some DB queries?

... and one from the checklist in the opening comment in the issue:

What about pagination with offset / length? What are we actually trying to do here? Some other pattern may be better if we want to iterate through things in chunks (use streaming or a cursor approach), but for one-off stuff to truncate the existing thing is probably fine.

First, to answer "What are we trying to solve here? Save some DB queries?"

In the current implementations, exporters that need data- or data variable-level metadata (namely, full DDI and Croissant) obtain it by calling ExportDataProvider.getDatasetFileDetails() which returns ALL of it - a dataset-worth - as one JsonArray. We have some prod. datasets with Ks of ingested tabular files ("datatables") and/or 100Ks of datavariables combined. This setup is no longer working for such cases, making these datasets unexportable.
The pagination solves this problem by obtaining this datavariable information in manageable chunks.
The new dataverse-spi implementation allows to retrieve offset-length tabfiles-worth of datavariables at a time. Note that using some fixed number of tabfiles at a time for pagination does improve things for these monster datasets, but is not really a good solution, since the same number of tabfiles can contain vastly different numbers of variables. So the new working implementation I have is using the following approach: it first obtains the (relatively cheap) dataset- and files-level metadata - that includes a sorted list of ingested datatables with the corresponding numbers of variables in each one. Which it then uses to request the variable information in variable-length chunks, such as not to exceed some sensible number of datavariables at a time.

Having answered that, I can answer the next part of the question:

What happens if the metadata changes between iterations? (This is of course only a potential problem for draft versions)

While the primary use case is the export in the traditional Dataverse sense - i.e., of published dataset versions - we now have support for producing Croissant for drafts as well. So, this may be a real problem. However, this is addressed by the implementation described above: The exporter obtains the list of the existing datatables early on, complete with the ids and the expected numbers of datavariables in each one. If it registers a mismatch while retrieving the actual datatables further down, it will have a chance to throw an exception and fail the export.

[poikilotherm]

Then the problem we are trying to solve specifically concerns methods relating to files:

1. dataverse-spi/src/main/java/io/gdcc/spi/export/ExportDataProvider.java

   Line 122 in 0947cc0

   JsonArray getDatasetFileDetails(ExportDataContext context);

   
   

   dataverse-spi/src/main/java/io/gdcc/spi/export/ExportDataProvider.java

   Line 157 in 0947cc0

   JsonArray getTabularDataDetails(ExportDataContext context);

I am somewhat torn about the inclusion of pagination in the ExportDataContext class. The 'context' encompasses very different things that way, which makes it difficult to understand. Furthermore, while some of the methods support pagination, it is not used by most and tends to be distracting when trying to understand how the context is to be used.

I would like to suggest a few improvements:

1. Rather than extracting the details for batch planning from the JsonObject, it would be much more convenient, reliable and self-explanatory to use a value object called FileSummary (or similar), retrievable from the provider as List<FileSummary> getDatasetFileSummary(context). This object can hold all the file metadata that is important for batch exporting, and thus becomes part of the API contract between core and the plugin. We can also reuse it later for exporting file-level metadata. (After all, we need a way to retrieve a metadata export for a single file when unifying the citation and exporter infrastructures.)
2. If we serve the file information a different way, we can change the provider contract to not include the files in the JsonObject when asking for the dataset metadata. This should make these objects much smaller and easier to handle. The original version without context (in my branch it's a simple default now) may still serve the original version that includes the files to keep it backward compatible for now and allow folks to upgrade.
3. Move pagination out of the context and add it to the method signatures where they are actually needed. This will make the API much more self-explanatory. If we are concerned that this might backfire, let's use a small pagination value object instead.
4. Keep the original method of getDatasetFileDetails() (no params) for backward compatibility, but mark it deprecated to discourage usage. The default implementation will use an offset=0 and limit=0. For simple cases, this will probably be sufficient (i.e. an exporter can make a decision based on information about the number of files in the dataset). We can mark it as deprecated if we want to discourage people from using it.
5. If we choose to use more value objects, let's discuss if we want to call the context class ExportDataContext. It may be fine to just call it Context. As it will only be used in the context of exporters (haha), that's probably long enough. If we keep the longer name it would feel strange to make pagination and file summary classes short named.

Should we discuss this during tech hour?