Repository: falconry/falcon

Issue:
Custom media handlers: Unexpected issue when providing custom json handler
This is in falcon-2.0

Look at the documentation [here][1] for using rapidjson for encoding/decoding json. By providing:

`extra_handlers={'application/json': json_handler}` we are still left with the default handler for content-type `application-json; charset=UTF-8`. This results in an unexpected behaviour when some client library (e.g. Retrofit for Android) includes the charset in the header. 

While the documentation should be updated, the expected behaviour is that if the handler for `application/json` is updated - it should also update the handler for variant with charset (or at least throw a warning) otherwise there is a possibility of hidden bugs. 

[1]: https://falcon.readthedocs.io/en/stable/api/media.html

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=h1) Report
> Merging [#1785](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=desc) (c212807) into [master](https://codecov.io/gh/falconry/falcon/commit/820c51b5f0af8d1c6e9fb8cccb560b6366f97332?el=desc) (820c51b) will **decrease** coverage by `32.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1785/graphs/tree.svg?width=650&height=150&src=pr&token=zOtkfNQ3h6)](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##            master    #1785       +/-   ##
============================================
- Coverage   100.00%   67.96%   -32.04%     
============================================
  Files           53       53               
  Lines         4445     4445               
  Branches       701      701               
============================================
- Hits          4445     3021     -1424     
- Misses           0     1414     +1414     
- Partials         0       10       +10     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/media/handlers.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL21lZGlhL2hhbmRsZXJzLnB5) | `100.00% <100.00%> (ø)` | |
| [falcon/asgi/app.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/reader.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVhZGVyLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/stream.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RyZWFtLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/request.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVxdWVzdC5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/response.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVzcG9uc2UucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/multipart.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvbXVsdGlwYXJ0LnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/structures.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RydWN0dXJlcy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/\_request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvX3JlcXVlc3RfaGVscGVycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `47.58% <0.00%> (-52.42%)` | :arrow_down: |
| ... and [24 more](https://codecov.io/gh/falconry/falcon/pull/1785/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=footer). Last update [820c51b...c212807](https://codecov.io/gh/falconry/falcon/pull/1785?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

adsahay: @vytas7 Please review this. I could no longer find the responsible documentation (which was present in 2.0) to add a remark about the effect of this change. I've tested different scenarios but I didn't find any tests to update - the usual media handler tests are all passing. 

As this is my first time contributing code please let me know if I'm missing something. 
adsahay: @vytas7 What is a good way (in the tests) to assert which media handler was called?
vytas7: @adsahay whatever you like :slightly_smiling_face: 
In the absence of better ideas, you could, for instance, increase some attribute (for each `content_type` deserialized) on the handler itself, to later `assert` it is in line with your expectation.
adsahay: Need some help with a failing test:

```=========================== short test summary info ============================
FAILED tests/test_utils.py::TestFalconTestingUtils::test_simulate_content_type[application/json]
FAILED tests/test_utils.py::TestFalconTestingUtils::test_simulate_content_type[application/json; charset=UTF-8]
========== 2 failed, 1730 passed, 864 skipped, 13 warnings in 16.12s ===========
ERROR: InvocationError for command /home/runner/work/falcon/falcon/.tox/py35/bin/coverage run -m pytest tests --ignore=tests/asgi (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   py35: commands failed
Error: Process completed with exit code 1.```

Is it ok to use mocks to identify if a function is called?
vytas7: @adsahay it seems that `unittest.mock` shipped with CPython 3.5.2 stdlib is not good enough for our use.

We should refactor that test, either by not relying on mocking at all (or restricting our use to `unittest.mock` 3.5 functionality) or by using another mocking utility such as [`pytest`'s `monkeypatch` fixture](https://docs.pytest.org/en/stable/monkeypatch.html).
vytas7: @adsahay I have taken the liberty of slightly adjusting your tests and the newsfragment.
Please check if I haven't vandalized them too much wrt your original intent.

Patch:
diff --git a/docs/_newsfragments/1717.newandimproved.rst b/docs/_newsfragments/1717.newandimproved.rst
new file mode 100644
index 0000000..eec51fa
--- /dev/null
+++ b/docs/_newsfragments/1717.newandimproved.rst
@@ -0,0 +1,5 @@
+:class:`Default media handlers <falcon.media.Handlers>` were simplified by
+removing a separate handler for the now-obsolete
+``application/json; charset=UTF-8``.
+As a result, providing a custom JSON media handler will now unambiguously cover
+both ``application/json`` and the former ``Content-type``.
diff --git a/falcon/media/handlers.py b/falcon/media/handlers.py
index 1ce1c25..ba15989 100644
--- a/falcon/media/handlers.py
+++ b/falcon/media/handlers.py
@@ -1,7 +1,7 @@
 from collections import UserDict
 
 from falcon import errors
-from falcon.constants import MEDIA_MULTIPART, MEDIA_URLENCODED
+from falcon.constants import MEDIA_JSON, MEDIA_MULTIPART, MEDIA_URLENCODED
 from falcon.media.json import JSONHandler
 from falcon.media.multipart import MultipartFormHandler, MultipartParseOptions
 from falcon.media.urlencoded import URLEncodedFormHandler
@@ -12,8 +12,7 @@ class Handlers(UserDict):
     """A :class:`dict`-like object that manages Internet media type handlers."""
     def __init__(self, initial=None):
         handlers = initial or {
-            'application/json': JSONHandler(),
-            'application/json; charset=UTF-8': JSONHandler(),
+            MEDIA_JSON: JSONHandler(),
             MEDIA_MULTIPART: MultipartFormHandler(),
             MEDIA_URLENCODED: URLEncodedFormHandler(),
         }
@@ -60,7 +59,6 @@ class Handlers(UserDict):
 
 # NOTE(vytas): An ugly way to work around circular imports.
 MultipartParseOptions._DEFAULT_HANDLERS = Handlers({
-    'application/json': JSONHandler(),
-    'application/json; charset=UTF-8': JSONHandler(),
+    MEDIA_JSON: JSONHandler(),
     MEDIA_URLENCODED: URLEncodedFormHandler(),
 })  # type: ignore


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
index af4b5cb..cad461d 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -10,8 +10,10 @@ from urllib.parse import quote, unquote_plus
 import pytest
 
 import falcon
+from falcon import media
 from falcon import testing
 from falcon import util
+from falcon.constants import MEDIA_JSON
 from falcon.util import deprecation, json, misc, structures, uri
 
 from _util import create_app, to_coroutine  # NOQA
@@ -874,6 +876,81 @@ class TestFalconTestingUtils:
         assert result.status_code == 200
         assert result.text == 'test'
 
+    @pytest.mark.parametrize('content_type', [
+        'application/json',
+        'application/json; charset=UTF-8',
+        'application/yaml',
+    ])
+    def test_simulate_content_type(self, content_type):
+        class MediaMirror():
+            def on_post(self, req, resp):
+                resp.media = req.media
+
+        app = create_app(asgi=False)
+        app.add_route('/', MediaMirror())
+
+        client = testing.TestClient(app)
+        headers = {'Content-Type': content_type}
+        payload = b'{"hello": "world"}'
+
+        resp = client.simulate_post('/', headers=headers, body=payload)
+
+        if MEDIA_JSON in content_type:
+            assert resp.status_code == 200
+            assert resp.json == {'hello': 'world'}
+        else:
+            # JSON handler should not have been called for YAML
+            assert resp.status_code == 415
+
+    @pytest.mark.parametrize('content_type', [
+        'application/json',
+        'application/json; charset=UTF-8',
+        'application/yaml'
+    ])
+    def test_simulate_content_type_extra_handler(self, asgi, content_type):
+        class TrackingJSONHandler(media.JSONHandler):
+            def __init__(self):
+                super().__init__()
+                self.deserialize_count = 0
+
+            def deserialize(self, *args, **kwargs):
+                result = super().deserialize(*args, **kwargs)
+                self.deserialize_count += 1
+                return result
+
+            async def deserialize_async(self, *args, **kwargs):
+                result = await super().deserialize_async(*args, **kwargs)
+                self.deserialize_count += 1
+                return result
+
+        resource = testing.SimpleTestResourceAsync() if asgi else testing.SimpleTestResource()
+        app = create_app(asgi)
+        app.add_route('/', resource)
+
+        handler = TrackingJSONHandler()
+        extra_handlers = {'application/json': handler}
+        app.req_options.media_handlers.update(extra_handlers)
+        app.resp_options.media_handlers.update(extra_handlers)
+
+        client = testing.TestClient(app)
+        headers = {
+            'Content-Type': content_type,
+            'capture-req-media': 'y',
+        }
+        payload = b'{"hello": "world"}'
+        resp = client.simulate_post('/', headers=headers, body=payload)
+
+        if MEDIA_JSON in content_type:
+            # Test that our custom deserializer was called
+            assert handler.deserialize_count == 1
+            assert resource.captured_req_media == {'hello': 'world'}
+            assert resp.status_code == 200
+        else:
+            # YAML should not get handled
+            assert handler.deserialize_count == 0
+            assert resource.captured_req_media is None
+            assert resp.status_code == 415
+
 
 class TestNoApiClass(testing.TestCase):
     def test_something(self):