grpc-gateway icon indicating copy to clipboard operation
grpc-gateway copied to clipboard

protoc-gen-openapiv2: `google.api.field_behavior = REQUIRED` annotates the wrong schema object

Open cloneable opened this issue 2 years ago • 4 comments

🐛 Bug Report

protoc-gen-openapiv2 offers to mark properties as required based on the google.api.field_behavior field option, but it adds the "required": ["<property>"], annotation to the property itself in addition to adding it to the object containing the property.

https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/#using-googleapifield_behavior

To Reproduce

import "protoc-gen-openapiv2/options/annotations.proto";
import "google/api/field_behavior.proto";

message TestMessage {
  option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_schema) = {
    json_schema: { required: [ "fieldOne", "fieldTwo" ] }
  };

  string field_one = 1;

  string field_two = 2 [(google.api.field_behavior) = REQUIRED];

  string field_three = 3 [(google.api.field_behavior) = REQUIRED];
}

Run protoc-gen-openapiv2.

Expected Output

    "testingTestMessage": {
      "type": "object",
      "properties": {
        "fieldOne": {
          "type": "string"
        },
        "fieldTwo": {
          "type": "string"
        },
        "fieldThree": {
          "type": "string"
        }
      },
      "required": [
        "fieldOne",
        "fieldTwo",
        "fieldThree"
      ]
    }

Actual Output

    "testingTestMessage": {
      "type": "object",
      "properties": {
        "fieldOne": {
          "type": "string"
        },
        "fieldTwo": {
          "type": "string",
          "required": [     <-- wrong
            "fieldTwo"
          ]
        },
        "fieldThree": {
          "type": "string",
          "required": [     <-- wrong
            "fieldThree"
          ]
        }
      },
      "required": [
        "fieldOne",
        "fieldTwo",
        "fieldTwo",     <-- duplicate
        "fieldThree"
      ]
    }

Your Environment

github.com/grpc-ecosystem/grpc-gateway/v2 v2.10.0

cloneable avatar Apr 10 '22 11:04 cloneable

Hi, thanks for the issue! That definitely looks wrong. If you're interested in helping to contribute a fix, the first step is usually to create a failing test. It looks like with what you've got here that shouldn't be too hard at all. Would you be interested in contributing a fix?

CC @ewhauser

johanbrandhorst avatar Apr 12 '22 00:04 johanbrandhorst

I am looking to contribute to this issue please assign it to me and I kind of know where to change. where to write the unit tests for change? I am planning to make the updates in the below function https://github.com/grpc-ecosystem/grpc-gateway/blob/5d41954a8eb8075aef0c72842296da38a151411d/protoc-gen-openapiv2/internal/genopenapi/template.go#L420

lakshkeswani avatar Jun 12 '22 00:06 lakshkeswani

CC @johanbrandhorst

lakshkeswani avatar Jun 12 '22 01:06 lakshkeswani

Thank you @lakshkeswani, I think we usually add tests to template_test in the same directory, but it is a bit of a mess so feel free to think about other ways to test it if you like. We should also see if we have examples of this in the current code already, and if not, introduce an example that reproduces this bug. The code was introduced in https://github.com/grpc-ecosystem/grpc-gateway/pull/1806 and it seems like we added some REQUIRED fields in that PR. Maybe see if they already exhibit the wrong behavior?

johanbrandhorst avatar Jun 12 '22 01:06 johanbrandhorst

This was fixed by #2769. Lets reopen if it persists.

johanbrandhorst avatar Aug 16 '22 15:08 johanbrandhorst