wix-incubator / accord

Accord: A sane validation library for Scala
http://wix.github.io/accord/
Other
529 stars 55 forks source link

localDate.each(_.isAfter(LocalDate.now()) is true) does not work #81

Closed buildreactive closed 7 years ago

buildreactive commented 7 years ago

It looks like Accord doesn't know about Java's date types:

localDate.each(_.isAfter(LocalDate.now()) is true)

Does not compile, so I thought I'd try this:

val z: Option[LocalDate] = p.dateOfBirth z.map((p: LocalDate) => p.isAfter(LocalDate.now()) is true)

But this results in the following error:

[error] /Users/zbeckman/Projects/Accenture/xiIntegrate/source/src/main/scala/common/validator/GuestDetailValidator.scala:19: Failed to extract object under validation from tree com.wix.accord.Validator[Boolean] (type=com.wix.accord.Validator[Boolean], raw=TypeTree())
[error]     z.map((p: LocalDate) => p.isAfter(LocalDate.now()) is true)

I ended up doing this:

if (localDate isDefined) localDate.get.isAfter(LocalDate.now()) is true

holograph commented 7 years ago

Indeed Accord doesn't have any specific features for dates. You've actually run into three separate issues:

localDate.each(_.isAfter(LocalDate.now()) is true) will never work because it's really syntactically incorrect, you're basically trying to apply the result of each as a function and it was never designed to work this way.

