From 1b7f268e952d121118ece756e0f33fce8d9fe72e Mon Sep 17 00:00:00 2001 From: alonsodomin Date: Fri, 21 Aug 2020 14:00:43 +0100 Subject: [PATCH 1/2] Fix exception thrown when expression is too short --- .../shared/src/main/scala/cron4s/errors.scala | 6 +- .../src/main/scala/cron4s/parsing/base.scala | 15 +++- .../src/main/scala/cron4s/parsing/lexer.scala | 8 +- .../main/scala/cron4s/parsing/parser.scala | 14 ++-- .../src/test/scala/cron4s/CronSpec.scala | 80 ++++++++++++------- 5 files changed, 81 insertions(+), 42 deletions(-) diff --git a/modules/core/shared/src/main/scala/cron4s/errors.scala b/modules/core/shared/src/main/scala/cron4s/errors.scala index 19934187..b48ca2f9 100644 --- a/modules/core/shared/src/main/scala/cron4s/errors.scala +++ b/modules/core/shared/src/main/scala/cron4s/errors.scala @@ -29,8 +29,10 @@ object Error { implicit val errorShow: Show[Error] = Show.show(_.getMessage) } -final case class ParseFailed(msg: String, found: String, position: Int) - extends Error(s"$msg at position $position but found '$found'") +case object ExprTooShort extends Error("The provided expression was too short") + +final case class ParseFailed(expected: String, position: Int, found: Option[String] = None) + extends Error(s"$expected at position ${position}${found.fold("")(f => s" but found '$f'")}") sealed trait ValidationError object ValidationError { diff --git a/modules/core/shared/src/main/scala/cron4s/parsing/base.scala b/modules/core/shared/src/main/scala/cron4s/parsing/base.scala index 942a07f6..ef417c69 100644 --- a/modules/core/shared/src/main/scala/cron4s/parsing/base.scala +++ b/modules/core/shared/src/main/scala/cron4s/parsing/base.scala @@ -18,8 +18,19 @@ package cron4s package parsing import scala.util.parsing.combinator.Parsers +import scala.util.parsing.input.{Position, NoPosition} private[parsing] trait BaseParser extends Parsers { - protected def handleError(err: NoSuccess): ParseFailed = - ParseFailed(err.msg, err.next.first.toString, err.next.pos.column) + protected def handleError(err: NoSuccess): _root_.cron4s.Error = { + err.next.pos match { + case NoPosition => ExprTooShort + case pos: Position => + val position = err.next.pos.column + val found = { + if (err.next.atEnd) None + else Option(err.next.first.toString).filter(_.nonEmpty) + } + ParseFailed(err.msg, position, found) + } + } } diff --git a/modules/core/shared/src/main/scala/cron4s/parsing/lexer.scala b/modules/core/shared/src/main/scala/cron4s/parsing/lexer.scala index 24ce6f1a..dc404037 100644 --- a/modules/core/shared/src/main/scala/cron4s/parsing/lexer.scala +++ b/modules/core/shared/src/main/scala/cron4s/parsing/lexer.scala @@ -87,13 +87,11 @@ object CronLexer extends RegexParsers with BaseParser { } private lazy val tokens: Parser[List[CronToken]] = - phrase( - rep1( - number | text | hyphen | slash | comma | asterisk | questionMark | blank - ) + rep1( + number | text | hyphen | slash | comma | asterisk | questionMark | blank ) - def tokenize(expr: String): Either[ParseFailed, List[CronToken]] = + def tokenize(expr: String): Either[_root_.cron4s.Error, List[CronToken]] = parse(tokens, expr) match { case err: NoSuccess => Left(handleError(err)) case Success(result, _) => Right(result) diff --git a/modules/core/shared/src/main/scala/cron4s/parsing/parser.scala b/modules/core/shared/src/main/scala/cron4s/parsing/parser.scala index 98ae1997..c1f41116 100644 --- a/modules/core/shared/src/main/scala/cron4s/parsing/parser.scala +++ b/modules/core/shared/src/main/scala/cron4s/parsing/parser.scala @@ -165,12 +165,14 @@ object CronParser extends Parsers with BaseParser { any[F].map(any2FieldWithAny) val cron: Parser[CronExpr] = { - (field(seconds) <~ blank) ~ - (field(minutes) <~ blank) ~ - (field(hours) <~ blank) ~ - (fieldWithAny(daysOfMonth) <~ blank) ~ - (field(months) <~ blank) ~ - fieldWithAny(daysOfWeek) ^^ { + phrase( + (field(seconds) <~ blank) ~! + (field(minutes) <~ blank) ~! + (field(hours) <~ blank) ~! + (fieldWithAny(daysOfMonth) <~ blank) ~! + (field(months) <~ blank) ~! + fieldWithAny(daysOfWeek) + ) ^^ { case sec ~ min ~ hour ~ day ~ month ~ weekDay => CronExpr(sec, min, hour, day, month, weekDay) } diff --git a/tests/shared/src/test/scala/cron4s/CronSpec.scala b/tests/shared/src/test/scala/cron4s/CronSpec.scala index 96697043..dc9f44f3 100644 --- a/tests/shared/src/test/scala/cron4s/CronSpec.scala +++ b/tests/shared/src/test/scala/cron4s/CronSpec.scala @@ -23,19 +23,57 @@ import cron4s.syntax.all._ import org.scalatest.matchers.should.Matchers import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.prop.TableDrivenPropertyChecks import scala.util.{Failure, Success} /** * Created by alonsodomin on 12/04/2017. */ -object CronSpec { +object CronSpec extends TableDrivenPropertyChecks { import CronField._ - final val AllEachExpr = "* * * * * *" - final val AnyDaysExpr = "* * * ? * ?" - - final val InvalidExprs = List(AllEachExpr, AnyDaysExpr) + final val AllEachExpr = "* * * * * *" + final val AnyDaysExpr = "* * * ? * ?" + final val TooLongExpr = "* * * ? * * *" + final val TooShortExpr = "* * * * *" + + final val InvalidExprs = + Table( + ("description", "expression", "expected error"), + ( + "all stars", + AllEachExpr, + InvalidCron( + NonEmptyList.of( + InvalidFieldCombination( + "Fields DayOfMonth and DayOfWeek can't both have the expression: *" + ) + ) + ) + ), + ( + "symbol ? at two positions", + AnyDaysExpr, + InvalidCron( + NonEmptyList.of( + InvalidFieldCombination( + "Fields DayOfMonth and DayOfWeek can't both have the expression: ?" + ) + ) + ) + ), + ( + "too long expression", + TooLongExpr, + ParseFailed("end of input expected", 2, Some(" ")) + ), + ( + "too short expression", + TooShortExpr, + ExprTooShort + ) + ) final val ValidExpr = CronExpr( SeveralNode(BetweenNode[Second](ConstNode(17), ConstNode(30)), ConstNode[Second](5)), @@ -45,38 +83,26 @@ object CronSpec { EachNode[Month], AnyNode[DayOfWeek] ) + } class CronSpec extends AnyFlatSpec with Matchers { import CronSpec._ - "Cron" should "not parse an expression with all *" in { + "Cron" should "not parse an invalid expression" in { val expectedError = InvalidFieldCombination("Fields DayOfMonth and DayOfWeek can't both have the expression: *") - val parsed = Cron(AllEachExpr) - parsed shouldBe Left(InvalidCron(NonEmptyList.of(expectedError))) - - val parsedTry = Cron.tryParse(AllEachExpr) - parsedTry should matchPattern { case Failure(InvalidCron(_)) => } - - intercept[InvalidCron] { - Cron.unsafeParse(AllEachExpr) - } - } - - it should "not parse an expression with ? in both DayOfMonth and DayOfWeek" in { - val expectedError = - InvalidFieldCombination("Fields DayOfMonth and DayOfWeek can't both have the expression: ?") - - val parsed = Cron(AnyDaysExpr) - parsed shouldBe Left(InvalidCron(NonEmptyList.of(expectedError))) + forAll(InvalidExprs) { (desc: String, expr: String, err: Error) => + val parsed = Cron(expr) + parsed shouldBe Left(err) - val parsedTry = Cron.tryParse(AnyDaysExpr) - parsedTry should matchPattern { case Failure(InvalidCron(_)) => } + val parsedTry = Cron.tryParse(expr) + parsedTry should matchPattern { case Failure(`err`) => } - intercept[InvalidCron] { - Cron.unsafeParse(AnyDaysExpr) + intercept[Error] { + Cron.unsafeParse(expr) + } } } From 8e10d4cff78a60194c02ec01e5d78d7cd176c64a Mon Sep 17 00:00:00 2001 From: alonsodomin Date: Fri, 21 Aug 2020 14:35:14 +0100 Subject: [PATCH 2/2] Format code --- build.sbt | 3 +-- modules/core/shared/src/main/scala/cron4s/parsing/base.scala | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/build.sbt b/build.sbt index bcdf788f..6acca5b2 100644 --- a/build.sbt +++ b/build.sbt @@ -68,8 +68,7 @@ val commonSettings = Def.settings( consoleImports := Seq("cron4s._"), initialCommands in console := consoleImports.value .map(s => s"import $s") - .mkString("\n"), - scalafmtOnCompile := true + .mkString("\n") ) ++ CompilerPlugins.All lazy val commonJvmSettings = Seq( diff --git a/modules/core/shared/src/main/scala/cron4s/parsing/base.scala b/modules/core/shared/src/main/scala/cron4s/parsing/base.scala index ef417c69..396f2c71 100644 --- a/modules/core/shared/src/main/scala/cron4s/parsing/base.scala +++ b/modules/core/shared/src/main/scala/cron4s/parsing/base.scala @@ -21,7 +21,7 @@ import scala.util.parsing.combinator.Parsers import scala.util.parsing.input.{Position, NoPosition} private[parsing] trait BaseParser extends Parsers { - protected def handleError(err: NoSuccess): _root_.cron4s.Error = { + protected def handleError(err: NoSuccess): _root_.cron4s.Error = err.next.pos match { case NoPosition => ExprTooShort case pos: Position => @@ -32,5 +32,4 @@ private[parsing] trait BaseParser extends Parsers { } ParseFailed(err.msg, position, found) } - } }