gRPC request validation (server-side)

[anjeyy]

I came across some repeating pattern, a request validation. Clients send a gRPC request and with this functionality just implement GrpcConstraintValidator with validation-logic and annotate with @GrpcConstraint and the validation is being processed. By the time the request enters implemented service via @GrpcService, the request was validated successful. Removes boilerplate-code and preserves the real business-logic inside @GrpcService.

Following a simple use case:

Proto-file - Consider your proto-file, here just a simple one to illustrate use-case

syntax = "proto3";
option java_multiple_files = true;
package com.github.anjeyy.proto.document;

import "google/protobuf/empty.proto";

message DocumentRequest{
  string docId = 1;
}

message DocumentResponse{
  string docId = 1;
  string title = 2;
  string person = 3;
  int32 filesize = 4;
}

service DocumentService{
  rpc getDocumentById(DocumentRequest) returns (DocumentResponse);
}

Now all you need to do, create Validator classes DocumentRequestValidator

@GrpcConstraint
public class DocumentRequestValidator implements GrpcConstraintValidator<DocumentRequest> {

    @Override
    public boolean isValid(DocumentRequest request) {
        return StringUtils.isNotBlank(request.getDocId());
    }
}

There can also be multiple Validator of the same Request-Message

@GrpcConstraint
public class SecondDocumentRequestValidator implements GrpcConstraintValidator<DocumentRequest> {

    @Override
    public boolean isValid(DocumentRequest request) {
        try{
            UUID.fromString(request.getDocId());
            return true;
        } catch (IllegalArgumentException exception){
            return false;
        }
}

Side-Note: If an uncatched exception is thrown inside a Validator the return type to the client is Status.INTERNAL.., otherwise it's Status.INVALID_ARGUMENT.. for failed validations.

---

Currently the implementation works well - the response to the client is exactly how it is inteded to be.

However, i came across a (cosmetic?) problem and i do not now how to resolve it elegantly, may be you guys have some suggestions. Situation: The validation fails (either with returning false from GrpcConstraintValidator.isValid(....) or a exception is trown inside the validation), in both cases the serverCall.close(...) is called and the console prints following thing

2021-01-23 21:26:45.810 ERROR [my-grpc-server,,,] 9804 --- [ault-executor-0] io.grpc.internal.SerializingExecutor     : Exception while executing runnable io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed@73f71ba0

java.lang.IllegalStateException: call already closed
	at com.google.common.base.Preconditions.checkState(Preconditions.java:511) ~[guava-29.0-android.jar:na]
	at io.grpc.internal.ServerCallImpl.closeInternal(ServerCallImpl.java:209) ~[grpc-core-1.31.1.jar:1.31.1]
	at io.grpc.internal.ServerCallImpl.close(ServerCallImpl.java:202) ~[grpc-core-1.31.1.jar:1.31.1]
	at io.grpc.PartialForwardingServerCall.close(PartialForwardingServerCall.java:48) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCall.close(ForwardingServerCall.java:22) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCall$SimpleForwardingServerCall.close(ForwardingServerCall.java:39) ~[grpc-api-1.31.1.jar:1.31.1]
	at net.devh.boot.grpc.server.metric.MetricCollectingServerCall.close(MetricCollectingServerCall.java:64) ~[grpc-server-spring-boot-autoconfigure-2.10.1.RELEASE.jar:2.10.1.RELEASE]
	at brave.grpc.TracingServerInterceptor$TracingServerCall.close(TracingServerInterceptor.java:120) ~[brave-instrumentation-grpc-5.12.4.jar:na]
	at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:174) ~[grpc-stub-1.31.1.jar:1.31.1]
	at io.grpc.PartialForwardingServerCallListener.onHalfClose(PartialForwardingServerCallListener.java:35) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:23) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener$SimpleForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:40) ~[grpc-api-1.31.1.jar:1.31.1]
	at brave.grpc.TracingServerInterceptor$TracingServerCallListener.onHalfClose(TracingServerInterceptor.java:153) ~[brave-instrumentation-grpc-5.12.4.jar:na]
	at io.grpc.PartialForwardingServerCallListener.onHalfClose(PartialForwardingServerCallListener.java:35) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:23) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener$SimpleForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:40) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.PartialForwardingServerCallListener.onHalfClose(PartialForwardingServerCallListener.java:35) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:23) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.ForwardingServerCallListener$SimpleForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:40) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.Contexts$ContextualizedServerCallListener.onHalfClose(Contexts.java:86) ~[grpc-api-1.31.1.jar:1.31.1]
	at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:331) ~[grpc-core-1.31.1.jar:1.31.1]
	at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:814) ~[grpc-core-1.31.1.jar:1.31.1]
	at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) ~[grpc-core-1.31.1.jar:1.31.1]
	at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123) ~[grpc-core-1.31.1.jar:1.31.1]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[na:na]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[na:na]
	at java.base/java.lang.Thread.run(Thread.java:834) ~[na:na]

My guess is there is not much we can do, since this message is from io.grpc.internal, but if you have any ideas, please let me know. Like i said for me this is more of a cosmetic error, sinde it can be printed more than once if the close is delegated or being called multiple times.

I'm interesseted in your feedback or in any way what improvements can be made. If you have questions, just let me know.

---

TODO

• [x] implementation ✔️
• [x] javadoc ✔️
• [ ] discussion ➖ (in progress)
• [ ] unit tests ➖ (in progress)
• [ ] docs with example 🟡
• [ ] review 🟡

[ST-DDT]

This looks like an interesting proposal. Is there a special reason, why you didn't choose existing validation implementations?

