reverb / swagger-spec

This fork of the swagger-spec is intended for 2.0 planning
Other
19 stars 15 forks source link

Use of URI format #88

Closed maxlinc closed 10 years ago

maxlinc commented 10 years ago

The use of URI format may be problematic, especially in host:

"host": {
  "type": "string",
  "format": "uri",
  "pattern": "^((?!\\:\/\/).)*$",
  "description": "The fully qualified URI to the host of the API."
}

I see a variety of potential issues:

In general I think "URI" is too general, and we should describe a more specific rule (even if it's hard to fully validate via the schema). In this example some options for more strict phrasing of the description are:

  1. A valid hostname section of a URI (allows "example.com")
  2. A valid hostname and port section of a URI (allows "example.com:8080")
  3. A valid valid hostname and port section of a URI Template (allows "{region}.example.com:8080")
  4. A valid authority section of a URI Template (allows "username:password@{region}.example.com:8042")

These descriptions all clarify that the other parts of a URI (scheme, path, query, and fragment) are not allowed, and more clearly indicate if URI Templates or "path parameters" may appear in the host.

webron commented 10 years ago

While working on the documentation of the root structure of the new spec, I came across the host definition and while attempting to document it, I reached similar conclusions to what you write here. I'd like to add a few observations.

According to json-schema, uri format is based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html. Based on that, I even if we stick with the uri format, I believe it is too permissive (assuming we want to allow the host name only). Based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html#collected-abnf we'd want to catch $4 only which appears in Appendix B.

That said, @maxlinc mention of the uri format is correct, and values such as hello:world are just as valid.

Json-Schema also allows the hostname format which corresponds to http://www.rfc-editor.org/rfc/rfc1034.txt. This one is more strict with regards to the structure (hello:world is invalid), but if I understand correctly, it does not include the port. I couldn't find any sample in the current swagger2 repo that uses a port, but I assume it should be part of the host field. We could solve it by adding a numeric port field, and limiting host to hostname.

And last but not least, allowing for templates. The {} template was raised in the past by current users, and in the current spec, it could be valid as the part of the host and the basePath as well. I'd vote to allow this. I don't remember (haven't gotten there yet) if we allow global parameter definitions, and while I don't like the idea of global parameters, this would be the only place where I'd allow such global definition, even make it mandatory, and limit it to path params as part of the host and basePath.

webron commented 10 years ago

Another possible reason to have a separate port field is that it would still allow for relative hosting using a separate port for the documentation and the API (maybe someone would want that).

fehguy commented 10 years ago

We're not supporting templates in the hostname right now.

On Aug 11, 2014, at 5:25 AM, webron notifications@github.com wrote:

While working on the documentation of the root structure of the new spec, I came across the host definition and while attempting to document it, I reached similar conclusions to what you write here. I'd like to add a few observations.

According to json-schema, uri format is based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html. Based on that, I even if we stick with the uri format, I believe it is too permissive (assuming we want to allow the host name only). Based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html#collected-abnf we'd want to catch $4 only which appears in Appendix B.

That said, @maxlinc mention of the uri format is correct, and values such as hello:world are just as valid.

Json-Schema also allows the hostname format which corresponds to http://www.rfc-editor.org/rfc/rfc1034.txt. This one is more strict with regards to the structure (hello:world is invalid), but if I understand correctly, it does not include the port. I couldn't find any sample in the current swagger2 repo that uses a port, but I assume it should be part of the host field. We could solve it by adding a numeric port field, and limiting host to hostname.

And last but not least, allowing for templates. The {} template was raised in the past by current users, and in the current spec, it could be valid as the part of the host and the basePath as well. I'd vote to allow this. I don't remember (haven't gotten there yet) if we allow global parameter definitions, and while I don't like the idea of global parameters, this would be the only place where I'd allow such global definition, even make it mandatory, and limit it to path params as part of the host and basePath.

— Reply to this email directly or view it on GitHub.

webron commented 10 years ago

And the other issues? On Aug 11, 2014 5:27 PM, "Tony Tam" notifications@github.com wrote:

We're not supporting templates in the hostname right now.

On Aug 11, 2014, at 5:25 AM, webron notifications@github.com wrote:

While working on the documentation of the root structure of the new spec, I came across the host definition and while attempting to document it, I reached similar conclusions to what you write here. I'd like to add a few observations.

According to json-schema, uri format is based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html. Based on that, I even if we stick with the uri format, I believe it is too permissive (assuming we want to allow the host name only). Based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html#collected-abnf we'd want to catch $4 only which appears in Appendix B.

That said, @maxlinc mention of the uri format is correct, and values such as hello:world are just as valid.

Json-Schema also allows the hostname format which corresponds to http://www.rfc-editor.org/rfc/rfc1034.txt. This one is more strict with regards to the structure (hello:world is invalid), but if I understand correctly, it does not include the port. I couldn't find any sample in the current swagger2 repo that uses a port, but I assume it should be part of the host field. We could solve it by adding a numeric port field, and limiting host to hostname.

And last but not least, allowing for templates. The {} template was raised in the past by current users, and in the current spec, it could be valid as the part of the host and the basePath as well. I'd vote to allow this. I don't remember (haven't gotten there yet) if we allow global parameter definitions, and while I don't like the idea of global parameters, this would be the only place where I'd allow such global definition, even make it mandatory, and limit it to path params as part of the host and basePath.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/reverb/swagger-spec/issues/88#issuecomment-51786966.

fehguy commented 10 years ago

Adding a separate field for port feels like overkill. I suggest host:port is allowed

On Aug 11, 2014, at 7:29 AM, webron notifications@github.com wrote:

And the other issues? On Aug 11, 2014 5:27 PM, "Tony Tam" notifications@github.com wrote:

We're not supporting templates in the hostname right now.

On Aug 11, 2014, at 5:25 AM, webron notifications@github.com wrote:

While working on the documentation of the root structure of the new spec, I came across the host definition and while attempting to document it, I reached similar conclusions to what you write here. I'd like to add a few observations.

According to json-schema, uri format is based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html. Based on that, I even if we stick with the uri format, I believe it is too permissive (assuming we want to allow the host name only). Based on http://xml2rfc.ietf.org/public/rfc/html/rfc3986.html#collected-abnf we'd want to catch $4 only which appears in Appendix B.

That said, @maxlinc mention of the uri format is correct, and values such as hello:world are just as valid.

Json-Schema also allows the hostname format which corresponds to http://www.rfc-editor.org/rfc/rfc1034.txt. This one is more strict with regards to the structure (hello:world is invalid), but if I understand correctly, it does not include the port. I couldn't find any sample in the current swagger2 repo that uses a port, but I assume it should be part of the host field. We could solve it by adding a numeric port field, and limiting host to hostname.

And last but not least, allowing for templates. The {} template was raised in the past by current users, and in the current spec, it could be valid as the part of the host and the basePath as well. I'd vote to allow this. I don't remember (haven't gotten there yet) if we allow global parameter definitions, and while I don't like the idea of global parameters, this would be the only place where I'd allow such global definition, even make it mandatory, and limit it to path params as part of the host and basePath.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/reverb/swagger-spec/issues/88#issuecomment-51786966.

— Reply to this email directly or view it on GitHub.

webron commented 10 years ago

Okay, so should the regex be changed accordingly? If I understand correctly, on top of supporting URIs such as this:is:something, it would also support sample.com/base/path which is also undesirable.

maxlinc commented 10 years ago

I've been wondering if we should start putting a custom value for "format" anywhere we're using a complex "pattern". This already has has both a format and a pattern, but they don't work together. I propose changing the format from "uri" to something like "uri_authority".

I'd leave both in place until the Markdown updates are done, because the format field might help document the intent of the regex. Once that's done, I'd look at feedback from implementors to decide if we should keep both, just pattern, or just format.

My reason for thinking this is:

If paramType is "body", the name MUST be "body"

or

If type is File, the consumes field MUST be "multipart/form-data", and the paramType MUST be "form"

  • I think it's useful for documenting the schema (even if the values are mostly ignored by validators). Think of it like a name for the regex. It's especially if the format names are linked to IETF or W3C standards. E.g. if something has a format of "rfc6570" in the schema, then in the markdown you could list that as the type, and add a reference to the RFC at the bottom of the document.
  • JSON-Schema is still a draft, and some formats we need are likely widely used and might make good additions to json-schema draft 5.

[getAuthority]: http://docs.oracle.com/javase/7/docs/api/java/net/URI.html#getAuthority()

fehguy commented 10 years ago

i'm not sure how to do this with json-schema. are custom formats supported?

webron commented 10 years ago

I think custom formats are supported with extensions for now, we'd better just document it in the spec rather than control it via the schema. There are plenty of other validations that are not done by the schema as it is. It gives us something to aspire to.

On 26 August 2014 09:50, Tony Tam notifications@github.com wrote:

i'm not sure how to do this with json-schema. are custom formats supported?

— Reply to this email directly or view it on GitHub https://github.com/reverb/swagger-spec/issues/88#issuecomment-53381889.

webron commented 10 years ago

For now I'll close this issue and leave it as is. If we manage to fine tune it, we should do it at the main swagger-spec repo.