HomeMesosphereNo notifications. 4 unresolved issues.

validation constraints for numeric, string, and array types
ClosedAll Users

Authored by jdef on Dec 19 2016, 9:17 AM.

Details

Summary

JSON validation rendered by the RamlTypeGenerator should take into consideration
the additional constraints specified in RAML (e.g. minItems, max, pattern).
This will reduce boilerplate in higher-level validation code and consistently
enforce constraints declared in RAML at JSON-parsing time.

Test Plan

sbt test

sample of generated code:

object Network {
  import play.api.libs.json.Reads._
  import play.api.libs.functional.syntax._
  implicit object playJsonFormat extends play.api.libs.json.Format[Network] {
    def reads(json: play.api.libs.json.JsValue): play.api.libs.json.JsResult[Network] = {
      val name = json.\("name").validateOpt[String](play.api.libs.json.JsPath.read[String](maxLength[String](63) keepAnd minLength[String](1) keepAnd pattern("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$".r)))
...

demo:

scala> play.api.libs.json.Json.parse("""{ "name": "this is not a valid name. this is not a valid name. this is not a valid name. this is not a valid name. this is not a valid name." }""").as[mesosphere.marathon.raml.Network]
play.api.libs.json.JsResultException: JsResultException(errors:List((/name,List(ValidationError(List(error.maxLength),WrappedArray(63)), ValidationError(List(error.pattern),WrappedArray())))))
  at play.api.libs.json.JsReadable$$anonfun$2.apply(JsReadable.scala:23)
  at play.api.libs.json.JsReadable$$anonfun$2.apply(JsReadable.scala:23)
  at play.api.libs.json.JsResult$class.fold(JsResult.scala:73)
  at play.api.libs.json.JsError.fold(JsResult.scala:13)
  at play.api.libs.json.JsReadable$class.as(JsReadable.scala:21)
  at play.api.libs.json.JsObject.as(JsValue.scala:76)
  ... 42 elided

Diff Detail

Repository
rMARATHON marathon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jdef retitled this revision from to validation constraints for numeric, string, and array types.Dec 19 2016, 9:17 AM
jdef updated this object.
jdef edited the test plan for this revision. (Show Details)
aquamatthias accepted this revision.Dec 19 2016, 12:28 PM

Thanks. I did not try this out. Does it work as expected?

This revision is now accepted and ready to land.Dec 19 2016, 12:28 PM
jdef updated this revision to Diff 1365.Dec 19 2016, 4:47 PM
  • more readable constraint code
jdef edited the test plan for this revision. (Show Details)Dec 19 2016, 4:55 PM

I updated the "Test Plan" section with some sample code and validation output.

jdef added a comment.Dec 19 2016, 4:58 PM

More generated code .. means less coverage?

[error] Coverage is below minimum [64.42% < 67.0%]
java.lang.RuntimeException: Coverage minimum was not reached
	at scoverage.ScoverageSbtPlugin$.scoverage$ScoverageSbtPlugin$$checkCoverage(ScoverageSbtPlugin.scala:261)
	at scoverage.ScoverageSbtPlugin$$anonfun$coverageReport0$1.apply(ScoverageSbtPlugin.scala:125)
	at scoverage.ScoverageSbtPlugin$$anonfun$coverageReport0$1.apply(ScoverageSbtPlugin.scala:104)
	at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
	at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
	at sbt.std.Transform$$anon$4.work(System.scala:63)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
	at sbt.Execute.work(Execute.scala:237)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
	at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
[error] (marathon/*:coverageReport) Coverage minimum was not reached

Bad ass. Seems like we can delete some of the wix validation now too?

More generated code is less coverage... no objections to lowering for this change.

jdef added a comment.Dec 19 2016, 9:33 PM

I'd rather delete wix validation code in a follow-up PR

jdef updated this revision to Diff 1369.Dec 19 2016, 9:35 PM
  • lower code coverage threshold beause we have more auto-generated code now
jdef updated this revision to Diff 1380.Dec 20 2016, 1:45 AM
  • fix RAML spec to get integration tests passing
jdef updated this revision to Diff 1383.Dec 20 2016, 2:14 AM
  • fix unit tests that broke because they were using invalid pod definitions
jdef requested a review of this revision.Dec 20 2016, 2:43 AM

several changes to the RAML spec were required because the serialized output was no longer readable (the new RAML-based validations failed against some serialized output)

one bugfix that we should probably backport to 1.4: PodInstanceStatus.instances.id should be string, not string.Name

src/test/scala/mesosphere/FutureTestSupport.scala
16

without this change, running the tests individually and examining log output is very, very difficult (read: noisy)

Bad ass. Seems like we can delete some of the wix validation now too?

More generated code is less coverage... no objections to lowering for this change.

src/test/scala/mesosphere/FutureTestSupport.scala
16

Checking for the result every 2 seconds is a significant slowdown. Please make this much faster.

This revision is now accepted and ready to land.Dec 20 2016, 2:46 AM
aquamatthias accepted this revision.Dec 20 2016, 4:52 PM

Looks good - thanks!

build.sbt
154 ↗(On Diff #1383)

Does not have to be on that Patch: generated sources should not count into coverage.

build.sbt
154 ↗(On Diff #1383)

They unfortunately do count

aquamatthias added inline comments.Dec 20 2016, 5:09 PM
build.sbt
154 ↗(On Diff #1383)
build.sbt
154 ↗(On Diff #1383)

We have actual code in the same package though ;)

Looks like the generator could use:

// $COVERAGE-OFF$

at the top of every file though.

jdef added inline comments.Dec 21 2016, 10:05 PM
build.sbt
154 ↗(On Diff #1383)

i'll give this a shot. better than artificially adjusting the expected coverage percentage

src/test/scala/mesosphere/FutureTestSupport.scala
16

define "much faster"? where's the value threshold? 0.5 seconds? 1 second? yes, tests should run quickly but it should also be easy to debug them. the old defaults did not lend themselves well to debugging

src/test/scala/mesosphere/FutureTestSupport.scala
16

I see no harm in the result getting checked often. The timeout is long, it just checks often (milliseconds if I'm not mistaken)

jdef updated this revision to Diff 1424.Dec 21 2016, 10:31 PM
  • use COVERAGE-OFF statement to subvert code coverage calculator w/ respect to RAML-generated scala files
jdef added inline comments.Dec 22 2016, 12:32 AM
src/test/scala/mesosphere/FutureTestSupport.scala
16

I changed this to 2s precisely because there IS harm in retries on the order of milliseconds - it significantly complicates debugging the tests.

i've reviewed overall build and test times between this and prior builds (of other SHAs, unrelated to this diff). there is no significant difference in build/test times. the time to execute unit tests (which I run very often, locally) are not noticably impacted by this change (I see no difference on my machine). so if you think there's a "significant slowdown" please provide some numbers to back that up because I just don't see it in the numbers that I'm looking at

jdef updated this revision to Diff 1430.Dec 22 2016, 1:03 AM

rebased to latest master

This revision was automatically updated to reflect the committed changes.