not yet implemented: expected one content entry, found 4

[brainstorm]

Thanks for this library! I'm giving it a go for work purposes (https://github.com/brainstorm/ica-rs), unfortunately:

% cargo run
   Compiling ica-rust v0.1.0 (/Users/rvalls/dev/umccr/ica-rust)
error: proc macro panicked
 --> src/main.rs:4:5
  |
4 |     generate_api!(spec = "openapi/gds.json");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: message: not yet implemented: expected one content entry, found 4

The offending .json can be obtained like so:

wget https://converter.swagger.io/api/convert?url=https%3A%2F%2Faps2.platform.illumina.com%2Fgds%2Fswagger%2Fv1%2Fswagger.json -O gds.json

I'll keep digging, I just wanted to make sure whether you or somebody else was planning to support multiple content entries before I deep dive on it ;)

[brainstorm]

So, the detected media types are the following:

% cargo run
   Compiling ica-rust v0.1.0 (/Users/rvalls/dev/umccr/ica-rust)
[/Users/rvalls/dev/umccr/progenitor/progenitor-impl/src/method.rs:1261] &self.content = {
    "application/json-patch+json": MediaType {
        schema: Some(
            Reference {
                reference: "#/components/schemas/VolumeFileListRequest",
            },
        ),
        example: None,
        examples: {},
        encoding: {},
        extensions: {},
    },
    "application/json": MediaType {
        schema: Some(
            Reference {
                reference: "#/components/schemas/VolumeFileListRequest",
            },
        ),
        example: None,
        examples: {},
        encoding: {},
        extensions: {},
    },
    "text/json": MediaType {
        schema: Some(
            Reference {
                reference: "#/components/schemas/VolumeFileListRequest",
            },
        ),
        example: None,
        examples: {},
        encoding: {},
        extensions: {},
    },
    "application/*+json": MediaType {
        schema: Some(
            Reference {
                reference: "#/components/schemas/VolumeFileListRequest",
            },
        ),
        example: None,
        examples: {},
        encoding: {},
        extensions: {},
    },
}
error: proc macro panicked
 --> src/main.rs:4:5
  |
4 |     generate_api!("./openapi/gds.json");
  |      ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: message: not yet implemented: expected one content entry, found 4

error: could not compile `ica-rust` due to previous error

Then I proceeded to collapse all of those MediaTypes to application/json, which seems to be the one supported by progenitor in the openapi/gds.json file, but then:

 % cargo run
   Compiling ica-rust v0.1.0 (/Users/rvalls/dev/umccr/ica-rust)
error: generation error for ./openapi/gds.json: invalid operation path
 --> src/main.rs:4:19
  |
4 |     generate_api!("./openapi/gds.json");
  |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^

So at this point I'm debating myself on whether that's a problem with https://converter.swagger.io/ or the support given to multi-type endpoints and if this is something worth adding to progenitor?

EDIT: I managed to generate the client stubs with openapi-generator for now in https://github.com/brainstorm/ica-rs/commit/2e37f50a17c582b9960fa8b5b7c1af863b6d4442 instead.

[brainstorm]

OTOH (a.k.a mental notes I have to put somewhere)... at least one of the mediatypes above looks odd/unnecessary from that OpenAPI definition: *+json... it's invalid as per RFC, but apparently keeps popping up?:

https://stackoverflow.com/questions/48277752/is-application-json-a-valid-accept-header-in-http

I wonder in which point of the API design those types get introduced? Perhaps fixing that upstream might be a better approach than having progenitor implementing it like the rest.

[ahl]

It looks like progenitor is unhappy about these paths:

    "/v1/files/{fileId}:completeUpload"
    "/v1/folders/{folderId}/sessions/{sessionId}:complete"

Progenitor assumed that templated path elements needed to be a full component (i.e. from / to /|$), but that doesn't appear to be the case. We'll rewrite our path templating.

[ahl]

There are still two outstanding items preventing this schema from working.

Request Bodies

        "requestBody": {
          "content": {
            "application/json-patch+json": {
              "schema": {
                "$ref": "#/components/schemas/VolumeFileListRequest"
              }
            },
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/VolumeFileListRequest"
              }
            },
            "text/json": {
              "schema": {
                "$ref": "#/components/schemas/VolumeFileListRequest"
              }
            },
            "application/*+json": {
              "schema": {
                "$ref": "#/components/schemas/VolumeFileListRequest"
              }
            }
          },
          "required": true
        },

Progenitor currently freaks out over this... which is pretty annoying. Certainly for the time being it would suffice to ignore content types that we don't understand (i.e. ignore everything except for application/json from this list)

default responses

The next is slightly more challenging:

        "responses": {
          "201": {
            "description": "Created new File.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/FileWriteableResponse"
                }
              }
            }
          },
          "400": {
            "description": "Bad request.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/ErrorResponse"
                }
              }
            }
          },
          "401": {
            "description": "Unauthorized.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/ErrorResponse"
                }
              }
            }
          },
          "403": {
            "description": "Forbidden.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/ErrorResponse"
                }
              }
            }
          },
          "409": {
            "description": "A conflict was found. Make sure the new File doesn't already exist.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/ErrorResponse"
                }
              }
            }
          },
          "default": {
            "description": "Unexpected issue. Please try your request again. If problem persists, please contact the system administrator.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/ErrorResponse"
                }
              }
            }
          }
        },

In particular, default makes this tricky as we can currently only handle a single success response type. The default response is defined like this:

The documentation of responses other than the ones declared for specific HTTP response codes. Use this field to cover undeclared responses. A Reference Object can link to a response that the OpenAPI Object's components/responses section defines.

That's fine, but it implied that it could cover intended success responses (other than those with status 201). I think a more useful interpretation might be to think of default as implying something unintended and therefore something that should be considered an error irrespective of the actual status code.

There's a related problem in here in that most default responses share an error type with the other error response codes... but not all. This would require handling responses of multiple types e.g.

pub enum CopyFolderError {
    Error400(types::ErrorResponse),
    Error401(types::ErrorResponse),
    Error403(types::ErrorResponse),
    Default(serde_json::Value),
}

This is a bit more work.