Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Update Error.java #24

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Update Error.java #24

wants to merge 12 commits into from

Conversation

nimmadi
Copy link

@nimmadi nimmadi commented Jul 23, 2018

Added path, errorType and extensions to Error class.

@chemdrew
Copy link
Contributor

http://facebook.github.io/graphql/June2018/#sec-Errors

GraphQL services should not provide any additional entries to the error format since they could conflict with additional entries that may be added in future versions of this specification.

We should not be providing default fields that do not conform to the GraphQL spec.
I am good with making the class extensible as described in #22

@chemdrew
Copy link
Contributor

And please keep all changes in this PR, do not close it to open a new one

@chemdrew
Copy link
Contributor

#23 relates here

@chemdrew chemdrew added the enhancement New feature or request label Jul 23, 2018
@nimmadi
Copy link
Author

nimmadi commented Jul 23, 2018

Yes, i have added whatever GraphQLError interface is providing the fields. Sample error message -->
{
"data": {
"findLoanAccounts": null
},
"errors": [
{
"extensions": {
"Empty": "Loan Accounts not found for search criteria [ #TIN = xxxx344]..."
},
"locations": [
{
"line": 2,
"column": 3
}
],
"errorType": "DataFetchingException",
"message": "Loan Accounts not found for search criteria [ #TIN = xxxx344]...",
"path": [
"findLoanAccounts"
]
}
]
}

@chemdrew
Copy link
Contributor

Unfortunately errorType is not defined in the GraphQL spec though and I am trying to keep parity with that. It would be best if we could make the Error class extensible so users can specify any number of fields if their servers create custom errors as well

@nimmadi
Copy link
Author

nimmadi commented Jul 23, 2018

Yes, you are right. Latest spec is not showing that property but GraphQLError interface from graphql-java artifact has that property. As per your suggestion, I have added one custom error class which supports additional properties now.

@coveralls
Copy link

coveralls commented Jul 23, 2018

Pull Request Test Coverage Report for Build 42

  • 11 of 12 (91.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 98.529%

Changes Missing Coverage Covered Lines Changed/Added Lines %
nodes/src/main/java/io/aexp/nodes/graphql/internal/CustomError.java 10 11 90.91%
Totals Coverage Status
Change from base Build 28: -0.2%
Covered Lines: 469
Relevant Lines: 476

💛 - Coveralls

@chemdrew
Copy link
Contributor

So, as mentioned in #22 - one of the most popular GraphQL server libraries allows error messages to be formatted any way the user specifies through the formatError function (https://github.com/graphql/express-graphql#options)
If we take this approach then every different server implementation will have to create its own custom class and that would be very ugly.

The solution I am envisioning is keeping the Error class in-sync with the GraphQL specification so that the majority of users can use this library with no additional configurations. And to solve the problem of custom error responses (in the case that graphql-java is providing here) we make Error extensible, either removing the final declaration of it or making it an interface and allowing the user to add their own implementation of it. The user then will have to supply their implementation of it to either the request builder or the template when making the request, not sure which makes more sense yet...

@smfaizalkhan
Copy link

@chemdrew
I was about to raise the same issue, but i was happy to notice that it is already opened and it is implemented by @coveralls CustomError
Im using the below dependancy

                 <groupId>io.aexp.nodes.graphql</groupId>
                  <artifactId>nodes</artifactId>
                   <version>0.5.0</version>

But i don't see a class of name CustomError and if so is it returned as part of
GraphQLResponseEntity<>.getErrors()
or
GraphQLResponseEntity<>.getCustomeErrors()

@chemdrew
Copy link
Contributor

@smfaizalkhan this pull request isn't merged so it is not in any release; please see my comment above for a better way of achieving this

@smfaizalkhan
Copy link

@chemdrew
Do you mean the
"And to solve the problem of custom error responses (in the case that graphql-java is providing here) we make Error extensible, either removing the final declaration of it or making it an interface and allowing the user to add their own implementation of it. The user then will have to supply their implementation of it to either the request builder or the template when making the request, not sure which makes more sense yet..."

If so ,the error class that we have if as below
public final class Error {
private String message;
private Location[] locations;
// const,getter,setter
}

@smfaizalkhan
Copy link

@chemdrew
My understanding based on the comments as highlighted below

And to solve the problem of custom error responses (in the case that graphql-java is providing here) we make Error extensible, either removing the final declaration of it or making it an interface and allowing the user to add their own implementation of it. The user then will have to supply their implementation of it to either the request builder or the template when making the request, not sure which makes more sense yet...

If so ,the error class that we have if as below
public final class Error {
private String message;
private Location[] locations;
// const,getter,setter
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants