Falcon: Custom media handlers: Unexpected issue when providing custom json handler

[adsahay]

This is in falcon-2.0

Look at the documentation here 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.

[open-collective-bot[bot]]

Hi :wave:,

Thanks for using Falcon. The large amount of time and effort needed to
maintain the project and develop new features is not sustainable without
the generous financial support of community members like you.

Please consider helping us secure the future of the Falcon framework with a
one-time or recurring donation.

Thank you for your support!

[kgriffs]

I agree we should make this better since it violates the principle of least-surprise. Scheduled for 3.0 in case someone wants to tackle it right away, otherwise it will need to be pushed to 3.1.

[vytas7]

FWIW, our media type negotiation machinery seems to automatically choose the application/json handler for requests sent as application-json; charset=UTF-8, even if an explicit handler for the latter is missing. In that light, could we just remove the explicit application-json; charset=UTF-8 handler from our codebase and the docs, now that we are defaulting to just application/json ourselves.

As a bonus, that would eliminate confusion in situations like @adsahay is describing.

I was testing the app below, and it seems to just work:

import falcon


class Resource:

    def on_post(self, req, resp):
        resp.media = req.get_media()


app = falcon.App()
app.req_options.media_handlers = falcon.media.Handlers({
    'application/json': falcon.media.JSONHandler(),
})
app.add_route('/mirror', Resource())

Thoughts?

[adsahay]

Yes this sounds great.

Also, it might be a stylistic preference, but in such situations if we can have a constant for all handlers (falcon.MIME_JSON) or something it will have the additional benefit of discoverability of available handlers in documentation / code completion, and avoid typos.

[adsahay]

If you want I can fix and send PR for this (my first falcon contribution!)

[vytas7]

Hi again @adsahay ,
We do have such constants (the stable version already has), maybe we just haven't updated all the docs with them yet. Or, we may have left explicit strings in selected places for clarity.

>>> import falcon
>>> falcon.MEDIA_JSON
'application/json'
>>> falcon.MEDIA_PNG
'image/png'
>>> falcon.MEDIA_TEXT
'text/plain; charset=utf-8'

@adsahay PRs are always welcome :slightly_smiling_face: :+1:
To be honest, I haven't fully explored the ramifications of removing the explicit UTF-8 JSON handler, it's just a quick idea that crossed my head. But maybe we could start a draft PR anyway?

@kgriffs thoughts on my suggestion to simplify the default JSON handlers by only keeping the application/json handler?

[kgriffs]

It could be that the key including the charset was left in to speed up the selection algorithm... regardless, I agree that it is not worth maintaining it since in most cases the charset will not be present in the string to begin with.

[vytas7]

@adsahay just checking, would you like to work on this by removing application-json; charset=UTF-8 from the default handlers?

As we proceed to remove it though, I would like to see a note added to the docs regarding speeding up the selection algorithm (needs to be verified with a benchmark!), as hinted by @kgriffs above. So that people could duplicate their handlers for a specific parameter set, if they expect that to be their common use case that needs to be optimized.

[adsahay]

Ok sure.

[vytas7]

@adsahay just checking again :slightly_smiling_face: Have you had a chance to do any work on this one?

[adsahay]

Not yet, slipped through the cracks. Will wrap it up tomorrow.

On Tue, 17 Nov 2020 at 12:36, Vytautas Liuolia [e-mail]
wrote:

@adsahay https://github.com/adsahay just checking again 🙂 Have you had
a chance to do any work on this one?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/falconry/falcon/issues/1717#issuecomment-728733045,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAC3X52J53AIVPNXEU26FPLSQIOIXANCNFSM4MN7TATQ
.

--
Aditya Sahay
@adsahay http://twitter.com/adsahay | sahay.co

[vytas7]

Awesome, looking forward to!

[CaselIT]

@adsahay just checking, would you like to work on this by removing application-json; charset=UTF-8 from the default handlers?

As we proceed to remove it though, I would like to see a note added to the docs regarding speeding up the selection algorithm (needs to be verified with a benchmark!), as hinted by @kgriffs above. So that people could duplicate their handlers for a specific parameter set, if they expect that to be their common use case that needs to be optimized.

As an alternative, can't we just add the handler to the dict once we have found it? I mean before the return here
https://github.com/falconry/falcon/blob/820c51b5f0af8d1c6e9fb8cccb560b6366f97332/falcon/media/handlers.py#L51-L58

we could cache the resolved media type as self.data[media_type] = self.data[resolved].

The drawback to this is that it would break the changing of the options after the application startup (without doing something more complex, like having a second dict used as cache). but it may be worth it.
Thoughts @kgriffs @vytas7 ?

[vytas7]

@CaselIT that would probably be a bit of a can of worms regarding thread safety.

However, to that end, we could use functools.lru_cache (as we already do for HTTP Status conversions).
We should benchmark and analyze different Content-Type scenarios, but it does sound like a possible optimization, yes :+1:

[CaselIT]

@CaselIT that would probably be a bit of a can of worms regarding thread safety.

Well you could do the call more than once, but the end result would still be correct (ie at the end the dict has the correct cached value).

[vytas7]

Note that while I find caching the result of _resolve_media_type() a great idea, this might be challenging to guarantee exactly the same behaviour as now, when users can modify MediaHandlers at any time. Probably we'd need to hook into its UserDict methods to invalidate cache upon any modification.

[CaselIT]

Yes, that was what i meant in this previous comment

The drawback to this is that it would break the changing of the options after the application startup

[vytas7]

Ah, right, sorry. OTOH, as I noted above, it builds upon UserDict, where we could possibly override all relevant methods in order to keep compatibility with the current behaviour, making this a pure optimization.

[CaselIT]

I'll open a new issue regarding this, since it's only tangentially related to this one