DELETE requests should return specified Content-Type #85

joeb1415 · 2019-05-03T08:55:27Z

DELETE requests are returning Content-Type: text/html, this is breaking some clients that expect application/json, even though there is no actual content, simply the type in the header broke a lookup. Looks like this was introduced here, as text/html is the Flask response default mimetype.

Rolling that back here and adding a test.

Sytten · 2019-05-03T14:18:27Z

Currently I am returning an empty string if the response is 204, can we write a test to make sure this behaviour still works. It was the only way to make it set the application/json. If the schema is none, we should probably not set the data in the returned response?

kazamatzuri · 2019-05-03T15:31:33Z

tests/test_rebar.py

@@ -248,6 +248,46 @@ def get_me():
        )
        self.assertEqual(resp.status_code, 400)

+    def test_validate_headers_delete(self):


if you add a docstring comment the testrunner output is going to improve in readability.
Not a blocking issue though

kazamatzuri

lgtm

kazamatzuri · 2019-05-03T15:32:56Z

oh well, I guess my review doesn't count;)

airstandley

I am not sure this is correct.
If no schema is defined, then we should not be assuming the structure of the response. It seems like it could be a legitimate use case to return a string that is "text/html" or any other content type.
This would be a breaking change for such a case.

The entire rebar system builds off the definition of explicit schemas for every potential response type.
I would question why the "bad" DELETE responses are not correctly defining a response schema, as that should address this.
Unless I am misunderstanding the issue?

Edit: I should clarify. I don't actually think returning anything other than a JSON from a API wrapped with rebar is a legitimate case we should support, but it is a case we currently support (to the best of my knowledge). I think it should be a very clear breaking change if we start enforcing JSON format returns rather than quietly attempting to dump them to a json.

joeb1415 · 2019-05-03T18:36:50Z

