protoc-gen-openapiv2: don't emit body objects containing only nested path parameters

[cloneable]

🚀 Feature

When path parameters are all part of a message in the body protoc-gen-openapiv2 emits a body property that is an empty object. While technically the field id is selected by body: "*" its sole field value is a path parameter and id is emitted as body object without properties.

service EntityService {
  rpc DeleteEntity(DeleteEntityRequest) returns (DeleteEntityResponse) {
    option (google.api.http) = {
      delete: "/entities/{id.value}"
      body: "*"
    };
  }
}

// message Entity {
//      EntityId id = 1;
//      etc.
// }

message EntityId {
        string value = 1;
}

message DeleteEntityRequest {
        EntityId id = 1;
        string other_field = 2;
}

message DeleteEntityResponse {}

    "/entities/{id.value}": {
      "delete": {
        "operationId": "EntityService_DeleteEntity",
        "responses": {
          "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/testingDeleteEntityResponse"
            }
          }
        },
        "parameters": [
          {
            "name": "id.value",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "type": "object",
              "properties": {
                "id": {
                  "type": "object"     <---
                },
                "otherField": {
                  "type": "string"
                }
              }
            }
          }
        ]
      }
    }

[johanbrandhorst]

Thanks for the issue, that definitely looks like a bug. Would you be interested in trying to fix this?

CC @oyvindwe.

[cloneable]

I can give it a try, but would have to wait until the weekend.

I think it's safe to remove the entire object when there are no defined fields left, right? If the message (here EntityId) used to contain other fields that were removed, but older clients still send them this shouldn't break. We still allow additionalProperties in the JSON object and unknown fields in the proto message.

[oyvindwe]

Great that you take a stab on this! I must admit I didn't consider this case when removing the path parameters.

I think it's safe to remove the entire object when there are no defined fields left, right?

Yes, I agree with that. Note that this must be handled in both branches of if len(b.Body.FieldPath) == 0 (the then branch is used for body="*"). There's a warning about generating an empty body that probably should be removed, as this would happen if all body message fields are path parameters, but I don't know if the entire body parameter can be removed, or if it has to be present as empty.

I think you need to fix this in both renderMessageAsDefinition and renderFieldAsDefinition recursively.

[cloneable]

So I just looked at this. I see how the mentioned functions build the schemas for body properties. Now somehow both need to tell the caller that it must not use the returned schema type (because it's an empty object). We could...

1. check if type == "object" and len(schema.properties) == 0 after the call and skip appending the field or body parameter respectively,
2. return nil and check for that and skip,
3. return a bool in addition to (openapiSchemaObject, error) and skip on false, or
4. add a Omit bool field to openapiSchemaObject or schemaCore and skip when true.

I think the bool return value is cleanest, but let me know what you think.

[johanbrandhorst]

Hm, it is a bit unusual to return both a boolean and an error from a function. Option 1 doesn't sound too bad to me.

[cloneable]

I was thinking since it's not an error but a successful call it should not be an error (like ErrEmptyObject), but I also don't want the returned schema to be simply nil, that's bad style, though this could be an option, too. I'll see what works well and open a PR. Let me know what you think there.

I've got one more question while thinking about this: I'm not sure we can always simply skip over empty object types. Sometimes they're intentionally empty, like google.protobuf.Empty or empty response messages. So I'm thinking we need to check if all fields were removed because they're mapped as path parameters and only then not emit an empty object types.

But then, the gateway doesn't forward additionalProperties as unknown proto fields. It has no way of knowing what tag number to use (unless there's an option for that?). So based on that assumption it might actually be always safe to omit empty object types in generated schemas, provided they're not marked as required.

[theshashankpal]

Hi @johanbrandhorst , It seems the issue is not assigned to anyone, and there is no latest update on the same. I am a student who needs to contribute to an OpenSource project in my coursework, so I was wondering if you can assign this to me. This seems to be a good initiation of my journey towards contribution to FOSS projects.

[oyvindwe]

But then, the gateway doesn't forward additionalProperties as unknown proto fields. It has no way of knowing what tag number to use (unless there's an option for that?). So based on that assumption it might actually be always safe to omit empty object types in generated schemas, provided they're not marked as required.

That assumption is sound - the gateway must reject or ignore unknown fields in the JSON message: https://developers.google.com/protocol-buffers/docs/proto3#json_options

@theshashankpal it sounds like @cloneable was investigating this earlier, but you are free to submit a PR.