Review Accept* headers handling in acceptparse.py

[whiteroses]

There are various bugs and issues around Accept* headers handling and conformance with specs, and it was agreed in #256 that the module would benefit from a review. This issue is intended for discussing questions that apply to all the headers or to the module as a whole, and for keeping track of related issues and pull requests. (Separate issues will be opened to focus on each Accept* header as required.)

In #256, we agreed that it may be a good idea to implement the handling of the four headers as their own separate classes, rather than inherit from the same Accept class. Continuing the discussion there:

I don't think we should continue to inherit from the same base class. Let's check and see what tests exist...

test_acceptparse.py would of course need to be updated, but there are quite a few tests in test_request.py that involve acceptparse.py too:

• test_language_parsing1
• test_language_parsing2
• test_language_parsing3
• test_mime_parsing1
• test_mime_parsing2
• test_mime_parsing3
• test_accept_best_match
• test_from_mimeparse
• test_request_put

simpleapp, used in quite a few of the tests, would also need to be updated.

If I understand them correctly, I think these tests are intended to be integration or functional? But they seem to test quite a few things that could be tested in unit tests — where we wouldn't need to create a request or a simpleapp (and test whether something is in the output of simpleapp) for every test. There also seems to be many tests and asserts in each test method. Of course some integration tests are necessary, but I was wondering if we need all of those tests, or whether some of them are already covered, or should be covered, by unit tests in acceptparse.py?

I don't see any major issues for the tests from separating out the handling for the four headers, though of course it would take some work to update them. Quite a few tests use request.accept.best_match() though, which we agreed in #256 to deprecate, so it may require a bit more thought as to what those tests should be testing instead.

...and see if some quick Google searches bring up any code that expects it to inherit from the same base class, but I think separating it out is a good idea.

I've looked at Werkzeug's implementation, and they also inherit from an Accept class. But there are quite a few open issues and pull requests regarding their Accept* headers handling, and I get the feeling that their implementation hasn't been given much attention for a long while. And I don't think Django implements anything to help with Accept* handling. Is there any other code you had in mind that we could look at @bertjwregeer?

[whiteroses]

The MasterClass class attribute is only set for NilAccept. This means that NoAccept, which inherit from NilAccept, also has the MasterClass of Accept, so:

>>> webob.request.Request.blank("/").accept_charset.MasterClass
<class 'webob.acceptparse.Accept'>
>>> webob.request.Request.blank("/").accept_encoding.MasterClass
<class 'webob.acceptparse.Accept'>
>>> webob.request.Request.blank("/").accept_language.MasterClass
<class 'webob.acceptparse.Accept'>

and all the methods for NoAccept that rely on MasterClass (__repr__, __add__ and __radd__) are not working correctly.

This suggests that there should be a NilClass for each Accept* class, rather than AcceptCharset and AcceptLanguage sharing a NilClass.

@bertjwregeer, @mmerickel, @stevepiercy: When a header is invalid, should it be treated as if the header is empty/not present in the request, in which case it would be a NilClass (NilAccept/NoAccept/MIMENilAccept) instance; or does a NilClass only represent when the header is empty or not present in the request?

[whiteroses]

The MasterClass class attribute is only set for NilAccept. This means that NoAccept, which inherit from NilAccept, also has the MasterClass of Accept, so:

>>> webob.request.Request.blank("/").accept_charset.MasterClass
<class 'webob.acceptparse.Accept'>
>>> webob.request.Request.blank("/").accept_encoding.MasterClass
<class 'webob.acceptparse.Accept'>
>>> webob.request.Request.blank("/").accept_language.MasterClass
<class 'webob.acceptparse.Accept'>

and all the methods for NoAccept that rely on MasterClass (__repr__, __add__ and __radd__) are not working correctly.

This suggests that there should be a NilClass for each Accept* class, rather than AcceptCharset and AcceptLanguage sharing a NilClass.

@bertjwregeer, @mmerickel, @stevepiercy: When a header is invalid, should it be treated as if the header is empty/not present in the request, in which case it would be a NilClass (NilAccept/NoAccept/MIMENilAccept) instance; or does a NilClass only represent when the header is empty or not present in the request?

[bertjwregeer]

And I don't think Django implements anything to help with Accept* handling. Is there any other code you had in mind that we could look at @bertjwregeer?

I was just thinking about a quick Google search to see if anyone is relying on the current internal implementation details of how WebOb does Accept handling. best_match for example may be something we have to continue supporting because it is widely used, if not, then we could potentially add a deprecation period on it.

Update:
I know we use best_match in Pyramid, and a quick glance it seems it is heavily used within OpenStack too, so we will want to continue supporting it in it's current form to not break anything, but we can update the algorithm and matching if necessary, as well as add additional ones that may make more sense.

---

If a header is invalid, it should return false for any "contains" information, so that would be a NoAccept.

NilAccept only makes sense in the case of Accept-Encoding where you don't want to send back gzipped data if the remote party doesn't support it.

For all the other Accept-* headers if it is not included, the server is allowed to reply with any content it deems fulfils the clients request.

[bertjwregeer]

And I don't think Django implements anything to help with Accept* handling. Is there any other code you had in mind that we could look at @bertjwregeer?

I was just thinking about a quick Google search to see if anyone is relying on the current internal implementation details of how WebOb does Accept handling. best_match for example may be something we have to continue supporting because it is widely used, if not, then we could potentially add a deprecation period on it.

Update:
I know we use best_match in Pyramid, and a quick glance it seems it is heavily used within OpenStack too, so we will want to continue supporting it in it's current form to not break anything, but we can update the algorithm and matching if necessary, as well as add additional ones that may make more sense.

---

If a header is invalid, it should return false for any "contains" information, so that would be a NoAccept.

NilAccept only makes sense in the case of Accept-Encoding where you don't want to send back gzipped data if the remote party doesn't support it.

For all the other Accept-* headers if it is not included, the server is allowed to reply with any content it deems fulfils the clients request.

[whiteroses]

One idea was to get .best_match() to call the new .lookup() (one of the new matching schemes) — but wouldn't that change in algorithm and matching be a breaking change?

---

I drew up a table to summarise what I could gather from RFC 7231 on expected behaviour for the four headers:

	Accept	Accept-Charset	Accept-Encoding	Accept-Language
