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

type+format error on allowable combination (format: uuid) #19

Closed
SirSpidey opened this issue Jan 30, 2019 · 19 comments
Closed

type+format error on allowable combination (format: uuid) #19

SirSpidey opened this issue Jan 30, 2019 · 19 comments
Assignees

Comments

@SirSpidey
Copy link
Member

SirSpidey commented Jan 30, 2019

Because the validator allows only the formats that are listed in the specification table, it produces an error for some that are usable. For example, "format": "uuid".

However, format is an open value, so you can use any formats, even not those defined by the OpenAPI Specification, such as:
• email
• uuid
• uri
• hostname
• ipv4
• ipv6
and others

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types
https://swagger.io/docs/specification/data-models/data-types/

error

Message :   Property type+format is not well-defined.
Path    :   definitions.ErrorResponse.properties.trace.type

source

"trace": {
  "type": "string",
  "format": "uuid",
  "description": "A unique identifier of the request."
}

The trace use case is from our API Implementation Handbook about errors.

@dpopp07
Copy link
Member

dpopp07 commented Jan 30, 2019

I think we should be letting users know when they aren't using an explicitly defined format but an error might be too extreme, since the spec declares users "MAY" use other formats. Perhaps this should be a warning.

@mkistler What are your thoughts?

@mkistler
Copy link
Contributor

The OpenAPI spec allows any value for format, but values other than the "well defined" values in the OpenAPI spec are ambiguous and thus must be discouraged.

@SirSpidey
Copy link
Member Author

@mkistler Because it is allowed, I think it should be marked as a warning, not an error.

@mkistler
Copy link
Contributor

mkistler commented Feb 1, 2019

That's not the criteria that we use to decide error vs warning. There's a long list issues we flag as errors that are allowed by the spec.

      "no_array_responses": "error",
      "no_parameter_description": "error",
      "invalid_type_format_pair": "error",
      "content_type_parameter": "error",  // allowed in v2
      "accept_type_parameter": "error",   // allowed in 
      "no_empty_descriptions": "error",
      "no_consumes_for_put_or_post": "error",

We flag things as errors when they are 1) common sources of error, 2) generally correctable without huge effort.

Arbitrary values in "format" have been very common in WDC API docs and they cause significant problems in the SDKs, since they result in incorrect types for parameters or properties. I know of no strong argument for using non-standard "format" values, and I am unaware of any effort that is needed to change them other than to edit the API doc.

@alkurbatov
Copy link

@mkistler Perhaps it would be better to allow users to whitelist some format types? In case 'I know what I am doing'.

@mkistler
Copy link
Contributor

We will be adding some types -- specifically identifier and crn -- soon.

Re: "I know what I am doing", I think the more important question is: "Does everyone know what you are doing?" format: gobbledegook might be perfectly clear to you, but the API doc is meant as a communication mechanism to others.

@alkurbatov
Copy link

Well, it depends on particular situation. In my case we use Marshmallow which has explicit uuid type of schema field and write docs in Openapi format for internal use. In such situation uuid format looks ok. May be when we publish the API to the outer world it will be not true, but now 'everyone knows' :)

@bartkummel
Copy link

@mkistler We use uuid quite often in OpenTripModel, see for yourself in our sources. I see the point you're making. But the fact is, you are stricter than the OAS standard. There is value in that, for sure. But there are also many use cases where that's simply too strict. Would it be an idea to introduce two modes of operation? Like "strict" and "standard" or something like that?

@mkistler
Copy link
Contributor

@bartkummel I think we'd be open to a configuration option for this. That might be tricky to design in a general manner, but we could start with something that enables your use case and grow it from there as needed.

Maybe a configuration like:

"allowedStringFormats": [ "uuid", "other" ]

Another option is to disable the invalid_type_format_pair rule altogether, but I'm guessing you want to keep that rule on for its benefits in catching other issues.

@bartkummel
Copy link

@mkistler Exactly. I've disabled the rule for now, but it would be nice to have something like you suggest.

@apestov
Copy link

apestov commented Jun 4, 2019

What do you think if extract to config all allowed type+format pairs for primitive types? Like

"validPrimitiveTypeFormatPairs": {
  "integer": ["int32", "int64"],
  "number": ["float", "double"],
  "string": ["byte", "binary", "date", "date-time", "password"],
  "boolean": []
}

e.g. for cases of using ahead OAI/OpenAPI-Specification#845

@bartkummel
Copy link

@apestov I assume your example is the default then? So we can add e.g. uuid to the string list in our own configuration file? Sounds good to me!

@apestov
Copy link

apestov commented Jun 6, 2019

@bartkummel It's more like a proposal that may (or may not) feet the openapi-validator project approaches.
@mkistler @dpopp07 What do you think? If it sounds ok I can try to prepare a PR.

@bartkummel
Copy link

@apestov Sure, that's how I interpreted it.

@mkistler
Copy link
Contributor

mkistler commented Jun 6, 2019

I think I would prefer an approach that only extended the list of allowed type+format values.

There is a well-defined set of type+format pairs in the spec, and I don't see any value in generating messages for any of these. So a design that would let the user (in all likelihood unintentionally) drop these from the "allowed" list only creates an opportunity for people to shoot themselves in the foot.

@apestov
Copy link

apestov commented Jun 6, 2019

That make sense.
Yes, the question would be there a case (project) that would like to have more strict restrictions than linter has by default at the moment or will have by default in some future versions?
It probably could be imagined that some project would like to allow only int64 with no int32 for integer format, while I don't have a particular case for.

@jamescooke
Copy link
Contributor

Could someone point me at a doc or give a pointer as to the best openapi v3 way to define fields as UUIDs using the standard "5 groups separated by hyphens, in the form 8-4-4-4-12 for a total of 36 characters" format so that the "Property type+format is not well-defined." error is not raised? Is this possible right now in Openapi v3?

@mkistler
Copy link
Contributor

You could use a pattern. This StackOverflow post recommends a number of different regex's that describe UUIDs.

@jamescooke
Copy link
Contributor

Thanks @mkistler - I completely missed that in the Swagger docs here: https://swagger.io/docs/specification/data-models/data-types/#pattern

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

9 participants