Falcon: In `Response.set_headers` method, when receiving mappings, check for interface compliance rather than concrete class

[tribals]

There is plenty of ways to implement dict-like behavior. In general, it is not necessary to subclass dict directly.

We must give the developers stable interface if he/she wants to pass their own mappings. For example, famous requests implements their own case insensitive dict-like mappnig for headers. If I pass such headers to set_headers, code breaks because of this check:

https://github.com/falconry/falcon/blob/3b8ca25a78790b343d1c875fe9fd5e7e5742a8f9/falcon/response.py#L625-L626

Better approach is to check for interface compliance:

from collections.abc import Mapping

if isinstance(headers, Mapping):
    headers = headers.items()

[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]

That makes sense. In fact, we could be even more lenient and just check whether the object implements items(). Regardless, the docs also need to be updated. We should also grep the code for isinstance() and see if there are other places that could use a similar treatment.

[myusko]

Hi @tribals @kgriffs

Hm, that's a common use case? in order to satisfy abc.Mapping interface, a user should to do something like that:

class MyMapping(abc.Mapping):
    def __getitem__(self, item):
        ... code

    def __iter__(self):
        ... code 

    def __len__(self):
        ... code

If that makes sense, I can send a PR.

[vytas7]

@mikeyusko What do you think of @kgriffs' idea to just check if the object quacks items()?

[myusko]

@vytas7 Yeah, if it is a use-case it would be better to implement own falcon mapping class.

e.g

class Mapping:
      ... some other stuff

     def items():
          ... code
------------------------------------------
import falcon

class CustomHeaders(falcon.Mapping):
    def items():
        ... code

[myusko]

@vytas7 I'll work on it.

[tribals]

@mikeyusko I think, the idea is exactly the opposite: we shouldn't implement our own dict-like class. There are too many of them already. In fact, it seems like every HTTP framework implements its own.

But there is completely no need.

What we want here, is just to iterate over key-value pairs. And this is exactly what .items() do. It's a very stable interface. So, IMO, we can depend on it.

So, code should look like this:

try:
    for k, v in headers.items():
        # do what need to be done
except AttributeError as e:
    raise ValueError('Headers must be iterable') from e

That will be enough.

Perhaps there is some framework with its own dict-like implementation which hasn't .items() method, we shouldn't maintain it. It's fault of dict-like class (and framework) developers because it is necessary to implement .items() method, so must be fixed by them.

Although, implementing your own dict-like class is a very fascinating exercise!

[myusko]

@tribals Hi,

Based on your initial message, I assumed that you're meant to create some interface, which a user should implement, in order to satisfy conditions.

But now, I understand what do you mean, also you approach means that, we won't check isinstance etc.

And it doesn't matter what a type we're retrieving, it should implement items() and it should be iterable, in general I like it:)

And as a result, we will able to do something like.

class CustomHeaders:
    HEADERS = {'foo': 'bar'}

    def items(self):
        # super custom flow, etc.
        return self.HEADERS.items()


class FooResource:
    def on_get(self, req, resp):
        resp.set_headers(CustomHeaders())

    def on_post(self, req, resp):
        resp.set_headers({'bar': 'foo'})

[tribals]

@mikeyusko

But now, I understand what do you mean, also you approach means that, we won't check isinstance etc.

Right. Interface doesn't mean .isinstance() checks.

[myusko]

@tribals Got it, also I'm totally fine to move forward with your approach 👍

cc @vytas7 @kgriffs

[vytas7]

@mikeyusko yeah, I think @kgriffs meant just checking whether the object had a callable attribute items().

Great clarifications @tribals , thanks!

I'm just not a big fan of that all-encompassing EAFP block, i.e. I would prefer to either avoid EAFP altogether or do EAFP just to get the items iterable by attempting to call the said attribute, and do the rest outside of that try... except. Cause a generic catch AttributeError can mask other nasty bugs.
But all this is much a matter of taste as well.

[myusko]

Ok, cool.

I'm going to close this https://github.com/falconry/falcon/pull/1627 PR and will push a new one in a few days.