Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monadic result model #42

Open
buildreactive opened this issue Jun 10, 2015 · 9 comments
Open

Monadic result model #42

buildreactive opened this issue Jun 10, 2015 · 9 comments
Assignees
Milestone

Comments

@buildreactive
Copy link

Thought I'd try a quick prototype. Ended up with this simple specs2 test:

val x = validate(new GPPersonInformation(GPID(1), simpleRandomName(), None, simpleRandomName(), Option("L")))

logger.trace(s"${x.getClass}")

if (x == com.wix.accord.Failure) {
    logger.error("FAILED")
}

Compiler reports:

[warn] /Users/zbeckman/Projects/Glimpulse/Server/project/glimpulse-server/test/models/TestPersonInformationDataModel.scala:62: com.wix.accord.Result and com.wix.accord.Failure.type are unrelated: they will most likely never compare equal
[warn] if (x == com.wix.accord.Failure) {
[warn] ^
[warn] one warning found

Also, you can run into name collisions with Scala Try / Success / Failure.

You might consider:

  1. Use something like Validation / ValidationSuccess / ValidationFailure.

  2. Add .isSuccess() and .isFailure() methods to make it easier to determine what is a success or failure. It seems we are required to write tests such as:

    if (x.isInstanceOf[Failure]) {
    logger.error("FAILED")
    }

Which is just plain ugly and hard to work with.

@packux
Copy link

packux commented Jun 10, 2015

A different approach would be to implement pattern matching for evaluating the output of validation i.e.

validate(GPPersonInformation(GPID(1), simpleRandomName(), None, simpleRandomName(), Option("L"))) match {
  case Success => //do sth on success
  case Failure => //do sth on failure
}

On name collisions, you may create aliases (if needed) when importing the specific accord types e.g.

import com.wix.accord.{Failure => ValidationFailure, Success => ValidationSuccess}

validate(GPPersonInformation(GPID(1), simpleRandomName(), None, simpleRandomName(), Option("L"))) match {
  case ValidationSuccess => //do sth on success
  case ValidationFailure => //do sth on failure
}

@buildreactive
Copy link
Author

All very good points (I had forgotten Scala can do aliases). True, we can use pattern matching, although we generally avoid it when there is a simple, single case e.g., "if (success) {...}" as it is simpler to read.

packux pushed a commit to packux/accord that referenced this issue Jun 11, 2015
@packux
Copy link

packux commented Jun 11, 2015

Continuing on this discussion, I think it would be very handy if the Result type would work in a similar fashion to the Try type of scala, i.e. provide map/flatMap facilities. That, would allow a syntax like the following

for {
   validatedDTO <- validate(DTOToValidate)
} yield validatedDTO 

or similarly

validate(DTOToValidate).map(validatedDTO => someDAO.insert(validatedDTO))

@holograph: Keeping in mind that this is not a small refactoring, would such a change make sense?

@buildreactive
Copy link
Author

+1. We have a few data types that mimic Try and it works nicely (for instance, a ProcedureResult that wraps stored procedure calls).

It's also nice for building cascading validation rules, so if any one of the rules fails the whole thing gets kicked out. For now we are doing this (or, using matching, but I don't like to use match when a single if does the trick equally well):

    val v = Try(validate(device))
        .flatMap(v => Try(validate(identity)))
        .flatMap(v => Try(validate(credential)))

    if (v.isSuccess && v.getOrElse(ValidationFailure) != ValidationSuccess) { ... }

Anything beyond v.isSuccess is probably unnecessary. I just code paranoid...

@holograph
Copy link
Contributor

@zbeckman I may be missing something but I don't see the point in having Try there; also, even with the current API you can do:

val v = validate(device) and validate(identity) and validate(credential)
if (v == com.wix.accord.Success) { ... } else { ... }

The difference from your original code is that Success is a singleton object, whereas a Failure is a case class which contains the violations. You'll need to either use v.isInstanceOf[Failure] or pattern match.

@packux Since Accord operates, as an intentional design goal, on pre-initialized objects (i.e. after deserialization, form input, database load etc.), it can't really offer scalaz-like "applicative builder" object construction. That is the obvious use case for a monadic result model.

On the other hand, it's entirely possible that I missed your point -- is validatedDTO simply the object passed to the validator to begin with?

holograph added a commit that referenced this issue Jun 11, 2015
- added isSuccess/isFailure, as discussed on #42
@packux
Copy link

packux commented Jun 12, 2015

@holograph : the idea is to provide a facility similar to the Try monad. That would allow an easy way to execute further functions on the value in case of Success or simply return a Failure.

For example, a map function would allow the following

case class Foo (foo: Int, bar: Int)

object Foo {
  implicit val fooValidator = validator[Foo] { f =>
       f.foo > 0
       f.bar > 0
  }
  impicit val restFormat = Json.restFormat[Foo]
}

object FooApp extends Controller {
   implicit val fooRestFormat = Foo.restFormat

  def createFoo = Action { request => 
    for {
       foo <- request.body.validate[Foo]
       validatedFoo <- validate(foo)
       fooID = fooDAO.insert(validatedFoo) // we assume this returns an Int
    } yield fooID

  }

}

In a similar fashion, flatMap would allow some nested validations for objects that are not available when we validate the initial object, for example

case class Foo (foo: Int, bar: Int)

object Foo {
  implicit val fooValidator = validator[Foo] { f =>
       f.foo > 0
       f.bar > 0
  }
  impicit val restFormat = Json.restFormat[Foo]
}

case class Bar (fooID: Int, bazz: Int)

object Bar {
  implicit val barValidator = validator[Bar] { b =>
       f.fooID > 0
       f.bazz > 0
  }
  impicit val restFormat = Json.restFormat[Bar]
}

object FooApp extends Controller {
   implicit val fooRestFormat = Foo.restFormat

  def createFoo = Action { request =>
   for {
       foo <- request.body.validate[Foo]       
       validatedFoo <- validate(foo)
       fooID <- fooDAO.insert(validatedFoo) // we assume this would return an Option[Int]
       validatedBar <- validate(Bar(fooID = fooID, bazz = 1)
       validBarID <- barDAO.insert(validBar) //we assume this would also return an Option[Int]
    } yield validBarID
  }
}

@holograph
Copy link
Contributor

Well that's the thing -- what does request.body.validate[Foo] do? It both deserializes and validates, and the deserialization aspect is completely outside of Accord's purview...

@holograph holograph changed the title Comparison to Failure or Success never compares as equal Monadic result model Jun 21, 2015
@voidcontext
Copy link

+1 for the monadic features, it would be more idiomatic and easier to integrate.

@holograph
Copy link
Contributor

I'm still missing some basic notion of what these wrap. Taking the previous example:

  def createFoo = Action { request =>
   for {
       foo <- request.body.validate[Foo]       
       validatedFoo <- validate(foo)
       fooID <- fooDAO.insert(validatedFoo) // we assume this would return an Option[Int]
       validatedBar <- validate(Bar(fooID = fooID, bazz = 1)
       validBarID <- barDAO.insert(validBar) //we assume this would also return an Option[Int]
    } yield validBarID
  }

There are too many clauses and types here; for example, why is foo seemingly validated twice? What is the type of validatedFoo? Also, you seem to be mixing monads (form validation, Accord validation and Option) without the appropriate transformers, so I'm not sure this could ever be made to work :-)

Given Accord's design decision to only validate complete objects (as opposed to scalaz's "applicative builder"-style iterative construction), the only sensible behavior I can think of is a "passthrough" i.e.:

val rawFoo: Foo = // ...
for (validatedFoo <- validate(rawFoo))   // validatedFoo is of type com.wix.accord.Success[Foo]
  yield validatedFoo.bar                 // Result is of type com.wix.accord.Success[Bar]

With a validation result being inherently success-biased (i.e. failures fall through, again similar to scalaz's disjunction). This feels to me to be of very limited value, but perhaps you can provide some additional examples of where this would be useful.

If you want to weigh in on this, the most useful comment would be "here's a piece of actual production code; here's what I would want it to look after refactoring", and don't forget to annotate the types so it's more obvious what you're looking for :-)

@holograph holograph added this to the 1.0 milestone Jul 27, 2015
@holograph holograph self-assigned this Jul 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants