Skip to content

Commit

Permalink
Notes about refactoring OAI exports for testability.
Browse files Browse the repository at this point in the history
This needs some sophisticated refactoring to be ready for unit tests.
  • Loading branch information
poikilotherm committed Dec 17, 2018
1 parent a7cc7b8 commit 81fbffe
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 1 deletion.
14 changes: 14 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/DatasetServiceBean.java
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,8 @@ public void exportAllAsync() {
/**
* Scheduled function triggering the export of all local & published datasets,
* but only on the node which is configured as master timer.
*
* TODO: this is not unit testable as long as dependent functions aren't.
*/
@Lock(LockType.READ)
@Schedule(hour = "2", persistent = false)
Expand All @@ -584,6 +586,18 @@ public void exportAll() {
}
}

/**
* TODO: this code needs refactoring to be unit testable:
* 1) Move the Logger/FileHandler stuff to a factory in a Service
* (Export or Logging service) a) to make it mockable and
* b) to have common, reusable code.
* 2) Move this to OAIRecordServiceBean. The additional pieces for a
* complete OAI export is in OAISetServiceBean, so it makes more
* sense to live there and use this service as a service.
* 3) Moving this to OAIRecordServiceBean makes findAllLocalDatasetIds(), etc
* mockable, so this class (DatasetServiceBean) does not need immediate action.
* @param forceReExport
*/
public void exportAllDatasets(boolean forceReExport) {
Integer countAll = 0;
Integer countSuccess = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,13 @@ public void markOaiRecordsAsRemoved(Collection<OAIRecord> records, Date updateTi
// (why these need to be in an EJB bean at all, what's wrong with keeping
// them in the loadable ExportService? - since we need to modify the
// "last export" timestamp on the dataset, being able to do that in the
// @EJB context is convenient.
// @EJB context is convenient.

/**
* TODO: This should be refactored to be the scheduled export routine.
* See DatasetServiceBean.exportAllDatasets().
* This should use an Export service as EJB to get things mockable.
*/
public void exportAllFormats(Dataset dataset) {
try {
ExportService exportServiceInstance = ExportService.getInstance(settingsService);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,11 @@ public void exportOaiSet(OAISet oaiSet, Logger exportLogger) {
/**
* Scheduled export of all local & published datasets for OAI interface harvesting.
* Only runs on the node configured as timer master.
*
* TODO: this code needs refactoring to be unit testable:
* Move the Logger/FileHandler stuff to a factory in a Service
* (Export or Logging service) a) to make it mockable and
* b) to have common, reusable code.
*/
@Lock(LockType.READ)
@Schedule(hour = "2", persistent = false)
Expand Down

2 comments on commit 81fbffe

@landreev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense overall, I think.
I'm not sure about moving the exportAllDatasets() method from DatasetServiceBean to the OAIRecordServiceBean though... Metadata exports is something we want to always be doing, even if the OAI server is disabled, and there are no OAI sets or records... So logically this probably belongs in the dataset service bean.
Making these things unit-testable would be nice; but I feel it would make more sense to handle it separately - that is, not in the same PR as #5371.

@poikilotherm
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About moving from DatasetServiceBean: the function OAIRecordServiceBean.exportAllFormats() is called from DatasetServiceBean.exportAllDatasets() anyway and does the heavy lifting on the selected datasets.

The Bean is created anyway, even if OAI is disabled. The timers go off and exports happen, no checks to be found if OAI is enabled or not. The call chain is as follows: Timer -> DatasetServiceBean.exportAll() -> DatasetServiceBean.exportAllDatasets() -> OAIRecordServiceBean.exportAllFormatsInNewTransaction() -> ExportService.exportAllFormats().

IMHO it makes more sense to use DatasetServiceBean in the OAIRecordServiceBean to receive (mockable) the datasets that need to be exported and work on those.

About unit testing and refactoring for it: alright. This will PR will contain no tests then, but the TODO comments.

Please sign in to comment.