Default output and error behavior for call:http,openapi,grpc

[matthias-pichler]

Currently the default output behavior for the call: http task is content which returns the content (i.e. body) of the response. This is very convenient since authors most likely want the response data. However it is very unintuitive when the API returns an error. Since jq will happily return null if non existing properties are accessed subsequent tasks might seem to work or the workflow might fail many steps downstream. Should we update the ergonomics around error handling?

1. Should call: http raise an error if a not-ok (outside of [200, 399]) status is reported?
2. Should the default output type be changed to response to report the whole response?
3. should we allow to specify an expected status (range)?

[cdavernas]

1. As far as I understand it, a communication error should be raised for non-success status code, with the detail being the body of the response, and the status being the response's status code. At least, it's what we're doing in Synapse.
2. My take on it is no, we shouldn't operate such transformation implicitly IMHO
3. Why not, good idea!

On a side note, this discussion also applies to OpenAPI and GRPC afaik.

[JBBianchi]

I agree with 1 & 2.
For 3, I'm not sure I understand the scope. I think it's valid at the try..catch level (I guess using when/exceptWhen), but I don't think it should be parameter of call. (A trickier one at the call level are the redirects, should we always follow them or enable a way to throw)

[matthias-pichler]

for 3) it wouldn't be at call level but inside with like

document: {}
do:
  - apiCall:
      call: http
      with:
        method: GET
        headers: {}
        endpoint: https://example.com
        expectedStatus: 200-204, 304, 307 # or something like it

[matthias-pichler]

@cdavernas

My take on it is no, we shouldn't operate such transformation implicitly IMHO

I think you misunderstood. I meant that we just change output to default to response instead of content here: https://github.com/serverlessworkflow/specification/blob/main/dsl-reference.md#http-call

I don't mean to implicitly change it at runtime when an error occurs

[matthias-pichler]

As far as I understand it, a communication error should be raised for non-success status code, with the detail being the body of the response, and the status being the response's status code. At least, it's what we're doing in Synapse.

That is ok with me 👍 we don't have to do all things listed in my initial comment ... some of the I see as being mutually exclusive anyways.

But I think having call: http raise a communication error for any status outside of 200-399 is ok and already solves the issue. It's just not documented unter call: http

In the future we could think about options like expectedStatus or followRedirects but they are not immediately necessary

[cdavernas]

for 3) it wouldn't be at call level but inside with like

I think it's an overkill personally, as success status codes are always the same: 200 >= statusCode < 300. If you want to make specific status-based checks, then you should use response mode and a switch or whatever.

I think you misunderstood. I meant that we just change output to default to response instead of content here:

To ensure consistency with all other features, content should be the default IMHO.

[matthias-pichler]

ok if we just document the error raised then it's fine 👍

200 >= statusCode < 300

should redirects really trigger an error?

[cdavernas]

should redirects really trigger an error?

No, indeed, sorry about that typo! It should instead be: 200 >= statusCode < 400

[cdavernas]

In .NET, however, only status code in 2xx are considered successfull. ChatGPT says the same.

So maybe, instead of deciding a redirect is not an error (which is wrong in many cases), we should, as you proposed above, include an additional followRedirects property or whatever, to determine whether success status codes are 2xx or 2xx AND 3xx. WDYT?

[matthias-pichler]

Yeah wether 3xx is an error or not differs between reality and specification ... in the HTTP spec there are

Informational responses (100 – 199)
Successful responses (200 – 299)
Redirection messages (300 – 399)
Client error responses (400 – 499)
Server error responses (500 – 599)

so explicitly not errors. In reality they often trigger errors in server side languages/libraries. Axios being an example from JS world where 3xx is also an error

we could also defer this decision and wait for actual user feedback 🤷 . Maybe redirects aren't as common after all

[cdavernas]

we could also defer this decision and wait for actual user feedback

I think we should follow standards and common implementations, and therefore not assume that redirections are ok/acceptable, which in many cases they might not be. As pointed out by you, assuming they are errors doesn't seem acceptable in many other cases.

As a matter of consequence, I second your (great) initial idea of adding a new property to decide whether or not the call expects/authorizes redirections.

[cdavernas]

But I think having call: http raise a communication error for any status outside of 200-399 is ok and already solves the issue. It's just not documented unter call: http

That's possible indeed. Let's document it properly under both HTTP and OpenAPI!

[hirenr]

Quick Qn: What would be the output object incase, it was a request error and hence no response object should exist.
I am thinking on the lines of axios error handling suggestion!! https://axios-http.com/docs/handling_errors

[cdavernas]

Good question!

Actually, no response object is expected to be returned by the HTTP response. It's the runtime that must create the error object when the request returns a response with a non-success status code. If the body of said response has content, it should be displayed as detail of the error. If the response is empty, then the error's detail property would be empty, too.