My thinking is that our response header should honor what our registry produces states. But what is happening now is we are letting the flask response default text/html get in there before we define what we intend for that header, so our intended application/json (or any provided produces value, is not used.

airstandley · 2019-05-03T19:25:50Z

My thinking is that our response header should honor what our registry produces states.

@joeb1415 That would make sense to me, but (as far as I can see) produces is only used by the swagger file generator; it is not part of the HandlerRegistry definition.
I'd be a fan of either:

Pulling the produces value from the swagger_generator
Adding the ability to define what content-type the HandlerRegistry produces via an init argument
Creating a V2 HandlerRegistry that enforces the production of a 'application/json' content type and defines a default schema to use when no schema is specified so we remove the ambiguous case of no/None schema.

I think that it should then be safe to either set the 'Content-Type' header based on what the registry 'produces', or at least error out when it does not match. That should solve what seems to be the root of your issue which is that we are relying on Flask's default mime type tools to set a header whose value our clients rely on being a certain value.

airstandley · 2019-05-03T19:48:54Z

@joeb1415 Looking into it a bit more, you could also modify your flask app to resolve this by changing the response_class for your application to one whose default_mimetype is "application/json".

joeb1415 · 2019-05-05T16:03:05Z

Is it not fair to standardize all responses as Content-Type JSON? This was the case prior to the commit I linked above, I feel like it's more of an accident that some return HTML now.

FWIW, v3 doesn't accept a produces at all, it just specifies application/json in all cases:
https://github.com/plangrid/flask-rebar/blob/master/flask_rebar/swagger_generation/swagger_generator_v3.py#L255

I'll take a quick look at adding an explicit control of this but I'm curious if it's actually a regression to change the current implementation back to JSON.

joeb1415 · 2019-05-05T16:15:36Z

I tried adding a produces attribute to the handler. LMK what folks think.

Sytten · 2019-05-05T16:34:32Z

I like it, it's clean. Maybe rebar could provide an enum or something similar with all the content types?

airstandley · 2019-05-06T16:34:04Z

Is it not fair to standardize all responses as Content-Type JSON? This was the case prior to the commit I linked above, I feel like it's more of an accident that some return HTML now.

Yes totally fair, but then I think we also should be standardizing on all responses defining a Schema. I just don't agree with the middle ground of not enforcing a schema but enforcing a content type.

barakalon · 2019-05-06T18:22:04Z

flask_rebar/rebar.py

@@ -102,6 +103,7 @@ def _wrap_handler(
    request_body_schema=None,
    headers_schema=None,
    marshal_schema=None,
+    produces=None,


I'm not a fan of this interface.

Its conflated with "produces" from OpenAPI 2.0.

Also, this doesn't actually guarantee that the handler actually returns the content-type specified here. It just patches the headers.

So maybe this should be a "headers" parameter, with extra headers to set on the response? That seems more flexible and more clear.

barakalon · 2019-05-06T18:22:26Z

tests/test_rebar.py

+        def delete_me():
+            return None, 204
+
+        # Test DELETE with non-None return schema


Can we make this a separate test method?

barakalon · 2019-05-06T18:23:44Z

tests/test_rebar.py

+        self.assertEqual(resp.status_code, 204)
+        self.assertEqual(resp.data.decode("utf-8"), "")
+        self.assertEqual(
+            next(header[1] for header in resp.headers.to_list() if header[0] == "Content-Type"),


I feel like it should be possible to do resp.headers['Content-Type'], or something similar?

barakalon · 2019-05-06T18:34:31Z

Chiming in a little late here:

There should be no "content-type" header when there is no content. However, its customary to add this header regardless to support clients that incorrectly attempt to parse this header.

Given that flask-rebar generally returns JSON, it seems totally reasonable to just make it default to "application/json" when there is no content.

With that - I think this is more reason to add actual content negotiation.

jab · 2019-05-06T22:09:36Z

tests/test_rebar.py

+        self.assertEqual(resp.status_code, 204)
+        self.assertEqual(resp.data.decode("utf-8"), "")
+        self.assertEqual(
+            next(header[1] for header in resp.headers.to_list() if header[0] == "Content-Type"),


As above. e.g. https://werkzeug.palletsprojects.com/en/0.14.x/quickstart/ includes the example

>>> response.headers['content-type'] 'text/plain; charset=utf-8'

joeb1415 · 2019-05-06T22:28:23Z

Looks like werkzeug will always pass a Content-Type header. I agree it makes sense to not specify a content type when there is no content, and we could take care of removing it on our end when the response schema is None, but perhaps this suggests that it is a standard header to always include.
https://github.com/pallets/werkzeug/blob/0.15.2/src/werkzeug/wrappers/base_response.py#L189-L191

Good thoughts on the more flexible headers param, I'll try that out.

barakalon · 2019-05-06T23:21:48Z

@joeb1415 FWIW, its also possible to set the headers with the return value: https://github.com/plangrid/flask-rebar/blob/master/flask_rebar/rebar.py#L66

joeb1415 · 2019-05-07T20:38:56Z

@barakalon mind taking another look to let me know if you still have changes or if we're ✅ ?

RookieRick · 2019-05-08T03:30:32Z

Gonna go ahead and merge this one and push out a patch version to close out incident - Opening a new issue #91 to hash out some of the finer details of content-type negotiation

barakalon · 2019-05-08T18:11:41Z

flask_rebar/request_utils.py

@@ -59,8 +65,17 @@ def response(data, status_code=200, headers=None):
    :rtype: flask.Response
    """
    resp = jsonify(data)
+
+    if not get_json_from_resp(resp=resp):


This seems like a lot of unnecessary processing (i.e. we're loading all responses into memory, even potentially very large ones). It also is strange behavior, in that it seems to be catering to another part of our code that isn't clear from just the context of this function.

What's the reasoning here?

Also, what if a developer actually wants to return an empty object? This will lead to very confusing behavior - it will replace their empty object with an empty string.

Or possibly even more possible, what if a developer wants to return an empty list?

Again, matching existing behavior:
https://github.com/plangrid/flask-rebar/blob/master/tests/test_rebar.py#L319

Would make sense to change that, but that would be a change from what we had previously.

Could save the processing here by just checking for other various almost-falsey cases. b"" or b"{}" don't evaluate to false, but do when decoded.

barakalon · 2019-05-08T18:14:18Z

flask_rebar/rebar.py


        schema = marshal_schema[status_code]  # May raise KeyError.

        # The schema may be declared as None to bypass marshaling (e.g. for 204 responses).
        if schema is None:
-            return make_response((data or "", status_code, headers))
+            return response(data=data or {}, status_code=status_code, headers=headers)


I see that we later replace {} with an empty string... but I'm not sure why?

An empty object is not empty content. I.e. if I have a marshal schema of {204: None}, this looks like it adds '{}' to the response content. Which wouldn't be right.

I replaced with an empty string to match the existing unit test:
https://github.com/plangrid/flask-rebar/blob/master/tests/test_rebar.py#L319

barakalon · 2019-05-08T18:17:06Z

Sorry for the delay @joeb1415

I think this introduced some subtle bugs in the way it handles "falsey" responses.

joeb1415 · 2019-05-08T18:54:21Z

@barakalon no worries! I think these changes were consistent, but let me know if you think otherwise. Also if we want to actually change what we return in the None schema case, I'm open to that too.

barakalon · 2019-05-08T21:05:53Z

@joeb1415 sorry, I'm going to push back again

By moving the logic into the underlying response function, which is used to marshal all responses, even when the schema is None, I think we've introduced a bug. And I think that's just not tested yet.

joeb1415 · 2019-05-08T21:33:14Z

@barakalon hmm, what do you make of the test I linked to there? We had already been returning an empty string when None response schema is specified (that test is unchanged). I'm not sure then in what case this could be a regression?

barakalon · 2019-05-08T21:35:32Z

Try adding a test where the schema is not None, but an object with no required fields, or a schema that is a list at the root level (i.e. initialize it with many=True), and then return {} or [] (respectively).

I would expect the response body to be {} or [], not empty.

joeb1415 · 2019-05-09T01:01:00Z

Gotcha. I added this test here, give it a check!
#93

joeb1415 added 2 commits May 3, 2019 01:47

DELETE requests should return specified Content-Type

98deb52

unused import

8fc5f48

joeb1415 requested a review from a team as a code owner May 3, 2019 08:55

ghost assigned airstandley May 3, 2019

joeb1415 added 2 commits May 3, 2019 08:01

Add test for non-None DELETE return schema

33568e4

Specify full return schema

b288060

kazamatzuri reviewed May 3, 2019

View reviewed changes

kazamatzuri approved these changes May 3, 2019

View reviewed changes

airstandley requested changes May 3, 2019

View reviewed changes

joeb1415 added 2 commits May 5, 2019 09:11

Add produces attribute to handler to control response Content-Type

fb7d7a0

Merge branch 'master' into DELETE-response-content-type

38ead9c

airstandley approved these changes May 6, 2019

View reviewed changes

Run black pre-commit formatting

dbf970c

barakalon suggested changes May 6, 2019

View reviewed changes

jab reviewed May 6, 2019

View reviewed changes

PR comments: Pass instead of to add handler

6796158

Ensure we return empty string for empty data

abe1e2b

RookieRick merged commit 8d1bca2 into master May 8, 2019

barakalon reviewed May 8, 2019

View reviewed changes

joeb1415 deleted the DELETE-response-content-type branch May 8, 2019 18:54

RookieRick mentioned this pull request May 9, 2019

Return empty object not empty string, if an empty non-null object response is specified #93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DELETE requests should return specified Content-Type #85

DELETE requests should return specified Content-Type #85

joeb1415 commented May 3, 2019

Sytten commented May 3, 2019

kazamatzuri May 3, 2019

kazamatzuri left a comment

kazamatzuri commented May 3, 2019

airstandley left a comment •

edited

joeb1415 commented May 3, 2019

airstandley commented May 3, 2019

airstandley commented May 3, 2019

joeb1415 commented May 5, 2019

joeb1415 commented May 5, 2019

Sytten commented May 5, 2019

airstandley commented May 6, 2019

barakalon May 6, 2019

barakalon May 6, 2019

barakalon May 6, 2019

barakalon commented May 6, 2019

jab May 6, 2019

joeb1415 commented May 6, 2019

barakalon commented May 6, 2019

joeb1415 commented May 7, 2019

RookieRick commented May 8, 2019

barakalon May 8, 2019

barakalon May 8, 2019

barakalon May 8, 2019

joeb1415 May 8, 2019

barakalon May 8, 2019

joeb1415 May 8, 2019

barakalon commented May 8, 2019

joeb1415 commented May 8, 2019

barakalon commented May 8, 2019

joeb1415 commented May 8, 2019

barakalon commented May 8, 2019

joeb1415 commented May 9, 2019

DELETE requests should return specified Content-Type #85

DELETE requests should return specified Content-Type #85

Conversation

joeb1415 commented May 3, 2019

Sytten commented May 3, 2019

Choose a reason for hiding this comment

kazamatzuri left a comment

Choose a reason for hiding this comment

kazamatzuri commented May 3, 2019

airstandley left a comment • edited

Choose a reason for hiding this comment

joeb1415 commented May 3, 2019

airstandley commented May 3, 2019

airstandley commented May 3, 2019

joeb1415 commented May 5, 2019

joeb1415 commented May 5, 2019

Sytten commented May 5, 2019

airstandley commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barakalon commented May 6, 2019

Choose a reason for hiding this comment

joeb1415 commented May 6, 2019

barakalon commented May 6, 2019

joeb1415 commented May 7, 2019

RookieRick commented May 8, 2019

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

barakalon commented May 8, 2019

joeb1415 commented May 8, 2019

barakalon commented May 8, 2019

joeb1415 commented May 8, 2019

barakalon commented May 8, 2019

joeb1415 commented May 9, 2019

airstandley left a comment •

edited