TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
957 stars 302 forks source link

Include proto message validations to swagger spec #1982

Open bafonins opened 4 years ago

bafonins commented 4 years ago

Summary

We should add validation properties to the swagger spec generated by grpc-gateway.

Why do we need this?

  1. We already define validation rules for messages, so why not including it in the resulting swagger schema?
  2. Share validation between the stack, console, js sdk and other oauth clients. Currently, in the console we hardcode validation rules like required, min_length, minimum and others which can result in inconsistencies between components.
  3. Intercept invalid payloads in console/js sdk and avoid making requests to the server.
  4. Helpful for authors of other oauth clients relying on the http API.

What is already there? What do you see now?

protoc-gen-validate validation annotations Swagger spec in api/api.swagger.json

What is missing? What do you want to see?

Properties in the swagger schema like: maximum, minumum, max_length, min_length, required, pattern. Also, see https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#fixed-fields-7

Environment

v3.5.2

How do you propose to implement this?

grpc-gateway uses protoc-gen-swagger to generate swagger definitions and url paths. List of supported fields. The plugin provides its own annotations for validation, see examples. The simplest solution would be to duplicate annotations. However, this would make the files less readable, imo.

Im not really familiar with the grpc ecosystem and maybe I am missing better solutions available out there.

Also, see related issue in the grpc-gateway repo https://github.com/grpc-ecosystem/grpc-gateway/issues/1093

Can you do this yourself and submit a Pull Request?

Yes, but need reviews and input from others. cc @johanstokking @rvolosatovs @htdvisser @KrishnaIyer

htdvisser commented 4 years ago

Good one.

One concern is that grpc.gateway.protoc_gen_swagger.options.openapiv2_field is pretty long, so we should probably split the rules over multiple lines, perhaps like this:

message SomeRequest {
  string some_string = 1 [
    (validate.rules).string = {
      min_bytes: 3, max_bytes: 32, pattern: "^[a-z0-9]+$"
    },
    (grpc.gateway.protoc_gen_swagger.options.openapiv2_field) = {
      min_length: 3, max_length: 32, pattern: "^[a-z0-9]+$"
    }
  ];
  uint64 some_uint64 = 2 [ // similar for other numbers
    (validate.rules).uint64 = {
      gte: 1, lte: 1000
    },
    (grpc.gateway.protoc_gen_swagger.options.openapiv2_field) = {
      minimum: 1, maximum: 1000
    }
  ];
}
htdvisser commented 2 years ago

Removing assignment and bumping back to triage for re-assignment and prioritization.

cc: @NicolasMrad

KrishnaIyer commented 1 year ago

Based on offline discussion:

Assignees can revisit this issue with their comments. Once we have an implementation plan, we can schedule this.