review of DANS-KNAW#22 and adding of extra test

rvanheest-DANS-KNAW · Aug 7, 2018 · bdff978 · bdff978
1 parent 050243d
commit bdff978
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 51 deletions.
diff --git a/src/main/scala/nl/knaw/dans/bag/Deposit.scala b/src/main/scala/nl/knaw/dans/bag/Deposit.scala
@@ -337,10 +337,11 @@ class Deposit private(val baseDir: File,
   }
 
   /**
-   * Saves the in-memory deposit to the file system. It saves all the bag-it files by calling `DansBag.save()`
-   * and the deposit.properties by calling `DepositProperties.save()`.
+   * Saves the in-memory deposit to the file system. It saves all the bag-it files by calling
+   * `DansBag.save()` and the deposit.properties by calling `DepositProperties.save()`.
    * As most methods in this library only manipulate the bag-it files and Deposit in memory, a call
-   * to `save()` is necessary to serialize the `nl.knaw.dans.bag.Deposit`
+   * to `save()` is necessary to serialize the `nl.knaw.dans.bag.Deposit`.
+   *
    * @return `scala.util.Success` if the save was performed successfully,
    *         `scala.util.Failure` otherwise
    */
@@ -362,15 +363,15 @@ object Deposit {
   /**
    * Creates a new `nl.knaw.dans.bag.Deposit` with an empty `nl.knaw.dans.bag.DansBag` inside.
    *
-   * @param baseDir the directory for the new Deposit
-   * @param algorithms the algorithms with which the checksums for the (payload/tag) files are calculated.
-   *                   If none are provided, SHA1 is used.
-   * @param bagInfo the entries to be added to `bag-info.txt`
-   * @param state the state to be set in the `deposit.properties`' state.label
-   * @param depositor the depositor to be set in the `deposit.properties`' `depositor.userId`
-   * @param bagStore the bagId for the `bag-dir` in this `nl.knaw.dans.bag.Deposit`
-   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit located at `baseDir`
-   *         else returns an exception
+   * @param baseDir    the directory for the new Deposit
+   * @param algorithms the algorithms with which the checksums for the (payload/tag) files are
+   *                   calculated. If none are provided, SHA1 is used.
+   * @param bagInfo    the entries to be added to `bag-info.txt`
+   * @param state      the state to be set in the `deposit.properties`' state.label
+   * @param depositor  the depositor to be set in the `deposit.properties`' `depositor.userId`
+   * @param bagStore   the bagId for the `bag-dir` in this `nl.knaw.dans.bag.Deposit`
+   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit
+   *         located at `baseDir`, else returns an exception
    */
   def from(baseDir: File,
            algorithms: Set[ChecksumAlgorithm] = Set(ChecksumAlgorithm.SHA1),
@@ -389,19 +390,21 @@ object Deposit {
   }
 
   /**
-   * Creates a new Deposit, as a parent-directory to the `payloadDir`. A new `DansBag` will be created
-   * in the `Deposit`, with the bag-it files, and the data files in the `data/` directory.
-   * However, no `metadata/` files will be created. These have to be added separately
+   * Creates a new Deposit, as a parent-directory to the `payloadDir`. A new `DansBag` will be
+   * created in the `Deposit`, with the bag-it files, and the data files in the `data/` directory.
+   * However, no `metadata/` files will be created. These have to be added separately.
    *
-   * @param payloadDir the directory containing the payload (data) files for the bag. The `Deposit` will
-   *                   be created here, and the payload files will be moved to the `data/` directory in the new `DansBag`
-   * @param algorithms The algorithms with which the checksums for the (payload/tag) files are calculated. if none provided SHA1 is used.
-   * @param bagInfo The entries to be added to `bag-info.txt`
-   * @param state The state to be set in the deposit.properties' state.label
-   * @param depositor The depositor to be set in the deposit.properties' depositor.userId
-   * @param bagStore The bagId for the bag-dir in this Deposit
-   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit located at `payloadDir`
-   *         else returns an exception
+   * @param payloadDir the directory containing the payload (data) files for the bag. The `Deposit`
+   *                   will be created here, and the payload files will be moved to the `data/`
+   *                   directory in the new `DansBag`
+   * @param algorithms the algorithms with which the checksums for the (payload/tag) files are
+   *                   calculated. If none provided SHA1 is used.
+   * @param bagInfo    the entries to be added to `bag-info.txt`
+   * @param state      the state to be set in the deposit.properties' state.label
+   * @param depositor  the depositor to be set in the deposit.properties' depositor.userId
+   * @param bagStore   the bagId for the bag-dir in this Deposit
+   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit
+   *         located at `payloadDir` else returns an exception
    */
   def createFromData(payloadDir: File,
                      algorithms: Set[ChecksumAlgorithm] = Set(ChecksumAlgorithm.SHA1),
@@ -422,10 +425,11 @@ object Deposit {
   }
 
   /**
-   * Reads the `baseDir` as a Deposit
-   * @param baseDir The directory containing the deposit
-   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit located at `baseDir`
-   *         else returns an exception
+   * Reads the `baseDir` as a Deposit.
+   *
+   * @param baseDir the directory containing the deposit
+   * @return if successful, returns a `nl.knaw.dans.bag.Deposit` object representing the deposit
+   *         located at `baseDir` else returns an exception
    */
   def read(baseDir: File): Try[Deposit] = {
     for {
@@ -437,6 +441,7 @@ object Deposit {
 
   /**
    * Returns the `baseDir` of the Deposit object
+   *
    * @param deposit the deposit to extract the `baseDir` from
    * @return returns the `baseDir` of the Deposit object
    */

diff --git a/src/main/scala/nl/knaw/dans/bag/DepositProperties.scala b/src/main/scala/nl/knaw/dans/bag/DepositProperties.scala
@@ -19,7 +19,7 @@ import java.nio.file.NoSuchFileException
 import java.util.UUID
 
 import better.files.File
-import nl.knaw.dans.bag.DepositProperties._
+import nl.knaw.dans.bag.DepositProperties.{ stateDescription, _ }
 import nl.knaw.dans.bag.SpringfieldPlayMode.SpringfieldPlayMode
 import nl.knaw.dans.bag.StageState.StageState
 import nl.knaw.dans.bag.StateLabel.StateLabel
@@ -40,8 +40,9 @@ case class DepositProperties(creation: Creation = Creation(),
                              staged: Staged = Staged()) {
 
   /**
-   * Writes the DepositProperties to `file` on the filesystem
-   * @param file the file-location to serialize the properties to
+   * Writes the `DepositProperties` to `file` on the filesystem.
+   *
+   * @param file the file location to serialize the properties to
    * @return `scala.util.Success` if the save was performed successfully,
    *         `scala.util.Failure` otherwise
    */
@@ -100,11 +101,12 @@ object DepositProperties {
   // @formatter:on
 
   /**
-   * Creates a DepositProperties object, with only mandatory properties set
-   * @param state the `State` to be set
+   * Creates a `DepositProperties` object, with only the mandatory properties set.
+   *
+   * @param state     the `State` to be set
    * @param depositor the accountname of the depositor
-   * @param bagStore the bagId to be used for this deposit
-   * @return returns a new DepositProperties
+   * @param bagStore  the bagId to be used for this deposit
+   * @return a new `DepositProperties`
    */
   def from(state: State, depositor: Depositor, bagStore: BagStore): DepositProperties = {
     DepositProperties(
@@ -115,10 +117,11 @@ object DepositProperties {
   }
 
   /**
-   * Reads a File as a deposit.properties file
-   * @param propertiesFile
-   * @return if successful it returns the DepositProperties representing the `propertiesFile`,
-   *         else a Failure with a NoSuchFileException is returned
+   * Reads a `File` as a `deposit.properties` file.
+   *
+   * @param propertiesFile the file to be converted to a `DepositProperties`
+   * @return if successful the `DepositProperties` representing the `propertiesFile`,
+   *         else a Failure with a NoSuchFileException
    */
   def read(propertiesFile: File): Try[DepositProperties] = {
     if (propertiesFile.exists && propertiesFile.isRegularFile)
@@ -133,25 +136,39 @@ object DepositProperties {
   }
 
   /**
-   * Loads a new DepositProperties object with the corresponding elements from the PropertiesConfiguration
-   * @param properties the PropertiesConfiguration containing all mandatory deposit properties
-   * @return if successful it returns a new DepositProperties representing the provided `properties`
-   *         else a Failure with a NoSuchElementException if not all deposit properties were present
+   * Loads a new `DepositProperties` object with the corresponding elements from the
+   * `PropertiesConfiguration`. `properties` should at least contain all mandatory properties.
+   *
+   * @param properties the `PropertiesConfiguration` containing at least all mandatory deposit properties
+   * @return if successful a new `DepositProperties` representing the provided `properties`
+   *         else a `Failure` with a `NoSuchElementException` if not all deposit properties were present
    */
   def load(properties: PropertiesConfiguration): Try[DepositProperties] = Try {
+    val creationTimestampValue = properties.getString(creationTimestamp)
+    val stateLabelValue = properties.getString(stateLabel)
+    val stateDescriptionValue = properties.getString(stateDescription)
+    val depositorUserIdValue = properties.getString(depositorUserId)
+    val bagStoreBagIdValue = properties.getString(bagStoreBagId)
+
+    require(creationTimestampValue != null, s"could not find mandatory field '$creationTimestamp'")
+    require(stateLabelValue != null, s"could not find mandatory field '$stateLabel'")
+    require(stateDescriptionValue != null, s"could not find mandatory field '$stateDescription'")
+    require(depositorUserIdValue != null, s"could not find mandatory field '$depositorUserId'")
+    require(bagStoreBagIdValue != null, s"could not find mandatory field '$bagStoreBagId'")
+
     DepositProperties(
       creation = new Creation(
-        timestamp = properties.getString(creationTimestamp)
+        timestamp = creationTimestampValue
       ),
       state = new State(
-        label = properties.getString(stateLabel),
-        description = properties.getString(stateDescription)
+        label = stateLabelValue,
+        description = stateDescriptionValue
       ),
       depositor = Depositor(
-        userId = properties.getString(depositorUserId)
+        userId = depositorUserIdValue
       ),
       bagStore = new BagStore(
-        bagId = properties.getString(bagStoreBagId),
+        bagId = bagStoreBagIdValue,
         archived = properties.getString(bagStoreArchived)
       ),
       identifier = Identifier(

diff --git a/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala b/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala
@@ -27,7 +27,7 @@ import scala.util.Failure
 
 class DepositPropertiesSpec extends TestSupportFixture with FileSystemSupport with TestDeposits with FixDateTimeNow {
 
-  "empty" should "create a deposit.properties object containing only the minimal required properties" in {
+  "from" should "create a deposit.properties object containing only the minimal required properties" in {
     val state = State(StateLabel.DRAFT, "this deposit is still in draft")
     val depositor = Depositor("myuser")
     val bagId = UUID.fromString("1c2f78a1-26b8-4a40-a873-1073b9f3a56a")
@@ -119,22 +119,37 @@ class DepositPropertiesSpec extends TestSupportFixture with FileSystemSupport wi
     props.staged.state shouldBe empty
   }
 
-  it should "fail if the file does not exist" in {
+  it should "fail when the file does not exist" in {
     val file = simpleDepositDirV0 / "non-existing-deposit.properties"
     inside(DepositProperties.read(file)) {
       case Failure(e: NoSuchFileException) =>
         e should have message s"$file does not exist or isn't a file"
     }
   }
 
-  it should "fail if the given better.files.File is not a regular file" in {
+  it should "fail when the given better.files.File is not a regular file" in {
     val file = simpleDepositDirV0
     inside(DepositProperties.read(file)) {
       case Failure(e: NoSuchFileException) =>
         e should have message s"$file does not exist or isn't a file"
     }
   }
 
+  it should "fail when deposit.properties does not contain the minimal required properties" in {
+    // state.description and state.label are missing
+    val file = (testDir / "deposit.properties").write(
+      """creation.timestamp=2018-05-25T20:08:56.210+02:00
+        |depositor.userId=myuser
+        |bag-store.bag-id=1c2f78a1-26b8-4a40-a873-1073b9f3a56a
+      """.stripMargin
+    )
+
+    inside(DepositProperties.read(file)) {
+      case Failure(e: IllegalArgumentException) =>
+        e should have message "requirement failed: could not find mandatory field 'state.label'"
+    }
+  }
+
   "copy" should "change properties" in {
     val props = simpleDepositPropertiesV0