Such as Java Bean validation or envoyproxy's validation (Source)

[anjeyy]

Interesting in a good or bad way? 🙂 I thought about the javax validation, since it is also used in the standard spring boot library, validating ReST requests and/or other Classes.

But I decided not to use javax validations, because the proto files generate the Java classes used for requests and there is no possibility to add validation annotation or other code to the generated classes. I mean you could validate them programmatically like I did with the ones right now, but it would have added an extra library in contrast to just simply implement the given GrpcConstraintValidator.

The second one mentioned ~~I was not aware of.~~ I was aware of, actually came across few days ago. My thought was to give a choice to whether validate in the proto file with new Syntax or enable developer to validate via Java code. (plus the library is in alpha, they state changes can happen often).

In general, what are your thoughts on this @ST-DDT?

[ST-DDT]

Interesting in a good or bad way? 🙂

In a good way. I already thought about adding support for it, I just wasn't sure which direction grpc would go regarding validation: https://github.com/grpc/grpc-java/issues/3926

But I decided not to use javax validations, because the proto files generate the Java classes used for requests and there is no possibility to add validation annotation or other code to the generated classes.

Well, there is actually no need to add the annotations to the classes itself. You can also use protoc to generate companion classes, such as the validators generated by envoyproxy. These could also be used to generate a programmatic configuration.

https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/?v=7.0#section-programmatic-constraint-definition

I'm actually quite torn which way is the best, but I would like to reuse existing APIs if possible to make this library/it's features more familiar for the users.

Also if the validation configuration is embedded into the proto file, then there won't be confusion why the server rejected a certain value and other implementations can reuse the validation constraints. grpc/proto is centered around .proto files, so IMO the validation should be added there as well.

[anjeyy]

Well, there is actually no need to add the annotations to the classes itself. You can also use protoc to generate companion classes, such as the validators generated by envoyproxy. These could also be used to generate a programmatic configuration.

I know that you can generate them programmatically, my thought was keep it as lightweight as possible since it would only contain one interface. But of course if you decide to use the generated proto and it works in combination with the javax validation, it does make sense to use it.

I'm actually quite torn which way is the best, but I would like to reuse existing APIs if possible to make this library/it's features more familiar for the users.

I mean if you look at it this way, most user which are using grpc-spring-boot-starter will use given features by the framework to validate their request or implement their own stuff. I think not that many people will be forwarded from the proto validation library/framework to the grpc-spring-boot-starter. So i think this is kinda pathbreaking, since you can decide what to offer.

Also if the validation configuration is embedded into the proto file, then there won't be confusion why the server rejected a certain value and other implementations can reuse the validation constraints. grpc/proto is centered around .proto files, so IMO the validation should be added there as well.

This is true, it offers some kind of documentation like a swagger file. For me this is kinda twisted, it offers the mentioned benefits, but you limit developer to use it and offer no alternative with explicit Java code. Would love to head some feedback about it.

What do you think about offering both ways?

[ST-DDT]

Also if the validation configuration is embedded into the proto file, then there won't be confusion why the server rejected a certain value and other implementations can reuse the validation constraints. grpc/proto is centered around .proto files, so IMO the validation should be added there as well.

This is true, it offers some kind of documentation like a swagger file. For me this is kinda twisted, it offers the mentioned benefits, but you limit developer to use it and offer no alternative with explicit Java code. Would love to head some feedback about it.

What do you think about offering both ways?

Having both ways might be a good solution. Maybe outline all three variants.

• Custom validators as proposed in this PR
• javax.validation
• envoyproxy (add the related bean to a config with @GrpcGlobalServerInterceptor)

But I would like to put the discussion down until 2.12 is released, because I'm really busy currently.

[anjeyy]

Hi @ST-DDT,

any new updates in this matter? No need to rush though. 🙂

[ST-DDT]

I'm really busy this month, so I might not have the time to get to it soon. But I have this on my ToDo list.

[sangyongchoi]

@anjeyy hello. I am a Korean developer. Found this PR while looking for grpc request validation. I made a validator in another open source by referring to this source code. If it's okay, can I send a PR to another open source? I'm asking because I reference your source!

[anjeyy]

@sangyongchoi Hey there. For me there is no issue in using this code snippet, if it benefits you. Do you have any concerns @ST-DDT ?

Is there anything new regarding validations in terms of already mentioned libraries? @ST-DDT

[sangyongchoi]

@anjeyy There is nothing new in terms of libraries. In my commit, I added a function to send a message when validation fails. thank you 😄

[ST-DDT]

Do you have any concerns @ST-DDT ?

No, I'm fine with it. That's what OpenSource is for.

[sangyongchoi]

@ST-DDT thank you 😄

[ddcprg]

@anjeyy @ST-DDT any joy here? This has been opened for a while

[ST-DDT]

@ddcprg Please explain what you are asking for exactly.

• Are you asking for reviews? This is still a draft. And the previous review suggestions haven't been imlemented yet.
• Are you asking for a progress update?
• Are you interested in continuing this PR?

[ddcprg]

@ST-DDT I was looking for a progress update. I can take a look at the suggestions if @anjeyy is no longer going to work on this change

[genuss]

I believe, there is no need to proceed with PR any further as protovalidate project should cover validation cases in a much abstract way. I'm talking about this one: https://github.com/bufbuild/protovalidate and specifically for JVM-based https://github.com/bufbuild/protovalidate-java

[ddcprg]

protovalidate falls short when it comes to add more complex validations since is all declarative, for example it is impossible to validate with declarative rules that the value of one of the request parameters is defined in another system, e.g. a database or third-party service. I think having both options also let users use whatever is more convenient for their use case