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

findDangerousChanges doesn't work as expected after lexicographic sort and deserialization #2150

Closed
mrtnzlml opened this issue Sep 2, 2019 · 2 comments · Fixed by #2151
Closed
Labels

Comments

@mrtnzlml
Copy link

mrtnzlml commented Sep 2, 2019

Hello! My colleague reported a bug where findDangerousChanges reports invalid dangerous change after serializing the schema with lexicographicSortSchema and then deserializing it again. I tried to reproduce the error here: kiwicom/graphql-bc-checker@f5fa6be#diff-b3245f2dbca2b4c14d7bc1ff7eef9c87

Note that we are using one schema which has unsorted args, then printing it into a string and later trying to compare this string (potentially saved on disk) with the actual schema. It should not report any dangerous changes but it does report this:

{
  "description": "RootQueryType.xxx arg yyy has changed defaultValue from {aaa: \\"aaa\\", bbb: \\"bbb\\"} to {bbb: \\"bbb\\", aaa: \\"aaa\\"}.",
  "type": "ARG_DEFAULT_VALUE_CHANGE",
}

Thanks for checking it! :)

@Neitsch Neitsch added the bug label Sep 2, 2019
@Neitsch
Copy link

Neitsch commented Sep 2, 2019

Yep, this looks like a true issue. For input type default values we apparently just compare it's stringified value instead of doing a deep comparison. Could you maybe lexicographically sort the other schema as well before doing the comparison (as a temporary workaround).
The proper fix we should probably put in place is to lexicographically sort the value we stringify.

@IvanGoncharov
Copy link
Member

@mrtnzlml @Neitsch Fixed in #2151 if you have time can you please review it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants