google / gnostic

A compiler for APIs described by the OpenAPI Specification with plugins for code generation and other API support tasks.
Apache License 2.0
2.03k stars 241 forks source link

protoc-gen-openapi: Nullable for fields in OpenAPI #347

Open jan-sykora opened 2 years ago

jan-sykora commented 2 years ago

Would it be possible to add an option to protoc-gen-openapi to generate proto3 fields labeled as optional to OpenAPI object properties with label nullable: true?

For example this proto file:

syntax = "proto3";

message Foo {
  optional int32 bar = 1;
}

when generated by protoc-gen-openapi with some new option (e.g. proto3-optional-nullable=true) would be generated as OpenAPI spec:

openapi: 3.0.3
components:
  schemas:
    Foo:
      properties:
        bar:
          type: integer
          format: int32
          nullable: true

If I understand it correctly:

The similar feature is supported in grpc-gateway/protoc-gen-openapiv2 - it supports option to generate object properties in OAS2 as nullable -> the main motivation for this option is that some other tools generating code from the (generated) OpenAPI spec cannot handle default (null) values set during the transition from proto3 marshaled wire format to openapi json.

This issue is related to #275.

morphar commented 2 years ago

Hi @jan-sykora, it would make a lot of sense to have this. I would even say that it should probably be default behaviour to add nullable for optional fields, since it's a non-breaking change that adds valuable information for both humans and machines.

jan-sykora commented 2 years ago

I have noticed a potential inconsistency regarding this mapping. When the optional proto field is a message, it is converted into OpenAPI as a schema component that is then referenced.

The problem is when there's a protofile like this:

message Foo {
  optional google.protobuf.Duration optional_bar_duration = 1;
  google.protobuf.Duration bar_duration = 1;
}

message google.protobuf.Duration {
  int64 seconds = 1;
  int32 nanos = 2;
}

then it's generated as

components:
  schemas:
    Foo:
      properties:
        optionalBarDuration:
          $ref: '#/components/schemas/Duration'
        barDuration:
          $ref: '#/components/schemas/Duration'
    Duration:
      type: object
      properties:
        seconds:
          type: integer
          format: int64
        nanos:
          type: integer
          format: int32

-> we cannot mark the whole Duration object as nullable because then it would be applied for both optional_bar_duration and bar_duration (but we want it to be applied only on optional_bar_duration).

jan-sykora commented 2 years ago

Also created related issue #351.

morphar commented 2 years ago

Good catch :) I agree that this should be a string. We should look into how we can handle optional message types best.

jeffsawatzky commented 1 year ago

@morphar / @jan-sykora the optional protocol buffer option doesn't actually make the field nullable. To make a nullable int32 (or any other primitive type nullable) you need to use one of the wrapper types.

366 added support for the wrapper types, however, it looks like it doesn't add the nullable: true property to the output. We can probably update that code to spit out nullable.

Maybe fix this along with #351 as they are sort of related to the well known types.