Skip to content

Add example of negative integer coercion to ID #480

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

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

IvanGoncharov
Copy link
Member

Changed from:

or integer (such as 4) input value

to

or non-negative integer (such as 0 or 4) input value

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Jul 3, 2018

I would appreciate if you could describe the motivation behind this proposal. I remember raising similar issue in past in a context of an empty string ("") IDs: #404

ATM I have some concerns about this constraint. It is a breaking change and I have a feeling that in might affect some of the API. Also, this limitation (as well as disallowing empty strings) might cause issues integrating with legacy systems (-1 also comes in mind, I have seen API that use such IDs). But also in general, I think IDs should be opaque. I see constraints like this as part of the application domain, so I feel that we should not enforce them in the spec.

@IvanGoncharov
Copy link
Member Author

@OlegIlyenko Looks like negative IDs is a thing:
https://dba.stackexchange.com/questions/2895/what-are-negative-keys-used-for

In this case, I think the specification should have an example of such conversion to show that this is intentional behavior.
I changed PR to:

or integer (such as 4 or -4) input value

@IvanGoncharov IvanGoncharov changed the title [RFC] Restrict ID coercion to non-negative integers Add example of negative integer coercion to ID Jul 3, 2018
@leebyron
Copy link
Collaborator

leebyron commented Aug 7, 2018

A good next step might be an analysis of what GraphQL libraries already have this restriction in place, to better understand the cost of a breaking change. I believe GraphQL js has had this for a long time and I know many libraries start as a port of that library

@IvanGoncharov
Copy link
Member Author

@leebyron After @OlegIlyenko suggestion I changed this PR to just add example of negative ID value so that it more explicit in the spec.
So no breaking changes in this PR.

@leebyron leebyron added 🤷‍♀️ Ambiguity An issue/PR which identifies or fixes spec ambiguity and removed clarification labels Oct 2, 2018
@leebyron leebyron merged commit 7133b59 into graphql:master Oct 2, 2018
@IvanGoncharov IvanGoncharov deleted the nonNegativeID branch November 29, 2018 13:41
@leebyron leebyron added this to the May2021 milestone Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷‍♀️ Ambiguity An issue/PR which identifies or fixes spec ambiguity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants