Open-EO / openeo-api

The openEO API specification
http://api.openeo.org
Apache License 2.0
91 stars 11 forks source link

Generic "Client" error #456

Closed soxofaan closed 1 year ago

soxofaan commented 2 years ago

There is the "Internal" error code for generic server errors (status 500): https://github.com/Open-EO/openeo-api/blob/f303d65a3291d4cd74dacc0e796803bb5d6fa03b/errors.json#L2-L9

Sometimes there are error situations where the problem is in the client request, outside of the context of a process, and it's too much of a edge case or overkill to define/standardize it. Can we also add a generic client error code (status 400)?

m-mohr commented 2 years ago

Yes, we could, but thinking a bit more about it I'm actually more in favor of removing the Internal error for servers, too. HTTP 400 (client-side) and 500 (server-side) are general purpose error codes that can be used for a variety of errors and should probably not be captured by a single error code that doesn't really add a lot on top. Instead, I think back-ends should simply send custom error codes that are more descriptive than "Internal" or "ClientError". Better would be codes like "ServerStorageExceeded" with a 500 or "AuthorizationHeaderInvalid" with a 400 code. Unifying a lot of different errors using a single code just makes it harder to figure out what happened and makes things more confusing IMHO. It also gives devs a "lazy" way to return not very useful errors. Looking at my GEE code I'm also throwing an "Internal" error way too often. ;-)

soxofaan commented 2 years ago

I would not remove the generic codes, they are useful as fallback in the outermost, generic exception handler, which kicks in when all other smarter and more context-aware handling failed. That's what we use the "500 Internal" for at least. We also use it in a couple of edge cases that don't have a dedicated error code, but then we try to change the error message to make it more descriptive at least.

It also gives devs a "lazy" way to return not very useful errors.

I think that by even removing standardization of "500 Internal" you make the situation even worse: not only will the lazy errors be un-useful for users, they even won't be properly handled by clients/tools (because they are not standardized). I think you solve the laziness problem by adding more generic options (e.g. "400 ClientError"), instead of removing options.

m-mohr commented 2 years ago

I can see and accept that most servers likely have a global fallback mechanism in case when an exception is not handled and that it falls back to the Internal error in this case. So agreed to not remove it.

On the other hand, I don't see that for the client error yet. In which case should that be used? Can you point me to some code where this could be used? If that's an exception handler around all your parsing/validation/execution of a user process, how can you distinguish that it really is a client and not a server error?

soxofaan commented 1 year ago

I don't have examples at the moment, it's been a while I was triggered to raise this issue.

I'm going to close this one. In our frameworks it's easy to define a generic client error anyway. I just wanted to poll if that idea could be interesting for the wider openEO ecosystem