Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to implement field validation for optional parameter #1256

Closed
eldad87 opened this issue Apr 30, 2020 · 3 comments
Closed

How to implement field validation for optional parameter #1256

eldad87 opened this issue Apr 30, 2020 · 3 comments

Comments

@eldad87
Copy link

eldad87 commented Apr 30, 2020

Hi,
I'm trying to implement a request with optional (omit) parameters with validation constrains.
As far as I understand, optional field can be achieved using oneof.
In addition, protoc-gen-validate supports oneof.

Unfortunately, when using oneof, the swagger definition and grpc-ecosystem are changed and I can't use the original field name.

Given the following proto:

syntax = "proto3";
package pb;

import "google/api/annotations.proto";
import "github.com/envoyproxy/protoc-gen-validate/validate/validate.proto";

service Account {
    rpc Update(UpdateRequest) returns (UpdateResponse) {
        option (google.api.http) = {
          put: "/v1/account/{ID}"
          body: "*"
        };
    }
}

message ID {
    uint32 ID = 1 [(validate.rules).uint32.gte = 0];
};

message UpdateRequest {
    int32 ID = 1;
    string FirstName = 2 [(validate.rules).string.min_len = 2];
    oneof LastName {
        bool LastNameNull = 3;
        string LastNameValue = 4 [(validate.rules).string.min_len = 2];
    }
}

message UpdateResponse {
    int32 ID = 1;
    string FirstName = 2;
    string LastName = 3;
}
  1. Whenever there is a oneof in the protobuf definition all the keys are simply added to the object: LastNameNull & LastNameValue. In other words, My request payload should contain LastNameValue instead of simply LastName.
    Keep in mind that my Request struct fields should have the same naming as my DB struct for marshaling purposes, therefore the following will not work for me As it adds LastNameValue isUpdateRequest_LastNameValue to the generated struct:
    oneof LastNameValue {
        bool LastNameNull = 3;
        string LastName = 4 [(validate.rules).string.min_len = 2];
    }
  1. If I try to send a request with FirstName defined, I get the following error json: cannot unmarshal string into Go value of type pb.isUpdateRequest_LastName.
    According to @ivucica's comment I should implement UnmarshalJSONPB for that object. Unfortunately pb.isUpdateRequest_LastName is an interface and therefore his suggestion is not applicable.

  2. What is your approach to unified DB & API validation errors?
    I have 2 validation levels. gRPC request level (max length etc) and DB objects (email already exists, etc) which can result in different naming convention, especially when nested structs are involved.
    I'm using the following interceptors to try and minimaize the situation by maintaining the same error structure.
    But still, I'm using different structs and therefore there will be different Naming in the error message & field name.

Please advise how to handle this situation,
Thanks!

@johanbrandhorst
Copy link
Collaborator

I would suggest using google.protobuf.StringValue last_name = 1 from the Well Known Type google/protobuf/wrappers.proto: https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto. That should still work with your validation and will allow it to be nullable.

@eldad87
Copy link
Author

eldad87 commented Apr 30, 2020

Hi @johanbrandhorst ,
Thank for your input. I will definitely give it a try.

Update:
I'm no longer able to marshal the gRPC request struct into other structs such as DB objects: Which means that I need to start writing code for struct conversion.

@johanbrandhorst
Copy link
Collaborator

I would say that the conversion of database validation errors into gRPC error is the responsibility of your gRPC server layer, not a gRPC interceptor. Whatever is returned from the gRPC server implementation should ideally already be a gRPC status. But this is the sort of thing that the #grpc channel on Gophers slack is great for discussing, so I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants