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

Always treat id as string, never a number (in JS) #1405

Closed
himdel opened this issue May 22, 2017 · 7 comments
Closed

Always treat id as string, never a number (in JS) #1405

himdel opened this issue May 22, 2017 · 7 comments
Assignees

Comments

@himdel
Copy link
Contributor

himdel commented May 22, 2017

Right now, some places treat entity ids as numbers, some as strings.

And that would be fine if the max region number were 9006. But unfortunately that's not the case, @tzumainn recently encountered an id looking like 58015000000000001 (so, region = 58015), which is bigger than 9007199254740991 (Number.MAX_SAFE_INTEGER in JS (ref)).

Thus, if we need to support regions over 9006, the UI needs to treat every id as a string, never a number.
Furthermore, we'll need the API to never send a number and always convert to a string.

@himdel
Copy link
Contributor Author

himdel commented May 22, 2017

Additional info from ☝️ May 22, 2017 5:31 PM...

https://github.com/ManageIQ/manageiq-gems-pending/blob/master/lib/gems/pending/appliance_console/database_configuration.rb#L30

REGION_RANGE = 0..9223371

# PG 9.2 bigint max 9223372036854775807 / ArRegion::DEFAULT_RAILS_SEQUENCE_FACTOR = 9223372
# http://www.postgresql.org/docs/9.2/static/datatype-numeric.html
# 9223372 won't be a full region though, so we're not including it.

@Fryguy
Copy link
Member

Fryguy commented May 25, 2017

Aside, I believe the REST API always does this anyway for the same reason.

@himdel
Copy link
Contributor Author

himdel commented Jun 5, 2017

Right now, the REST API is sending numbers in the JSON, not strings :( .. but @abellotti mentioned a plan to make API send cids instead of those numbers. (Alberto, is there anything ..trackable we can link from here?)

@abellotti
Copy link
Member

just added this: https://www.pivotaltracker.com/story/show/146613947

Thanks,

/cc @imtayadeway @jntullo @gtanzillo

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2017

Right now, the REST API is sending numbers in the JSON, not strings :( .. but @abellotti mentioned a plan to make API send cids instead of those numbers. (Alberto, is there anything ..trackable we can link from here?)

Oh wow that's super surprising...I brought this up very early on in the development of the API 😕

@himdel
Copy link
Contributor Author

himdel commented Jun 29, 2017

(PR: ManageIQ/manageiq#15430 )

@himdel
Copy link
Contributor Author

himdel commented Aug 3, 2018

(Also related: #3243, but that PR gives me a 500 right now.)

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

3 participants