Add support for runtime dispatch in validation rules

[holograph]

Sometimes it is necessary to dynamically decide on a validation rule based on a runtime value, for example when validating a polymorphic message. There are various syntactic and implementation considerations here, but at the very least the following syntax should be supported:

Pattern match

  case class Test(p: Int, n: String)

  val vt = validator[Test] { t =>
    t.p match {
      case 0 => t.n is empty
      case 1 => t.n should startWith("test")
    }
  }

Common control structures, e.g. if:

  case class Test(p: Int, n: String)

  val vt = validator[Test] { t =>
    if (t.p == 0)
      t.n is empty
    else
      t.n should startWith("test")
  }

[holograph]

As an aside, the following is a completely valid workaround with 0.5.x:

  val vt = validator[Test] { t =>
    ((t.p should be == 0) and (t.n is empty)) or
    ((t.p should be == 1) and (t.n should startWith("test")))
  }

[lxwbr]

We are eagerly interested in that feature and hope to see some progress on it in the next milestone. Related issue: https://github.com/mesosphere/marathon/issues/3388

[holograph]

I took a few stabs at it yesterday; I think the primary issue here is how to model the resulting violations to reflect the runtime branch choice. For the example above, which option is preferable?

Option A: Add branch information to rendered descriptions

This requires an overhaul of the description generation framework to make it pluggable/stateful (essentially the same mechanism required for #57). Example result would be:

// Note the "(for p=1)" suffix:
RuleViolation(value = "failure", description = "n (for p=1)", constraint = "must start with 'test'")

Disadvantages:

• Lots of groundwork required to rearchitect the description framework.
• Additional work required to add DSL support for customizing the descriptions.

Option B: Aggregate branch violations with a GroupViolation

Example output:

GroupViolation(
  value = 1,
  description = "p", 
  constraint = "Violations detected for branch",
  children = Set(
    RuleViolation(value = "failure", description = "n", constraint = "must start with 'test'")
  )
)

This is relatively simple to implement, but the result model provides no new semantics. The question is, are clients interested in a detailed result model (including branching information)? If so, there's option (C) below.

Option C: Add branch markers to result model

Branch(
  value = 1,
  description = "p",
  violations = Set(
    RuleViolation(value = "failure", description = "n", constraint = "must start with 'test'")
  )
)

This might be the cleanest option in terms of implementation and result; it doesn't include any information about the specific control structure in place, merely the expression which caused the branch (as rendered by the existing description framework) and its runtime value. The main disadvantage here is that this is essentially a breaking change (i.e any code dealing with results beyond success/failure will no longer compile).

[lxwbr]

First of all - thank you very much on trying to make Accord better, we really appreciate that!

I would agree on B or C equally. Regarding option C - we do a mapping of GroupViolation and RuleViolation to plain text in order to generate error messages for our JSON format. We would need to adjust that one in order to handle Branch as well, but I do not see a big issue here.

Our concerns are actually directed towards the DSL, meaning writing short, comprehensible validators. How would the validation part look like? Would it still be like:

  case class Test(p: Int, n: String)

  val vt = validator[Test] { t =>
    t.p match {
      case 0 => t.n is empty
      case 1 => t.n should startWith("test")
    }
  }

I was actually asked if there would be a nicer way of validating a pattern match like this.

Additionally your example for Common control structures, e.g. if: is also a typical pattern, where a nicer solution was asked for already.

[holograph]

That would depend on your definition of "nicer", or at least "nice enough". Another possible representation can be seen on #59:

val vt = validator[Test] { t =>
    t.n is conditional(t.p) {
      case 0 => empty
      case 1 => startWith("test")
    }
  }

Not sure this is any clearer or easier to grok... Obviously any alternative ideas would be appreciated.

[lxwbr]

I cannot think of any better way myself to be honest. Luckily that is being a rare case, so I do not see a lot of pressure in putting conciderable effort in here. I might let you know if things change here though.

/cc @meichstedt

[lxwbr]

Having a second thought on it, I really might prefer the conditional representation. Would be nice to see it in the next release.

[holograph]

Quite a lot of progress on this, in fact all that remains is cleanup and documentation. You can see the progress on the issue58 branch; all tests now pass, including stuff like this:

    case class ControlStructureTest( field1: Int, field2: String )

    // If-else support
    val ifElseChain = validator[ ControlStructureTest ] { cst =>
      if ( cst.field1 < 0 )
        cst.field2 should startWith( "-" )
      else if ( cst.field1 == 0 )
        cst.field2 is equalTo( "0" )
      else
        cst.field2 is notEmpty
    }

    // Full-blown pattern match support, including default cases and proper descriptions
    val patternMatchWithGuard = validator[ ControlStructureTest ] { cst =>
      cst.field1 match {
        case n if n < 0 => cst.field2 should startWith( "-" )
        case 0          => cst.field2 is equalTo( "0" )
        case n if n > 0 => cst.field2 is notEmpty
      }
    }

There's also support for conditional as described above, but honestly I'm not sure there's any point in retaining it since the match syntax is at least as sensible:

  case class GuardedConditionalTest( cond: Int, value: String )

  val guardedConditionalValidator = validator[ GuardedConditionalTest ] { gct =>
    conditional( gct.cond ) {
      case value if value < 0 => gct.value should startWith( "-" )
      case value if value == 0 => gct.value should be == "zero"
    }
  }

@zunder Your feedback would be greatly appreciated.

[holograph]

I ended up going with my gut and skipping the conditional clause. Cleaned up a whole swath of code, and I believe native control structures (if/else, pattern matching) are more natural.

[holograph]

Feature complete, will be released with 0.6 (hopefully this week).