Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'host' regex and format: uri conflict #420

Merged
merged 1 commit into from
Jul 30, 2015
Merged

Conversation

hornc
Copy link
Contributor

@hornc hornc commented Jul 22, 2015

I believe the "format": "uri" contradicts the regex pattern below in the host specification.

RFC 2396 URI require a scheme (e.g. 'http') but the swagger schema documentation https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#swaggerHost and the pattern regex specifically exclude schemes.

The description 'fully qualified URI' is therefore incorrect.

I've removed "format": "uri" and updated the description to better explain the field and gave an example. I didn't mention the optional :<port number> as I thought that might be too much detail for a description, but maybe that would be helpful too?

Before making this change I tested the json-schema "format": "hostname" in case that gave the desired validations, but that hostname does not accept port numbers, so this looks like it needs to be a custom format as it is NOT a rfc1034 compliant hostname as referenced from the json-schema docs, http://json-schema.org/latest/json-schema-validation.html#anchor114

@webron
Copy link
Member

webron commented Jul 30, 2015

@hornc - thanks for the PR.

I don't mind the description to include the optional port number as well, but since it's mentioned in the spec it's not too bad.

FWIW, back when creating the schema I checked with the people behind JSON Schema regarding the meaning of the uri format. It feels like it is not well defined, and some see it as a full URI and some see it as a partial URI (which seems to be the option agreed upon more). Based on that, I used the uri format. However, it looks like some validators don't respect that decision, so I'm fine with removing the format.

webron added a commit that referenced this pull request Jul 30, 2015
Fix 'host' regex and format: uri conflict
@webron webron merged commit 0b574df into OAI:master Jul 30, 2015
@hornc
Copy link
Contributor Author

hornc commented Jul 31, 2015

Thanks @webron. I thought I linked to all the waypoints in my investigations above, but I seem to have missed the URI specific ones, which are a series of hops between documents.

The json-schema docs say of the uri format: "A string instance is valid against this attribute if it is a valid URI, according to [RFC3986]." source

As I interpreted RFC3986 a "URI" must have the scheme, although that document does mention "URI references" which seem to be the things that are more flexible and may be what you mean by a partial URI, which do sound like they are more commonly used, see these examples on wikipedia.

The above is just to let you know my investigation process when I tried to make sense of it all, there were too many document sections to cross reference, I meant to include them all in the PR description!

@webron
Copy link
Member

webron commented Jul 31, 2015

@hornc - thanks for the details. It's easy to miss things when writing a specification (json schema, swagger), and some things are left open for interpretation to some extent. As far as I'm concerned, I'm not looking for the most strict schema, but rather something that helps and guides. Eventually, the source of truth is the spec itself and not the schema (especially since some parts of the spec cannot be covered by the schema). Again, thanks for taking the time to investigate this issue and pinpoint its source.

@testn
Copy link

testn commented Oct 21, 2015

@webron
Copy link
Member

webron commented Oct 21, 2015

@testn - just tested it and it looks like the host does allow a port. How did you test it to see something else? As for ipv6 - you are correct. Can you open a separate issue on it? I'll try resolving it as soon as I can.

@testn
Copy link

testn commented Oct 21, 2015

Ah never mind I forgot to unescape the backslash. Regarding ipv6, I already opened #489

@webron
Copy link
Member

webron commented Oct 21, 2015

@testn - yeah, the escaping got me too several times in the past, no worries. Thanks for opening the ipv6 issue.

@testn
Copy link

testn commented Oct 21, 2015

Also the spec is not clear that host actually can contain port information as well even though it is confusing when you can do http/https at the same as mentioned in #214

@webron
Copy link
Member

webron commented Oct 21, 2015

From https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#swaggerHost -

The host (name or ip) serving the API. This MUST be the host only and does not include the scheme nor sub-paths. It MAY include a port.

And yes, as you mentioned #214 is a known issue and we'll hopefully deal with it better in the next version of the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants