http.send fails to yield a response body if it is not JSON

[agurney]

Expected Behavior

The language reference describes http.send as simply returning the response body as a component of output. It sounds from the documentation that this body member ought to be a string, which could then be further processed with json.unmarshal if it's JSON, or otherwise if it's not.

Actual Behavior

If the HTTP server response is parsable as JSON, that appears in output.body as a term, i.e. unmarshaled from a string. If the HTTP server responds with a body that doesn't contain valid JSON, what we get is null. The output has nothing to indicate what happened to the body.

See

opa/topdown/http.go

Line 275 in 0d9cd4a

json.NewDecoder(resp.Body).Decode(&resultBody)

for logic that unconditionally tries to decode the string as JSON.

Steps to Reproduce the Problem

Invoke http.send against a url that will return a non-JSON response payload.

Additional Info

If nothing else, the documentation should describe the current state of affairs. This would make it easier to write Rego using http.send since it would be more obvious what can be done with the output, and which kinds of responses can be handled in general.

For speaking to non-JSON endpoints, one possibility is to add a raw_body member to output. This would preserve the existing behavior while also providing a more general capability for non-JSON data.

Another would be to fall back to body containing the raw response body if JSON parsing fails, or if the Content-type header doesn't indicate JSON. But it is probably confusing if body is sometimes parsed JSON and sometimes a raw string, and the Content-type is unfortunately not always reliable in the wild.

The idea in #1176 (comment) also seems interesting in the longer term, replacing the low-level http.send facility with a higher-level abstraction over remote data sources.

[tsandall]

@agurney thanks for filing this. The raw_body option feels like the right choice at this point.

I think we should do a few things...

1. Update the docs to reflect that body contains the (deserialized) JSON value from the response message body.

2. Extend the built-in function to return a raw_body field in the output if the response Content-Type does not specify application/json (note this should probably be a contains check to deal with variants like application/json;param=value and application/json+vendor.kind)

3. Extend the built-in function with an optional parameter to disable JSON decoding when the Content-Type is missing from the response (so that this can be used in tandem with clients expecting raw_body.)

I think we'll need to keep an eye on this as I'm not sure how well binary data will get handled.

[tsandall]

@ashutosh-narkar can you take a look at this?

[ashutosh-narkar]

Yes I will look into this.

[agurney]

Thanks for jumping on this so speedily! Taking a look at the PR now.