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

Add a hint when decoding allowedValues #245

Merged

Conversation

kean
Copy link
Contributor

@kean kean commented Dec 21, 2021

I've tested OpenAPIKit with over 400k lines of OpenAPI specs. The most common issue I've encountered so far has to do with string enums and allowed values and the interplay of OpenAPIKit.AnyCodable, Yams, and Double. Here are a couple of examples:

Issues

Jira

searcherKey:
  type: string
  enum:
    - com.atlassian.jira.plugin.system.customfieldtypes:cascadingselectsearcher
    ...

I get AnyCodable with value 0 of type Int in the allowed values.

It happens because of the sexagesimal support in Yams. When it sees ":" in a value, it assumes it's a number. I think it's supposed to bail out when it sees it has letters, but it doesn't and returns 0.

Sexagesimal is just one of the crazy pre-2009 YAML features and I'm not sure why it's supported by Yams. I'm currently using a fork in my project.

Square

up_to:
  type: string 
  enum:
    - inf

I get AnyCodable with value Double.infinity in the allowed values.

This is a similar problem, but this time it's AnyCodable that causes an issue. YAML doesn't enforce strings to have quotes. So when AnyCodable calls if let double = try? container.decode(Double.self) {, Yams attempts to initialize a Double with the given string. And Double("inf") returns Double.infinity.

Suggested Fix

I suggest to add a hint when decoding allowedValues. When we know that Format.self is JSONTypeFormat.StringFormat.self, we can decode [String].self instead of [AnyCodable].self. It's more reliable and is better from the performance perspective as well.

Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's quite clever.

@mattpolzin
Copy link
Owner

mattpolzin commented Dec 21, 2021

If you don't mind, can you open a PR for this branch against main? There are a few other things I also plan to port to a new release of OpenAPIKit 2.x as well as releasing with 3.0.0.

@mattpolzin mattpolzin merged commit e84a82e into mattpolzin:release/3_0 Dec 21, 2021
@kean
Copy link
Contributor Author

kean commented Dec 21, 2021

If you don't mind, can you open a PR for this branch against main?

Yeah, absolutely. Cherry-pick: #246

For future references, here's another common example:

- in: query
  name: closed
  schema:
    type: string
      enum:
        - true
        - false

These values become booleans (AnyCodable with Bool). Same does "on" and "off" (because pre-2009 YAML).

@kean kean deleted the kean/allowed-values-hint branch December 21, 2021 13:16
@kean
Copy link
Contributor Author

kean commented Dec 21, 2021

And sexagesimals in the current version of Yams become a problem everywhere where the type is determined dynamically, not just in enums.

For example the "example" becomes AnyCodable with value 0 (Int):

webhook-config-url:
  type: string
  description: The URL to which the payloads will be delivered.
  example: https://example.com/webhook
  format: uri

I think we can be more clever about the types in case too. We know the type upfront. It's still going to be a problem with dictionaries though, so I think I'll have to continue using my Yams fork.

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.

2 participants