grpc-java
grpc-java copied to clipboard
feat: add request validator
Motivation
validation of grpc requests is inconvenient
class HelloRequestGrpcService : HelloServiceGrpcKt.HelloServiceCoroutineImplBase() {
override suspend fun hello(request: HelloRequest): HelloResponse {
if(request.message != "valid request") {
throw StatusRuntimeException…
}
return super.hello(request)
}
}
// If implement a validator interceptor I think the source can be cleaned up.
class HelloRequestGrpcService : HelloServiceGrpcKt.HelloServiceCoroutineImplBase() {
override suspend fun hello(request: HelloRequest): HelloResponse {
return super.hello(request)
}
}
Modifications
add grpc request validator add grpc request validation interceptor
Result
You just need to implement a validator suitable for the request type.
if you are thinking of approving this or, tell me add test code. Any other suggestions are welcome.
Question
why fail compileJava ? It works file on my computer
- This will need to be reviewed in our API review meetings
- Please add appropriate copyright in all of your files
- "why fail compileJava ?" : could you paste the relevant error messages? I don't see any CI finished yet
@ejona86 consider adding this to the next API review meeting?
FYI: don't make too many changes yet, except for your own benefit. The validation portion itself seems unlikely to go in, at least similar to how it looks now. I do think there is probably value in having a helper for an interceptor to validate/process a request, though. It isn't the common case (that would be using metadata), but it happens enough and it is easy enough to get wrong.
@ejona86 hi, I have fun making this. I modify code that you reviewed. but i not found way that delay next (17 line in Request Validation Interceptor.java). can you give me hint or way?
Thanks for considering my opinion.
API review meeting:
- The validator part of this seems like it offers little value.
- It'd have more boilerplate, indirection, and complexity. Since each method tends to have its own request message type there's little sharing of validators so the code sample would require a lot more boilerplate; the sample is only short because the validation isn't written.
- It'd be less obviously correct. It'd be easy to accidentally forget to have validation for a type and it'd be hard to notice during code review or code inspection.
- We could have an interceptor utility that makes it easier to process request messages. Although it's unclear how it'd handle streaming, in terms of behavior and dealing with synchronization. It'd be best to have a few concrete use cases (validation in general could be one of them, like using protobuf annotations).
@ejona86 thanks you to sharing. Should I tell my use case?
ps. If unnessary, you can close this pr.
@sangyongchoi, if sharing your use-case helps explain how this would reduce boilerplate, then that would be helpful to share.
@ejona86 purpose of my code was the separation of responsibility. I think good that add way like @Min, @Max for reduce boilerplate.
Closing. As mentioned above we aren't that interested in this sort of validation framework as it isn't commonly a win. We're okay with this sort of thing existing and would be happy to have utilities that make it easier to create such a thing (like an interceptor tool that delays the RPC processing until the request is received).