gRPC request validation (server-side) #487

anjeyy · 2021-01-23T20:31:17Z

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

ST-DDT · 2021-01-24T17:04:53Z

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 · 2021-01-24T18:29:19Z

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 · 2021-01-24T20:05:14Z

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: grpc/grpc-java#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 · 2021-01-26T19:01:15Z

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 · 2021-01-30T22:53:05Z

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 · 2021-05-13T12:33:05Z

Hi @ST-DDT,

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

ST-DDT · 2021-05-14T09:17:36Z

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 · 2022-06-23T18:11:39Z

@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 · 2022-06-24T06:57:59Z

@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 · 2022-06-24T07:04:06Z

@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 · 2022-06-24T10:17:46Z

Do you have any concerns @ST-DDT ?

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

sangyongchoi · 2022-06-24T10:18:40Z

@ST-DDT thank you 😄

sangyongchoi

@anjeyy please confirm my comment

sangyongchoi · 2022-07-01T18:53:14Z

...nfigure/src/main/java/net/devh/boot/grpc/server/validation/RequestValidationInterceptor.java

+            Metadata headers,
+            ServerCallHandler<ReqT, RespT> next) {
+
+        Listener<ReqT> delegate = next.startCall(call, headers);


if what i know is right, reason printing call already closed is next.startCall is non thread-safe.

solution : https://groups.google.com/g/grpc-io/c/_osH2D6L9Ck

I'm not sure what exactly happens, but it's probably not related to mutli-threading, but "owning" the server call instance.
You only close the call, but you never tell the control structure, that you have done so, thus the request processing "continues" as normal. Then the close you have send kicks in and the server tries to properly close the connection, thus sending the close twice. Which is probably the exception you are seeing.

Instead of (or maybe in addition to) closing the call instance, you have to throw a RuntimeStatusException. This will cause the control structure to shutdown with you error status and thus avoids the duplicate close.
(I haven't tested this, just my guess from your one sentence without stacktrace)

I did something similar here:
https://github.com/yidongnan/grpc-spring-boot-starter/blob/master/grpc-server-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/ExceptionTranslatingServerInterceptor.java#L44

Alternatively do it like this (This is also from a validation framework, so it might work better for your usecase):
https://github.com/envoyproxy/protoc-gen-validate/blob/5ef93ae28a92362ede78aaa49c5c3e290c70e324/java/pgv-java-grpc/src/main/java/io/envoyproxy/pgv/grpc/ValidatingServerInterceptor.java

oh.. thank you 😄

ddcprg · 2023-12-18T16:39:36Z

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

ST-DDT · 2023-12-18T23:04:01Z

@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 · 2023-12-19T09:16:26Z

@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 · 2023-12-19T10:15:49Z

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 · 2023-12-21T15:39:25Z

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

grpc server-side validation

e4e32e7

anjeyy marked this pull request as draft January 23, 2021 20:39

ST-DDT added this to the 2.13.0 milestone May 14, 2021

ST-DDT modified the milestones: 2.13.0, 2.14.0 Nov 25, 2021

sangyongchoi reviewed Jul 1, 2022

View reviewed changes

ST-DDT modified the milestones: 2.14.0, 2.16.0 Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC request validation (server-side) #487

gRPC request validation (server-side) #487

anjeyy commented Jan 23, 2021 •

edited

Loading

ST-DDT commented Jan 24, 2021

anjeyy commented Jan 24, 2021 •

edited

Loading

ST-DDT commented Jan 24, 2021 •

edited

Loading

anjeyy commented Jan 26, 2021

ST-DDT commented Jan 30, 2021

anjeyy commented May 13, 2021

ST-DDT commented May 14, 2021

sangyongchoi commented Jun 23, 2022

anjeyy commented Jun 24, 2022 •

edited

Loading

sangyongchoi commented Jun 24, 2022

ST-DDT commented Jun 24, 2022

sangyongchoi commented Jun 24, 2022

sangyongchoi left a comment •

edited

Loading

sangyongchoi Jul 1, 2022

ST-DDT Jul 1, 2022

sangyongchoi Jul 2, 2022

ddcprg commented Dec 18, 2023

ST-DDT commented Dec 18, 2023

ddcprg commented Dec 19, 2023 •

edited

Loading

genuss commented Dec 19, 2023

ddcprg commented Dec 21, 2023

gRPC request validation (server-side) #487

Are you sure you want to change the base?

gRPC request validation (server-side) #487

Conversation

anjeyy commented Jan 23, 2021 • edited Loading

TODO

ST-DDT commented Jan 24, 2021

anjeyy commented Jan 24, 2021 • edited Loading

ST-DDT commented Jan 24, 2021 • edited Loading

anjeyy commented Jan 26, 2021

ST-DDT commented Jan 30, 2021

anjeyy commented May 13, 2021

ST-DDT commented May 14, 2021

sangyongchoi commented Jun 23, 2022

anjeyy commented Jun 24, 2022 • edited Loading

sangyongchoi commented Jun 24, 2022

ST-DDT commented Jun 24, 2022

sangyongchoi commented Jun 24, 2022

sangyongchoi left a comment • edited Loading

Choose a reason for hiding this comment

sangyongchoi Jul 1, 2022

Choose a reason for hiding this comment

ST-DDT Jul 1, 2022

Choose a reason for hiding this comment

sangyongchoi Jul 2, 2022

Choose a reason for hiding this comment

ddcprg commented Dec 18, 2023

ST-DDT commented Dec 18, 2023

ddcprg commented Dec 19, 2023 • edited Loading

genuss commented Dec 19, 2023

ddcprg commented Dec 21, 2023

anjeyy commented Jan 23, 2021 •

edited

Loading

anjeyy commented Jan 24, 2021 •

edited

Loading

ST-DDT commented Jan 24, 2021 •

edited

Loading

anjeyy commented Jun 24, 2022 •

edited

Loading

sangyongchoi left a comment •

edited

Loading

ddcprg commented Dec 19, 2023 •

edited

Loading