Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cloneable opened this issue Apr 10, 2022 · 5 comments

Comments

@cloneable
Copy link

🐛 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

@johanbrandhorst
Copy link
Collaborator

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

@lakshkeswani
Copy link
Contributor

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

func renderMessageAsDefinition(msg *descriptor.Message, reg *descriptor.Registry, customRefs refMap, pathParams []descriptor.Parameter) (openapiSchemaObject, error) {

@lakshkeswani
Copy link
Contributor

CC @johanbrandhorst

@johanbrandhorst
Copy link
Collaborator

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 #1806 and it seems like we added some REQUIRED fields in that PR. Maybe see if they already exhibit the wrong behavior?

@johanbrandhorst
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants