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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING CHANGE: Change GraphQLID to only use strings #345

Merged
merged 3 commits into from Sep 17, 2019

Conversation

smyrick
Copy link
Contributor

@smyrick smyrick commented Sep 17, 2019

馃摑 Description

Due to deserialization issues that can happen with UUID, we are only going to support Strings as the explicit type for GraphQLID. You can still use UUID in your data fetchers but you will have to do the parsing yourself and handle the exceptions if it is not a valid UUID.

Example of client input

Screen Shot 2019-09-17 at 11 37 24 AM

馃敆 Related Issues

Fixes #317

Fixes ExpediaGroup#317

Due to deserliazation issues that can happen with UUID, we are only going to support Strings as the explicit type for GraphQLID. You can still use UUID in your data fetchers but you will have to do the parsing yourself and handle the exceptions if it is not a valid UUID.
@smyrick smyrick added type: enhancement New feature or request changes: major Changes require a major version labels Sep 17, 2019
@smyrick
Copy link
Contributor Author

smyrick commented Sep 17, 2019

Example of error that can happen with current implementation #317 (comment)

@smyrick smyrick merged commit 2955286 into ExpediaGroup:master Sep 17, 2019
@smyrick smyrick deleted the graphql-id-string branch September 17, 2019 20:34
@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #345 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #345      +/-   ##
============================================
- Coverage     96.83%   96.82%   -0.01%     
  Complexity      275      275              
============================================
  Files            82       82              
  Lines           980      977       -3     
  Branches        182      182              
============================================
- Hits            949      946       -3     
  Misses            6        6              
  Partials         25       25
Impacted Files Coverage 螖 Complexity 螖
...group/graphql/exceptions/InvalidIdTypeException.kt 100% <100%> (酶) 1 <1> (酶) 猬囷笍
...ediagroup/graphql/generator/types/ScalarBuilder.kt 100% <100%> (酶) 7 <2> (酶) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 1c85582...cb8eed8. Read the comment docs.

dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
* Change GraphQLID to only use strings

Fixes ExpediaGroup#317

Due to deserliazation issues that can happen with UUID, we are only going to support Strings as the explicit type for GraphQLID. You can still use UUID in your data fetchers but you will have to do the parsing yourself and handle the exceptions if it is not a valid UUID.

* Drop extra id fields

* Fix unit testS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: major Changes require a major version type: enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Change @GraphQLID to only work on strings
3 participants