Comments on a property of a message that are other proto messages become title breaking Swagger generation

[yinzara]

🐛 Bug Report

This is similar in nature to #2670 but is related to message properties that refer to another proto message (or repeated messages)

When you have a message and that field has a comment on it that doesn't contain a blank line, the comment becomes the "title" of the body parameter making it fail generation with the Swagger generator as it's much too long and contains extra lines. It really should become the "description" or "summary" but definitely not the "title". We should really always prevent the "title" from having any line breaks in it at all. Right now we only consider double line breaks.

To Reproduce

When you have the following:

message MyRequestMessage {
  // a comment about my body parameter that is quite long
  // and may have multiple lines
  // that should become the description of the body parameter or potentially the "summary"
  AnotherMessage my_body_parameter = 1;
  string a_query_parameter = 2;
}
...
service MyService {
  // Makes a request
  rpc MakeMyRequest(MyRequestMessage) returns (google.type.Empty) {
    option (google.api.http)              = {
            post: "/a/path
            body: "my_body_parameter"
     }
  }
}

## Expected behavior

What it should produce:
```json
  "/a/path": {
    "post": {
      "summary": "Makes a request",
      "operationId": "MyService_MakeMyRequest",
      "responses": {
      "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/googletypeEmpty"
         }
      },
      "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/googlerpcStatus"
          }
        }      
      },
      "parameters": [
        {
            "name": "a_query_a_parameter",
            "in": "query",
            "required": false,
            "type": "string"
        },
        {
            "name": "my_body_parameter",
            "summary": "a comment about my body parameter that is quite long\nand may have multiple lines\nthat should become the description of the body parameter or potentially the \"summary\"",
            "in": "body",
            "required": true,
            "schema": {
              ...
            }
        }
      ]
    }
  }

Actual Behavior

The generated OpenAPI specification is:
```json
  "/a/path": {
    "post": {
      "summary": "Makes a request",
      "operationId": "MyService_MakeMyRequest",
      "responses": {
      "200": {
            "description": "A successful response.",
            "schema": {
              "$ref": "#/definitions/googletypeEmpty"
         }
      },
      "default": {
            "description": "An unexpected error response.",
            "schema": {
              "$ref": "#/definitions/googlerpcStatus"
          }
        }      
      },
      "parameters": [
        {
            "name": "a_query_a_parameter",
            "in": "query",
            "required": false,
            "type": "string"
        },
        {
            "name": "my_body_parameter",
            "title": "a comment about my body parameter that is quite long\nand may have multiple lines\nthat should become the description of the body parameter or potentially the \"summary\"",
            "in": "body",
            "required": true,
            "schema": {
              ...
            }
        }
      ]
    }
  }

Cause

This is caused by the renderFieldAsDefinition method in template.go:

	comments := fieldProtoComments(reg, f.Message, f)
	if len(comments) > 0 {
		// Use title and description from field instead of nested message if present.
		paragraphs := strings.Split(comments, paragraphDeliminator)
		schema.Title = strings.TrimSpace(paragraphs[0])
		schema.Description = strings.TrimSpace(strings.Join(paragraphs[1:], paragraphDeliminator))
	}

This handling is inconsistent with the handling in renderMessageAsDefinition as processes through updateOpenAPIDataFromComments which would end up setting the "Summary" property from the message primary comments.

[johanbrandhorst]

Thanks for the detailed report, seems reasonable to me, do you think it's as simple as changing that Title to Summary?

[yinzara]

Honestly that's probably the quickest fix but it would end up being a breaking change for people who actually used that functionality to be the "title" so that their code generated with specific filenames.

You could also fix it like:

If it doesn't have a "title", the generated code would use a different name.

if len(comments) > 0 {
        paragraphs := strings.Split(comments, paragraphDeliminator)
        if len(paragraphs) == 0 {
                schema.Description = strings.TrimSpace(paragraphs[0])
        } else {
	    	schema.Title = strings.TrimSpace(paragraphs[0])
    		schema.Description = strings.TrimSpace(strings.Join(paragraphs[1:], paragraphDeliminator))
        }
  }

With this fix at least if you were using it properly with a double new line that the title would remain unchanged.

Either way it would become a breaking change to the generated code if you have comments at all on the body property.

[yinzara]

I'm realizing this problem is more widespread than I was thinking.

Even the comments on a field are coming across as "title" when there is no "\n\n" in them.

This is definitely contrary to the spirit and understanding I had around the use of these.

"title" is like "property or message display name", not "property description" and a proto comment is intended to be a description of that thing, not the title of it. I understand when it contains a "\n\n" then it could be spilt and the first line assumed to be the title (though I even question that decision).

Maybe we just need a global config setting that says "Comments are always only summaries or descriptions" and never try to parse the "title" from it and only let the OpenAPI extension specify the title.

The generated code in every language only uses the proto comments to add comments to code, never to change the code that is generated.

[johanbrandhorst]

Hm, I see what you mean. I vaguely recall having a discussion around inferring titles from comments (and settling on the double newline as seen today) but I buy that it'd be nicer to just fill the description from the comment if there aren't any double newlines, and maybe even if there are. The user can always set the title via the field option annotation anyway.

I'm also not concerned with this as a "breaking change". Comments tend to change all the time and I think this is actually an improved heuristic. Title should be populated from the field name, description/summary should be populated from the comment. I think we can just make that change and "ask for forgiveness" if it causes any unexpected changes. I'm not sure if we need a heuristic for deciding on using Summary or Description or if we should just always use Description. What do you think?

[yinzara]

I'm totally cool with that :) I can put the PR together.

[yinzara]

How about leaving the spirit of the current functionality of using the first split lines as the summary whenever it's supported and the rest for the description. Otherwise just set the summary if supported or description if it is not.

i.e.

// just a summary
// and still a summary
message BingBash { // title is "Bing Bash" or "BingBash"

  // just the summary
  // and still the summary
  string a_simple_property = 1; // title is "A Simple Property"
}

// in the summary
// still in the summary
// 
// now in the description
// 
// still in the description
message FooBar { // title is "Foo Bar" or "FooBar".

  // the summary
  // still summary
  //
  // description
  //
  // still description
  string my_property = 1; // title is "My Property" or maybe just "my_property" or "MyProperty". I like the first option the best.  Would need to split words and fix case
}

[johanbrandhorst]

Mostly moving from title to summary then? Sounds good to me.