From 73cdce6c992dd0af535247a8f73e761fa38dcbe2 Mon Sep 17 00:00:00 2001 From: lindareijnhoudt Date: Tue, 7 Aug 2018 14:39:58 +0200 Subject: [PATCH] Issue 2 and 3: Add documentation to Deposit and DepositProperties (#22) * Add javadoc style documentation to public methods * Follow an intelliJ suggestion * Rename 'empty' to 'from';Complete the docs. * Add param description * More explicit doc on createFromData * review of #22 and adding of extra test * remove Option's from 'DepositProperties.load' --- src/main/scala/nl/knaw/dans/bag/Deposit.scala | 68 +++++++++++-- .../nl/knaw/dans/bag/DepositProperties.scala | 98 +++++++++++++------ .../scala/nl/knaw/dans/bag/v0/DansV0Bag.scala | 2 +- .../knaw/dans/bag/DepositPropertiesSpec.scala | 25 ++++- .../scala/nl/knaw/dans/bag/DepositSpec.scala | 12 +-- 5 files changed, 157 insertions(+), 48 deletions(-) diff --git a/src/main/scala/nl/knaw/dans/bag/Deposit.scala b/src/main/scala/nl/knaw/dans/bag/Deposit.scala index a657b77b..4c0ea09b 100644 --- a/src/main/scala/nl/knaw/dans/bag/Deposit.scala +++ b/src/main/scala/nl/knaw/dans/bag/Deposit.scala @@ -336,6 +336,15 @@ class Deposit private(val baseDir: File, withDepositProperties(newProperties) } + /** + * 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`. + * + * @return `scala.util.Success` if the save was performed successfully, + * `scala.util.Failure` otherwise + */ def save(): Try[Unit] = { for { _ <- bag.save() @@ -351,22 +360,52 @@ object Deposit { val depositPropertiesName = "deposit.properties" - def empty(baseDir: File, - algorithms: Set[ChecksumAlgorithm] = Set(ChecksumAlgorithm.SHA1), - bagInfo: Map[String, Seq[String]] = Map.empty, - state: State, - depositor: Depositor, - bagStore: BagStore): Try[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 + */ + def from(baseDir: File, + algorithms: Set[ChecksumAlgorithm] = Set(ChecksumAlgorithm.SHA1), + bagInfo: Map[String, Seq[String]] = Map.empty, + state: State, + depositor: Depositor, + bagStore: BagStore): Try[Deposit] = { if (baseDir.exists) Failure(new FileAlreadyExistsException(baseDir.toString)) else for { bag <- DansBag.empty(baseDir / bagStore.bagId.toString, algorithms, bagInfo) - properties = DepositProperties.empty(state, depositor, bagStore) + properties = DepositProperties.from(state, depositor, bagStore) _ <- properties.save(depositProperties(baseDir)) } yield new Deposit(baseDir, bag, properties) } + /** + * 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 + */ def createFromData(payloadDir: File, algorithms: Set[ChecksumAlgorithm] = Set(ChecksumAlgorithm.SHA1), bagInfo: Map[String, Seq[String]] = Map.empty, @@ -379,12 +418,19 @@ object Deposit { for { bagDir <- moveBag(payloadDir, bagStore.bagId) bag <- DansBag.createFromData(bagDir, algorithms, bagInfo) - properties = DepositProperties.empty(state, depositor, bagStore) + properties = DepositProperties.from(state, depositor, bagStore) _ <- properties.save(depositProperties(payloadDir)) } yield new Deposit(payloadDir, bag, properties) } } + /** + * 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 { bagDir <- findBagDir(baseDir) @@ -393,6 +439,12 @@ object Deposit { } yield new Deposit(baseDir, bag, properties) } + /** + * Returns the `baseDir` of the Deposit object + * + * @param deposit the deposit to extract the `baseDir` from + * @return returns the `baseDir` of the Deposit object + */ implicit def depositAsFile(deposit: Deposit): File = deposit.baseDir private def moveBag(payloadDir: File, bagId: UUID): Try[File] = Try { diff --git a/src/main/scala/nl/knaw/dans/bag/DepositProperties.scala b/src/main/scala/nl/knaw/dans/bag/DepositProperties.scala index 3bbb5428..83acb31e 100644 --- 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 @@ -39,6 +39,13 @@ case class DepositProperties(creation: Creation = Creation(), springfield: Springfield = Springfield(), staged: Staged = Staged()) { + /** + * 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 + */ def save(file: File): Try[Unit] = Try { new PropertiesConfiguration { setDelimiterParsingDisabled(true) @@ -93,7 +100,15 @@ object DepositProperties { val stagedState = "staged.state" // @formatter:on - def empty(state: State, depositor: Depositor, bagStore: BagStore): DepositProperties = { + /** + * 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 a new `DepositProperties` + */ + def from(state: State, depositor: Depositor, bagStore: BagStore): DepositProperties = { DepositProperties( state = state, depositor = depositor, @@ -101,6 +116,13 @@ object DepositProperties { ) } + /** + * 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) Try { @@ -113,31 +135,51 @@ object DepositProperties { Failure(new NoSuchFileException(s"$propertiesFile does not exist or isn't a file")) } + /** + * 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( - doi = Option(properties.getString(doiIdentifier)) + identifier = new Identifier( + doi = properties.getString(doiIdentifier) ), curation = new Curation( - userId = Option(properties.getString(dataManagerUserId)), - email = Option(properties.getString(datamanagerEmail)), - isNewVersion = Option(properties.getString(isNewVersion)), - required = Option(properties.getString(curationRequired)), - performed = Option(properties.getString(curationPerformed)) + userId = properties.getString(dataManagerUserId), + email = properties.getString(datamanagerEmail), + isNewVersion = properties.getString(isNewVersion), + required = properties.getString(curationRequired), + performed = properties.getString(curationPerformed) ), springfield = new Springfield( domain = properties.getString(springfieldDomain), @@ -181,14 +223,14 @@ case class State(label: StateLabel, description: String) { case class Depositor(userId: String) -case class Identifier(doi: Option[String] = None) +case class Identifier(doi: Option[String] = None) { + def this(doi: String) = { + this(Option(doi)) + } +} case class BagStore(bagId: UUID, archived: Option[Boolean] = None) { - def this(bagId: UUID, archived: Boolean) = { - this(bagId, Option(archived)) - } - def this(bagId: String, archived: String) = { this(UUID.fromString(bagId), Option(archived).map(BooleanUtils.toBoolean)) } @@ -205,15 +247,15 @@ case class Curation(dataManager: DataManager = DataManager(), isNewVersion: Option[Boolean] = Option.empty, required: Option[Boolean] = Option.empty, performed: Option[Boolean] = Option.empty) { - def this(userId: Option[String], - email: Option[String], - isNewVersion: Option[String], - required: Option[String], - performed: Option[String]) = { - this(DataManager(userId, email), - isNewVersion.map(BooleanUtils.toBoolean), - required.map(BooleanUtils.toBoolean), - performed.map(BooleanUtils.toBoolean), + def this(userId: String, + email: String, + isNewVersion: String, + required: String, + performed: String) = { + this(DataManager(Option(userId), Option(email)), + Option(isNewVersion).map(BooleanUtils.toBoolean), + Option(required).map(BooleanUtils.toBoolean), + Option(performed).map(BooleanUtils.toBoolean), ) } diff --git a/src/main/scala/nl/knaw/dans/bag/v0/DansV0Bag.scala b/src/main/scala/nl/knaw/dans/bag/v0/DansV0Bag.scala index 393b6e46..e4291ec1 100644 --- a/src/main/scala/nl/knaw/dans/bag/v0/DansV0Bag.scala +++ b/src/main/scala/nl/knaw/dans/bag/v0/DansV0Bag.scala @@ -220,7 +220,7 @@ class DansV0Bag private(private[v0] val locBag: LocBag) extends DansBag { if (destinationPath.exists) throw new FileAlreadyExistsException(destinationPath.toString(), null, "already exists in payload") - if (fetchFiles.find(_.file == destinationPath).isDefined) + if (fetchFiles.exists(_.file == destinationPath)) throw new FileAlreadyExistsException(destinationPath.toString(), null, "already exists in fetch.txt") if (!destinationPath.isChildOf(data)) throw new IllegalArgumentException(s"a fetch file can only point to a location inside the bag/data directory; $destinationPath is outside the data directory") diff --git a/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala b/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala index ab826f01..8bfc8f93 100644 --- a/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala +++ b/src/test/scala/nl/knaw/dans/bag/DepositPropertiesSpec.scala @@ -27,12 +27,12 @@ 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") - val bagStore = new BagStore(bagId, archived = false) - val props = DepositProperties.empty(state, depositor, bagStore) + val bagStore = BagStore(bagId, archived = Option(false)) + val props = DepositProperties.from(state, depositor, bagStore) props.creation.timestamp.toString(ISODateTimeFormat.dateTime()) shouldBe fixedDateTimeNow.toString(ISODateTimeFormat.dateTime()) @@ -119,7 +119,7 @@ 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) => @@ -127,7 +127,7 @@ class DepositPropertiesSpec extends TestSupportFixture with FileSystemSupport wi } } - 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) => @@ -135,6 +135,21 @@ class DepositPropertiesSpec extends TestSupportFixture with FileSystemSupport wi } } + 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 diff --git a/src/test/scala/nl/knaw/dans/bag/DepositSpec.scala b/src/test/scala/nl/knaw/dans/bag/DepositSpec.scala index 1aca4e3b..73a19539 100644 --- a/src/test/scala/nl/knaw/dans/bag/DepositSpec.scala +++ b/src/test/scala/nl/knaw/dans/bag/DepositSpec.scala @@ -49,7 +49,7 @@ class DepositSpec extends TestSupportFixture baseDir.toJava shouldNot exist bagDir.toJava shouldNot exist - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Success(deposit) => baseDir.toJava should exist bagDir.toJava should exist @@ -66,7 +66,7 @@ class DepositSpec extends TestSupportFixture val bagId = UUID.randomUUID() val bagStore = BagStore(bagId) - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Success(deposit) => deposit.bag.baseDir.listRecursively.toList should contain only( deposit.bag.baseDir / "data", @@ -87,7 +87,7 @@ class DepositSpec extends TestSupportFixture val bagId = UUID.randomUUID() val bagStore = BagStore(bagId) - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Success(deposit) => (deposit.baseDir / "deposit.properties").toJava should exist } @@ -102,7 +102,7 @@ class DepositSpec extends TestSupportFixture val bagId = UUID.randomUUID() val bagStore = BagStore(bagId) - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Success(deposit) => val props = new PropertiesConfiguration(deposit.baseDir / "deposit.properties" toJava) props.getKeys.asScala.toList should contain only( @@ -131,7 +131,7 @@ class DepositSpec extends TestSupportFixture val bagId = UUID.randomUUID() val bagStore = BagStore(bagId) - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Success(deposit) => deposit.creationTimestamp shouldBe fixedDateTimeNow deposit.stateLabel shouldBe state.label @@ -174,7 +174,7 @@ class DepositSpec extends TestSupportFixture baseDir.toJava should exist baseDir.isRegularFile shouldBe true - inside(Deposit.empty(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { + inside(Deposit.from(baseDir, algorithms, bagInfo, state, depositor, bagStore)) { case Failure(e: FileAlreadyExistsException) => e should have message baseDir.toString() }