hapi-swagger / hapi-swagger

A Swagger interface for hapi
https://hapi.dev/
MIT License
913 stars 420 forks source link

documentation is not rendered if schema includes .allow( null ) (v3) #212

Closed adrian-gierakowski closed 8 years ago

adrian-gierakowski commented 8 years ago

this is related to: https://github.com/OAI/OpenAPI-Specification/issues/229

it did work with v2, however I can't keep using v2 since I have just upgraded to hapi 12 for now I am using my own fork of v2 with hapi 12 added to peerDependecies in package.json

is there a possible workaround so that I can update hapi-swagger to v3 without hacing to change my API?

devinivy commented 8 years ago

@adrian-gierakowski I think it's the mapping from joi's "allow" to json-schema's "enum". I expect the change goes here: https://github.com/glennjones/hapi-swagger/blob/25b0f02010c8bade8c2481ed8af86db182d3b1f3/lib/properties.js#L155

@glennjones would a PR be accepted?

glennjones commented 8 years ago

Hi @devinivy and @adrian-gierakowski I want to try and keep the output of the plug-in in a form where it validates against the OpenAPI spec. As the OpenAPI project leads point out in https://github.com/OAI/OpenAPI-Specification/issues/229 null is not an allow value in the current version of the spec. There is some discussion about using a x-* property to extended the data with the concept of null but without support in the Swagger-UI project this maybe a bit pointless.

I am busy with two other issue at the moment which should land tomorrow. If you wish to look at this great, but any fix needs produce valid JSON and I don't want to change the Swagger UI JavaScript unless we think are change will be accepted into their codebase.

If you do have the time that would be great, can you please tell me either way as I don't want to duplicate work already untaken by someone else.

As temp measure I am adding this to the list of unsupport features

devinivy commented 8 years ago

@glennjones Thanks for taking a look into this! Currently a string with null allowed (Joi.string().allow(null)) is being turned into,

{
  "type": "string",
  "enum": [
    null
  ]
}

Two thoughts here:

  1. If "enum": [null] isn't allowed by the OpenAPI spec then we shouldn't allow it, as you're saying.
  2. I don't think the json produced above is expressing what I'm trying to say with joi. I think enum should be used with joi's valid() rather than with allow(). Does that seem right?

In any case, I can certainly push a fix to address thought number 1 above. Joi.string().allow(null) would then look like,

{
  "type": "string"
}
adrian-gierakowski commented 8 years ago

@devinivy : thanks for this! At least this way I will be able to use the v3 without changing my API. I will just include in the description of the property that null is allowed. Hopefully OpenAPI will allow properties to be null in next major version