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

Do not force "" for http status code #2171

Closed
pkuczynski opened this issue Mar 5, 2020 · 22 comments
Closed

Do not force "" for http status code #2171

pkuczynski opened this issue Mar 5, 2020 · 22 comments

Comments

@pkuczynski
Copy link

In JSON the key which is a number must be wrapped into "". This is not the case of yaml format. Different formats, different requirements. It does not make sense to force json requirements into yaml and this makes it really annoying to have to use "400": ... in YAML

Please also note that https://editor.swagger.io does not wrap status codes into "", which is correct, but contradictory to this spec:

I propose to remove the requirement of providing status codes as strings in YAML and only keep it for JSON, leaving the alignment for the transformation tool between two formats.

I already created #2147, which got rejected, so now I am opening this issue as the proposal for the next version of the spec.

@notEthan
Copy link
Contributor

notEthan commented Mar 7, 2020

I think this would not be an improvement to OpenAPI. in terms of general principles, OpenAPI exists to aid interoperability. changes that add ambiguity to how an API can be represented in OpenAPI are counter to that. every tool would now have to look for numeric keys in addition to string keys. it opens up the possibility for collisions if both integer 200 and string "200" are present. it brings in issues of comparing numbers (is 200 equal to 200.0? in some languages all numbers are parsed to floats).

having a single, consistent way to represent an http status obviates all these problems. I appreciate that the aesthetics of having to quote these, when you don't normally have to quote object keys in YAML, is suboptimal. but it is best for keys to remain as strings.

it's unfortunate that the swagger editor uses integer keys. they shouldn't. it looks like these are implicitly converted to strings by their YAML parser (which is not usual and seems plainly incorrect according to the YAML spec - but that's far afield). I will open an issue to correct their examples (unless, of course, the consensus on this issue goes the other way).

@pkuczynski
Copy link
Author

pkuczynski commented Mar 7, 2020

In the HTTP/1.1 standard (RFC 7231: https://tools.ietf.org/html/rfc7231#page-47) response status code is described as:

three-digit integer code giving the result of the attempt to understand and satisfy the request.

In all of the languages and libraries I know, it is implemented as an integer.

Forcing in OpenAPI spec to represent response status code as a string is misleading, against HTTP standard and all of the software implementations.

What's worse is that it's done only to cover the limitations of the JSON format not being able to handle numeric kets or to avoid double keys in YAML. I believe such things should be covered by the linting tools, instead OpenAPI spec introducing misleading requirements.

@MikeRalphson
Copy link
Member

The responses object is defined as a map of string keys to response objects. This is to facilitate the use of range codes like 4XX and the default value.

@notEthan
Copy link
Contributor

notEthan commented Mar 7, 2020

In all of the languages and libraries I know, it is implemented as an integer.

I've worked with a lot of http libraries, the string/integer decision seems like a coin toss sometimes. arguably an integer is more appropriate, but it doesn't really matter. OpenAPI documents are JSON (even when they are YAML), and in JSON, object keys are strings, not integers.

@pkuczynski
Copy link
Author

I am keen to see the library which exposes response status code as string :)

The internals of OpenAPI spec does not matter so much. If the format (like YAML) offering easy transformation to what OpenAPI uses under the hood, this should not force on the format to use something not natural. YAML keys wrapped into "" is not common for this format.

I believe OpenAPI spec should focus on the value it offers (and it's undoubtedly great!) instead of specifying format details.

@pkuczynski
Copy link
Author

@MikeRalphson responses could be defined as "integer | string" enum to satisfy the case you have mentioned.

@hkosova
Copy link
Contributor

hkosova commented Mar 14, 2020

@pkuczynski your proposal would allow situations like this

responses:
  200:
    ...
  "200":
    ...

which would need additional wording in the Spec to ensure the uniqueness of map keys and, as a result, require additional work from tooling vendors.

Using a single data type (string in this case) helps keep things simple.

@pkuczynski
Copy link
Author

pkuczynski commented Mar 15, 2020

@hkosova this is a very unlikely situation, and I think very easy to spot when reviewing the spec (source or rendered to a nice doc) - duplicated keys would really stand out. And still, if one would miss it, he could use one of the many linting tools to ensure the syntax of the spec is correct.

I believe that it would be better to say that response code need to be unique, instead of forcing "" on the format, where they are not needed and very not natural to use.

@philsturgeon
Copy link
Contributor

@pkuczynski unlikely based on what? Two people merge a change to an OpenAPI document, both adding 404, one uses string the other doesn't. Now you have a "404" and a 404.

@pkuczynski
Copy link
Author

This should be picked up by git as a merge conflict, so the second-person would see it and resolve it. In a rare case of mistake, validation should report it...

@philsturgeon
Copy link
Contributor

That's not actually how git works. 😅

@pkuczynski
Copy link
Author

I know how git works, trust me :)

@philsturgeon
Copy link
Contributor

I'm sure.

Anyway, due to the need to support 4XX and the default value, this does need to be a string. Thanks for the interesting discussions.

@karenetheridge
Copy link
Member

This entire ticket, and #2147 before it, doesn't make sense. In JSON the only valid format for an object key is a string, so any yaml-to-json translator will turn a numeric key into a stringy key.

For example:

echo "---^M200: a value^M404: another value" | yaml2json
{
  "200" : "a value",
  "404" : "another value"
}

(where yaml2json is a bash function, and I can guarantee that these YAML and JSON backends work properly (the YAML one having been written by the inventor of YAML):

yaml2json () 
{ 
    perl -MYAML::XS -MJSON::MaybeXS -wle'print JSON()->new->pretty->indent_length(2)->canonical->encode(Load(do { local $/; <> }))' $*
}

@pkuczynski
Copy link
Author

@karenetheridge I agree with you. That's why I believe spec should not mention that keys need to be strings, as it is obvious for json and does enforce people to write ugly yaml:

responses:
  "400":
    description: This is a description

instead of totally fine and more yaml-natural way:

responses:
  400:
    description: This is a description

That was the whole point if this issue: don't mention the string format for status code and leave that up to the file format.

Do you know what I mean @karenetheridge?

@pkuczynski
Copy link
Author

I am referring to this sentence from https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md

Screenshot 2020-04-29 at 23 18 39

@karenetheridge
Copy link
Member

@pkuczynski I see what you mean. I agree, I think, but I will sit this one out as I am 1. not heavily involved in OpenAPI, and 2. work in a language that only has stringy keys so the YAML loading would work fine for me in any case (I apologize for my privilege blinding me to the oppression that others are dealing with) 😁

@pkuczynski
Copy link
Author

At least one person understands what I am talking, thank you @karenetheridge :)
This issue is closed so I guess its a lost case anyway...

@handrews
Copy link
Member

@pkuczynski @karenetheridge what @philsturgeon is this:

detective-eye:~ handrews$ cat foo.yaml 
---
400: foo
"400": bar

detective-eye:~ handrews$ python
Python 2.7.16 (default, Jan 27 2020, 04:46:15) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import yaml
>>> yaml.load(open('foo.yaml'))
{400: 'foo', '400': 'bar'}
>>> 

Given a long list of errors, where there's already a 400 buried in there somewhere, someone could easily add a "400", which would absolutely not be a git conflict. Now you've got two different status 400 responses which some languages, such as Python, can parse preserving both entries, but others, such as JavaScript with the popular js-yaml package, cannot:

detective-eye:json-schema-tools handrews$ node
> yaml = require('js-yaml');
> fs   = require('fs');
> yaml.safeLoad(fs.readFileSync('/Users/handrews/foo.yaml', 'utf8'));
{ YAMLException: duplicated mapping key at line 3, column 1:
    "400": bar
    ^
    at generateError (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at storeMappingPair (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:308:7)
    at readBlockMapping (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1071:9)
    at composeNode (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
    at readDocument (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
    at loadDocuments (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/Users/handrews/src/json-schema-tools/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
  name: 'YAMLException',
  reason: 'duplicated mapping key',
  mark:
   Mark {
     name: null,
     buffer: '---\n400: foo\n"400": bar\n\n\u0000',
     position: 13,
     line: 2,
     column: 0 },
  message:
   'duplicated mapping key at line 3, column 1:\n    "400": bar\n    ^' }
> 

Saving two keystrokes to quote your keys is not worth this headache.

@pkuczynski
Copy link
Author

I know what @philsturgeon is trying to say, buts I simply don't think OpenAPI should be worried about details of formatting. It should be left alone to the specific format itself. It.would be enough to say, that error codes should be unique and the linting tool should be the one holding the guard.

Currently, some yaml linters happily accept duplicated keys no matter if they in or without quotes. I used following example:

--- 
responses: 
  404: abc
  404: cde
  "404": abcd

And following lint parsers considered this code totally valid (despite using quotes):

They clearly don't follow the spec https://yaml.org/spec/1.2/spec.html#id2759572, but it would only mean that your enforcement of putting things into quotes does not serve the purpose. It should rather formulate that keys should be uniq. Thats also what YAML 1.2 spec says, but not JSON spec (JSON's RFC4627 requires that mappings keys merely “SHOULD” be unique)

Following some other YAML parsers online, implementing YAML 1.2 spec you will notice that they would report duplicated keys in this example:

--- 
responses: 
  404: abc
  "404": cde

So if python yaml parser does not do that, you should report an error there, instead of forcing people to write in yaml response statuses wrapped in quotes

@philsturgeon
Copy link
Contributor

I appreciate the mentions but I have to keep coming back here to unsubscribe.

@handrews
Copy link
Member

@pkuczynski Pointing out the wobbly inconsistency of YAML tooling is really not making your case any stronger. I think I'll unsubscribe from this, too, as I find the whole complaint utterly baffling.

I'm of the "there should be one way to do things" mentality when it comes to specs like this, and there's clearly only one option that works for every format and every use of that field (which is not limited to numbers). Everything else is needlessly complicated.

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

No branches or pull requests

7 participants