Fix bugs when using resolvers with dictionaries

[OtherBarry]

Allows skipping resolvers by adding always_resolve = False to a schema config, which will not run any resolvers if the GetterDict object is a dictionary. This is still a breaking change, as always_resolve defaults to True, but this was kept to ensure that new features with context are easy to use.

Additionally, this now wraps dicts passed to resolvers in a _DictToObjectWrapper instance, allowing resolvers to always use attributes. As having dicts or objects passed to resolvers meant resolvers would only work for input or output, otherwise they would have convolutedly handle accessing data from a parameter that could be an object or a dict.

Closes #871

[vitalik]

Hi @OtherBarry

I've been checking this for a while, and I think there more issues than wins:

• more complexity (more ifs and cheks and wrapperrs) gives worse performance comparing to pydantc's BaseModel (it already suffers alot with DjangoGetter and _convert_result methods)
• always_resolve - this very wage and most users will not have clue what's it about ( and I myslef cannot explain either why dicts should not be resolved with special flag)
• and I think default behaviour should be that resolve_ works always (no matter of input)

but as for making easier backwards compatibility - maybe context should notify the stage at which resolver is working

    class Data(Schema):

    @staticmethod
    def resolve_name(obj, context: Optional[dict]):
        return ...

currently

• if Data is used for parsing request - it will have a request in context
• if parsing for response - context empty

maybe the way it should be:

• request in -> like it is now context has request object
• response -> context will have 'reqeust' and "is_response" flag = True, to notify that this is response processing

that way with is_response flag you can achive previous behaviour

Let me know what you think

[OtherBarry]

@vitalik thanks for taking a look at this.

more complexity (more ifs and cheks and wrapperrs) gives worse performance comparing to pydantc's BaseModel (it already suffers alot with DjangoGetter and _convert_result methods)

I'm not sure this is really the case. In a standard flow, there's only one extra if statement (a simple isinstance check) and one extra dictionary get. I would be surprised if this made a noticeable impact on performance, but very happy to run some benchmarks or something to compare if you can point me to them.

always_resolve - this very wage and most users will not have clue what's it about ( and I myslef cannot explain either why dicts should not be resolved with special flag)

The primary motivation for this was input vs output. The resolvers are intended for converting more complex server side data structures into simpler json for the client. It doesn't make sense to me for a client to send json that doesn't match the OpenAPI schema, and have that automatically resolved into something else without them having any knowledge of it, so having some way to skip that is nice.

Agreed that having it be decided by dicts / not-dicts isn't the best way to do it, but the logic was that inputs will always be dicts, and outputs (in my experience at least) are almost always not dicts. I think the context flags you mention are a better way to do it. I'd also note that most users not having a clue about something would preclude half the features of most libraries/programming languages.

and I think default behaviour should be that resolve_ works always (no matter of input)

100% agree, that's why always_resolve defaults to True.

maybe the way it should be:

• request in -> like it is now context has request object
• response -> context will have 'reqeust' and "is_response" flag = True, to notify that this is response processing

Big fan of this. Ideally there'd still be a way to skip resolving at a Schema level, but to be fair I've never had a schema with more than a couple of resolvers, so not really a problem.

---

To be honest, the biggest part of this for me now is the dict to object wrapper. It doesn't make sense to me to have resolvers that are going to be run on input (dicts) and output (likely not dicts), if there's no unified way to access the data. Without this for every value you want to get in the resolver, you'd have to check the input type: e.g.

@classmethod
def resolve_value(cls, obj):
    if isinstance(obj, dict):
        foo = obj["foo"]
        bar = obj["bar"]
    else:
        foo = obj.foo
        bar = obj.bar
     return f"{foo} {bar}"

Compared to this with the dict wrapper:

@classmethod
def resolve_value(cls, obj):
     return f"{obj.foo} {obj.bar}"

It also works nicer with type hinting, as you can type hint it as the expected object, and it'll work whether it's a dict or the expected object.

---

If you're happy with the above, I can replace the always_resolve logic with the new context logic in this PR, keeping the wrapper for dicts. Otherwise I can make a separate PR, or just leave it to you. Let me know what works for you.

[vitalik]

To be honest, the biggest part of this for me now is the dict to object wrapper. It doesn't make sense to me to have resolvers that are going to be run on input (dicts) and output (likely not dicts), if there's no unified way to access the data. Without this for every value you want to get in the resolver, you'd have to check the input type: e.g.

@classmethod
def resolve_value(cls, obj):
    if isinstance(obj, dict):
        foo = obj["foo"]
        bar = obj["bar"]
    else:
        foo = obj.foo
        bar = obj.bar
     return f"{foo} {bar}"

well the thing is that when handle response - it's not always that you can get an object - you are pretty free to return custom list of dicts :

@api.get('/test', response=list[Some])
def test(request):
       qs = SomeModel.objects.all()
       return [
           {
              "id": item.id, 
              "some-special": item.some_function(), # <-- here the key has a "-"
          } for item in qs
       ]

so guaranteeing an object(non-dict) in resolve_xxx is not possible - the value passed to resolve can be both dict and any object

On the other hand you can use your _DictToObjectWrapper object inside resolver

@classmethod
def resolve_value(cls, obj):
   obj = _DictToObjectWrapper(obj) # if isinstance dict ?
   return f"{obj.foo} {obj.bar}"

or create decorator that will enforce this

def force_obj_decorator(func):
    def wrapper(*args, **kwargs):
        obj = _DictWrapper(args[0])
        return func(obj, **kwargs)

    return wrapper



class Data(Schema):
    x: int

    @staticmethod
    @force_obj_decorator # <<<-------------
    def resolve_x(obj, context):
        return obj.x+ 1

and even make some metaclass that will apply this decorator for all resolvers:

from ninja.schema import ResolverMetaclass

class DictToObjMeta(ResolverMetaclass):
    def __new__(cls, name, bases, namespace, **kwargs):
        result_cls = super().__new__(cls, name, bases, namespace, **kwargs)
        for attr, resolver in result_cls._ninja_resolvers.items():
            resolver._func = force_obj_decorator(resolver._func) # <---- !!!!!!!!!!!
        return result_cls


class Data(Schema, metaclass=DictToObjMeta) # <---- !!!!!!!!!!!
    x: int

    @staticmethod
    def resolve_x(obj, context):
        print("resolve_x", obj, context)
        return obj.x + 1

[OtherBarry]

The non attribute accessible parameters are the primary issue I guess, which there's no perfect solution to. I'm fine to leave that part out as well then, as long as there's a good explanation in the docs. Given how much it stumped me, I can see this being confusing behaviour for new users (or maybe I'm just a bit slow 😅).