Fix scalapb-validate setup instructions by bumping sbt-protoc #1264
Conversation
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
bjaglin
force-pushed
the
scripted-pgv
branch
2 times, most recently
from
938a5d2 to
70ba7a2
Compare
bjaglin
changed the title
bjaglin
changed the title
bjaglin
force-pushed
the
scripted-pgv
branch
2 times, most recently
from
3ec851c to
426d253
Compare
|
Investigating |
|
OK TO TEST |
bjaglin
force-pushed
the
scripted-pgv
branch
from
426d253 to
9fb3944
Compare
The lack of path separator was a false track (and a misleading ivy log output?!). The regression was caused by a subtle behavior change exposed by thesamet/sbt-protoc@80824b0#diff-a241d0444c16c14576b308aa58a0f4f2ed70a3a6021454cd8a71882764513144R582-R583, causing an eager evaluation of the resolver, which in the case of |
bjaglin
changed the title
bjaglin
changed the title
bjaglin
force-pushed
the
scripted-pgv
branch
2 times, most recently
from
0855562 to
a648ac9
Compare
|
cc @gontard |
| @@ -29,7 +29,7 @@ val root = project.in(file(".")) | |||
| "org.scalatest" %% "scalatest" % "3.1.2" % "test", | |||
| "org.scalatestplus" %% "junit-4-12" % "3.1.2.0" % "test" | |||
| ), | |||
| Compile / PB.generate := ((Compile / PB.generate) dependsOn ( | |||
| PB.artifactResolver := (PB.artifactResolver dependsOn ( | |||
| @@ -0,0 +1 @@ | |||
| > compile | |||
There was a problem hiding this comment.
is there more we can test beyond 'it compiles'?
There was a problem hiding this comment.
good point, I'll add a basic Main
There was a problem hiding this comment.
First commit amended. I wanted to go even further and verify/show that validate_at_construction could be enabled so that clients sending invalid payloads would automatically get an error (maybe with a cutom exception handler to map scalapb.validate.ValidationFailurescalapb.validate.ValidationException to INVALID_ARGUMENT) without any specific custom code on the server implementation, but I ran into scalapb/scalapb-validate#70 (comment) so that will be for the next round.
There was a problem hiding this comment.
maybe with a cutom exception handler to map
scalapb.validate.ValidationFailuretoINVALID_ARGUMENT)
that might even be reasonable to make part of the default handler
There was a problem hiding this comment.
that might even be reasonable to make part of the default handler
I got the type wrong, it's scalapb.validate.ValidationException, which extends com.google.protobuf.InvalidProtocolBufferException. Would it make sense to map InvalidProtocolBufferException to INVALID_ARGUMENT ?
There was a problem hiding this comment.
scalapb/scalapb-validate@8529a55 is just out, so I took the opportunity of pushing
e4a39c6 to this PR.
End-to-end testing asserting that a payload violating a PGV rule is rejected automatically with a INVALID_ARGUMENT is not trivial in the scripted, as it requires wiring a client, a server, and forging an invalid payload, which is impossible with validate_at_construction. I guess an option would be to use Java and Java stubs (with no PGV support) as client. I won't have time to look at that in the near future.
There was a problem hiding this comment.
that sounds entirely reasonable!
Fixes scripted 10-scalapb-validate, see * scalapb/ScalaPB#873 (comment) * scalapb/scalapb-validate#70 Other changes * cacheClassLoaders is now deprecated, see thesamet/sbt-protoc@2a730a4 * Forcing recompilation is no longer necessary as the cache gets invalidated if a sandbox genererator classpath is updated, see thesamet/sbt-protoc@80824b0 * The new classloader caching implementation resolves sandboxed generators artifacts earlier, even if they are no sources. That is the case for Test / protocGenerate, which was starting before codeGenProject / Compile / publishLocal, causing: > Error downloading com.lightbend.akka.grpc:akka-grpc-codegen_2.12: > 1.1.0-5-c5975cd5
bjaglin
force-pushed
the
scripted-pgv
branch
from
a648ac9 to
c0f2f73
Compare
Follows #1248 which turned out to give non-working instructions for scala/sbt (see scripted failure on first commit). The instructions (untouched) only work with
See scalapb/scalapb-validate#70