Authress-Engineering / openapi-explorer

OpenAPI Web component to generate a UI from the spec.
Apache License 2.0
316 stars 42 forks source link

Add support for "additionalProperties" in multipart/form-data request bodies #176

Closed adam-homeboost closed 1 year ago

adam-homeboost commented 1 year ago

Openapi allows an object to declare that it will accept non-specified "extra" properties by declaring "additionalProperties" to be true. This is not a problem when executing json payloads from the explorer, but for GET requests and multipart/form-data requests, the form constructed by the explorer is not usable to set these fields.

Here is an image upload example, using multipart/form-data POST.

image

Note that the ability to add extra fields is clearly recognized and displayed (the any-key element), however there is no ability to set the actual key value and there is no ability to set more than one extra parameter.

wparad commented 1 year ago

GET requests don't have bodies, so I'm not totally sure what you would be expecting there. From your screen shot I can see that there must be a bug for multipart/form-data bodies. Can you provide a minimal repo spec so that we can triage and begin work on fixing this?

Thanks, Warren

adam-homeboost commented 1 year ago

GET requests can have query string parameters and the openapi spec has a format that allows for "extra" parameters to be added there with unknown names. Types can be specified for those. The explorer is less functional for this use case although the examples in the openapi spec are a bit obscure to me. It is certainly possible that I am stating things wrongly.

See usage of "additionalProperties" in https://spec.openapis.org/oas/latest.html#parameter-object-examples

adam-homeboost commented 1 year ago

Minimal schema with examples for multipart/form-data POST and GET use cases:

{
  "openapi": "3.1.0",
  "info": {
    "title": "Multipart upload schema",
    "version": "1"
  },
  "paths": {
    "/api/images": {
      "post": {
        "summary": "Upload Image",
        "parameters": [],
        "requestBody": {
          "content": {
            "multipart/form-data": {
              "schema": {
                "properties": {
                  "photo": {
                    "type": "string",
                    "format": "binary",
                    "title": "Photo"
                  }
                },
                "additionalProperties": true,
                "type": "object",
                "required": [
                  "photo"
                ]
              }
            }
          },
          "required": true
        }
      }
    },
    "/api/get-example": {
      "get": {
        "summary": "GET example",
        "parameters": [
          {
            "in": "query",
            "name": "freeForm",
            "schema": {
              "type": "object",
              "additionalProperties": true
            },
            "style": "form"
          }
        ]
      }
    }
  }
}
wparad commented 1 year ago

You got it mostly right. That's correct with the GET, the spec doesn't allow "unknown" query parameters, but it does allow a query parameter object. As you defined it above in your minimal spec, this would be the spec's expected input for the GET query:

https://path.server.com?freeForm=property1,value1,property2,value2

Here the object freeForm is expected to look like:

{
  "property1": "value1",
  "property2": "value2"
}

I'm guessing though, didn't want the freeForm text in the query, and wanted this: https://path.server.com?property1=value1&property2=value2

You can see this from the style examples in the spec

So, before we add explicitly handling for this functionality, I want to confirm that is your expectation.

Perhaps you meant to also set the explode property to true. This is highly unregular, but looking from the spec, it might be the closest thing to what you actually want. I'm not sure that the spec has a well defined understanding of this however.

image

although looking at this, an empty freeForm is likely going to cause you a problem, so I would probably say in your case you need Simple + explode.

However, I'm still a bit concerned that this chart might actually be wrong, and not mean what it means, there a number of rows that don't contain the prefix color= when it should be there, which is in itself and indication that this behavior is a unknown.

adam-homeboost commented 1 year ago

I'm a bit on the fence for the GET use case since I am finding the spec examples a little confusing. The way you described it with explode makes a lot more sense to me and more closely matches what I would have expected.

However, I care far more about the multipart/form-data use case as that is the one I really need the most. On the GET case I am happy to defer to those more expert than me and I can live without it.

wparad commented 1 year ago

Let's assign this issue for handling the form-data POST body and ignore the GET for now. If it comes up for someone in the future, we can revisit what the expected interpretation is. the form-data POST body is an obvious bug, and the user should be able to enter both the key and the value any number of times.

Honestly I'm going to assume the spec table is just wrong. All of these should start with color= followed by a urlencoded safe version of the string in some format. In the highlighted row we should expect the result to be:

color=URLENCODED(R=100&G=200&B=150)

wparad commented 1 year ago

This has been fixed in 1.0.577.