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

Change the error message to be consumer targeted #2096

Merged
merged 2 commits into from
Apr 18, 2022

Conversation

telemenar
Copy link
Contributor

@telemenar telemenar commented Apr 15, 2022

The existing "must not be null" message if read by the caller of a gql api could be confusing because nothing they provided was null.

Changed the message from "must not be null" to "the requested element is null which the schema does not allow"

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@StevenACoffman
Copy link
Collaborator

@RobinCPel Would you mind reviewing this websocket PR?

@coveralls
Copy link

coveralls commented Apr 17, 2022

Coverage Status

Coverage remained the same at 74.497% when pulling 9ee3cd6 on observeinc:tweakErrorMessage into 5a49764 on 99designs:master.

@telemenar
Copy link
Contributor Author

Oh, I need to re-run generate after cherry-picking this from a v0.17.2. And it looks like array ordering for the error messages isn’t stable as the failing test passes on my infra. I’ll fix both of these on Monday.

Chris Pride added 2 commits April 18, 2022 16:51
Fix the test to not be sensitive to array ordering.
Re-generate on master as there was a schema change.
@telemenar
Copy link
Contributor Author

telemenar commented Apr 18, 2022

Sorry for the noise with this I messed up my rebase and ended up with reparented commits I didn't intend to reparent.

Also this isn't necessarily web-socket related, this error message applied outside of the web-socket context.

To simplify review a little bit since this makes a bunch of noise in generated files the main file with explicit changes is:

codegen/type.gotpl

And then these test files:

codegen/testserver/followschema/nulls_test.go
codegen/testserver/followschema/time_test.go
codegen/testserver/singlefile/nulls_test.go
codegen/testserver/singlefile/time_test.go

@StevenACoffman
Copy link
Collaborator

Huh, so is the array order for the error message stable now and this is ok to merge as is? Or is there something else to be done to make the tests not flaky?

@telemenar
Copy link
Contributor Author

telemenar commented Apr 18, 2022

In the tests I moved from exact string compare on the error to separate Contains checks for each element I expect to exist:
9ee3cd6#diff-2ce323bb1a7505e85e08b7e71b9a3802f8ca0b7d649d47be188b95f32b54ca2f

So the tests no longer care about the error message ordering, it should be good to go.

@StevenACoffman
Copy link
Collaborator

Thanks so much! I appreciate the wording improvement.

@StevenACoffman StevenACoffman merged commit e3f04b4 into 99designs:master Apr 18, 2022
@frederikhors
Copy link
Collaborator

@telemenar I really appreciated this change.

Now that I updated my project's gqlgen version I noticed the diff in new generated files and found out 84 times the string: "the requested element is null which the schema does not allow".

Can we use a const for it?

Since I'm not familiar with the gqlgen code could you create a PR for this? What do you think?

Maybe something smarter would be: "if there are at least two occurrences of this string create a const".

@telemenar telemenar deleted the tweakErrorMessage branch September 29, 2023 18:26
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

Successfully merging this pull request may close these issues.

4 participants