The second option (introducing the local variable) won't work because:

  1. Free inline code is not supported within a validation block (see #6; it's an old issue but not trivial to support, and I've seen few if any compelling reasons to do so);
  2. You're actually mapping over the option, which evaluates to another option -- that's not how Accord works. More in a second.

Finally, the third option will technically work but doesn't quite express what you mean as I understand it. What you want is to say "p.dateOfBirth has a value, the following expression has to be true: ...". You can do this in one of two ways:

// Inline transform: simple, but may result in ugly descriptions:
p.dateOfBirth.map(_ isAfter LocalDate.now()).each is true
// ... which is why you should probably add a custom description:
p.dateOfBirth.map(_ isAfter LocalDate.now()).each as "Date of birth is in the future" is true

The better solution would be to add proper validation rules, e.g.:

import com.wix.accord._
import com.wix.accord.ViolationBuilder._

// See http://wix.github.io/accord/dsl.html#custom-combinators
def aFutureDate: Matcher[LocalDate] =
  new NullSafeValidator(_ isAfter LocalDate.now(), _ -> "is not a future date")

implicit val myValidator = validator[Person] { p =>
  // ...
  p.dateOfBirth.each is aFutureDate
}

I'm definitely amenable to adding matchers on dates (built-in JVM types; JODA support will probably have to go in its own module), pull requests are most welcome :-)

buildreactive commented 7 years ago

Thank you for the in depth answer! And yes, I'd vote to adding Java date support (probably not so much Joda, I love it but it is now deprecated in favor of Java 8 native types). In the meantime I'm going to digest what you describe above.

Can I suggest you add a few more complex examples to the Accord web site? For example,

p.dateOfBirth.map(_ isAfter LocalDate.now()).each is true

Now that I read it, it makes perfect sense. I wouldn't not have gotten there on my own. (I guess what I'm saying is, more in-depth documentation on the use of each, both usage and limitations, would be excellent.

buildreactive commented 7 years ago

As for date support, simply adding:

def future: Matcher[LocalDate] = new NullSafeValidator(_ isAfter LocalDate.now(), _ -> "is not in the future")
def past: Matcher[LocalDate] = new NullSafeValidator(_ isBefore LocalDate.now(), _ -> "is not in the past")

Would probably go a long way.

buildreactive commented 7 years ago

Found a problem with macro expansion... When I try to use this:

p.dateOfBirth.map(_ isBefore LocalDate.now()).each as "date of birth is in the future" is true

I get a macro expansion exception:

[error] /Users/zbeckman/Projects/Accenture/xiIntegrate-2/source/src/main/scala/common/validator/GuestDetailValidator.scala:12: exception during macro expansion:
[error] java.lang.UnsupportedOperationException: Position.start on NoPosition
[error]     at scala.reflect.internal.util.Position.fail(Position.scala:17)
[error]     at scala.reflect.internal.util.UndefinedPosition.start(Position.scala:94)
[error]     at scala.reflect.internal.util.UndefinedPosition.start(Position.scala:90)
[error]     at com.wix.accord.transform.MacroHelper$class.startPos(MacroHelper.scala:70)
[error]     at com.wix.accord.transform.ValidationTransform.startPos(ValidationTransform.scala:24)
[error]     at com.wix.accord.transform.ExpressionDescriber$$anonfun$1.applyOrElse(ExpressionDescriber.scala:53)
[error]     at com.wix.accord.transform.ExpressionDescriber$$anonfun$1.applyOrElse(ExpressionDescriber.scala:53)
[error]     at scala.runtime.AbstractPartialFunction.apply(AbstractPartialFunction.scala:36)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1657)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1654)
[error]     at scala.reflect.internal.Trees$$anonfun$traverseMemberDef$1$1.apply$mcV$sp(Trees.scala:1209)
[error]     at scala.reflect.api.Trees$Traverser.atOwner(Trees.scala:2507)
[error]     at scala.reflect.internal.Trees$class.traverseMemberDef$1(Trees.scala:1203)
[error]     at scala.reflect.internal.Trees$class.itraverse(Trees.scala:1328)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.api.Trees$Traverser.traverse(Trees.scala:2475)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1658)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1654)
[error]     at scala.reflect.api.Trees$Traverser.traverseTrees(Trees.scala:2484)
[error]     at scala.reflect.api.Trees$Traverser.traverseParams(Trees.scala:2492)
[error]     at scala.reflect.internal.Trees$$anonfun$itraverse$1.apply$mcV$sp(Trees.scala:1329)
[error]     at scala.reflect.api.Trees$Traverser.atOwner(Trees.scala:2507)
[error]     at scala.reflect.internal.Trees$class.itraverse(Trees.scala:1329)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.api.Trees$Traverser.traverse(Trees.scala:2475)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1658)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1654)
[error]     at scala.reflect.api.Trees$Traverser.traverseTrees(Trees.scala:2484)
[error]     at scala.reflect.internal.Trees$class.traverseComponents$1(Trees.scala:1284)
[error]     at scala.reflect.internal.Trees$class.itraverse(Trees.scala:1330)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.api.Trees$Traverser.traverse(Trees.scala:2475)
[error]     at scala.reflect.internal.Trees$CollectTreeTraverser.traverse(Trees.scala:1658)
[error]     at scala.reflect.internal.Trees$TreeContextApiImpl.collect(Trees.scala:118)
[error]     at com.wix.accord.transform.ExpressionDescriber$class.prettyPrint(ExpressionDescriber.scala:53)
[error]     at com.wix.accord.transform.ExpressionDescriber$class.describeTree(ExpressionDescriber.scala:127)
[error]     at com.wix.accord.transform.ValidationTransform.describeTree(ValidationTransform.scala:24)
[error]     at com.wix.accord.transform.ValidationTransform.rewriteOne(ValidationTransform.scala:41)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$rewriteValidationRules$1.applyOrElse(ValidationTransform.scala:192)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$rewriteValidationRules$1.applyOrElse(ValidationTransform.scala:190)
[error]     at scala.PartialFunction$OrElse.apply(PartialFunction.scala:167)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$1.applyOrElse(ValidationTransform.scala:118)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$1.applyOrElse(ValidationTransform.scala:118)
[error]     at scala.PartialFunction$OrElse.apply(PartialFunction.scala:167)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$3.applyOrElse(ValidationTransform.scala:144)
[error]     at com.wix.accord.transform.ValidationTransform$$anonfun$3.applyOrElse(ValidationTransform.scala:144)
[error]     at scala.PartialFunction$OrElse.apply(PartialFunction.scala:167)
[error]     at com.wix.accord.transform.PatternHelper$$anon$2.traverse(PatternHelper.scala:85)
[error]     at scala.reflect.api.Trees$Traverser.traverseTrees(Trees.scala:2484)
[error]     at scala.reflect.internal.Trees$class.traverseComponents$1(Trees.scala:1234)
[error]     at scala.reflect.internal.Trees$class.itraverse(Trees.scala:1330)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.internal.SymbolTable.itraverse(SymbolTable.scala:16)
[error]     at scala.reflect.api.Trees$Traverser.traverse(Trees.scala:2475)
[error]     at com.wix.accord.transform.PatternHelper$$anon$2.traverse(PatternHelper.scala:87)
[error]     at com.wix.accord.transform.PatternHelper$class.collectFromPattern(PatternHelper.scala:89)
[error]     at com.wix.accord.transform.ValidationTransform.collectFromPattern(ValidationTransform.scala:24)
[error]     at com.wix.accord.transform.ValidationTransform.transformed(ValidationTransform.scala:210)
[error]     at com.wix.accord.transform.ValidationTransform$.apply(ValidationTransform.scala:230)
[error]   implicit val guestDetailValidator = validator[GuestDetail] { (p: GuestDetail) =>

So I end up replacing it with this, which just seems less... nice:

    // Note, the following should work but blows with a macro expansion error that is hard to track down. Reported on Accord github:
    //    p.dateOfBirth.map(_ isBefore LocalDate.now()).each as "date of birth is in the future" is true
    //    p.dateOfBirth.map(_ isAfter LocalDate.now().minusYears(150)).each as "birthdate must be less than 150 years ago" is true
    if (p.dateOfBirth.isDefined) {
      p.dateOfBirth.get isBefore LocalDate.now() as "date of birth is in the future" is true
      p.dateOfBirth.get isAfter LocalDate.now().minusYears(150) as "birthdate must be less than 150 years ago" is true
    }
holograph commented 7 years ago

Can you tell me which Scala version you're using? I've had a bitch of a time getting code reprinting (the thing that failed, needed for automatic description generation) to work properly, and it's entirely possible I missed some subtle corner cases.

buildreactive commented 7 years ago

We are using Scala 2.11.8.

buildreactive commented 7 years ago

By the way... This is showing elsewhere, not just date related:

    implicit val groupValidator = validator[Group] { (p: Group) =>
        p.group.map(_.length).each as "group length is not between 1 and 3 characters" is between(1, 3)
    }

Also causes the same exception.

I've filed this under #89.

holograph commented 7 years ago

I've opened a separate issue (#90) for supporting native Java time/calendar functions, feedback is welcome.

holograph commented 7 years ago

As I understand it, we're done here (all issues are either resolved or handled separately). @zbeckman any objection to my closing this issue?

buildreactive commented 7 years ago

Confirmed that 0.7-SNAPSHOT is good as far as the map() issue. Close away!

holograph commented 7 years ago

Thanks for the followup!