openAIP / openaip

Public issue tracker of www.openaip.net.
39 stars 3 forks source link

Invalid OpenAPI spec #309

Closed simonseyer closed 1 year ago

simonseyer commented 1 year ago

First: awesome API, thank you 🙏

I wanted to utilize the OpenAPI spec to generate a Swift client using apple/swift-openapi-generator. The generator (and also the Swagger editor) complained about a few things:

  1. Extra id path parameter in paths that don't have an ID, e.g. /airspaces (those can just be removed)
  2. The nesting of components.parameters.rfc sourceId and contentType (can be fixed by removing the nesting, i.e. by creating components.parameters.rfcSourceId and components.parameters.rfcContentType)
  3. In /utils/convert/airspaces.responses.200 the content must be nested in an application/json as the other responses
  4. [only a problem for the swift generator] "type": "array", "items": false for the coordinates wasn't deemed correct (maybe it could be a normal array or an object with latitude and longitude)

I fixed it locally for now. I would also create a PR if you point me to the source of the spec.

simonseyer commented 1 year ago

Would also be great if the parameter list (where applicable) could include the field(s) required for authentication:

{
    "parameters": [
        {
            "in": "header",
            "name": "x-openaip-client-id",
            "schema": {
                "type": "string"
            },
            "required": false,
            "description": "...",
            "example": "b112652dd403e0a683f6a5416f0d9f46"
        },
        {
            "in": "query",
            "name": "apiKey",
            "schema": {
                "type": "string"
            },
            "required": false,
            "description": "...",
            "example": "b112652dd403e0a683f6a5416f0d9f46"
        }
    ]
}

The example is obviously not a real key

reskume commented 1 year ago

The new schema is online. I also updated the swagger UI to the newest version. The new schema addresses issues 2.), 3.) and 4.).

Issue 1.) is intended to be this way. Each global list endpoint also takes an "id" query parameter value. Regarding the authentication parameters: they are available via the "security" property. From what I understand this is how the swagger API (and ultimately the UI) is intended to work and represent those authentication params. Maybe I'm wrong here but I would rather not duplicate content.

simonseyer commented 1 year ago

Hi @reskume. Thank you for the immediate action. I will check out the updated version.

Issue 1.) is intended to be this way. Each global list endpoint also takes an "id" query parameter value.

For the /airspaces/{id}, for example, this makes perfect sense, but for /airspaces I'm not sure if I can follow. Why would I use the endpoint without the id parameter to then supply an id anyway? Also, it is mandatory, so with code generated from the spec I can't even call the second endpoint without an id which I don't have when I search for objects.

Regarding the authentication parameters: they are available via the "security" property. From what I understand this is how the swagger API (and ultimately the UI) is intended to work and represent those authentication params. Maybe I'm wrong here but I would rather not duplicate content.

You are right. Found out it's a limitation of the Swift generator: https://github.com/apple/swift-openapi-generator/issues/37

reskume commented 1 year ago

The id query parameter was an import from our internally used API spec module. In this module, the id parameter was set to be required and I accidentally re-used it without looking into it on the "list" endpoints as a query parameter. Sorry for that. I replaced those occurrences with a definition that makes this parameter optional. The reason behind the id query parameter being available on the "list" endpoints is e.g. that it enables us to do proximity checks required in form field validation instead of defining a complete new endpoint for this logic or doing everything with multiple requests on the client.

The new version is currently being deployed ans should be available soon. Please check if this new spec now works for you.

simonseyer commented 1 year ago

Hi @reskume. Sorry for the late reply!

When opening the spec in the Swagger editor there are still errors reported unfortunately:

Semantic error at paths./airports.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/airports" path

Structural error at paths./airports.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./airspaces.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/airspaces" path

Structural error at paths./airspaces.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./hotspots.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/hotspots" path

Structural error at paths./hotspots.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./navaids.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/navaids" path

Structural error at paths./navaids.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./reporting-points.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/reporting-points" path

Structural error at paths./reporting-points.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./hang-glidings.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/hang-glidings" path

Structural error at paths./hang-glidings.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

Semantic error at paths./obstacles.get.parameters.10.name
Path parameter "id" must have the corresponding {id} segment in the "/obstacles" path

Structural error at paths./obstacles.get.parameters.10.required
should be equal to one of the allowed values
allowedValues: true

I read this as the Open API spec not allowing optional path parameters and expects all path parameters to have a corresponding placeholder ({id}) in the URL. Probably the assumption is that a URL with/without path parameter is a different endpoint (which actually is the case in your spec since /airports/123 would be the /airports/{id} endpoint instead of the /airports endpoint with optional path paramter id).

reskume commented 1 year ago

Hi,

Thanks for the feedback again. I'll have to take another look. I definitely changed the API spec to use another custom parameter definition. The original problem was the usage of a global reusable parameter definition that is used for the single document that require the ID parameter to be present. I assume that the cache still holds the old schema. I'll clear the API cache asap.

Simon Seyer @.***> schrieb am Mi. 30. Aug. 2023 um 15:16:

Hi @reskume https://github.com/reskume. Sorry for the late reply!

When opening the spec in the Swagger editor https://editor.swagger.io/ there are still errors reported unfortunately:

Semantic error at paths./airports.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/airports" path

Structural error at paths./airports.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./airspaces.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/airspaces" path

Structural error at paths./airspaces.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./hotspots.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/hotspots" path

Structural error at paths./hotspots.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./navaids.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/navaids" path

Structural error at paths./navaids.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./reporting-points.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/reporting-points" path

Structural error at paths./reporting-points.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./hang-glidings.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/hang-glidings" path

Structural error at paths./hang-glidings.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

Semantic error at paths./obstacles.get.parameters.10.name Path parameter "id" must have the corresponding {id} segment in the "/obstacles" path

Structural error at paths./obstacles.get.parameters.10.required should be equal to one of the allowed values allowedValues: true

I read this as the Open API spec not allowing optional path parameters and expects all path parameters to have a corresponding placeholder ({id}) in the URL. Probably the assumption is that a URL with/without path parameter is a different endpoint (which actually is the case in your spec since /airports/123 would be the /airports/{id} endpoint instead of the /airports endpoint with optional path paramter id).

— Reply to this email directly, view it on GitHub https://github.com/openAIP/openaip/issues/309#issuecomment-1699148729, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABROSQAVFH2ZNKSY2LXWE3LXX44JLANCNFSM6AAAAAA3DZV5T4 . You are receiving this because you were mentioned.Message ID: @.***>

reskume commented 1 year ago

I accidentally used "path" instead of "query". Should be good now.

simonseyer commented 1 year ago

Looks good. Thank you 🙏