Header not in request	User agent will accept any media type in response	User agent will accept any charset in response	User agent has no preferences regarding content-codings	User agent will accept any language in response
Header is empty	Can be empty (#()). No guidance on what to do if empty, so left to assume that we are to treat as None of available representations listed as acceptable in header	Invalid if empty (1#())	Can be empty (#()). User agent does not want any content-coding in response	Invalid if empty (1#())
None of available representations listed as acceptable in header	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation	Send a response without any content-coding	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation

accept_property's fget() does not distinguish between when the header is not in the request (req.environ.get(key) is None) and when the header is empty (req.environ.get(key) == ''):

    def fget(req):
        value = req.environ.get(key)
        if not value:
            return NilClass()
        return AcceptClass(value)

So if __contains__ is used to test for acceptability, then it needs to have different behaviour for when the header is not in the request, and when the header is empty. For example, for Accept-Encoding: when the header is not in the request, all content-codings are acceptable (__contains__ should return True); when the header is empty, the user agent does not want any content-coding (__contains__ should return False). So with the current design, no single NilClass can handle this.

I cannot find anything in the RFC on what should happen when a header is invalid. But from what I understand,

NilAccept only makes sense in the case of Accept-Encoding where you don't want to send back gzipped data if the remote party doesn't support it.

NilAccept.__contains__ always returns True, so if we don't want to use a content-coding unless the header says it's acceptable, then wouldn't we want __contains__ to always return False for an invalid header?

And

If a header is invalid, it should return false for any "contains" information, so that would be a NoAccept.

As we can see from the table, for the other three headers, the RFC says that any representation is acceptable if the header is not in the request; and if header is in the request but none of the available representations are listed as acceptable in the header, then disregard header field (which in turn means that any representation is acceptable) or send HTTP 406 (which is not usually done and generally not recommended). So it seems to me that we would want to do the same for an invalid header (consider any representation acceptable, i.e. the test for acceptability/__contains__ should always return True; or, respond with HTTP 406).

This means we can treat an invalid header like the "None of available representations listed as acceptable in header" row in the table. This is somewhat in line with how the properties are currently set up in request.py, except the 406 response is not an option (on which more later):

accept = accept_property('Accept', '14.1', MIMEAccept, MIMENilAccept)
accept_charset = accept_property('Accept-Charset', '14.2', AcceptCharset)
accept_encoding = accept_property('Accept-Encoding', '14.3',
                                  AcceptClass=AcceptEncoding,
                                  NilClass=NoAccept)
accept_language = accept_property('Accept-Language', '14.4', AcceptLanguage)

But we can't know whether a header is valid until the header is parsed, and that parsing happens in the Accept* classes; the decision as to whether the header gets an Accept* class or a NilClass happens before that, in accept_property's fget(), as we saw earlier.

As we can see from the table, the expected behaviour for the three rows are different. The current implementation handles the first two rows with a NilClass, but does not distinguish between the two. And it currently has no concept of an invalid header; if my reasoning is correct, an invalid header should be handled in the same way as the third row, the handling of which is in the Accept* class. So an invalid header would be an Accept* class; and an empty header or a header that is not in the request would be a NilClass — except we would need some way to distinguish between the two, and pass that information to the NilClass so that it can handle the two differently. (This at least means we would not have to worry about parsing before choosing between Accept* class and NilClass.)

But that leaves me with the question: is there a reason I may not be aware of why the current implementation uses Accept* classes and NilClasses, rather than just one class for each header? At the moment I can only see one benefit of less branching; but as mentioned above, we have more than two possibilities, so there will be branching in each class anyway. It seems to me that one class for each header would be a lot easier to understand and work with.

I mentioned that I was going to come back to the 406 response: I think we should support it as an option, with a method parameter. But we don't get method parameters with __contains__. I mentioned in the Accept-Language review issue my concerns with using __contains__; I understand that we likely need to keep it working the same for backward compatibility, but I'd like to propose that we offer a method to test for acceptability (with Accept-Language, we already know we will have the methods for the matching schemes), that also allows the raising of a 406 response (e.g. through a raise_406_not_acceptable parameter) instead of just disregarding the header.

If it's okay, I can try to implement these proposed changes and let you see how they look before deciding?

[whiteroses]

One idea was to get .best_match() to call the new .lookup() (one of the new matching schemes) — but wouldn't that change in algorithm and matching be a breaking change?

---

I drew up a table to summarise what I could gather from RFC 7231 on expected behaviour for the four headers:

	Accept	Accept-Charset	Accept-Encoding	Accept-Language
Header not in request	User agent will accept any media type in response	User agent will accept any charset in response	User agent has no preferences regarding content-codings	User agent will accept any language in response
Header is empty	Can be empty (#()). No guidance on what to do if empty, so left to assume that we are to treat as None of available representations listed as acceptable in header	Invalid if empty (1#())	Can be empty (#()). User agent does not want any content-coding in response	Invalid if empty (1#())
None of available representations listed as acceptable in header	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation	Send a response without any content-coding	Send 406 Not Acceptable, or disregard header field by treating response as if it is not subject to content negotiation

accept_property's fget() does not distinguish between when the header is not in the request (req.environ.get(key) is None) and when the header is empty (req.environ.get(key) == ''):

    def fget(req):
        value = req.environ.get(key)
        if not value:
            return NilClass()
        return AcceptClass(value)

So if __contains__ is used to test for acceptability, then it needs to have different behaviour for when the header is not in the request, and when the header is empty. For example, for Accept-Encoding: when the header is not in the request, all content-codings are acceptable (__contains__ should return True); when the header is empty, the user agent does not want any content-coding (__contains__ should return False). So with the current design, no single NilClass can handle this.

I cannot find anything in the RFC on what should happen when a header is invalid. But from what I understand,

NilAccept only makes sense in the case of Accept-Encoding where you don't want to send back gzipped data if the remote party doesn't support it.

NilAccept.__contains__ always returns True, so if we don't want to use a content-coding unless the header says it's acceptable, then wouldn't we want __contains__ to always return False for an invalid header?

And

If a header is invalid, it should return false for any "contains" information, so that would be a NoAccept.

As we can see from the table, for the other three headers, the RFC says that any representation is acceptable if the header is not in the request; and if header is in the request but none of the available representations are listed as acceptable in the header, then disregard header field (which in turn means that any representation is acceptable) or send HTTP 406 (which is not usually done and generally not recommended). So it seems to me that we would want to do the same for an invalid header (consider any representation acceptable, i.e. the test for acceptability/__contains__ should always return True; or, respond with HTTP 406).

This means we can treat an invalid header like the "None of available representations listed as acceptable in header" row in the table. This is somewhat in line with how the properties are currently set up in request.py, except the 406 response is not an option (on which more later):

accept = accept_property('Accept', '14.1', MIMEAccept, MIMENilAccept)
accept_charset = accept_property('Accept-Charset', '14.2', AcceptCharset)
accept_encoding = accept_property('Accept-Encoding', '14.3',
                                  AcceptClass=AcceptEncoding,
                                  NilClass=NoAccept)
accept_language = accept_property('Accept-Language', '14.4', AcceptLanguage)

But we can't know whether a header is valid until the header is parsed, and that parsing happens in the Accept* classes; the decision as to whether the header gets an Accept* class or a NilClass happens before that, in accept_property's fget(), as we saw earlier.

As we can see from the table, the expected behaviour for the three rows are different. The current implementation handles the first two rows with a NilClass, but does not distinguish between the two. And it currently has no concept of an invalid header; if my reasoning is correct, an invalid header should be handled in the same way as the third row, the handling of which is in the Accept* class. So an invalid header would be an Accept* class; and an empty header or a header that is not in the request would be a NilClass — except we would need some way to distinguish between the two, and pass that information to the NilClass so that it can handle the two differently. (This at least means we would not have to worry about parsing before choosing between Accept* class and NilClass.)

But that leaves me with the question: is there a reason I may not be aware of why the current implementation uses Accept* classes and NilClasses, rather than just one class for each header? At the moment I can only see one benefit of less branching; but as mentioned above, we have more than two possibilities, so there will be branching in each class anyway. It seems to me that one class for each header would be a lot easier to understand and work with.

I mentioned that I was going to come back to the 406 response: I think we should support it as an option, with a method parameter. But we don't get method parameters with __contains__. I mentioned in the Accept-Language review issue my concerns with using __contains__; I understand that we likely need to keep it working the same for backward compatibility, but I'd like to propose that we offer a method to test for acceptability (with Accept-Language, we already know we will have the methods for the matching schemes), that also allows the raising of a 406 response (e.g. through a raise_406_not_acceptable parameter) instead of just disregarding the header.

If it's okay, I can try to implement these proposed changes and let you see how they look before deciding?

[whiteroses]

I said in the previous comment that

So an invalid header would be an Accept* class; and an empty header or a header that is not in the request would be a NilClass — except we would need some way to distinguish between the two, and pass that information to the NilClass so that it can handle the two differently. (This at least means we would not have to worry about parsing before choosing between Accept* class and NilClass.)

This is wrong: I forgot that for Accept-Charset and Accept-Language, the header is also invalid if it is empty, as shown in the second row in the table. So if we stay with the current design of splitting between the Accept* class and the NilClass, then for two of the classes, an empty header would be an Accept* class, and for the other two, an empty header would be a NilClass. We would need to check in accept_property's fget() whether the header is Accept-Charset or Accept-Language before we can know whether to create an Accept* class or a NilClass. It really seems to me that the NilClasses no longer make sense.

[whiteroses]

I said in the previous comment that

So an invalid header would be an Accept* class; and an empty header or a header that is not in the request would be a NilClass — except we would need some way to distinguish between the two, and pass that information to the NilClass so that it can handle the two differently. (This at least means we would not have to worry about parsing before choosing between Accept* class and NilClass.)

This is wrong: I forgot that for Accept-Charset and Accept-Language, the header is also invalid if it is empty, as shown in the second row in the table. So if we stay with the current design of splitting between the Accept* class and the NilClass, then for two of the classes, an empty header would be an Accept* class, and for the other two, an empty header would be a NilClass. We would need to check in accept_property's fget() whether the header is Accept-Charset or Accept-Language before we can know whether to create an Accept* class or a NilClass. It really seems to me that the NilClasses no longer make sense.

[whiteroses]

Ok, the NilClasses don't make sense because they are responsible for the cases where the header is not in the request, where the header is empty, and where the header is invalid. But a class just for the case where the header is not in the request makes sense I think: see the first row of the table, where the meaning is pretty much the same across the four headers. (Edit: this is essentially NilAccept, although given that empty headers would no longer be NilAccept, and each header would need their own subclass for their header-specific methods, we could change the name to something like NoHeader, which I think would be more self-explanatory?) We can inherit from that class to get a subclass for each header (say, AcceptNoHeader, AcceptCharsetNoHeader, AcceptEncodingNoHeader, and AcceptLanguageNoHeader), and implement header-specific methods for the case where there is no header in the request. The Accept* classes can handle the cases where the header is empty (whether that is valid or not), where the header is invalid, and where the header is valid and not empty. That would make sense, I think, and none of the decision making and complexity (other than checking whether a header is None) would need to go into accept_property's fget().

[whiteroses]

Ok, the NilClasses don't make sense because they are responsible for the cases where the header is not in the request, where the header is empty, and where the header is invalid. But a class just for the case where the header is not in the request makes sense I think: see the first row of the table, where the meaning is pretty much the same across the four headers. (Edit: this is essentially NilAccept, although given that empty headers would no longer be NilAccept, and each header would need their own subclass for their header-specific methods, we could change the name to something like NoHeader, which I think would be more self-explanatory?) We can inherit from that class to get a subclass for each header (say, AcceptNoHeader, AcceptCharsetNoHeader, AcceptEncodingNoHeader, and AcceptLanguageNoHeader), and implement header-specific methods for the case where there is no header in the request. The Accept* classes can handle the cases where the header is empty (whether that is valid or not), where the header is invalid, and where the header is valid and not empty. That would make sense, I think, and none of the decision making and complexity (other than checking whether a header is None) would need to go into accept_property's fget().

[mmerickel]

There's a lot to sort through here. The hangup seems to be how to handle the corner cases in the table above. I think your analysis is spot on. We can likely handle a MissingAccept class separately from the individual Accept* classes which can deal with the spec for each header individually. They may want to share a common parser and a few other things but making them separate sounds good. As far as the 406 issues, I think it'd be nice to have a separate API from __contains__ that more explicitly can perform the match and possibly return None or raise if nothing can be matched. However obviously we need to continue supporting __contains__ for compatibility and keep the current semantics which appear to be correct.

I highly doubt we need to worry about any compatibility concerns with regard to any aspect of the accept class hierarchy including MasterClass, NilAccept, etc. We can form something that makes sense and worry about the fallout of that afterward.

If we go with this, do you have any concerns left that I missed?

[mmerickel]

There's a lot to sort through here. The hangup seems to be how to handle the corner cases in the table above. I think your analysis is spot on. We can likely handle a MissingAccept class separately from the individual Accept* classes which can deal with the spec for each header individually. They may want to share a common parser and a few other things but making them separate sounds good. As far as the 406 issues, I think it'd be nice to have a separate API from __contains__ that more explicitly can perform the match and possibly return None or raise if nothing can be matched. However obviously we need to continue supporting __contains__ for compatibility and keep the current semantics which appear to be correct.

I highly doubt we need to worry about any compatibility concerns with regard to any aspect of the accept class hierarchy including MasterClass, NilAccept, etc. We can form something that makes sense and worry about the fallout of that afterward.

If we go with this, do you have any concerns left that I missed?

[whiteroses]

Should we call the class MissingAccept? I was going to suggest something like NoHeader (so AcceptLanguageNoHeader etc.), as the Accept* headers are not in the request by default, so they're not really missing as such?

Thanks for reviewing! I'm continuing the discussion on __contains__ in the issue for Accept-Language, as you can see. A couple of other things brought up in this thread are:

bertjwregeer said,

I know we use best_match in Pyramid, and a quick glance it seems it is heavily used within OpenStack too, so we will want to continue supporting it in it's current form to not break anything, but we can update the algorithm and matching if necessary, as well as add additional ones that may make more sense.

to which I replied and asked,

One idea was to get .best_match() to call the new .lookup() (one of the new matching schemes) — but wouldn't that change in algorithm and matching be a breaking change?

And:

simpleapp [in test_request.py], used in quite a few of the tests, would also need to be updated.

If I understand them correctly, I think these tests are intended to be integration or functional? But they seem to test quite a few things that could be tested in unit tests — where we wouldn't need to create a request or a simpleapp (and test whether something is in the output of simpleapp) for every test. There also seems to be many tests and asserts in each test method. Of course some integration tests are necessary, but I was wondering if we need all of those tests, or whether some of them are already covered, or should be covered, by unit tests in acceptparse.py?

Thanks!

[whiteroses]

Should we call the class MissingAccept? I was going to suggest something like NoHeader (so AcceptLanguageNoHeader etc.), as the Accept* headers are not in the request by default, so they're not really missing as such?

Thanks for reviewing! I'm continuing the discussion on __contains__ in the issue for Accept-Language, as you can see. A couple of other things brought up in this thread are:

bertjwregeer said,

I know we use best_match in Pyramid, and a quick glance it seems it is heavily used within OpenStack too, so we will want to continue supporting it in it's current form to not break anything, but we can update the algorithm and matching if necessary, as well as add additional ones that may make more sense.

to which I replied and asked,

One idea was to get .best_match() to call the new .lookup() (one of the new matching schemes) — but wouldn't that change in algorithm and matching be a breaking change?

And:

simpleapp [in test_request.py], used in quite a few of the tests, would also need to be updated.

If I understand them correctly, I think these tests are intended to be integration or functional? But they seem to test quite a few things that could be tested in unit tests — where we wouldn't need to create a request or a simpleapp (and test whether something is in the output of simpleapp) for every test. There also seems to be many tests and asserts in each test method. Of course some integration tests are necessary, but I was wondering if we need all of those tests, or whether some of them are already covered, or should be covered, by unit tests in acceptparse.py?

Thanks!

[whiteroses]

I now think invalid headers should be treated like the first row in the table, rather than the third. So AcceptLanguage handles case where the header is valid; AcceptLanguageNoHeader handles case where the header is not in request (None); and AcceptLanguageInvalidHeader handles case where the header is invalid. AcceptLanguageNoHeader and AcceptLanguageInvalidHeader will have a common base class, as they both need to respond according to the first row in the table.

[whiteroses]

I now think invalid headers should be treated like the first row in the table, rather than the third. So AcceptLanguage handles case where the header is valid; AcceptLanguageNoHeader handles case where the header is not in request (None); and AcceptLanguageInvalidHeader handles case where the header is invalid. AcceptLanguageNoHeader and AcceptLanguageInvalidHeader will have a common base class, as they both need to respond according to the first row in the table.

[bertjwregeer]

With the PR I just accepted, can this be closed @whiteroses ?

[bertjwregeer]

With the PR I just accepted, can this be closed @whiteroses ?

[whiteroses]

Thanks for merging! There is still the documentation changes we discussed, and at least one issue on Pyramid's use of WebOb's accept handling, but will close this now and open more specific issues and PRs for them when I get a chance (will have more time in December!)

[whiteroses]

Thanks for merging! There is still the documentation changes we discussed, and at least one issue on Pyramid's use of WebOb's accept handling, but will close this now and open more specific issues and PRs for them when I get a chance (will have more time in December!)