Getting indexed description when composing validators

[buildreactive]

I'm seeing a problem with inappropriately indexed descriptions when I try to compose several different validations. This shows up when I tried to build out the feedback on #82 into real code:

  def aValidGuestID: Validator[Int] = between(1, 99)

  def aValidPaxNumber(bookingNumber: String) = new Validator[String] {
    def apply(pax: String): Result =
      Try(pax.replace(BookingPrefix.concat(bookingNumber), "").toInt) match {
        case scala.util.Failure(_) => pax -> "is not a valid PAX number"
        case scala.util.Success(id) => aValidGuestID(id)
      }
  }

  def validForBookingNumber(bookingNumber: String) = validator[GuestDetail] { gd =>
    gd.paxNumber is aValidPaxNumber(bookingNumber)

    () // Shut up discarded non-Unit value warning
  }

  implicit val guestBookingValidator = validator[GuestBooking] { (p: GuestBooking) =>
    p.booking is valid
    p.guestDetails is notEmpty
    p.guestDetails.size is between(1, 5)
    p.guestDetails.each is valid
     p.guestDetails.each is validForBookingNumber(p.booking.bookingNumber)

    () // Shut up discarded non-Unit value warning
  }

The above code generates the following error during runtime:

java.lang.IllegalArgumentException: Cannot combine description 'Indexed(0,AccessChain(WrappedArray(paxNumber)))' with 'AccessChain(WrappedArray(guestDetails))'

I tried to work around the problem by using gd.paxNumber as “paxNumber” is aValidPaxNumber(bookingNumber) but this didn't solve the problem (although it does change the result ever so slightly):

java.lang.IllegalArgumentException: Cannot combine description 'Indexed(0,Explicit(paxNumber))' with 'AccessChain(WrappedArray(guestDetails))

This is posing a nasty little problem for me in our current project – it would be great if a solution can be developed... Thanks!

[buildreactive]

FYI we are using Scala 2.11.8.

[holograph]

Managed to reproduce the problem, I'll follow up when I have something useful to say...

[holograph]

Current status: Problem reproduced successfully and is well-understood.

The bad news: The description model is fundamentally limited, and I can't believe I hadn't thought of - or run into - this before; it basically cannot represent interleaved indexed- and named-access. To clarify, the example above logically fails to validate p.guestDetails(0).paxNumber, which has no representation in the current description model (it can only represent indexed access on the last indirection, e.g. p.guestDetails.paxNumber(0) which is obviously insufficient).

I'll try to code up a proper representation in the next day or two; unfortunately this will be a breaking API change, and I'm still wondering if this necessitates a (depressingly thin) 0.7 release.

[holograph]

TL;DR there's a workaround, see below.

I've managed to whittle this down to the following reproduction case:

case class TestElement( property: String )
case class CollectionOfIndirections( field: Seq[ TestElement ] )

val elementValidator = validator[ TestElement ] { c => c.property is notEmpty }
val collectionOfIndirectionsValidator =
  // Designed to produce descriptions such as coi.coll[0].property
  validator[ CollectionOfIndirections ] { coi => coi.field.each is elementValidator }

val sample = CollectionOfIndirections( Seq( TestElement( "" ) ) )
val result = validate( sample )( collectionOfIndirectionsValidator )    // <-- Exception here

The reason this has never come up is that Accord espouses using is valid with implicit resolution; valid wraps the results in a GroupViolation and nests subsequent validation issues, which is why this never came up.

So:

1. A valid workaround in your case is p.guestDetails.each is valid(validForBookingNumber(p.booking.bookingNumber)) (adjust for taste), which will work with 0.6 but give you nested group violations.
2. This brings the question of whether or not flattening violations to the upper layer (i.e no nesting but more complex descriptions) is a good idea and should even be allowed; on the other hand, allowing more detailed descriptions is less work than adjusting the API to forbid that in a type-safe way. I need to give it some more thought.

[holograph]

@zbeckman Does this workaround solve your immediate problem?

[buildreactive]

Well... Yes. But... ;-)

The workaround works, but it's bulky. So I'm torn between doing it "right" and just doing something much simpler:

  def validForBookingNumber(seq: Seq[GuestDetail], bookingNumber: String): Boolean = {
    Try {
      seq.map { guest =>
        (guest.paxNumber.replace(BookingPrefix.concat(bookingNumber), "").toInt) match {
          case n: Int if (n >= 0 && n <= 99) => true
          case _ => throw new Exception
        }
      }
    }.isSuccess
  }

meh... But yes, it does work, and I'll probably go with the "right" way if nothing else then I'll have a documented instance of how to do it. ;-)

[holograph]

@zbeckman Fixed! You should be able to verify with 0.7-SNAPSHOT. I'll wait on your feedback and then release 0.6.1.