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

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

Closed
buildreactive opened this issue Nov 8, 2016 · 11 comments
Closed
Assignees
Labels
Milestone

Comments

@buildreactive
Copy link

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
Copy link
Contributor

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 Support inline expressions in validators #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 :-)

@holograph holograph self-assigned this Nov 9, 2016
@buildreactive
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

We are using Scala 2.11.8.

@buildreactive
Copy link
Author

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
Copy link
Contributor

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

@holograph holograph added bug and removed question labels Nov 28, 2016
@holograph holograph added this to the 0.6.1 milestone Nov 28, 2016
@holograph
Copy link
Contributor

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
Copy link
Author

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

@holograph
Copy link
Contributor

Thanks for the followup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants