You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/06/15 16:23:42 UTC
[GitHub] [skywalking-banyandb] e11jah opened a new pull request, #134: api: add validation for proto message
e11jah opened a new pull request, #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134
Add validations for gRPC APIs.
Closes [#issue 9073](https://github.com/apache/skywalking/issues/9073)
Add dependencies,
- `protoc-gen-validate`: v0.6.7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] e11jah commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
e11jah commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1157320926
> > > I took a look at `measure`:
> > > ```
> > > * please fix relevant issues of `measure`, `property` and `stream`. In `rpc.proto`
> > >
> > > * all fields in request messages are required.
> > > ```
> >
> >
> > I have changed it. PTAL once again.
>
> CI didn't pass. Please check it carefully on your computer.
The generated pgv file has a problem when calling functions from the same package. And it also doesn't have the license header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#discussion_r898668283
##########
api/proto/banyandb/common/v1/common.proto:
##########
@@ -33,9 +34,9 @@ enum Catalog {
// Metadata is for multi-tenant, multi-model use
message Metadata {
// group contains a set of options, like retention policy, max
- string group = 1;
+ string group = 1 [(validate.rules).string.min_len = 1];
Review Comment:
thank you for your reminding, the group field seems not be mandatory since `Group` only has the `name`. Let's remove this checker here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on code in PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#discussion_r898607469
##########
api/proto/banyandb/common/v1/common.proto:
##########
@@ -33,9 +34,9 @@ enum Catalog {
// Metadata is for multi-tenant, multi-model use
message Metadata {
// group contains a set of options, like retention policy, max
- string group = 1;
+ string group = 1 [(validate.rules).string.min_len = 1];
Review Comment:
https://github.com/apache/skywalking-banyandb-java-client/blob/14e2e4f7c0ec5d7a33f567121375194527e5da0d/src/main/java/org/apache/skywalking/banyandb/v1/client/metadata/Group.java#L78
For `Group`, we currently do not duplicate this field. If this rule is imposed, `banyandb-java-client` should also be modified to adapt this change.
@hanahmily
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#discussion_r898588767
##########
api/proto/banyandb/measure/v1/query.proto:
##########
@@ -50,7 +51,7 @@ message QueryResponse {
// QueryRequest is the request contract for query.
message QueryRequest {
// metadata is required
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// time_range is a range query with begin/end time of entities in the timeunit of milliseconds.
model.v1.TimeRange time_range = 2;
Review Comment:
`time_range` is required
##########
api/proto/banyandb/measure/v1/query.proto:
##########
@@ -50,7 +51,7 @@ message QueryResponse {
// QueryRequest is the request contract for query.
message QueryRequest {
// metadata is required
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// time_range is a range query with begin/end time of entities in the timeunit of milliseconds.
model.v1.TimeRange time_range = 2;
Review Comment:
and `tag_projection` is required.
##########
api/proto/banyandb/measure/v1/write.proto:
##########
@@ -39,9 +40,9 @@ message DataPointValue {
// WriteRequest is the request contract for write
message WriteRequest {
// the metadata is required.
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// the data_point is required.
- DataPointValue data_point = 2;
+ DataPointValue data_point = 2 [(validate.rules).message.required = true];
Review Comment:
`timestamp` is required, `tag_families` should contain one item at least.
##########
api/proto/banyandb/measure/v1/topn.proto:
##########
@@ -49,7 +50,7 @@ message TopNResponse {
// TopNRequest is the request contract for query.
message TopNRequest {
// metadata is required
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// time_range is a range query with begin/end time of entities in the timeunit of milliseconds.
model.v1.TimeRange time_range = 2;
Review Comment:
`time_range`, `top_n` and `agg` are required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1173208270
@e11jah any updates?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#discussion_r898587813
##########
api/proto/buf.lock:
##########
@@ -0,0 +1,7 @@
+# Generated by buf. DO NOT EDIT.
Review Comment:
Add this file to https://github.com/apache/skywalking-banyandb/blob/808fc8faec769de5f7b915eba7998f30a6de5bd3/.licenserc.yaml#L63
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1165152943
> > > > I took a look at `measure`:
> > > > ```
> > > > * please fix relevant issues of `measure`, `property` and `stream`. In `rpc.proto`
> > > >
> > > > * all fields in request messages are required.
> > > > ```
> > >
> > >
> > > I have changed it. PTAL once again.
> >
> >
> > CI didn't pass. Please check it carefully on your computer.
>
> The generated pgv file has a problem when calling functions from the same package. And it also doesn't have the license header.
We have removed the header checker for generated files. Please fix these conficts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] e11jah commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
e11jah commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1166805901
`protoc-gen-validate` has some issue when validating enum which defined in sibling package with same name, so I temporarily removed enum validation.
Details in https://github.com/envoyproxy/protoc-gen-validate/pull/589
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 merged pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
lujiajing1126 merged PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] e11jah commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
e11jah commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1171139252
> > `protoc-gen-validate` has some issue when validating enum which defined in sibling package with same name, so I temporarily removed enum validation.
> > Details in [envoyproxy/protoc-gen-validate#589](https://github.com/envoyproxy/protoc-gen-validate/pull/589)
>
> Would you pls leave some "TODO" comments on the enum fields to remind us to check the upstream's progress?
Yes, I would.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] e11jah commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
e11jah commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1157268834
> I took a look at `measure`:
>
> * please fix relevant issues of `measure`, `property` and `stream`. In `rpc.proto`
>
> * all fields in request messages are required.
I have changed it. PTAL once again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
hanahmily commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1168845042
> `protoc-gen-validate` has some issue when validating enum which defined in sibling package with same name, so I temporarily removed enum validation.
>
> Details in [envoyproxy/protoc-gen-validate#589](https://github.com/envoyproxy/protoc-gen-validate/pull/589)
Would you pls leave some "TODO" comments on the enum fields to remind us to check the upstream's progress?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1157271609
You've only added validation annotations, but actually you are not calling `Validate()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] e11jah commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
e11jah commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1157317682
> You've only added validation annotations, but actually you are not calling `Validate()`?
We need to add interceptors on the server side. I'll do it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on code in PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#discussion_r898609740
##########
api/proto/banyandb/stream/v1/query.proto:
##########
@@ -54,7 +55,7 @@ message QueryResponse {
// QueryRequest is the request contract for query.
message QueryRequest {
// metadata is required
- common.v1.Metadata metadata = 1;
+ common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
Review Comment:
`projection` is required and contains at least one item, otherwise nothing will be returned
##########
api/proto/banyandb/common/v1/common.proto:
##########
@@ -33,9 +34,9 @@ enum Catalog {
// Metadata is for multi-tenant, multi-model use
message Metadata {
// group contains a set of options, like retention policy, max
- string group = 1;
+ string group = 1 [(validate.rules).string.min_len = 1];
Review Comment:
https://github.com/apache/skywalking-banyandb-java-client/blob/14e2e4f7c0ec5d7a33f567121375194527e5da0d/src/main/java/org/apache/skywalking/banyandb/v1/client/metadata/Group.java#L78
For `Group`, we currently do not duplicate this field. If this rule is imposed, `banyandb-java-client` should also be modified to adapt this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on pull request #134: api: add validation for proto message
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on PR #134:
URL: https://github.com/apache/skywalking-banyandb/pull/134#issuecomment-1157272174
> > I took a look at `measure`:
> > ```
> > * please fix relevant issues of `measure`, `property` and `stream`. In `rpc.proto`
> >
> > * all fields in request messages are required.
> > ```
>
> I have changed it. PTAL once again.
CI didn't pass. Please check it carefully on your computer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org