Add Support for RPC Comments

[evanlknapp]

I would like to use protolock with a plugin that I plan to use to verify custom logic in my .proto files. Specifically, for some of our RPC's we have an internal way of decorating them as "Incubating" (under development) and therefor we want to have them skipped when doing validation as they may be in flux a bit. My intention was to mark each of these RPC's with a "@protolock:skip" comment and make a rule that if protolock finds an RPC with the "Incubating" tag to make a warning for it.

But I noticed that RPC's don't have Comments right now. Was there any plan to add support for this? Or should I go ahead with adding support for this myself?

[nilslice]

@evanlknapp - I see, though I'm not sure about storing literal comments in the proto.lock file.. does adding a no-op option to the RPC work instead? You don't need to use the option in your codebase, but rather to decorate and check the RPC for catching in your plugin.

I would love to have support for // @protolock:skip on the RPCs (and every other data type in addition to message). If that is a preferable method for you, I would recommend we go this route:

1. add the skip logic to RPC types
2. add a Skipped boolean to each type that can interpret a skip hint rather than omitting the type from the proto.lock file.
3. modify the logic to skip applying ruleFuncs to data types if x.Skipped == true

Though there could be a big difference between an "incubated" RPC being addressed with a warning, and a generally "skipped" RPC which you simply want protolock to ignore.

I'd gladly accept a PR that implements the above (or something similar if you have other ideas) -- I personally will have limited time to look into this for a couple weeks. Let me know what you think, thanks!

[evanlknapp]

@nilslice I think I may have confused you. Here's an example of what I have.

service TestService {
   rpc getTestObjects(TestRequest) returns (TestResponse)

   // @protolock:skip
   rpc getTestObjectsUnstable(UnstableRequest) returns (UnstableResponse) {
      option (incubating) = "true";
   } 
}

So my intention is to make sure that any "incubating" RPC's are left out of the proto.lock file. So my custom rule would basically say that if it finds any RPC's with the "incubating" option throw a warning suggesting to add the "@protolock:skip" comment.

However more and more as I think about this, this doesn't sound like a version compatibility rule as much as a linter type rule. So while adding the ability for RPC's to have comments and skip logic may be valid, I am going to back off from this feature suggestion as my use case doesn't feel valid. Feel free to either keep this issue open for future development or close.

Thanks again for your input.

[evanlknapp]

Hey @nilslice so I am circling back to this one to revisit the inclusion of comments. Generally I would agree with you about being skeptical about including comments in the proto.lock file, however my workplace has a use case. So now that required as a keyword has been removed in proto3 we have taken to indicating that a field for a message is required (mainly for our docs, as we enforce it manually in our implementation) by having a field comment including the string (Required).

I was hoping to implement a rule for my custom plugin that would prevent adding required fields to a message. To do this I would need to have comments accessible in the proto.lock file. While I do agree with your feelings on comments I was wondering if you could think of any way I could accomplish this without changing how our protobuf files indicate required. Maybe a configuration for the parser including comments that would default to false? Suggestions are welcome.

[nilslice]

@evanlknapp -

Maybe a configuration for the parser including comments that would default to false?

That is something we could certainly consider... If you have some time to take a stab at this, feel free. If you find a solution that meets your needs this way, we can merge it.

If not, I'm mainly concerned with the possibility of bloating file size, and needing to deserialize excess data in the form of comments. I understand your use case completely, but have to reject the inclusion of comments (at least for now, in lieu of what might be a ton of testing to see how comments impact parsing speed, file size, etc).

Alternatively, I would suggest adding another option to the field, where required=true or similar. This is probably the easiest way to get protolock support for your check (assuming I understand the goal).

There may be other ways to consider. Would you be able to share an example of a message with fields that you'd like to check for? And if possible, include the warning you'd like to issue when/if the plugin finds a problematic field?

[evanlknapp]

As an example:
Stable version:

message PersonMessage {
   // (Required) Person's First Name
   string firstName = 1;
   string lastName = 2;
}

Updated version:

message PersonMessage {
   // (Required) Person's First Name
   string firstName = 1;
   // (Required) Person's Last Name
   string lastName = 2;
}

I would like the updated version to show and error as it has added a required field saying something like: Message "PersonMessage" added field "lastName" as a required field.