application_json should not be exposed in API types

[timburks]

In the end-to-end example, the generated bookstore.proto contains the following messages:

message ListShelvesResponse {
  repeated Shelf shelves = 1;
}

message ListShelvesOK {
  ListShelvesResponse application_json = 1;
}

message ListShelvesResponses {
  ListShelvesOK ok = 1;
}

As a result, the generated API includes messages with a field named "application_json". For example, to create a shelf, we suggest using this curl command:

curl -X POST \
  http://localhost:51051/shelves \
  -H 'Content-Type: application/json' \
  -d '{
  "application_json": {
    "name": "Books I need to read",
    "theme": "Non-fiction"
    }
}'

After looking more carefully at the OpenAPI Spec, I believe this is incorrect. Specifically the description of Response Objects says that content is "a map containing descriptions of potential response payloads."

Based on that, I think a more appropriate call would be this:

curl -X POST \
  http://localhost:51051/shelves \
  -H 'Content-Type: application/json' \
  -d '{
  "name": "Books I need to read",
  "theme": "Non-fiction"
}'

I recently implemented an alternate bookstore API that uses the Google Cloud Datastore API to store books and shelves (see server.go for the Datastore-based implementation). This required some small changes to bookstore.yaml but did not significantly change the messages produced by gnostic-grpc - they still include application_json fields. But here I also hand-wrote a bookstore.proto that seems to me to produce a more appropriate REST API for the associated spec.

Do we agree that this is a problem? If so, what changes are needed to produce the hand-written bookstore.proto instead of the current gnostic-grpc output?

[LorenzHW]

Yes, I agree the output of gnostic-grpc is not perfect but rather reflects the given input in its entirety. This is due to the fact that the surface model captures more information from the input than before. To solve this problem, I believe we should choose one of the following approaches:

• Modifying the surface model in a way such that it only creates types for the content of type 'application/json' like we had before. This would be a PR in the gnostic main repository.
• Modifying gnostic-grpc in a way such that it ignores all content types except 'application/json'.

I tend towards the second approach, because I believe the surface model should provide as much information as possible and plugins should handle that information for their own needs.

I think it is not possible to produce the hand-written bookstore.proto 1:1, since we don't always have input files exactly structured like yours. We always have to create some 'generic' proto messages which are usable for every input file. However, we can definitely make the messages of the outputted .proto file less nested and/or name the fields differently.

Now, assuming that we can ignore all content types except 'application/json', I expect following results for request parameters (it would similar for response objects):
Input:

paths:
  "/shelves":
    post:
      operationId: createShelf
      requestBody:
        content:
          application/json:
            schema:
              "$ref": "#/components/schemas/shelf"
components:
  schemas:
    shelf:
      properties:
        name:
          type: string
        theme:
          type: string
      type: object

Output:

message Shelf {
  string name = 1;
  string theme = 2;
}

message CreateShelfRequestBody {
  Shelf shelf = 1;
}

message CreateShelfParameters {
  CreateShelfRequestBody request_body = 1;
}

service Bookstore {
  rpc CreateShelf ( CreateShelfParameters ) returns ( CreateShelfResponses ) {
    option (google.api.http) = { post:"/shelves" body:"request_body"  };
  }
}

Resulting in following curl command:

curl -X POST \
  http://localhost:51051/shelves \
  -H 'Content-Type: application/json' \
  -d '{
  "shelf": {
    "name": "Books I need to read",
    "theme": "Non-fiction"
    }
}'

Just as a side note for your understanding:
Input:

paths:
  "/shelves":
    post:
      operationId: createShelf
      requestBody:
        content:
          application/json:
            schema:
              properties:
                name:
                  type: string
                theme:
                  type: string
              type: object

Output:

message CreateShelfRequestBody {
  string name = 1;
  string theme = 2;
}

message CreateShelfParameters {
  CreateShelfRequestBody request_body = 1;
}

service Bookstore {
  rpc CreateShelf ( CreateShelfParameters ) returns ( CreateShelfResponses ) {
    option (google.api.http) = { post:"/shelves" body:"request_body"  };
  }
}

Curl:

curl -X POST \
  http://localhost:51051/shelves \
  -H 'Content-Type: application/json' \
  -d '{
  "name": "Books I need to read",
  "theme": "Non-fiction"
}'

Hence, we need the 'generic' messages such as 'CreateShelfParameters', because not every input file uses the 'component' section.

So I suggest that I start implementing the second approach. If it is done properly, I believe it can also be used for gnostic-go-generator, because there would be the same problem as described in this issue if this PR is merged.

What do you think?

[timburks]

Thanks for the thorough analysis! I agree with your preferred second approach.

The uncertainty here might be because we don't have a clear definition of the intended purpose of the surface model. Originally it was an intermediate representation of an API intended to support multi-language code generation (though it was originally just for Go). I'd like to keep/strengthen this based on the idea that nearly any programming language interface will consist of types/structures and methods/functions. Revisiting gnostic-go-generator, I think there are some things that will need to be added (service address, auth info) but we can do this over time. I agree with you that it seems too restrictive to only support "application/json" content types and that we should keep them in the surface model and possibly in generated clients, but since gnostic-grpc is about representing a gRPC API with REST/JSON, it should only support "application/json" content and should not expose "application/json" fields in content wrappers.

Related to your example, I am uncomfortable with the fact that a surface model will vary depending on whether a content schema is described inline or with a reference. Since schema references often correspond to named structs, I'm inclined to force all inlined schemas to be converted to references, either with a linter rule or a transformation. I don't think we should do that here, but perhaps in a separate tool or component.

The change you propose might not require any changes to the surface model, but rather than making implicit assumptions in gnostic-gprc, I would be open to adding some information to the surface Type message that indicates that a Type corresponds to a content map of Media Type-specific structures.

[LorenzHW]

I don't think we should do that here, but perhaps in a separate tool or component.

Cool! Maybe this could be added to the ideas list of GSoC projects. Something along the line:

• Create a tool that takes as input a bunch of surface model types and methods, searches for inline objects, and replaces them with a reference to a new type.

The change you propose might not require any changes to the surface model, but rather than making implicit assumptions in gnostic-gprc, I would be open to adding some information to the surface Type message that indicates that a Type corresponds to a content map of Media Type-specific structures.

I believe that the surface model type message currently represents multiple parts of an API description. For example, schemas of the components sections are also represented with the this message.

So to summarize: We are now aiming for following curl command with the implicit assumption that grpc-gnostic is used with 'application/json' as content type.

curl -X POST \
  http://localhost:51051/shelves \
  -H 'Content-Type: application/json' \
  -d '{
  "shelf": {
    "name": "Books I need to read",
    "theme": "Non-fiction"
    }
}'

Edit:
For the future: I believe with additional assumptions, such as: 'every request/response body is a reference to a type', we are able to create an output file that is more similar to the hand-written one.