Switch to ValidatedNel / Validated instead of Either #369

sihil · 2016-10-06T14:06:00Z

This makes much of the code a bit cleaner and easier to reason about and in come cases means that we can return multiple errors. As a bonus some of the Json reads code becomes a little easier to reason about as well.

This adds an API endpoint to which YAML can be sent for validation and some very rudimentary web pages that let you do the same (your eyes will hurt!)

Would be interested in doing a code review of all of this in person with anyone who has used cats before.

This makes much of the code a bit cleaner and easier to reason about and in come cases means that we can return multiple errors. As a bonus some of the Json reads code becomes a little easier to reason about as well. This adds an API endpoint to which YAML can be sent for validation and some very rudimentary web pages that let you do the same (your eyes will hurt!)

adamnfish · 2016-10-07T09:26:48Z

magenta-lib/src/main/scala/magenta/input/models.scala

-  def :::(other: ConfigErrors) = ConfigErrors(other.errors ::: errors)
-  def ::(other: ConfigError) = ConfigErrors(other :: errors)
+object ConfigError {
+  def nel(context: String, message: String) = NonEmptyList.of(ConfigError(context, message))


Is this useful?

No - have removed - aliasing NonEmptyList helps.

adamnfish · 2016-10-07T09:27:43Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

-        _ <- validateDependencies(label, deployment, config.deployments).right
-      } yield deployment
-    }
+      applyTemplates(label, rawDeployment, config.templates).andThen { templated =>


Not enamoured with the layout of this, makes it needlessly hard to see the steps having the andThen calls all over the place. Nice thing about for comps is they make that so clear.

:minor_point:

adamnfish · 2016-10-07T09:31:22Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

+  private[input] def resolveDeployment(label: String, templated: DeploymentOrTemplate, globalStacks: Option[List[String]], globalRegions: Option[List[String]]): ValidatedNel[ConfigError, Deployment] = {
+    (Validated.fromOption(templated.`type`, ConfigError.nel(label, "No type field provided")) |@|
+      Validated.fromOption(templated.stacks.orElse(globalStacks), ConfigError.nel(label, "No stacks provided")) |@|
+      Validated.fromOption(templated.regions.orElse(globalRegions), ConfigError.nel(label, "No regions provided"))) map { (deploymentType, stacks, regions) =>


I think this bit should also validate the lengths of stacks and regions are > 0 so you can match on stackHead :: stacks to avoid the unsafe NEL creation. Or some such*.

disclaimer: other approaches exist

For better or worse I've pimped Option[List] to provide this niceness.

adamnfish · 2016-10-07T09:33:16Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentTypeResolver.scala

      deploymentType <- availableTypes.find(_.name == deployment.`type`)
        .toRight(ConfigError(deployment.name, s"Unknown type ${deployment.`type`}")).right
      deploymentWithActions <- resolveDeploymentActions(deployment, deploymentType).right
      deployment <- verifyDeploymentParameters(deploymentWithActions, deploymentType).right
    } yield deployment
+    Validated.fromEither(result).leftMap(NonEmptyList.of(_))


Does leftMap(NonEmptyList.of) not typecheck?

adamnfish · 2016-10-07T09:40:23Z

magenta-lib/src/main/scala/magenta/input/resolver/Resolver.scala

 import magenta.deployment_type.DeploymentType
 import magenta.graph.{DeploymentTasks, Graph}
 import magenta.input._
+import magenta.{DeployParameters, DeploymentResources}

 object Resolver {


adamnfish · 2016-10-07T09:43:31Z

magenta-lib/src/main/scala/magenta/input/resolver/TaskResolver.scala

-    val deploymentTypeEither = deploymentTypes.find(_.name == deployment.`type`).
-      toRight(ConfigError(deployment.name, s"Deployment type ${deployment.`type`} not found"))
+    val validatedDeploymentType = Validated.fromOption(deploymentTypes.find(_.name == deployment.`type`),
+      NEL.of(ConfigError(deployment.name, s"Deployment type ${deployment.`type`} not found")))


This seems to me to be more sensible than ConfigError.nel (see above).

Yep - that was before I started aliasing NonEmptyList => NEL in all the imports. Am thinking about a global type alias.

adamnfish · 2016-10-07T09:47:10Z

magenta-lib/src/test/scala/magenta/fixtures/ValidatedValues.scala

+  * Two functions are pimped onto Validated for the purposes of testing and if the value is not of the expected type
+  * then the test will fail detailing what the unexpected value was.
+  */
+trait ValidatedValues {


Nice.

By analogy with EitherValues this would be .valid.value. I realise that's mainly an artifact of Either's projections but the .value is the common bit (see also OptionValues). The trait scalatest has for Future is called Futures, presumably because it doesn't have the value accessor syntax.

:minor_point:

I've left it as I can't think of an alternative.

adamnfish · 2016-10-07T10:08:25Z

riff-raff/app/controllers/api.scala

+        case Invalid(errors) =>
+          Json.obj(
+            "response" -> Json.obj(
+              "status" -> "ok",


Is the status really "ok" here?

Yes. I think so. It has successfully given you a validation result. That result might be a failure, but it is still a 200 response which in the API world is OK. See the case above where you don't even give it valid data to validate in which case you do get an error.

adamnfish · 2016-10-07T10:16:51Z

riff-raff/app/views/validation/validationErrors.scala.html

+@(implicit request: Security.AuthenticatedRequest[AnyContent, com.gu.googleauth.UserIdentity], errors: NonEmptyList[ConfigError])
+
+@main("Validation errors", request) {
+    <div class="clearfix"><p>&nbsp;</p></div>


The frontend needs some attention. It has that everywhere.

I think you really need a spacer.gif to get into the flavour.

adamnfish · 2016-10-07T10:18:16Z

philwills

This generally looks good, but probably should talk through some of it in person.

philwills · 2016-10-07T14:07:06Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

-  def resolve(config: RiffRaffDeployConfig): List[Either[ConfigError, Deployment]] = {
-    config.deployments.map { case (label, rawDeployment) =>
+  def resolve(config: RiffRaffDeployConfig): ValidatedNel[ConfigError, List[Deployment]] = {
+    config.deployments.traverseU[ValidatedNel[ConfigError, Deployment]] { case (label, rawDeployment) =>


scala/scala#5102 means that the weird U bit will go away in 2.12.

philwills · 2016-10-07T14:14:34Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

-      regions <- templated.regions.orElse(globalRegions).toRight(ConfigError(label, "No regions provided")).right
-    } yield {
+  private[input] def resolveDeployment(label: String, templated: DeploymentOrTemplate, globalStacks: Option[List[String]], globalRegions: Option[List[String]]): ValidatedNel[ConfigError, Deployment] = {
+    (Validated.fromOption(templated.`type`, NEL.of(ConfigError(label, "No type field provided"))) |@|


I think a custom error type with it's own Semigroup might avoid the littering of NEL.of.

Let me give that a bash.

philwills · 2016-10-07T14:14:45Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

-    } yield {
+  private[input] def resolveDeployment(label: String, templated: DeploymentOrTemplate, globalStacks: Option[List[String]], globalRegions: Option[List[String]]): ValidatedNel[ConfigError, Deployment] = {
+    (Validated.fromOption(templated.`type`, NEL.of(ConfigError(label, "No type field provided"))) |@|
+      Validated.fromOption(templated.stacks.orElse(globalStacks), NEL.of(ConfigError(label, "No stacks provided"))) |@|


It tend to find mapN easier to talk about than |@|, but that's definitely personal preference.

I had a play with that and couldn't see how to get it to work and can't find any examples. Seems to work for higher kinded types with one type parameter rather than two or more or have I missed something?

philwills · 2016-10-07T14:19:19Z

magenta-lib/src/main/scala/magenta/input/resolver/DeploymentResolver.scala

-        parameters        = templated.parameters.getOrElse(Map.empty)
+        name = label,
+        `type` = deploymentType,
+        stacks = NEL.fromListUnsafe(stacks),


Couldn't this Unsafe be avoided by lifting into the NEL within the Validated clause?

e.g.

Validated.fromOption(templated.stacks.orElse(globalStacks).flatMap(NEL.fromList), NEL.of(ConfigError(label, "No stacks provided")))

philwills · 2016-10-07T14:22:26Z

magenta-lib/src/main/scala/magenta/input/resolver/package.scala

+
+package object resolver {
+  implicit class RichValidated[E, A](validated: Validated[E, A]) {
+    def flatMap[EE >: E, B](f: A => Validated[EE, B]): Validated[EE, B] = validated.andThen(f)


I think there are some quite good reasons for not calling this flatMap

The reason it is here is to make for comprehensions work and eliminate the uses of andThen across the codebase.

- pimping Validated with `flatMap` means we can write much clearer code by switching to for comprehensions and avoid the use of `andThen` - using traverseU means that we don't have to produce singleton lists and labouriously combine them Leaving the use of andThen in the tests.

…group

sihil · 2016-10-07T15:52:55Z

OK - I've made some more changes to move from ValidatedNel to Validated with a custom error class. Cleans up quite a lot of the boilerplate as @philwills guessed. I also found a couple of places I hadn't entirely converted to Validated.

sihil added the DO NOT MERGE label Oct 6, 2016

sihil changed the title ~~An initial experiement to use ValidatedNel instead of Either~~ An initial experiment to use ValidatedNel instead of Either Oct 6, 2016

sihil force-pushed the sh-cats-and-validated branch 2 times, most recently from 29bc4e5 to 03dc907 Compare October 6, 2016 19:15

sihil force-pushed the sh-cats-and-validated branch from 03dc907 to 4b83d6e Compare October 6, 2016 20:01

sihil removed the DO NOT MERGE label Oct 6, 2016

sihil changed the title ~~An initial experiment to use ValidatedNel instead of Either~~ Switch to ValidatedNel / Validated instead of Either Oct 6, 2016

adamnfish reviewed Oct 7, 2016

View reviewed changes

sihil force-pushed the sh-cats-and-validated branch 2 times, most recently from d62b6bb to d1365e1 Compare October 7, 2016 14:22

philwills reviewed Oct 7, 2016

View reviewed changes

sihil force-pushed the sh-cats-and-validated branch from d1365e1 to a5b8e5c Compare October 7, 2016 14:29

sihil force-pushed the sh-cats-and-validated branch from a5b8e5c to ff600b3 Compare October 7, 2016 14:52

Switch from ValidatedNel to Validated with a custom ConfigErrors Semi…

ac25eab

…group

sihil merged commit 5bc2184 into master Oct 7, 2016

sihil deleted the sh-cats-and-validated branch October 7, 2016 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to ValidatedNel / Validated instead of Either #369

Switch to ValidatedNel / Validated instead of Either #369

sihil commented Oct 6, 2016 •

edited

Loading

adamnfish Oct 7, 2016

sihil Oct 7, 2016 •

edited

Loading

adamnfish Oct 7, 2016

sihil Oct 7, 2016

adamnfish Oct 7, 2016

sihil Oct 7, 2016

adamnfish Oct 7, 2016

sihil Oct 7, 2016

adamnfish Oct 7, 2016

adamnfish Oct 7, 2016

sihil Oct 7, 2016

adamnfish Oct 7, 2016 •

edited

Loading

sihil Oct 7, 2016

adamnfish Oct 7, 2016

sihil Oct 7, 2016

adamnfish Oct 7, 2016

sihil Oct 7, 2016

philwills Oct 7, 2016

adamnfish commented Oct 7, 2016

philwills left a comment

philwills Oct 7, 2016

philwills Oct 7, 2016

sihil Oct 7, 2016

philwills Oct 7, 2016

sihil Oct 7, 2016

philwills Oct 7, 2016

philwills Oct 7, 2016

sihil Oct 7, 2016

sihil commented Oct 7, 2016

Switch to ValidatedNel / Validated instead of Either #369

Switch to ValidatedNel / Validated instead of Either #369

Conversation

sihil commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

sihil Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnfish Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnfish commented Oct 7, 2016

philwills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihil commented Oct 7, 2016

sihil commented Oct 6, 2016 •

edited

Loading

sihil Oct 7, 2016 •

edited

Loading

adamnfish Oct 7, 2016 •

edited

Loading