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

Distinguish errors of validators, within the executor and within business logic #1373

Open
wutsch0 opened this issue Apr 28, 2023 · 3 comments

Comments

@wutsch0
Copy link

wutsch0 commented Apr 28, 2023

Hi,
accoring to your error-handling documentation there are 3 kinds of errors: Syntax, Validation and Execution.

Syntax errors seem to be well covered by GraphQL\Error\SyntaxError, but for all other error cases the base GraphQL\Error\Error is used, which makes it impossible to distinguish errors qualitatively. This would e.g. be needed to filter out errors in bug trackers. E.g. if triggered by clients which send invalid queries, such as leaving out required parameters or sending incorrect parameter types.
Theoretically it would also be possible to distinguish those kinds of cases by properties provided on GraphQL\Error\Error. So far there is only a category property which could be used, but it changes based on if a previous exceptions is provided during construction time. As both cases appear within the codebase, this property can not be used to distinguish such use cases at the moment.

Introducing more error types via exceptions inheriting from GraphQL\Error\Error seems to be the cleaner approach, but would have the disadventage of breaking backwards compatibility, since e.g. the visitors of validation rules should return more specific exceptions as of now.
A backwards compatible way could be to introduce additional properties in GraphQL\Error\Error as described above, but would have the disadventage of not enabling a hierachy within exceptions.

What is your opinion on that topic?

@spawnia
Copy link
Collaborator

spawnia commented Apr 30, 2023

So far there is only a category property

No more, we got rid of that in v15.

I am not convinced I agree with the categorization the docs put forth. In my opinion, syntax errors and validation errors fall in the same category - they are both caused by clients sending invalid requests.

@wutsch0
Copy link
Author

wutsch0 commented May 2, 2023

I am not convinced I agree with the categorization the docs put forth. In my opinion, syntax errors and validation errors fall in the same category - they are both caused by clients sending invalid requests.

Yes, in some way they do. To me it would be fine to have them in one category, as long as they would be distinguishable from other errors which occure within the business logic implementation

@spawnia
Copy link
Collaborator

spawnia commented May 2, 2023

Perhaps you can take a look at the exception classes defined in https://github.com/webonyx/graphql-php/tree/master/src/Error and their usage. I am open for pull requests that introduce an improved hierarchy.

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

No branches or pull requests

2 participants