#2735 - replacement of ModelValidation

[lxwbr]

@aquamatthias @kolloch @gkleiman @meichstedt Hey guys, we decided to give Accord Validation a try and replace our ModelValidation, which was still using Java's BeanValidation. The new library is capable of keeping track of the path down to the occurrence of the error. It is also using macros and detects property names automatically, so there is no need in propagating them explicitly. Validation rules can be constructed in an easy manner like this:

implicit val appDefinitionValidator = validator[V2AppDefinition] { appDef =>
    appDef.id is valid
    appDef.dependencies is valid
    appDef.upgradeStrategy is valid
    appDef.storeUrls is every(urlsCanBeResolvedValidator)
}

As it is an implicit validator, you could easily validate an appDefinition from type V2AppDefinition like this:

import mesosphere.marathon.api.v2.Validation._
...
validate(appDefintion)
...

We already tried the new validation out on some API methods. E.g. a following POST request in /v2/apps

{
  "id": "/test",
  "dependencies": ["-_","213","---_--_"]
}

will return an error sequence, which looks very similar to the one we are already used to:

{
  "errors": [
    {
      "attribute": "dependencies[0]",
      "error": "must match regular expression '^(([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])\\.)*([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])|(\\.|\\.\\.)$'"
    },
    {
      "attribute": "dependencies[2]",
      "error": "must match regular expression '^(([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])\\.)*([a-z0-9]|[a-z0-9][a-z0-9\\-]*[a-z0-9])|(\\.|\\.\\.)$'"
    }
  ]
} 

How do you feel about the new way of validating objects? You could have a look at e.g. AppsResource.scala in order to see the new validation in action.

[aquamatthias]

Shouldn't you compare num to the number of apps ?

[lxwbr]

Thanks, you are right. I committed a change on that.

[aquamatthias]

What do you think @kolloch @meichstedt @gkleiman ??

[kolloch]

@Alexanderweber I just read your nice summary and it looks very good.

You say that the new error messages "look very similar". Does that mean they contain the same JSON fields in error responses, just that the messages are slightly different? We should only add fields to the JSON wherever possible to keep compatibility.

Furthermore, I would like to ensure that the following things are possible (I would expect it but better be certain). You don't have to address them at once in the patch, I am more interested in feasibility.

• Error codes along with error messages: Currently, our error reporting sucks. I would much like to report an error code along with the error message. That could be the I18N text ID. It would also be nice to have a map of named arguments for the I18N template in the JSON, e.g.:

{
  "errors": [
    {
      "attribute": "dependencies.[0].path",
      "errorCode": "string.maxLengthExceeded", // just a silly example
      "errorArgs": {
          "value": "/this/is/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/long/for/a/path",
         "maxLength": 256
      },
      "error": "The value '/this/is/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/really/long/for/a/path' exceed the maximum length of 256 characters."
    }
  ]
}

That design allows automated parsing of errors and also allows (optionally) I18n by the frontend.

• How can we make sure that the fieldNames in our messages correspond to field names in our JSON?
• "Transforming" the error paths according to other API versions: We may have to support multiple API versions in the same code base that use different JSON attribute names or even a different structure.

[lxwbr]

@kolloch Hey Peter, I did mean exactly that - JSON field names will stay the same, but the content of messages might be slightly different.

• I am not sure about the error codes at this point to be honest. Violation classes of the Accord framework expect Strings as error messages. I was actually talking to @aquamatthias about related topic: if we could match a violation against its type instead of the message string in our testing routines. An error code would do the job there. But I need to think about a possible realisation. As of this issue the feature is implemented in a separate branch, so there might be a i18n support in future releases.
• The correspondence is provided by property names in validated classes. The framework figures out the property name through macros. As long as property names of the case class correspond to the JSON field names (which is the case as we are using play json, correct me if I am wrong), everything should correspond correctly.
• That might be a problem, but I can still imagine it being possible, as field names can be explicitly be set during the validation as well.

[mwasn]

@kolloch Regarding error codes: The lack of error codes respectively missing documentation of those has actually been a demand of a customer. I´m going to create a separate story for implementation and documentation.

[lxwbr]

Merged master into this branch. Checks passed. @aquamatthias Please get back to me if you are ready to merge this pull request.

[aquamatthias]

Forgot to remove comment or test?

[lxwbr]

@wendigo Nice to hear. Let me know if you'll have any issues using the new approach. I am hoping for a merge during today.

[aquamatthias]

This comment here is:

• outdated: since the problem with the seq is not valid any longer
• in the wrong place: it probably should be moved to V2AppUpdate.appDefinitionValidator, where we don't do appDef.healthChecks is valid

[lxwbr]

@aquamatthias Regarding your second remark: do you mean the comment should be moved to appDefintionValidator? I did do so in c4cb9e2, it is now in V2AppDefinition.appDefinitionValidator.

[wendigo]

@Alexanderweber sure thing!

[aquamatthias]

Hey @Alexanderweber sorry that I did not mention that before: why did you remove the whole test class here? Would it not make sense to use the new validation logic here as well?

[lxwbr]

@aquamatthias Sorry for late reply. As I was deleting the whole validation folder with annotation validation, I did not pay attention, that there was also one test included in there. Sorry for that, I added it back again adjusting it to new validation model. I need to merge the master into the branch again, I'll do so by the end of the day.

[aquamatthias]

@Alexanderweber: Thanks. Please rebase.

[aquamatthias]

This is a great addition. Thanks!

[wendigo]

Nice! :)

[holograph]

Hi everyone, sorry to raise the dead but I missed the original discussion when it was still ongoing. I'm the maintainer of Accord, and would absolutely love your input on how to add these features cleanly!

In particular, Accord is currently quite naïve about how it handles violations, in that it simply encodes them as strings. There's an open issue (wix-incubator/accord#21) that deals with the various implementation options, but lacking external input I was never sufficiently happy with any of them. It's a somewhat long-winded discussion, but if you have some fresh ideas on what this should look like (e.g. how to maintain type safety and/or offer a clean DSL), please do weigh in! There's also a Gitter channel if you prefer the more personal touch.

Lastly, I'm curious about the "path" generation/transformation properties mentioned by @kolloch; Accord automatically generates a string description for each "thing" being validated. A "thing" could be property access, modifier (.each or .size) or an arbitrary expression, and in all cases Accord does its best to figure out a good representation of the inspected property. This can be overridden per-rule using as, but the description generation mechanism is not yet pluggable, is fairly naïve, and could use a lot of improvement. Our own services don't normally externalize validation errors, and so again I would dearly appreciate input on how this could be modelled.

Thank you for trying out Accord, I hope you've been enjoying it so far!

[nnordrum]

This also appears to have broken relative paths in dependencies, or at least changed how they are represented.

"dependencies":["../hub"]

I have this in my deployment of a selenium grid, and this worked before 0.15.3, but doesn't now.

How do you represent this relatively now? All the examples and tests seem to be full path, which I would rather not do for what I think would think would be relatively obvious reasons.