search

profusion / sgqlc

Simple GraphQL Client
https://sgqlc.readthedocs.io/
ISC License
513 stars 85 forks source link

GraphQL and JSON errors are not printed #141

Closed philon123 closed 3 years ago

philon123 commented 3 years ago

.. instead they are logged as INFO severity. However the text GraphQL query failed with 1 errors is logged with ERROR severity. It leads to the annoying situation that the script output tells you there is an error, but not which one.

I tried to decrease the log level to INFO so that the error details will be printed, however it's still not being printed. What did I do wrong? url = "http://localhost:8080/v1/graphql" endpoint = HTTPEndpoint(url) endpoint.logger.setLevel(logging.INFO)

I suggest to keep both the GraphQL query failed with.. text and the error details on the same log level - either WARNING or ERROR.

It would also be nice to have a flag on BaseEndpoint.__call__ to optionally throw an exception on error. Or is it a design decision to never throw an error? I believe in the JS world it's custom that all API calls return a tuple (error, result) and you always need to check if error == None before dealing with the results.

maaft commented 3 years ago

I also think having an option to do error handling is a must.

barbieri commented 3 years ago

guys, the error is converted to GraphQL format, that is, the returned object is {"errors": [{"message"....}]} so you can still access it, you can even get the actual exception in the "exception" key of the error array item:

https://github.com/profusion/sgqlc/blob/master/sgqlc/endpoint/http.py#L212 https://github.com/profusion/sgqlc/blob/master/sgqlc/endpoint/http.py#L234

The reason it's not being thrown is a design decision to cope with GraphQL: it may return data AND errors. For HTTP, okay, it would never come with any data and a single error, but for standard queries/mutations, you may have partial results, which is totally valid (ie: some fields fail, if they are nullable, they will be made null and errors is appended with the message + path). Then all the errors are normalized to be handled in the GraphQL format (dict with errors key).

As for the way to set the logging level, it looks correct, not sure what's happening... just be sure to call this after the logging.basicConfig() and the likes.

maaft commented 3 years ago

Ahh, I was looking at (op + data) and the error was gone there. Thanks!

barbieri commented 3 years ago

can I close? Or is there anything else?

barbieri commented 3 years ago

could you test #145?

philon123 commented 3 years ago

thanks a bunch for the explanation! Now that I know how errors work in GraphQL, I understand the original design decision.

Now I even have mixed feelings about your new commit that throws an exception if there are only errors in the result. It means that when making a call, I need to both put a try/catch around it AND check for the errors list in the result. Doesn't that just increase the complexity?

Regarding http errors, not connected to GraphQL at all, I think they should raise an exception immediately. To handle GraphQL errors, you could add an option "raise_on_error" (default False), which enables exceptions to be thrown for each GraphQL-error returned from the server. What do you think of these suggestions?

Either way, the examples in the (great!) docs should include error handling using the best practice, as defined by you! That way, people like me will be guided correctly :)

barbieri commented 3 years ago

yes, it increases the complexity a bit but OTOH saves bugs... since people can't just "let's assume server never fails". The error list is often ignored if there is at least some partial response, very few people will handle both at the same time.

I'll see if I do some more examples/docs on the error handling, patches are welcome