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

[OpenAPI] Safeint emits int64 format #2367

Closed
bterlson opened this issue Sep 5, 2023 · 9 comments · Fixed by #2933
Closed

[OpenAPI] Safeint emits int64 format #2367

bterlson opened this issue Sep 5, 2023 · 9 comments · Fixed by #2933
Assignees
Labels
design:needed A design request has been raised that needs a proposal triaged:core
Milestone

Comments

@bterlson
Copy link
Member

bterlson commented Sep 5, 2023

While there may not be an accurate format specifier for safeint, I believe the current emit of format: int64 feels incorrect. The entire point of safeint is an int that fits safely into a double, so the ideal JS emit for this would indeed be a double. But the emitted OpenAPI doesn't indicate this, so a conservative emitter would pick BigInteger (at severe ergonomic cost).

I believe we have a few options, any of which would be better IMO:

  1. argue for adding int53 to the format registry and use that if accepted
  2. just use int53
  3. use double
  4. omit format entirely
@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Sep 7, 2023
@markcowl markcowl added this to the [2023] October milestone Sep 7, 2023
@connorjs
Copy link
Contributor

connorjs commented Sep 10, 2023

I had never seen “safe int” before in type definitions, but I liked the idea at first look. However, I found this issue when looking to request a “Big Integer” format (a string format).

If bigInteger existed, I do not think I would use safeint to model. Instead, I would use int32, decimal/float64, or bigInteger.

Therefore,

  1. Should TypeSpec add a bigInteger string format?
  2. I vote for option (3) or (4) - using double (float64?) or removing safeint

Edit: Smithy has bigInteger (and bigDecimal) for what it’s worth: Smithy simple types.

@bterlson
Copy link
Member Author

@connorjs I think it would be a mistake to use a hypothetical bigInteger for integers in the safeint range above 2^32-1. I've seen that in practice there are many integers that don't fit in 32 bits but which fit comfortably in 53 bits, and using a bigInteger type for those adds significant overhead to languages whose natural number type is a double (e.g. JS/JSON). For example, when emitting a bigInteger in JSON Schema, it would be serialized as a string to avoid precision loss (though this is configurable).

We effectively do have bigInteger and bigDecimal types - these are named integer and decimal respectively. Since these types do not have any limits on range or precision, emitting these types to a bigInt/bigDec type is likely the only reasonable choice to avoid data loss. We should definitely call this out in the docs, though, I'll file an issue for that.

@connorjs
Copy link
Contributor

Thanks for the information @bterlson! Good points. That also clarifies the intent of integer and decimal to me: the API needs to qualify their range/it's not explicit. It confirms my existing use of int32 instead of integer where appropriate.

@timotheeguerin
Copy link
Member

Proposal

Current behavior is we emit type: integer, format: int64 for safeint Playground

Option 1: int53

Use a new format called int53 which would accurately represent the range of values that can be represented by a safeint in Typespec.

It wasn't really added as a common format in the openapi format registry but seemed to have looked like a good candidate. OAI/OpenAPI-Specification#1517

This is a nice option as it give a clear indication of the type of integer that is being represented.

Option 2: int64 with maxValue

Keep it as an int64 but set the maximum property to the max value of a safeint.

Other alternatives

  • Use double: Javascript does have support for float64/double so it would be an equivalent representation but also a bit misleading.
  • Omit the format and just use type: integer. This feels even more wrong as it would suggest that any integer value going to infinity is valid.

@markcowl markcowl assigned mikekistler and unassigned bterlson Nov 2, 2023
@markcowl markcowl modified the milestones: [2023] November, Backlog Nov 6, 2023
@mikekistler
Copy link
Member

The "double-int" format has been approved and merged into the OAI Format Registry.

https://spec.openapis.org/registry/format/double-int.html

I think the autorest and openapi3 emitters should render the safeint type as type: integer, format: double-int.

@timotheeguerin
Copy link
Member

Agree on changing openapi3,

@johanste what do you think about autorest emitter respecting this format, it will for sure not work with autorest and the linter?

@mikekistler
Copy link
Member

Oh good point! Maybe just the openapi3 emitter, and leave autorest as is.

@markcowl
Copy link
Contributor

  • wait on this change
  • keep current behavior by default and add an option to use double-int
    • when to change the default
    • what data should we / can we gather to determine this
      • list of influential customers
  • add doubleInt scalar
  • add warning on emitting safeInt without specific encoding
  • [separately]: Add OpenAPI decorator corresponding to format

@markcowl
Copy link
Contributor

new config option: safeint-strategy: "int64" | "double-int"

timotheeguerin added a commit that referenced this issue Mar 1, 2024
fix  #2367 
Format was added to the schema registry so we are safe to use that now
https://spec.openapis.org/registry/format/double-int.html

---------

Co-authored-by: Brian Terlson <brian.terlson@microsoft.com>
markcowl pushed a commit to markcowl/cadl that referenced this issue Mar 8, 2024
…soft#2933)

fix  microsoft#2367 
Format was added to the schema registry so we are safe to use that now
https://spec.openapis.org/registry/format/double-int.html

---------

Co-authored-by: Brian Terlson <brian.terlson@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants