Add support for non-json reponse result to http.send builtin. #1264

ashutosh-narkar · 2019-03-14T20:58:25Z

Signed-off-by: Ashutosh Narkar anarkar4387@gmail.com

agurney

Thanks again for jumping on this so quickly. Getting the raw_body is great; I just have a thought around the option for controlling whether decoding is attempted in cases where the Content-type isn't claimed to be JSON.

agurney · 2019-03-15T15:40:49Z

docs/book/language-reference.md

+
+| Built-in | Inputs | Description |
+| ------- |--------|-------------|
+| <span class="opa-keep-it-together">``http.send(request, output)``</span> | 3+ | ``http.send`` executes a HTTP request and returns the response.``request`` is an object containing keys ``method``, ``url`` and  optionally ``body``, ``enable_redirect``, ``disable_json_decode``, ``headers``, ``tls_use_system_certs``, ``tls_ca_cert_file``, ``tls_ca_cert_env_variable``, ``tls_client_cert_env_variable``, ``tls_client_key_env_variable`` or ``tls_client_cert_file``, ``tls_client_key_file`` . For example, ``http.send({"method": "get", "url": "http://www.openpolicyagent.org/", "headers": {"X-Foo":"bar", "X-Opa": "rules"}}, output)``. ``output`` is an object containing keys ``status``, ``status_code``, ``body`` and ``raw_body`` which represent the HTTP status, status code, JSON value from the response body and response body as string respectively. Sample output, ``{"status": "200 OK", "status_code": 200, "body": {"hello": "world"}, "raw_body": "{\"hello\": \"world\ "}"``}. By default, http redirects are not enabled. To enable, set ``enable_redirect`` to ``true``. Also ``disable_json_decode`` is set to ``true`` by default. This means if the HTTP server response does not specify the ``Content-type`` as ``application/json``, the response body will not be JSON decoded ie. output's ``body`` field will be ``nil``. To change this behaviour, set ``disable_json_decode`` to ``false``.|


I like the logic of always returning a raw_body, and if possible returning a JSON body - that helps with the common existing case of JSON responses, while allowing more straightforward code in other cases (compared to conditionally switching between having either a body or raw_body).

The disable_json_decode flag seems a bit hard to understand. I find disable-if-true to be confusing in general. If we rephrase this as force_json_decode, which is false by default, then we get the behavior:

Responses claiming to be application/json will always be JSON-decoded into body, if possible.

Other responses will not be so decoded, unless force_json_decode is set.

@agurney I tend to agree however the only downside is that change would be backwards incompatible (i.e., users that had integrated their policy with an endpoint that doesn't set Content-Type properly would be broken.)

That said, now (if any) is the time to make backwards incompatible changes.

agurney · 2019-03-15T15:40:54Z

topdown/http.go

+		return nil, err
+	}
+
+	if isContentTypeJSON(resp.Header) || (!isContentTypeJSON(resp.Header) && !disableJSONDecode) {


suggest if (isContentTypeJSON(resp.Header) || forceJSONDecode) { ... }

ashutosh-narkar · 2019-03-19T00:17:55Z

A new field force_json_decode has been added to the http.send command. The behavior is similar to what @agurney suggested.

tsandall

Left a couple minor comments. Also, please add a bullet to the Unreleased section in the change log so that we don't forget to include a release note about this breaking change.

tsandall · 2019-03-19T15:19:09Z

docs/book/language-reference.md

-| ------- |-------------|
-| <span class="opa-keep-it-together">``output := http.send(request)``</span> | ``http.send`` executes a HTTP request and returns the response.``request`` is an object containing keys ``method``, ``url`` and  optionally ``body``, ``enable_redirect``, ``headers``, ``tls_use_system_certs``, ``tls_ca_cert_file``, ``tls_ca_cert_env_variable``, ``tls_client_cert_env_variable``, ``tls_client_key_env_variable`` or ``tls_client_cert_file``, ``tls_client_key_file`` . For example, ``http.send({"method": "get", "url": "http://www.openpolicyagent.org/", "headers": {"X-Foo":"bar", "X-Opa": "rules"}}, output)``. ``output`` is an object containing keys ``status``, ``status_code`` and ``body`` which represent the HTTP status, status code and response body respectively. Sample output, ``{"status": "200 OK", "status_code": 200, "body": null``}. By default, http redirects are not enabled. To enable, set ``enable_redirect`` to ``true``.|
+
+| Built-in | Inputs | Description |


Nit: It looks like the merge re-added the Inputs column that we got rid of in one of the recent docs improvement PRs. Please scrap this column and fix the built-in call format to match the others.

tsandall · 2019-03-19T15:19:53Z

docs/book/language-reference.md

+
+| Built-in | Inputs | Description |
+| ------- |--------|-------------|
+| <span class="opa-keep-it-together">``http.send(request, output)``</span> | 3+ | ``http.send`` executes a HTTP request and returns the response.``request`` is an object containing keys ``method``, ``url`` and  optionally ``body``, ``enable_redirect``, ``force_json_decode``, ``headers``, ``tls_use_system_certs``, ``tls_ca_cert_file``, ``tls_ca_cert_env_variable``, ``tls_client_cert_env_variable``, ``tls_client_key_env_variable`` or ``tls_client_cert_file``, ``tls_client_key_file`` . For example, ``http.send({"method": "get", "url": "http://www.openpolicyagent.org/", "headers": {"X-Foo":"bar", "X-Opa": "rules"}}, output)``. ``output`` is an object containing keys ``status``, ``status_code``, ``body`` and ``raw_body`` which represent the HTTP status, status code, JSON value from the response body and response body as string respectively. Sample output, ``{"status": "200 OK", "status_code": 200, "body": {"hello": "world"}, "raw_body": "{\"hello\": \"world\ "}"``}. By default, http redirects are not enabled. To enable, set ``enable_redirect`` to ``true``. Also ``force_json_decode`` is set to ``false`` by default. This means if the HTTP server response does not specify the ``Content-type`` as ``application/json``, the response body will not be JSON decoded ie. output's ``body`` field will be ``nil``. To change this behaviour, set ``force_json_decode`` to ``true``.|


Nit: By default, [http->HTTP] redirects.

tsandall · 2019-03-19T15:21:11Z

topdown/http.go

+	}
+
+	if isContentTypeJSON(resp.Header) || forceJSONDecode {
+		json.NewDecoder(&buf).Decode(&resultBody)


What about errors?

Added a note.

tsandall · 2019-03-19T15:23:53Z

topdown/http.go


 	result := make(map[string]interface{})
 	result["status"] = resp.Status
 	result["status_code"] = resp.StatusCode
 	result["body"] = resultBody
+	result["raw_body"] = string(resultRawBody)


Out of curiosity, what happens if the raw body contains binary characters?

If the response body contains binary characters, the raw body will have them too.

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

ashutosh-narkar force-pushed the http-send-non-json branch from e722ebe to dbdb248 Compare March 14, 2019 21:29

agurney reviewed Mar 15, 2019

View reviewed changes

ashutosh-narkar force-pushed the http-send-non-json branch from dbdb248 to c6f12d5 Compare March 19, 2019 00:08

tsandall reviewed Mar 19, 2019

View reviewed changes

Add support for non-json reponse result to http.send builtin.

e3ad773

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

ashutosh-narkar force-pushed the http-send-non-json branch from b5f33bc to e3ad773 Compare March 19, 2019 21:34

ashutosh-narkar merged commit 880d73f into open-policy-agent:master Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for non-json reponse result to http.send builtin. #1264

Add support for non-json reponse result to http.send builtin. #1264

ashutosh-narkar commented Mar 14, 2019

agurney left a comment

agurney Mar 15, 2019

tsandall Mar 15, 2019

agurney Mar 15, 2019

ashutosh-narkar commented Mar 19, 2019

tsandall left a comment

tsandall Mar 19, 2019

ashutosh-narkar Mar 19, 2019

tsandall Mar 19, 2019

ashutosh-narkar Mar 19, 2019

tsandall Mar 19, 2019

ashutosh-narkar Mar 19, 2019

tsandall Mar 19, 2019

ashutosh-narkar Mar 19, 2019

Add support for non-json reponse result to http.send builtin. #1264

Add support for non-json reponse result to http.send builtin. #1264

Conversation

ashutosh-narkar commented Mar 14, 2019

agurney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Mar 19, 2019

tsandall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment